| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-09 | |||
| 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... | |
| 16:43:51 | mriedem | Configured mechanism driver names: ['openvswitch', 'linuxbridge'] | |
| 16:43:58 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Detach volumes on _rebuild_default_impl failure https://review.openstack.org/442105 | |
| 16:43:58 | mriedem | so maybe just the defaults for ML2? | |
| 16:45:17 | lyarwood | stephenfin / mdbooth; if you have any review time left today https://review.openstack.org/#/c/460243/ has been respun to include the fix from https://review.openstack.org/#/c/531233/ | |
| 16:46:38 | esberglu | mriedem: That's just running the CI we've been using since last release with no networking implementation | |
| 16:46:55 | esberglu | About to merge a patch that fixes SEA CI | |
| 16:48:18 | efried | esberglu IOW, neither network patch is yet running a CI that tests networking, but soon the SEA patch will run CI with SEA networking? | |
| 16:48:40 | efried | esberglu And... if we were to run the OVS patch once that is done, it would what, fail? | |
| 16:49:11 | esberglu | efried: No it would pass, but it would be testing SEA networking | |
| 16:49:24 | efried | esberglu How, without the SEA patch? | |
| 16:49:41 | esberglu | efried: PowerVM is running SEA CI on all nova changes as of yesterday | |
| 16:49:54 | efried | esberglu How, without the SEA patch? | |
| 16:50:07 | esberglu | It is applying the SEA patch | |