| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-21 | |||
| 16:49:11 | mriedem | which is where i noticed it while debugging a cold migration test today | |
| 16:52:25 | kashyap | Hmm. So it is the actual guest | |
| 16:53:16 | kashyap | Damned if I could put a finger on why. Let me ask the libvirt folks, if they can think of anything | |
| 16:53:45 | kashyap | Wish I could reproduce the issue. Damn sure it won't if I try locally | |
| 16:54:47 | kashyap | < aw> kashyap: is it the VM or the guest OS? I've had fedora fail to shutdown in a timely way for all sorts of reasons, some that I still haven't figured out | |
| 16:54:47 | kashyap | One dev asks: | |
| 16:55:14 | mriedem | don't know, but these test vms in the gate runs are just cirros images | |
| 16:55:19 | kashyap | (I've told it's CirrOS; and it's the VM, aka. QEMU process) | |
| 17:01:49 | openstackgerrit | Elod Illes proposed openstack/nova stable/ocata: Fix incompatible version handling in BuildRequest https://review.opendev.org/660456 | |
| 17:02:00 | kashyap | Can anyone please remind, where is the guest boot log from the serial console here: http://logs.openstack.org/56/656656/12/check/nova-multi-cell/6a403ba/compute1/logs/ | |
| 17:09:07 | sean-k-mooney | i think i just spent an hour debuging a unit test failure because teh unit tests in osc are wrong an i coppied them to create my own... | |
| 17:15:21 | sean-k-mooney | nope i am wrong it was 2 hours but now they pass | |
| 17:30:31 | sean-k-mooney | aspiers: mriedem i just updated https://review.opendev.org/#/c/643578/ if there are any other changes you would like let me know with a comment :) | |
| 17:30:44 | aspiers | sean-k-mooney: ok will take a look thanks! | |
| 17:53:28 | aspiers | efried / anyone got any thoughts on whether it still makes sense to use nova.virt.libvirt.utils.file_open for mocking file opens? | |
| 17:53:42 | openstackgerrit | Merged openstack/nova master: Add detection of SEV support from QEMU/AMD-SP/libvirt on AMD hosts https://review.opendev.org/633855 | |
| 17:53:50 | openstackgerrit | Merged openstack/nova master: Extract provider tree functional tests into new file. https://review.opendev.org/660157 | |
| 17:54:02 | aspiers | \o/ first real SEV patch lands in nova! | |
| 17:55:15 | aspiers | file_open() was added in 2011 by Soren with the original framework for testing the libvirt driver | |
| 17:55:57 | aspiers | https://opendev.org/openstack/nova/commit/bb622e6d7c921894fd0e7697a0003630989d4f35 | |
| 17:56:25 | aspiers | he wrote "I hope eventually to make it similar to fakelibvirt in style (e.g. keep track of files created and deleted and attempts to open a file that it doesn't know about, you'll get proper exceptions with proper errnos set and whatnot)." | |
| 17:57:06 | aspiers | meanwhile, a ton of other tests just use mock.mock_open() | |
| 17:57:21 | aspiers | sean-k-mooney: any thoughts? | |
| 18:00:16 | efried | aspiers: The reason it exists is so that fake_libvirt_utils can mock it | |
| 18:00:24 | efried | and fake_libvirt_utils is dying in a fire. | |
| 18:00:29 | efried | https://review.opendev.org/#/c/642558/8 | |
| 18:00:48 | efried | IMO there's no reason for it to exist; we should just use mock_open | |
| 18:00:49 | aspiers | efried: my point was that a whole bunch of stuff needs to (and does) mock open, not just that | |
| 18:00:57 | aspiers | and it seems we have two different ways of doing it right now | |
| 18:01:01 | efried | yup | |
| 18:01:18 | efried | If you want to do the cleanup to kill file_open, I would support that. | |
| 18:01:42 | efried | meanwhile, definitely don't do anything that relies of fake_libvirt_utils.file_open | |
| 18:01:53 | aspiers | file_open seems much less popular right now than mock_open | |
| 18:01:55 | efried | and save yourself having more cleanup by avoiding mocking file_open - just mock open. | |
| 18:02:00 | efried | yeah. | |
| 18:02:15 | efried | Nice intention, but clearly nobody in the last eight years has cared enough to make it go. | |
| 18:02:23 | aspiers | having said that, Soren's original intention does make sense ... right | |
| 18:02:50 | efried | Just be wary of side effects. I had a situation recently where mocking open at the test method level was too broad and effed up, like, a dynamic import or something. | |
| 18:03:23 | aspiers | but file_open is actually also defined in nova/virt/libvirt/utils.py | |
| 18:03:40 | efried | if f == 'thing_i_care_about': | |
| 18:03:40 | efried | def fake_open(f, ...): | |
| 18:03:40 | efried | orig_open = builtins.open | |
| 18:03:40 | efried | so I had to do the | |
| 18:03:41 | efried | return orig_open(f, ...) | |
| 18:03:41 | efried | else: | |
| 18:03:41 | efried | do_the_mock_thing | |
| 18:03:42 | efried | thing. | |
| 18:04:18 | aspiers | isn't that what mock_open is supposed to help avoid? | |
| 18:04:22 | efried | yes, it's defined in fake_libvirt_utils because it was defined in libvirt.utils and apparently whoever set that up didn't know how to mock (or maybe mock wasn't properly powerful back then, dunno). | |
| 18:05:03 | efried | aforementioned series is getting rid of fake_libvirt_utils and just mocking the utils directly. | |
| 18:05:14 | aspiers | ah | |
| 18:05:24 | aspiers | makes sense, since a bunch of stuff is doing @mock.patch('nova.virt.libvirt.utils.file_open', ... | |
| 18:05:40 | aspiers | I'll stare at that review until I understand it | |
| 18:06:38 | efried | aspiers: re "isn't that what mock_open is supposed to help avoid?" -- not that I know of. Is there a way to make the mock automatically trigger conditionally based on the inputs? | |
| 18:07:07 | aspiers | I *think* there is, IIRC | |
| 18:07:17 | aspiers | I may have even done it recently, but would have to check | |
| 18:07:48 | efried | I thought mock_open was just good for abstracting the context manager awkwardness and providing an easy way to specify "output". | |
| 18:08:08 | efried | I don't see any conditionally-use-the-real-open mentioned in https://docs.python.org/3.3/library/unittest.mock.html#mock-open | |
| 18:10:03 | aspiers | Yeah I'm probably thinking of something else | |
| 18:11:02 | aspiers | mock is very powerful, but I find the API pretty damn confusing | |
| 18:11:15 | aspiers | I don't think it's the best designed API in the world | |
| 18:11:33 | aspiers | rspec is quite a bit easier to use | |
| 18:13:48 | aspiers | efried: maybe I was just thinking of side_effect, which would presumably at least avoid the need for orig_open | |
| 18:13:52 | efried | aspiers: We should totally use mox | |
| 18:14:29 | efried | I don't see how side_effect would avoid needing orig_open | |
| 18:14:54 | aspiers | Ah, yeah maybe not | |
| 18:15:01 | efried | side_effect lets you replace the generic no-op method with a method of your own design. | |
| 18:15:17 | efried | that method would still have to have a condition to orig_open if that was a thing you needed. | |
| 18:15:36 | efried | no need for that, this is good discussion. | |
| 18:15:45 | aspiers | ;) | |
| 18:15:48 | efried | helps clarify it in my mind if nothing else | |
| 18:15:59 | aspiers | I was thinking that inside the side_effect context, the built-in open would still be available | |
| 18:16:05 | aspiers | but maybe it's not | |
| 18:16:05 | efried | ah. Yeah, no. | |
| 18:16:17 | efried | you would recurse into your mock. | |
| 18:16:21 | aspiers | Right | |
| 18:16:28 | efried | and then other kinds of curse | |
| 18:16:37 | aspiers | That's necessary to support re-entrancy cases, I guess | |
| 18:16:52 | efried | also because it was the simplest to implement :) | |
| 18:18:03 | aspiers | I guess you were already well aware of https://governance.openstack.org/tc/goals/rocky/mox_removal.html | |
| 18:18:35 | aspiers | I never tried mox, so it's a shame if that's nicer to work with, but I guess consistency trumps niceness | |
| 18:20:20 | efried | aspiers: I was totally kidding. We've been working on removing mox for years now. | |
| 18:20:39 | efried | I find it very difficult to understand, but that's probably mostly because I'm not used to it. | |
| 18:21:03 | efried | weird method-chain-y way of setting things up | |
| 18:21:25 | efried | method chaining and pep8 don't mix very well | |
| 18:22:38 | aspiers | Oh right :) | |
| 18:31:44 | mriedem | sean-k-mooney: i've noticed that the ksa only client stuff we're doing with port bindings in the nova/network/neutronv2/api.py stuff is not propagating the request id into the neutron requests which makes correlating the nova and neutron logs tricky | |
| 18:36:17 | mriedem | https://bugs.launchpad.net/nova/+bug/1829914 | |
| 18:36:19 | openstack | Launchpad bug 1829914 in OpenStack Compute (nova) "nova context request_id is not propagated for port binding operations in neutron" [Medium,Triaged] | |
| 18:36:22 | aspiers | efried: since you mentioned the idea of partial patching with pass-through to the original function in the cases which don't match specific parameters, what's the rationale for doing that? | |
| 18:36:34 | aspiers | efried: for example I see it here: https://opendev.org/openstack/nova/src/branch/master/nova/tests/unit/virt/libvirt/test_driver.py#L18199 | |
| 18:37:35 | aspiers | but if a test ends up hitting code which tests for file existence on the test system, shouldn't the test bomb out rather than accidentally succeed just because the original function luckily returns the right thing to make the test pass? | |
| 18:38:03 | aspiers | AFAICS that would apply for both os.path.exists and file.open | |
| 18:39:22 | aspiers | e.g. I want to mock the result of os.path.exists('/sys/module/kvm_amd/parameters/sev'), but if the code happens to also run os.path.exists('something/else') then that should be mocked too | |
| 18:44:16 | aspiers | I suppose assert_called_once_with() will take care of that | |
| 18:48:34 | aspiers | ohhhh, it needs to be able to find stuff like placement-policy.yaml | |
| 18:52:26 | aspiers | OK, so I need to reuse patch_exists() which is currently in TestGuestConfigSysinfoSerialOS | |
| 18:53:29 | aspiers | maybe I can move it to nova.test.TestCase | |
| 18:55:00 | mriedem | sean-k-mooney: it looks like the cross-cell resize revert cold migration flow is failing because when reverting on the dest host, we first delete the active dest host port binding and then on the source host, we update the port's binding:host_id to point at the source host, but that fails in neutron with a not found error i think because it's complaining that the dest host port binding is already gone | |
| 18:55:07 | mriedem | which seems weird, i'm not sure why it would care | |