| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-09 | |||
| 15:33:37 | mriedem | as for this test https://review.openstack.org/#/c/267587/83/nova/tests/unit/virt/libvirt/test_driver.py@19520 - i can't sort out the global mock nature that makes it fail when running the entire test module | |
| 15:33:56 | mriedem | https://review.openstack.org/#/c/267587/83/nova/tests/unit/virt/libvirt/test_driver.py@19520 is a challenge for mdbooth | |
| 15:35:21 | mdbooth | mriedem: Have you done the bisect thing? | |
| 15:35:25 | mriedem | mdbooth: nope | |
| 15:35:45 | mdbooth | I believe the instructions were originally written by sdague | |
| 15:35:46 | mriedem | i thought that splitting the tests out into it's own class would help so they weren't running with the other tests in LibvirtConnTestCase but that didn't help | |
| 15:36:49 | mdbooth | mriedem: This one: https://wiki.openstack.org/wiki/Testr#Reproducing_Failures | |
| 15:37:39 | mriedem | oh cool, i'll try that | |
| 15:37:41 | mdbooth | The output of that process is a minimal test ordering which causes the failure. | |
| 15:37:49 | mdbooth | Then you just squint at it for a bit. | |
| 15:38:09 | mriedem | johnthetubaguy: can you drop the -2 on this? https://review.openstack.org/#/c/140733/ | |
| 15:38:47 | mriedem | johnthetubaguy: also, i plumbed in the multiattach API functional changes last night - https://review.openstack.org/#/c/271047/ - still need to cleanup some test fallout from some refactor that was needed, but it should be ok to start reviewing the flow | |
| 15:38:48 | johnthetubaguy | mriedem: done | |
| 15:39:09 | mriedem | you have to use a new microversion to bfv or attach with a multiattach volume, | |
| 15:39:11 | johnthetubaguy | mriedem: ah cool, I am chasing some ironic traits stuff today, but will try get to that soon | |
| 15:39:16 | mriedem | and then we have min compute version checks and such | |
| 15:39:27 | johnthetubaguy | mriedem: all sounds sensible | |
| 15:39:43 | mriedem | it got a bit gross because i had to change a volume_id param to a volume dict in one of the lower down methods | |
| 15:40:03 | mriedem | to avoid multiple volume GET calls | |
| 15:41:30 | ildikov | mriedem: johnthetubaguy: the functional tests run into a KeyError on the volume which should be an easy fix if I can finally find the spot where it actually goes wrong... :) | |
| 15:41:49 | mriedem | ildikov: it's probably a GET response from the CinderFixture | |
| 15:41:55 | ildikov | mriedem: was it multiple GET calls? | |
| 15:42:09 | ildikov | mriedem: the fakes returns a volume dict with 'multiattach' | |
| 15:42:09 | breton | hi | |
| 15:42:24 | ildikov | mriedem: or I found another volume dict that contained it... | |
| 15:42:32 | mriedem | ildikov: https://github.com/openstack/nova/blob/master/nova/tests/fixtures.py#L1332 | |
| 15:42:38 | mriedem | ^ doesn't contain multiattach=False | |
| 15:43:02 | mriedem | nor do the ones below | |
| 15:43:15 | mriedem | same thing in CinderFixtureNewAttachFlow | |
| 15:43:57 | mriedem | ildikov: i could have made the API code a lot of easier on tests if i just did volume.get('multiattach') but that's a cop out since the volume response from cinder's API should always have that key set | |
| 15:44:38 | breton | Is liberty affected by https://bugs.launchpad.net/nova/+bug/1664931? Is there any sense in porting https://review.openstack.org/#/c/519684/7 ? | |
| 15:44:40 | openstack | Launchpad bug 1664931 in OpenStack Compute (nova) pike "[OSSA-2017-005] nova rebuild ignores all image properties and scheduler filters (CVE-2017-16239)" [High,Fix committed] - Assigned to Matt Riedemann (mriedem) | |
| 15:45:09 | ildikov | mriedem: the test calls this: https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/fakes.py#L618 | |
| 15:45:16 | mriedem | breton: the OSSA-2017-005 calls out the affected versions | |
| 15:45:27 | ildikov | mriedem: which then goes here: https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/fakes.py#L568 | |
| 15:45:52 | mriedem | ildikov: i thought you were asking about functional tests | |
| 15:46:16 | mriedem | https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/fakes.py#L586 should have probably never been True | |
| 15:46:39 | breton | mriedem: so do i understand it right that all versions <=14.0.10 are affected? | |
| 15:46:43 | ildikov | mriedem: I do | |
| 15:47:35 | ildikov | mriedem: https://github.com/openstack/nova/blob/master/nova/tests/functional/api_sample_tests/test_volumes.py#L247 | |
| 15:48:22 | ildikov | mriedem: and just the old flow tests fail | |
| 15:48:41 | mriedem | breton: https://github.com/openstack/nova/commit/d6ca1cc02ab58615a6bb1b337db34dd06525112e introduced the regression, | |
| 15:48:42 | breton | mriedem: i am unsure because in liberty request specs were not used for instance rebuild | |
| 15:48:45 | mriedem | in 12.0.0 which was liberty | |
| 15:48:59 | ildikov | mriedem: and I'm surely just blind and hate all the fake stuff... | |
| 15:49:38 | mriedem | ildikov: oh i didn't realize the functional tests were using unit test stubs.... | |
| 15:49:46 | mriedem | they should be using the CinderFixture, but that's not a change to make here | |
| 15:50:12 | ildikov | I got surprised too :) | |
| 15:50:54 | breton | mriedem: understood, thanks | |
| 15:51:41 | mriedem | ildikov: https://github.com/openstack/nova/blob/master/nova/tests/functional/api_sample_tests/test_volumes.py#L368 | |
| 15:51:54 | mriedem | the 2.49 test is using the CinderFixture as well as the fakes/stubs | |
| 15:52:14 | mriedem | and the fixture is loaded after the stubs in the parent class setup | |
| 15:52:19 | mriedem | so the fixture overrides the GET call | |
| 15:52:24 | mriedem | and the fixture doesn't have multiattach in the response | |
| 15:52:30 | mriedem | so just fix the fixtures since you have to do that anyway | |
| 15:52:49 | ildikov | the test seemed to call it explicitely | |
| 15:52:54 | ildikov | anyway, will fix that | |
| 15:53:18 | mriedem | the test calling fakes.stub_volume_get and the actual api runtime code calling it via the fixture are different thigns | |
| 15:53:19 | mriedem | *things | |
| 15:53:51 | ildikov | or in other words it's a mess :) | |
| 15:54:19 | mriedem | yeah, the functional tests shouldn't be using stubs from the unit tests, they should use the fixture long-term | |
| 15:54:32 | mriedem | it's ultimately trading one set of stubs for another | |
| 15:57:19 | smcginnis | mriedem: Are these Cinder or Nova tests you are referring to? | |
| 15:57:26 | mriedem | nova | |
| 15:57:44 | Roamer` | johnthetubaguy, now that mriedem seems to be kinda sorta okay with the StorPool libvirt volume driver (thanks!) and it passed the Zuul checks (yay!), could you remove your procedural -2? thanks in advance! | |
| 15:57:52 | smcginnis | OK, good. | |
| 15:57:56 | mriedem | Roamer`: he already did | |
| 15:58:20 | Roamer` | mriedem, johnthetubaguy, ahhhhh, right... he did indeed... thanks! | |
| 15:58:26 | Roamer` | (today is NOT my day for reading and parsing stuff) | |
| 16:02:19 | ildikov | smcginnis: got scared for a moment? :) | |
| 16:04:38 | smcginnis | ildikov: Yep. ;) | |
| 16:04:59 | smcginnis | ildikov: Our tests already need a lot of work. I was just hoping that it wasn't worse than I already knew. | |
| 16:05:24 | ildikov | smcginnis: I know that part :) | |
| 16:05:39 | ildikov | smcginnis: but you know no one is perfect, even Nova has it's flows :) | |
| 16:05:46 | ildikov | s/it's/its/ | |
| 16:05:53 | smcginnis | ildikov: Shh, you can't say that over here. :D | |
| 16:06:37 | ildikov | smcginnis: they all know I'm just expressing my love :) | |
| 16:11:12 | tovin07 | mriedem, can you take a quick look at this small fix (already had +2 from stephenfin): https://review.openstack.org/#/c/519664/ | |
| 16:13:19 | stephenfin | mriedem: Done (the release note) | |
| 16:13:57 | openstackgerrit | Merged openstack/nova master: Move aggregates from report client to ProviderTree https://review.openstack.org/521685 | |
| 16:19:17 | bauzas | is it me or the gate is in the weeds ? | |
| 16:19:36 | bauzas | I'm still waiting a single change to be merged since this morning | |
| 16:26:30 | mriedem | edleafe: comments on the alternate hosts patch for resize https://review.openstack.org/#/c/526436/21 | |
| 16:27:01 | mriedem | bauzas: thankfully you can take this opportunity to review some patches | |
| 16:27:37 | hrw | guys: where 'default' VM is defined? Nova decides what to include or does it just use defaults from libvirt? | |
| 16:27:43 | efried | bauzas Yeah, https://review.openstack.org/#/c/521685/ took 8.5h to merge this morning. | |
| 16:28:18 | hrw | I mean things like networking, graphics, usb, storage. not amount of ram/vcpu/storage | |
| 16:29:13 | openstackgerrit | Merged openstack/nova master: Fix race condition in retrying migrations https://review.openstack.org/531022 | |
| 16:30:15 | bauzas | mriedem: yup, I'm just working on a new PS for the vGPU change, but I hope to look at reviewing changes by tomorrow | |
| 16:32:34 | stephenfin | lyarwood: Have you got this the right way round? https://review.openstack.org/#/c/530744/ | |
| 16:33:47 | stephenfin | Wait - O comes before P | |
| 16:34:27 | lyarwood | :D | |
| 16:37:56 | mriedem | tovin07: done | |
| 16:38:09 | edleafe | mriedem: thx. Found a bug in my own code, so I'm working on fixing it | |
| 16:39:12 | cdent | functional tests love you more | |
| 16:42:18 | mriedem | efried: powervm ci is going to use SEA rather than OVS for networking right? | |
| 16:42:35 | efried | For the immediate future, yes. esberglu ^ | |
| 16:42:52 | mriedem | so what networking is this running against? https://review.openstack.org/#/c/422512/ | |
| 16:42:55 | mriedem | if SEA isn't yet there | |
| 16:43:35 | efried | I forget that answer. esberglu is on the way... | |