| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-29 | |||
| 10:59:56 | mdbooth | I hate python | |
| 11:00:00 | aspiers | ;) | |
| 11:00:01 | kashyap | Sigh | |
| 11:00:12 | aspiers | yeah, Python APIs aren't the best | |
| 11:00:19 | aspiers | biab | |
| 11:00:41 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: DNM:test https://review.opendev.org/661931 | |
| 11:01:09 | kashyap | mdbooth: Do you have a preference given that the above goes bonkers on non-PY37? | |
| 11:01:20 | kashyap | s/preference/preference,/ | |
| 11:01:51 | mdbooth | kashyap: My test was bogus... | |
| 11:02:15 | kashyap | Ah | |
| 11:02:28 | aspiers | I figured it out | |
| 11:02:35 | kashyap | Please educate | |
| 11:02:40 | aspiers | with self.assertRaises(ex) as cm: | |
| 11:02:49 | aspiers | ... do assertions on cm.exception ... | |
| 11:02:56 | mdbooth | raised = self.assertRaises(fakelibvirt.libvirtError, ...) | |
| 11:03:12 | mdbooth | self.assertEqual("Foo", raised.foo) | |
| 11:03:36 | aspiers | Oh, context manager only works with >= 3.1 :-/ | |
| 11:03:52 | mdbooth | py27 | |
| 11:04:08 | aspiers | Although IIRC maybe we don't use native py27 unittest? | |
| 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/ | |