Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-21
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
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
19:30:29 mriedem looking at the ml2 plugin code that also looks correct,
19:30:43 mriedem originally i had also assumed the deleting the active binding would automatically re-activate the inactive binding, but that's not the case
19:31:00 mriedem it just deletes the active port binding resource and i think that eventually makes the vif_type=unbound
19:31:14 mriedem which is why i have https://review.opendev.org/#/c/594139/
19:32:00 openstackgerrit Merged openstack/nova stable/stein: Fix retry of instance_update_and_get_original https://review.opendev.org/659317
19:32:04 sean-k-mooney yes if there is no active port binding having it revert to vif_type=unbound make sense
19:32:07 mriedem my guess is there might be a bug in neutron when (1) delete the active binding and then (2) change the port's binding:host_id to a new host (for the inactive binding), it must try to do some cleanup of the old binding and fails b/c it doesn't exist anymore
19:32:51 sean-k-mooney well thats the think i dont think its valide to just update teh inactive binding
19:33:18 sean-k-mooney that part of why the other live migration issue was happening
19:34:01 sean-k-mooney the neutron port bidning api seems to only work if you interact with it exactly as they expect with no auto recovery if we delete or update things
19:34:39 sean-k-mooney the spec didnt say we were allowed to interact with it in thos other ways so its fair but likely it could be made more robust
19:34:47 mriedem yeah maybe, i'd accept that if i opened a neutron bug and that was the response
19:35:00 mriedem or if mlavalle said that was the case
19:36:17 sean-k-mooney i also would not expect much push back if we specified the set of addtion behavior we intuitivly expect to work and proposed them as an extention
19:36:37 mriedem i don't really care about making those edge cases work,
19:36:45 mriedem i'd just like clarity on the api, since it's not documented in the api-ref
19:37:05 mriedem so maybe that's the neutron bug is just lack of docs
19:37:32 sean-k-mooney right i more or less keep going back to the spec to determin what the expected behavior is
19:37:47 sean-k-mooney my guess is what happening is becasue of https://specs.openstack.org/openstack/neutron-specs/specs/ocata/portbinding_information_for_nova.html#effects-on-existing-apis
19:38:01 sean-k-mooney Update Port with host_id will still re-trigger port binding for a host. The difference will be update_port() will only action on the active binding.
19:38:11 mriedem "The difference will be update_port() will only action on the active binding."
19:38:12 mriedem yeah
19:38:18 mriedem and if the active binding doesn't exist....
19:38:29 mlavalle mriedem: has the src inactive binding benn deleted when you try to go back?
19:38:39 mriedem mlavalle: no
19:38:51 mriedem well, not as far as i know anyway
19:39:04 mriedem nova isn't explicitly deleting the inactive source host binding
19:39:09 sean-k-mooney the correfct thing to do based on the spec would be to reactivate the souce binding
19:39:27 mlavalle in that case, I think that whn going back, activate tje source binding
19:39:28 sean-k-mooney rather then update it
19:39:57 sean-k-mooney which is what mriedem is doing in https://review.opendev.org/#/c/594139/ correct
19:40:31 sean-k-mooney ya in setup_networks_on_host
19:41:34 mriedem ok yeah that's what i'll try (when i can push up the change)
19:42:29 sean-k-mooney mriedem: that is what you are already doing in https://review.opendev.org/#/c/594139/1/nova/network/neutronv2/api.py@423
19:42:39 sean-k-mooney instance.host should be the souce host
19:42:50 mriedem sean-k-mooney: that's not a patch in my cross-cell series though
19:42:57 sean-k-mooney oh ok
19:42:57 mriedem it's just a related thing
19:43:01 mriedem and ^ isn't merged anyway
19:44:34 sean-k-mooney i wonder if we call _update_port_binding_for_instance in teh revert flow
19:45:00 sean-k-mooney that might be why https://review.opendev.org/#/c/653506/ was fixign your cross cell issue
19:45:16 mriedem i'm not using https://review.opendev.org/#/c/653506/ in the cross-cell series

Earlier   Later