| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-29 | |||
| 11:04:40 | mdbooth | I think it inherits at some point | |
| 11:04:51 | aspiers | testtools>=2.2.0 | |
| 11:04:56 | aspiers | in test-requirements.txt | |
| 11:05:17 | aspiers | https://testtools.readthedocs.io/en/latest/for-test-authors.html#improved-assertraises | |
| 11:05:29 | aspiers | "Note that this is incompatible with the assertRaises in unittest2 and Python2.7." | |
| 11:06:33 | mdbooth | kashyap: Sec... | |
| 11:06:42 | aspiers | I would try the approach in that link | |
| 11:06:47 | aspiers | which is basically what mdbooth suggested | |
| 11:06:53 | kashyap | mdbooth: Nod | |
| 11:07:21 | aspiers | I suspect his suggestion only works *because* we are using testtools not native unittest | |
| 11:07:30 | aspiers | but yeah, we'll see in a few seconds :) | |
| 11:07:47 | mdbooth | ...works | |
| 11:07:49 | mdbooth | code is: | |
| 11:07:51 | aspiers | \o/ | |
| 11:08:01 | mdbooth | + raised.get_error_code()) | |
| 11:08:01 | mdbooth | + self.assertEqual(fakelibvirt.VIR_ERR_SYSTEM_ERROR, | |
| 11:08:01 | mdbooth | instance) | |
| 11:08:01 | mdbooth | ~ raised = self.assertRaises(fakelibvirt.libvirtError, drvr._destroy, | |
| 11:08:22 | mdbooth | you can add more | |
| 11:08:37 | mdbooth | ^^^ tested on py(2|3)7 | |
| 11:09:37 | kashyap | mdbooth: Thanks! Let me try | |
| 11:12:00 | kashyap | mdbooth: That get_error_code() needs a mock, no? | |
| 11:12:23 | kashyap | @mock.patch.object(fakelibvirt.libvirtError, 'get_error_code') | |
| 11:12:26 | mdbooth | kashyap: No | |
| 11:12:56 | mdbooth | kashyap: I didn't look, but it must already be implemented by fake libvirtError | |
| 11:13:16 | mdbooth | It certainly works and returns the expected value | |
| 11:19:51 | kashyap | mdbooth: You're of course correct | |
| 11:27:28 | kashyap | mdbooth: Before I take CI resources, does it look OK to you: http://paste.openstack.org/show/752232/ | |
| 11:30:31 | kashyap | aspiers: BTW, on self.assertTrue(mock_warning.called) vs. mock_warning.assert_called_once() -- I went with the former as that seemed to be the "pattern" | |
| 11:30:53 | aspiers | OK | |
| 11:31:01 | aspiers | I don't really mind much either way | |
| 11:31:26 | aspiers | Latter feels a bit more idiomatic to me (after all, presumably they implemented it for a reason) but whatever :) | |
| 11:31:57 | kashyap | Yeah, I see what you mean, though. I like the latter less obtuse | |
| 11:46:24 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Rework 'EBUSY' (SIGKILL) error handling code path https://review.opendev.org/639091 | |
| 11:50:01 | kashyap | aspiers: BTW, I fully share your horror of super-long methods; I just made an elaborate TODO to address it into 3 other refactor commits, based on your remarks. | |
| 11:52:46 | cdent | (super-long {methods,packages,tests,files,documents})-- | |
| 11:53:04 | kashyap | cdent: :-) | |
| 11:53:26 | cdent | but | |
| 11:53:31 | cdent | (super-log emails)++ ;) | |
| 11:53:39 | cdent | damn: long! | |
| 11:54:10 | kashyap | mdbooth: Hopefully I addressed "all things", including Adam's nits. Hope that one test suffices? Or do we need _another_ test? (I don't want to add more without a very good reason) | |
| 12:34:05 | openstackgerrit | François Palin proposed openstack/nova stable/rocky: Include all network devices in nova diagnostics https://review.opendev.org/661962 | |
| 12:40:09 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Don't version links to reno docs https://review.opendev.org/661967 | |
| 12:52:48 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Rework 'EBUSY' (SIGKILL) error handling code path https://review.opendev.org/639091 | |
| 12:55:49 | kashyap | mdbooth: Addressed the two nits ^^; can I have my +1 back? :D | |
| 12:55:51 | kashyap | (Thanks!) | |
| 12:57:44 | artom | sean-k-mooney, tempest-full-py3/nova-multinode all passed on top of https://review.opendev.org/#/c/644881/18 | |
| 12:58:17 | artom | Need to fix unit tests now | |
| 12:58:42 | artom | Unless mriedem or dansmith want to sanity check the approach to make sure it's acceptable first | |
| 12:58:50 | artom | (The revert resize thing) | |
| 13:11:32 | aspiers | kashyap: great :) | |
| 13:12:30 | openstackgerrit | François Palin proposed openstack/nova stable/rocky: Include all network devices in nova diagnostics https://review.opendev.org/661962 | |
| 13:15:12 | sean-k-mooney | artom well its a race or was so you might want to do a few rechecks to confirm but cool ill take a look | |
| 13:16:24 | sean-k-mooney | artom: although im also plannign to change how hybrid plug works to fix a different issue... | |
| 13:17:58 | sean-k-mooney | i need to get back to working on this https://review.opendev.org/#/c/602432/ but if i end up altering the beahvior i geuss we could revert or update your change if hybrid plug nolonger matters | |
| 13:18:53 | sean-k-mooney | this is actully next on my todo list once i get the spec im working on finished | |
| 13:19:13 | kashyap | mdbooth: Haha, you caught me sneaking in the \d+ | |
| 13:19:45 | mdbooth | kashyap: It makes no difference :) | |
| 13:20:31 | kashyap | Yeah, indeed | |
| 13:20:49 | mdbooth | i.e. I don't care if you don't change it | |
| 13:24:44 | artom | sean-k-mooney, I don't think it was a race in this case | |
| 13:24:58 | artom | We were blocking on the wait before Neutron could ever send us the event | |
| 13:25:11 | artom | When waiting in the compute manager | |
| 13:25:33 | HD|Laptop | hello all. I have an ISO image (with drivers) imported with --container-format bare --disk-format iso. | |
| 13:25:35 | sean-k-mooney | but only when using a speicic firewall driver | |
| 13:25:36 | artom | But yeah, a few rechecks won't harm - they'll happen regardless when I fix the unit tests | |
| 13:25:59 | HD|Laptop | Starting from ISOs with server create --image <OS ISO image> works fine and the ISO gets mapped in qemu-kvm as a CDROM drive | |
| 13:26:41 | HD|Laptop | but when trying to attach an ISO via volume, it gets mapped as "-device virtio-blk-pci" despite setting hw_cdrom_bus='ide', hw_disk_bus='ide' on both image and volume | |
| 13:27:04 | HD|Laptop | how do I force the volume with the driver ISO to be mapped as -device ide-cd? | |
| 14:03:33 | mriedem | bauzas: can you take a look at this functional regression recreate test patch? https://review.opendev.org/#/c/661822/ | |
| 14:03:38 | mriedem | related to your favorite, request spec | |
| 14:17:25 | stephenfin | mriedem: You think we can push ahead with this now? https://review.opendev.org/#/c/629597/ | |
| 14:20:19 | stephenfin | efried: Easy docs fix here that'll need to be backported after https://review.opendev.org/661967 | |
| 14:22:28 | efried | stephenfin: +2 | |
| 14:23:33 | mriedem | stephenfin: you will have to decouple it from https://review.opendev.org/#/c/620111/ | |
| 14:24:16 | mriedem | which is waiting on how https://review.opendev.org/#/c/637217/ gets sorted out which i'm not actively working on | |
| 14:24:46 | stephenfin | okidok | |
| 14:33:55 | sean-k-mooney | mriedem: johnthetubaguy when one of ye have time can you look at this backport for os-vif | |
| 14:34:15 | sean-k-mooney | we dont use upper constraits on all stable branches so we want to fix that | |
| 14:34:34 | sean-k-mooney | it still need more backports but that one is blocking a different backport too | |
| 14:39:52 | mriedem | stephenfin: do you plan on backporting this? https://review.opendev.org/#/c/661967/ | |
| 14:40:04 | mriedem | sean-k-mooney: did you intend on pasting a link to a backport? | |
| 14:40:12 | stephenfin | mriedem: yeah, once it's merged | |
| 14:40:19 | mriedem | stephenfin: ok, then you should have a bug for it | |
| 14:40:44 | stephenfin | oh yeah, good point that. I can do that now | |
| 14:41:11 | sean-k-mooney | mriedem: yes https://review.opendev.org/#/c/658780/ is the queens backport | |
| 14:41:44 | sean-k-mooney | mriedem: of using upper constaints | |
| 14:42:22 | sean-k-mooney | there is also the backport of https://review.opendev.org/#/q/topic:bug/1825888+(status:open+OR+status:merged) but that needs to be done on stien and rocky first | |
| 14:50:17 | mriedem | sean-k-mooney: https://review.opendev.org/#/c/658780/ looks like it's doing the trick but i can't tell at all from the tox logs, a comment in line if you have the answer | |
| 14:50:35 | mriedem | http://logs.openstack.org/80/658780/4/check/openstack-tox-py35/b70c01b/tox/py35-1.log | |
| 14:50:35 | mriedem | the tox logs make it look like they are installing from master u-c: | |
| 14:50:42 | mriedem | cmd: /home/zuul/src/opendev.org/openstack/os-vif/.tox/py35/bin/pip install -U -c/home/zuul/src/opendev.org/openstack/requirements/upper-constraints.txt -r/home/zuul/src/opendev.org/openstack/os-vif/requirements.txt -r/home/zuul/src/opendev.org/openstack/os-vif/test-requirements.txt | |
| 14:50:52 | mriedem | Collecting oslo.versionedobjects===1.31.3 (from -c /home/zuul/src/opendev.org/openstack/requirements/upper-constraints.txt (line 235)) Downloading http://mirror.bhs1.ovh.openstack.org/pypifiles/packages/fc/55/09c89a3c1724732f435b1639640ac58f4ab485f07c26aa5d1e452633040f/oslo.versionedobjects-1.31.3-py2.py3-none-any.whl (74kB) | |
| 14:51:56 | sean-k-mooney | hum ill take a closer look again. the tox job should overdie the uppercontarints file use to use teh one it cloned by setting the env varible | |
| 14:52:17 | mriedem | it is using the queens constraints, it's just not obvious at all from the logs | |
| 14:52:19 | sean-k-mooney | sphix was the package that was breaking the follow up package | |
| 14:53:23 | sean-k-mooney | it was previously uncpped v7 or this failed but v8 works after it was rebased on the constraitns patch https://review.opendev.org/#/c/655694/8 | |
| 14:54:17 | mriedem | yup i can see http://logs.openstack.org/94/655694/7/check/openstack-tox-py35/c0ad3dc/tox/py35-3.log | |
| 14:55:27 | mriedem | ok +W on the queens uc change | |
| 14:57:24 | sean-k-mooney | thanks when you have time it would be good to merge the bugfix backport too but as i said it still need to be backported on stien and rock + we wont be doing a release for a while so it not urgent | |