| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-04 | |||
| 19:30:37 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Functional tests for NUMA live migration https://review.opendev.org/672595 | |
| 19:30:37 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Deprecate CONF.workarounds.enable_numa_live_migration https://review.opendev.org/640021 | |
| 19:31:51 | artom | dansmith, ^^ | |
| 19:32:20 | artom | (FWIW, I had the -patch27 in my local branch name for some reason, fixed that) | |
| 19:36:16 | dansmith | artom: one more thing I didn't catch last time, but it's easy | |
| 19:36:28 | dansmith | artom: otherwise I think it looks good, probably time for mriedem to have a go at it | |
| 19:41:07 | aspiers | can anyone point me at where the fake server instance object comes from in https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/compute/test_migrate_server.py#L579 ? | |
| 19:41:18 | aspiers | I've trawled through the test setup code and can't find it | |
| 19:43:04 | aspiers | ah | |
| 19:43:12 | aspiers | admin_only_action_common.py | |
| 19:53:09 | aspiers | still don't get it | |
| 19:53:38 | aspiers | if I use _stub_instance_get() then I get "Exception: This test uses methods that set internal oslo_db state, but it does not claim to use the database. This will conflict with the setup of tests that do use the database and cause failures later." | |
| 19:59:20 | melwitt | aspiers: is there a link to a patch you're working on that will give more context? my first guess is maybe your new test is accessing an attribute on instance that isn't populated, so it's trying to lazy-load it from the database, which is flagging your test as USES_DB = True | |
| 19:59:27 | openstackgerrit | Merged openstack/nova master: Remove unused methods https://review.opendev.org/679918 | |
| 20:00:17 | aspiers | melwitt: I'll upload | |
| 20:01:06 | melwitt | that error comes from a test inheriting from NoDBTestCase and then accessing the database | |
| 20:05:16 | aspiers | melwitt: does this test avoid it by patching out the meat of the code path (i.e. resize)? https://github.com/openstack/nova/blob/master/nova/tests/unit/api/openstack/compute/test_migrate_server.py#L124 | |
| 20:05:34 | aspiers | so it's effectively only testing the HTTP stuff in the top layer? | |
| 20:07:06 | melwitt | probably, that stub will fill in most of the instance objects attributes. so if anything in the code path accesses the attributes, they are already filled in, | |
| 20:07:42 | melwitt | with objects, if an attribute is *not* filled in and it is accessed, the object will fall back on a read from the database to get the value | |
| 20:08:07 | openstackgerrit | Merged openstack/nova master: Convert nova-lvm job to zuul v3 https://review.opendev.org/674345 | |
| 20:08:29 | melwitt | so it will test the full code path, just not the database accesses (if there were going to be any) | |
| 20:08:30 | efried | sean-k-mooney: in case this doesn't show up in your inbox/dashboard/whatever, would you please respond to this os-vif release patch: https://review.opendev.org/#/c/680094/ | |
| 20:09:10 | aspiers | melwitt: ahhh maybe that's it | |
| 20:10:09 | aspiers | melwitt: maybe it's missing the flavor or image_meta | |
| 20:10:39 | melwitt | aspiers: so sometimes what happens is you're working on a new test whose code path will access an attribute that's not stubbed out/filled in, and then it falls back on a read from the db, and if you're inherited from NoDBTestCase, that error will raise | |
| 20:11:18 | aspiers | no, just adding a call to self._stub_instance_get() is enough to trigger this db-related failure, *even* if I don't use the returned fake instance | |
| 20:11:31 | aspiers | so it's not my test code which is triggering the error | |
| 20:11:54 | melwitt | yeah if you look in https://github.com/openstack/nova/blob/master/nova/tests/unit/fake_instance.py#L45 you can see what's been filled in | |
| 20:11:59 | aspiers | yeah I spotted that | |
| 20:12:04 | melwitt | oh, hm | |
| 20:12:12 | aspiers | but this makes no sense, because other tests happily call that same method | |
| 20:13:40 | aspiers | melwitt: uploading now | |
| 20:13:52 | openstackgerrit | Adam Spiers proposed openstack/nova master: Reject live migration and suspend on SEV guests https://review.opendev.org/680158 | |
| 20:13:56 | aspiers | there ^^^ | |
| 20:14:40 | aspiers | melwitt: https://review.opendev.org/#/c/680158/1/nova/tests/unit/api/openstack/compute/test_migrate_server.py@592 causes this db error | |
| 20:14:51 | aspiers | why do other identical lines in the same file not cause it? | |
| 20:16:22 | melwitt | at a glance, I notice the other tests calling _stub_instance_get are also mocking the live_migrate method in compute_api | |
| 20:16:35 | aspiers | right, that's what I was saying earlier | |
| 20:16:43 | aspiers | they mock the meat of the code path | |
| 20:17:19 | aspiers | but the call to _stub_instance_get() isn't inside the mocking?! | |
| 20:17:49 | aspiers | so it can't be that | |
| 20:17:57 | aspiers | super weird | |
| 20:18:06 | openstackgerrit | Adam Spiers proposed openstack/nova master: Reject live migration and suspend on SEV guests https://review.opendev.org/680158 | |
| 20:18:28 | melwitt | you're saying the _stub_instance_get call itself will not return and raises that error? | |
| 20:18:33 | aspiers | yes | |
| 20:18:39 | melwitt | O.o | |
| 20:18:53 | melwitt | ok, that is unexpected | |
| 20:19:24 | aspiers | oh no wait | |
| 20:19:32 | aspiers | OK I get it now | |
| 20:19:42 | aspiers | _stub_instance_get() has side effects of course | |
| 20:19:47 | aspiers | it patches stuff | |
| 20:19:53 | aspiers | so it doesn't matter if I don't use the return value | |
| 20:20:55 | aspiers | yeah it's the lookup of instance.image_meta which fails | |
| 20:21:33 | mriedem | dansmith: artom: ack, probably going to have to be in the morning at this point, just got off a call | |
| 20:21:42 | melwitt | aspiers: where's the lookup? I'm curious | |
| 20:22:04 | aspiers | melwitt: https://review.opendev.org/#/c/680158/2/nova/compute/api.py@218 | |
| 20:22:13 | aspiers | melwitt: I think my mocking of that hardware method is broken probably | |
| 20:22:24 | aspiers | mocking in the wrong place is my guess | |
| 20:22:44 | aspiers | need to mock in nova.compute.api | |
| 20:22:54 | melwitt | ok, I thought you said _stub_instance_get was blowing up itself | |
| 20:22:59 | aspiers | yeah I was wrong | |
| 20:23:03 | melwitt | and I was curious where in there it accesses db | |
| 20:23:04 | melwitt | ok | |
| 20:24:44 | aspiers | ohh doh | |
| 20:25:05 | aspiers | patching hardware.get_mem_encryption_constraint doesn't stop the calculation of the parameters being passed to it | |
| 20:25:16 | aspiers | so I have to patch instance.image_meta too, or just set it | |
| 20:31:29 | dansmith | mriedem: np | |
| 20:41:39 | aspiers | melwitt: got it working finally | |
| 20:41:48 | aspiers | melwitt: thanks for your help :) | |
| 20:41:51 | melwitt | yay | |
| 20:43:03 | openstackgerrit | Merged openstack/nova master: Modernize nova-lvm job https://review.opendev.org/674356 | |
| 20:45:55 | openstackgerrit | Adam Spiers proposed openstack/nova master: Reject live migration and suspend on SEV guests https://review.opendev.org/680158 | |
| 20:46:17 | aspiers | melwitt: https://review.opendev.org/#/c/680158/2..3/nova/tests/unit/api/openstack/compute/test_suspend_server.py@47 | |
| 20:47:09 | openstackgerrit | Adam Spiers proposed openstack/nova master: Enable booting of libvirt guests with AMD SEV memory encryption https://review.opendev.org/666616 | |
| 20:48:10 | melwitt | aspiers: oh, so you didn't need to stub compute_api | |
| 20:48:19 | aspiers | melwitt: right, it was just image_meta | |
| 20:48:43 | aspiers | I didn't want to stub compute_api because that's where the desired exception gets thrown | |
| 20:49:15 | melwitt | yeah, makes sense | |
| 20:55:27 | openstackgerrit | Matt Riedemann proposed openstack/nova master: FUP for I66d8f06f19c5c631e33208580428aa843abb38d2 https://review.opendev.org/678951 | |
| 20:57:14 | aspiers | oh crap, circular imports | |
| 21:01:38 | aspiers | what's the ASCII smiley for a facepalm? | |
| 21:09:34 | aspiers | efried: make sense if I move sev_enabled() from libvirt.utils to virt.hardware to break the circular import? | |
| 21:10:04 | efried | aspiers: without looking specifically at the patch, that sounds like a reasonable thing. | |
| 21:10:17 | aspiers | I don't think it's possible to move get_mem_encryption_constraint() and its helpers in the other direction | |
| 21:10:24 | aspiers | which is a shame, cause they're libvirt-specific | |
| 21:10:29 | efried | except that I still resent that nova.virt.hardware is libvirt-specific | |
| 21:10:35 | aspiers | right | |
| 21:10:36 | efried | yeah | |
| 21:10:42 | aspiers | thing is, they depend _get_flavor_image_meta which is shared | |
| 21:10:43 | efried | that ship has sailed though. | |
| 21:10:49 | aspiers | OK | |
| 21:11:06 | aspiers | efried: I suggested to kashyap yesterday that we actually move the machine type stuff to a separate machine_types file | |
| 21:11:30 | aspiers | libvirt.utils is a kinda dumb bag of random shit | |
| 21:11:34 | efried | if you're just grasping for shit to do, you could factor the libvirt-specific stuff out of nova.virt.hardware and into a nova.virt.libvirt.hardware module, leaving anything that's actually generic (assuming there is anything) in nova.virt.hardware. | |
| 21:12:06 | efried | but we probably have more important things to do | |
| 21:12:13 | aspiers | but I dunno, maybe I can | |
| 21:12:33 | aspiers | however I'm very conscious that once this runway window closes, that's my last chance to land SEV in Train gone | |
| 21:13:11 | aspiers | anyway yet again it's 10pm and I haven't had dinner | |
| 21:13:17 | aspiers | time to tear myself away | |