| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-17 | |||
| 14:48:59 | jaypipes | efried: ya, getting there... | |
| 14:49:07 | efried | Thx | |
| 14:50:39 | edleafe | As long as we're begging for reviews, it would be nice to get the last alternate hosts patch merged: https://review.openstack.org/#/c/526436/ | |
| 14:50:47 | ameeda | jaypipes: so If I remove my functional test. you would accept that and merge ? | |
| 14:51:04 | jaypipes | ameeda: not without a change to unit tests. | |
| 14:51:10 | edleafe | Matt was good with it before he left for holiday - just wanted some more eyes on it | |
| 14:51:30 | ameeda | jaypipes: what kind of change you want on unit tests ? | |
| 14:51:55 | jaypipes | ameeda: at least something that demonstrates the code changes in your patch work as expected. | |
| 14:52:39 | ameeda | jaypipes: so If I want to checking the length of the instance_metadata DB record's values, what do you think | |
| 14:54:01 | ameeda | jaypipes: what about this "images = self._image_api.get_all(self.admin_api)" , it will get the info from instance_metadata DB | |
| 14:54:11 | jaypipes | no. | |
| 14:54:19 | jaypipes | the image api doesn't touch the nova db. | |
| 14:54:44 | ameeda | jaypipes: how you can help me to check record manually ? | |
| 14:54:48 | jaypipes | ameeda: https://review.openstack.org/#/c/531512/6/nova/tests/functional/db/test_allocation_candidates.py | |
| 14:54:58 | jaypipes | ameeda: check out the utility method on line 1453 there | |
| 14:55:13 | jaypipes | ameeda: that shows an example of how to query the nova DB from a functional test manually. | |
| 14:55:35 | jaypipes | ameeda: you would need to do something like that. only instead of hitting the API DB, you'd want to hit the nova cell DB | |
| 14:55:47 | jaypipes | ameeda: so something like this: | |
| 14:55:57 | jaypipes | with self.db.get_engine().connect() as conn: | |
| 14:56:39 | jaypipes | return conn.execute("SELECT key, value FROM instance_metadata WHERE instance_uuid = '%s'" % instance.uuid) | |
| 14:56:54 | jaypipes | ameeda: I will leave it up to you to figure it out from here. | |
| 14:57:28 | ameeda | jaypipes: thank you very much, I appropriate that from you, Thank you for your time | |
| 14:57:46 | jaypipes | np | |
| 14:58:16 | ameeda | jaypipes: this case must be in unit test, so I should remove functional test ? | |
| 14:58:33 | jaypipes | ameeda: no, you can keep the functional test. | |
| 14:59:04 | jaypipes | ameeda: and instead of asserting the return value from self.images_api.get(), you would instead assert the length of the DB record | |
| 15:00:00 | ameeda | jaypipes: well, so I will add the code to check the record in functional test ? | |
| 15:00:29 | jaypipes | ameeda: yes | |
| 15:00:38 | ameeda | great ! | |
| 15:01:05 | ameeda | jaypipes: again thank you very much, I was feeling bad before, I have to leave now, I will complete the work tomorrow. | |
| 15:01:24 | jaypipes | ameeda: np | |
| 15:06:11 | sean-k-mooney | is mriedem around this week? | |
| 15:11:13 | edleafe | sean-k-mooney: on holiday without computer | |
| 15:12:26 | bauzas | sean-k-mooney: needing a os-vif release ? | |
| 15:12:34 | bauzas | I can do that | |
| 15:13:02 | bauzas | in theory, I have powers for that, heh | |
| 15:13:14 | sean-k-mooney | bauzas: ya https://review.openstack.org/#/c/534807/ | |
| 15:13:55 | bauzas | sean-k-mooney: I guess you only need a +1 ? | |
| 15:14:04 | sean-k-mooney | bauzas: oh ya your in the nova-stable branch group | |
| 15:14:05 | bauzas | because neither matt nor I have +2s | |
| 15:14:30 | bauzas | on the releases repo | |
| 15:14:30 | sean-k-mooney | bauzas: ya i need a +1 form a nova core and then infra need to +2 it | |
| 15:14:44 | bauzas | sean-k-mooney: ok, briefly looking | |
| 15:14:56 | bauzas | in theory, it's a +1 from the PTL or the Release Management liaison | |
| 15:15:03 | sean-k-mooney | bauzas: technically yes | |
| 15:15:08 | bauzas | lucky for you, I'm the nove liaison :) | |
| 15:15:15 | bauzas | nova* (of course) | |
| 15:16:23 | sean-k-mooney | edleafe: ah that explains why i have not seen him this week. he mentioned he would be away but was not sure if it was all week | |
| 15:18:41 | bauzas | sean-k-mooney: +1d FWIW, I leave you chase the release folks | |
| 15:20:01 | sean-k-mooney | bauzas: no worries. thanks | |
| 15:22:06 | openstackgerrit | Tetsuro Nakamura proposed openstack/nova master: Fix comments in get_all_with_shared() https://review.openstack.org/533195 | |
| 15:23:41 | gibi | ildikov: I have only question in the multiattach api patch, https://review.openstack.org/#/c/271047/55/nova/compute/api.py@3700 | |
| 15:24:40 | openstackgerrit | Tetsuro Nakamura proposed openstack/nova master: Fix allocation_candidates not to ignore shared RPs https://review.openstack.org/533396 | |
| 15:26:43 | stephenfin | sean-k-mooney: You might do a follow up patch to 'openstack/requirements' to blacklist os-vif 1.8.0 too | |
| 15:27:29 | ildikov | gibi: hmm, I'm not sure I fully understand the question | |
| 15:27:51 | sean-k-mooney | stephenfin: oh good point i guess we should wait for the auto generated one for 1.8.1 to be submitted then i can modify it to blacklist 1.8.0 | |
| 15:27:53 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP libvirt: QEMU native LUKS decryption for volumes https://review.openstack.org/523958 | |
| 15:29:28 | gibi | ildikov: so in the _check_attach_and_reserve_volume we differentiate between two cases | |
| 15:29:38 | sean-k-mooney | stephenfin: i think it will just updat this one https://review.openstack.org/#/c/532987/ but ill wait for it to be submitted. | |
| 15:30:08 | gibi | ildikov: and if this is the case when the user attaches a new volume to a running instance then we don't check the multiattach capabilities | |
| 15:30:11 | sean-k-mooney | stephenfin: by the way can you take a look at https://review.openstack.org/#/c/533766/ this is the nova unit test change that is required for os-vif 1.8.x | |
| 15:30:44 | gibi | ildikov: at least based on the code comment I commented on | |
| 15:31:06 | stephenfin | sean-k-mooney: Stuck in meetings til EOD (Intel flashbacks, heh) but I'll try squeeze it in. If not, I'll tackle it tomorrow (y) | |
| 15:31:07 | ildikov | gibi: we always check this first: if volume['multiattach'] and not supports_multiattach: | |
| 15:31:38 | gibi | ildikov: true, but the compute version is not checked in the if branch | |
| 15:31:39 | stephenfin | sean-k-mooney: Oh, wait. It's tiny. Done | |
| 15:31:41 | sean-k-mooney | stephenfin: cool no problem, its pretty trivial | |
| 15:31:55 | sean-k-mooney | ya just missing the datapath field. | |
| 15:32:07 | gibi | ildikov: it is only checked in the else branch | |
| 15:32:49 | ildikov | gibi: ah ok, I see now | |
| 15:33:46 | gibi | ildikov: it could be that we check the compute version is some other point in the call stack for the if case | |
| 15:36:32 | ildikov | gibi: we do fail in reserve_block_device_name here: https://review.openstack.org/#/c/531244/9/nova/compute/rpcapi.py | |
| 15:37:48 | tetsuro | bauzas: Hi, if you have time, could you have a look at https://review.openstack.org/#/c/534807/ ? | |
| 15:40:47 | ildikov | gibi: which might not be satisfying enough for that 'if' | |
| 15:41:33 | ildikov | gibi: but that's my guess for now :) | |
| 15:42:18 | bauzas | sean-k-mooney: saw https://review.openstack.org/#/c/534807/ ? | |
| 15:42:22 | bauzas | just respin | |
| 15:42:26 | gibi | ildikov: thanks I think I see now. compute.api._attach_volume() calls _create_volume_bdm that will call the rpc | |
| 15:42:40 | bauzas | tetsuro: you pointed me sean-k-mooney's patch, was that intentional ? | |
| 15:42:43 | gibi | ildikov: in case of attaching a volume to an existing instance | |
| 15:43:07 | ildikov | gibi: yep, that flow sounds correct | |
| 15:43:28 | gibi | ildikov: so it fails before _check_attach_and_reserve_volume is called in _attach_volume | |
| 15:43:29 | tetsuro | bauzas: sorry, just a mistake | |
| 15:43:44 | gibi | ildikov: this is why we don't need an extra check in _check_attach_and_reserve_volume for the attach volume case | |
| 15:44:21 | tetsuro | bauzas: could you have a look at https://review.openstack.org/#/c/531347/ ? | |
| 15:44:52 | bauzas | tetsuro: short patch, will look :) | |
| 15:45:42 | ildikov | gibi: +1 | |
| 15:45:47 | tetsuro | bauzas: thanks in advance | |
| 15:46:50 | smcginnis | bauzas: Hey, are you good with the new version on https://review.openstack.org/#/c/534807/ ? | |
| 15:47:07 | smcginnis | bauzas: I assume so, but would be good to have your +1 on there for historical reference. | |
| 15:47:22 | bauzas | smcginnis: I'm on it | |
| 15:47:23 | bauzas | +1d | |
| 15:47:28 | gibi | ildikov: then +2 from me :) | |
| 15:47:35 | smcginnis | bauzas: Thanks! | |
| 15:47:39 | bauzas | smcginnis: thanks for the catch on a minor release | |
| 15:48:08 | smcginnis | bauzas: Wouldn't have been the end of the world, but I think this version is more appropriate. ;) | |
| 15:48:28 | ildikov | gibi: much appreciated | |
| 15:48:32 | bauzas | smcginnis: any release documentation about semver we have could help me | |
| 15:48:45 | ildikov | gibi: will update the tests if I have to upload a new version | |
| 15:49:07 | bauzas | in general, I struggle with identifying .y releases, and just accept .zs | |