| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-02-02 | |||
| 14:08:35 | openstackgerrit | Eric Fried proposed openstack/nova-specs master: Granular Resource Request Syntax (Rocky) https://review.openstack.org/540179 | |
| 14:11:42 | mriedem | lyarwood: mdbooth: can you comment on melwitt's question here https://review.openstack.org/#/c/539739/2/nova/virt/libvirt/driver.py@7430 | |
| 14:11:43 | openstackgerrit | Eric Fried proposed openstack/nova master: Avoid inventory DELETE API (no conflict detection) https://review.openstack.org/539712 | |
| 14:11:51 | mriedem | i don't see anything ever setting connection_info['volume_id'] | |
| 14:12:44 | figleaf | giblet: nice | |
| 14:13:08 | mdbooth | mriedem: When I was working on an HLR for a telco many, many moons ago, we'd have called that sort of request tromboning :) | |
| 14:13:27 | figleaf | giblet: working on fixing the merge conflict | |
| 14:13:59 | mriedem | mdbooth: ok i don't know what that means | |
| 14:14:10 | ildikov | mriedem: when we did the connection_info translation for the new attach API we didn't explicitly add the volume_id field to 'data' there so it doesn't seem to come from Cinder either | |
| 14:14:18 | lyarwood | mriedem: yup looking now | |
| 14:15:33 | mdbooth | mriedem: It was a request sent externally only to be bounced back to an internal source :) | |
| 14:16:16 | mdbooth | mriedem: Ah, that's interesting. I made some comments about that in review. | |
| 14:16:23 | mriedem | mdbooth: ok? i asked you guys because you were reviewing/working on the luks patch that i'm trying to fix. | |
| 14:16:54 | mdbooth | mriedem: Sorry, IRC failure. My mind was just wandering... | |
| 14:17:12 | mdbooth | mriedem: So I don't think we should be fetching stuff out of connection_info at all | |
| 14:17:27 | mriedem | this line specifically can be using bdm.volume_id | |
| 14:17:33 | mdbooth | I let it pass in review because I believed we always set volume_id in practise | |
| 14:17:38 | mdbooth | in connection_info | |
| 14:17:40 | mriedem | i was just asking if there was some specific logic as to why it was using connection_info | |
| 14:17:52 | lyarwood | mriedem: vol['id'] no? | |
| 14:18:10 | cdent | fried_rice: you'll probably appreciate the fun of https://bugs.launchpad.net/nova/+bug/1747001 | |
| 14:18:11 | openstack | Launchpad bug 1747001 in OpenStack Compute (nova) "Use of parse.urlencode with dict in nova/tests/unit/scheduler/client/test_report.py can result in unpredictable query strings and thus unreliable tests" [Low,Triaged] | |
| 14:18:13 | mdbooth | No, it was only out of expediency to avoid a larger refactor | |
| 14:18:14 | lyarwood | mriedem: we don't set the id on these LibvirtLiveMigrateBDMInfo objects | |
| 14:18:22 | mriedem | lyarwood: vol['id'] would be the primary key of the bdm | |
| 14:18:41 | mriedem | block_device_mapping = driver.block_device_info_get_mapping( | |
| 14:18:41 | mriedem | block_device_info) | |
| 14:18:46 | mdbooth | Although not *much* larger... | |
| 14:18:48 | lyarwood | right sorry, volume_id | |
| 14:19:01 | mriedem | so i'll fix in my change, good thing mel noticed it, | |
| 14:19:11 | mriedem | was just wondering if there was any good reason behind this being here, and i guess there isn't | |
| 14:19:14 | lyarwood | that and the detach lookup | |
| 14:19:36 | fried_rice | cdent: Appreciating the fun... | |
| 14:19:39 | lyarwood | mdbooth: which refactor? Do you have a link to the comment talking about that? | |
| 14:19:45 | mdbooth | mriedem: If we're going to change that code, we should entirely stop poking around inside connection_info, because we *always* have volume_id in the caller context | |
| 14:19:57 | mdbooth | mriedem: i.e. we can just pass in volume_id | |
| 14:20:14 | lyarwood | nack, lets fix the bug and then talk about that refactor | |
| 14:20:16 | fried_rice | cdent: I thought I had dealt with that issue... | |
| 14:20:23 | mriedem | mdbooth: with 6 days to RC1, i want to just fix the bug | |
| 14:20:40 | mdbooth | It's really not a big refactor, just shuffling some argument | |
| 14:20:57 | giblet | ildikov, mriedem: I left some comments in https://review.openstack.org/#/c/532689 | |
| 14:20:59 | mdbooth | And it will be more robust as you're no longer beholden to the vagiaries of connection_info | |
| 14:21:26 | lyarwood | mriedem's suggested change isn't | |
| 14:21:30 | mdbooth | With 6 days to RC1, personally I'd just buckle down and do it. No heuristics means no late surprises. | |
| 14:22:25 | mriedem | with 6 days to RC1 how about we just revert https://review.openstack.org/#/c/523958/ ? | |
| 14:22:28 | mriedem | would that help? | |
| 14:22:32 | bauwser | FWIW, we have more than 50 open bugs | |
| 14:22:36 | bauwser | 6 days before RC1 | |
| 14:22:41 | bauwser | that's what it tramples me | |
| 14:22:55 | bauwser | 50 "new" bugs, my bad | |
| 14:23:12 | bauwser | and I don't have so much time reviewing those bug reports | |
| 14:23:25 | mriedem | lee answered my question so i'll just make this fix and we can move on | |
| 14:23:30 | bauwser | so, if anyone can help me on that, many thanks | |
| 14:23:33 | lyarwood | please, thanks :) | |
| 14:23:45 | mriedem | bauwser: several of those new bugs are old-ish at this point | |
| 14:23:59 | mriedem | like, "should instance group policies be empty?" | |
| 14:24:05 | bauwser | several, but I don't know if we have regressions yet | |
| 14:24:11 | mriedem | i don't know if that's a bug or what so i didn't know how to triage it | |
| 14:24:21 | bauwser | okay, I'll really need to help you | |
| 14:24:22 | mdbooth | Meh, this is how we play virt driver Jenga :( | |
| 14:24:32 | bauwser | ... once I'm done with other internal work :) | |
| 14:24:56 | mriedem | bauwser: i went through these after the meeting yesterday, there is only one new one in there since then | |
| 14:25:04 | bauwser | cool then | |
| 14:25:21 | bauwser | anyway, let's not diverge | |
| 14:26:59 | ildikov | giblet: tnx | |
| 14:28:36 | mriedem | lyarwood: looks like there were no tests for that pre_live_migration code | |
| 14:28:45 | mriedem | i was thinking it would be in https://review.openstack.org/#/c/523958/18/nova/tests/unit/virt/libvirt/test_migration.py but those don't actually call the driver from what i can tell | |
| 14:28:56 | mriedem | i added an exception to where volume_id was set in that block in pre_live_migratoin and nothing failed | |
| 14:29:46 | mriedem | will see if i can wrangle some existing test to add that condition | |
| 14:29:55 | cdent | fried_rice: this one is fun too https://bugs.launchpad.net/nova/+bug/1747003 | |
| 14:29:56 | openstack | Launchpad bug 1747003 in OpenStack Compute (nova) "A bad _RC_CACHE can rarely cause unit tests to fail" [Undecided,New] | |
| 14:30:03 | lyarwood | mriedem: is there nothing in _test_pre_live_migration_works_correctly_mocked ? | |
| 14:31:10 | fried_rice | cdent: Are you proposing fixes, or setting me up to review them? | |
| 14:31:11 | mriedem | no | |
| 14:32:02 | mriedem | i'll update https://review.openstack.org/#/c/523958/18/nova/tests/unit/virt/libvirt/test_migration.py | |
| 14:32:08 | mriedem | oops, test_pre_live_migration_vol_backed_works_correctly_mocked | |
| 14:32:26 | cdent | fried_rice: I'm making conversation. I'll propose fixes at some point. The RC_CACHE one is easy, the other one somewhat more effortful. | |
| 14:32:54 | Spazmotic | And just in time for my pre-sleepytime begging session to bother the first shift folks - If any core gets a chance to review this Live Migration XenAPI commit i'd apprecaite it. https://review.openstack.org/#/c/538415/ | |
| 14:33:07 | fried_rice | cdent: Okay. I wasn't ragging on you, just didn't want to duplicate effort if I decided to go off and work on one of 'em. | |
| 14:33:10 | Spazmotic | I know ya'lls busy, but just figured i'd throw it out there while I eat cereal :D | |
| 14:33:16 | openstackgerrit | Ed Leafe proposed openstack/nova master: Fix invalid UUIDs in test_compute.py https://review.openstack.org/531479 | |
| 14:33:53 | lyarwood | mriedem: re the connection_info.get('volume_id') lookup, I'm pretty sure that actually works, I can't find where but I recall one of the bdm layers using connection_info to store connection_info['data'] | |
| 14:34:47 | openstackgerrit | Ed Leafe proposed openstack/nova master: Escalate UUID validation warning to error in test https://review.openstack.org/540386 | |
| 14:35:36 | mriedem | connection_info['data'] != connection_info['volume_id'] | |
| 14:35:55 | mriedem | lyarwood: i'm assuming you meant that line of code to be the same as the others that looked for connection_info['data']['volume_id'] | |
| 14:36:02 | mriedem | it was probably a typo, that wasn't tested so it wasn't noticed | |
| 14:40:50 | lyarwood | mriedem: right, I'm pretty confused how the tempest tests I've been working on have passed without this | |
| 14:41:03 | mriedem | lyarwood: volume-backed live migratoin isn't enabled in tempest by default | |
| 14:41:08 | mriedem | so the tests likely aren't getting run | |
| 14:41:15 | lyarwood | mriedem: it is in my DNM change | |
| 14:41:18 | lyarwood | mriedem: and locally | |
| 14:41:24 | lyarwood | mriedem: where these tests have been passing | |
| 14:41:47 | lyarwood | http://logs.openstack.org/50/536350/5/check/legacy-tempest-dsvm-multinode-live-migration/539c51d/job-output.txt.gz for example | |
| 14:44:26 | mriedem | idk, i don't see the lvm volume driver setting connection_info['volume_id'] | |
| 14:45:33 | lyarwood | mriedem: right, anyway, ignore that, I'll look into how these actually passed now, it doesn't change the fix and lack of unit tests | |
| 14:46:01 | lyarwood | mriedem: happy to take over the updating the fix if you don't have time today btw | |
| 14:46:10 | lyarwood | mriedem: this is my mess, I should really be the one to clean it up | |
| 14:47:04 | mriedem | i've got it, just updating test_pre_live_migration_vol_backed_works_correctly_mocked so it doesn't fake out the bdm stuff that the compute manager is doing, | |
| 14:47:13 | mriedem | i.e. actually going to make that test go through driver.get_block_device_info | |
| 14:47:42 | mriedem | bauwser: question in https://review.openstack.org/#/c/540168/ | |