| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-21 | |||
| 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 | so I had to do the | |
| 18:03:40 | efried | orig_open = builtins.open | |
| 18:03:40 | efried | def fake_open(f, ...): | |
| 18:03:40 | efried | if f == 'thing_i_care_about': | |
| 18:03:41 | efried | do_the_mock_thing | |
| 18:03:41 | efried | else: | |
| 18:03:41 | efried | return orig_open(f, ...) | |
| 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 | efried | ah. Yeah, no. | |
| 18:16:05 | aspiers | but maybe it's not | |
| 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 | |
| 18:55:28 | mriedem | but we might have the same issue for live migration rollbacks which we don't test in the gate | |
| 18:57:35 | mriedem | i.e. i think we might need https://review.opendev.org/#/c/594139/ | |
| 19:01:41 | openstackgerrit | Dustin Cowles proposed openstack/nova master: WIP: Use SDK instead of ironicclient for add/remove instance info from node https://review.opendev.org/659691 | |
| 19:08:41 | efried | aspiers: Looks like you figured it out, but yeah, sometimes the code you're calling goes off and does things outside of the purview of the thing you're testing, and sometimes those things hit the thing you're mocking, if you're mocking something low-level like open or file existence, so you only want to use your mock in the code paths that you care about, and otherwise leave them unaffected. | |
| 19:08:47 | openstackgerrit | Hamdy Khader proposed openstack/os-vif master: OVS DPDK port representors support https://review.opendev.org/658786 | |
| 19:15:46 | aspiers | efried: yeah, it hadn't occurred to me that os.path.exists() might be called on files which are actually inside the virtualenv, like placement-policy.yaml | |
| 19:16:03 | aspiers | efried: but when I blanket-mocked it, placement blew up in my face :) | |
| 19:16:21 | aspiers | efried: so I'm moving patch_exists() to nova.test.TestCase for reuse | |
| 19:19:07 | efried | ight | |
| 19:21:40 | sean-k-mooney | mriedem: ill take a look. once we have activated the dest binding it should have atomicly delete teh source binding so that makes sense | |
| 19:26:45 | mriedem | i thought activating the dest binding just makes the source binding automatically inactive, but not deleted | |
| 19:26:50 | openstackgerrit | Adam Spiers proposed openstack/nova master: Move patch_exists() to nova.test.TestCase for reuse https://review.opendev.org/660500 | |
| 19:26:52 | aspiers | efried: ^^^^ | |
| 19:30:03 | sean-k-mooney | mriedem: actully looking at the spec https://specs.openstack.org/openstack/neutron-specs/specs/ocata/portbinding_information_for_nova.html#usage-by-nova you are correct but i had a feeling it was different for some reason | |