Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-05-21
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
19:45:48 mriedem what fixed the cross-cell issue was i needed to activate the dest host bindings before calling migrate_instance_finish https://review.opendev.org/#/c/634832/29/nova/compute/manager.py@4625
19:46:09 mriedem same as live migration in _post_live_migration
19:47:23 sean-k-mooney actully i missed something the cross cell migration/resize is a cold migration. you choose to use the the multiple port binding flow for cross cell migrations?
19:48:14 mriedem yes
19:48:24 sean-k-mooney ah ok
19:48:43 mriedem because we want to validate early that we can create the port binding on the dest host in the target cell
19:48:50 sean-k-mooney then yes that makes sense that you would have to also add that
19:49:29 sean-k-mooney yep cross cell is proably even more imporant then live migration in that its likely more expensive to try again
19:55:26 sean-k-mooney i think the answer is no but does the super conductor ever call the cell conductor or does it always interact directly with a compute node. that a general cellsv2 question not related to your patches
19:56:14 sean-k-mooney i just realised i dont think i have ever seen code in the conductor call another conductor but i have nerver actully looked either
20:00:26 mriedem it calls the compute
20:00:48 sean-k-mooney and the compute talks to its local cell conductor as needed
20:13:05 mriedem yes
20:32:30 sean-k-mooney mriedem: thanks for the correction/comments on https://review.opendev.org/#/c/643578 ill fix that up tomorrow
20:33:02 sean-k-mooney mriedem: are you ok with the verb 'resurrect' by the way
20:34:38 mriedem from the forum session etherpad there is a note that mordred wants to talk about naming
20:34:47 mriedem https://etherpad.openstack.org/p/DEN-osc-compute-api-gaps
20:35:01 openstackgerrit Eric Fried proposed openstack/nova master: Support old & new versions of svm and avx traits https://review.opendev.org/660515
20:35:12 sean-k-mooney ill ping him and dean to review once i fix up the last few bits
20:36:55 mriedem dansmith may or may not have strong feelings about how evacuate gets named in osc
20:37:12 sean-k-mooney ill also proably submit a patch to try and swap over the nova live migration job to uses it but im not 100% certin a depens-on will work. i could add it to the requred project which should work but no sure we want to do that in general
20:37:46 mriedem you could test it with a dnm/wip patch until it's released in osc
20:38:04 sean-k-mooney ya i can try that
20:38:23 sean-k-mooney its more i dont think we want to use osc in the gate form mater in general
20:38:34 sean-k-mooney so i can juse a dnm patch that does for testing
20:38:52 sean-k-mooney then drop the source install bit after its released
20:39:55 sean-k-mooney i just dont want to adde the python-openstackclient as a required project in the final version of the gate job patch but as you siad tis fine for a dnm or wip patch
20:40:31 mriedem we already use osc in the gate
20:40:33 mriedem for devstack and grenade
20:40:52 mriedem using the things we want users to use in our testing is a good thing
20:43:13 dansmith mriedem: I dunno.. I definitely want a different name for it, but don't think we should change the API.. having it be named something different in osc seems confusing, especially since it will be a new operation that isn't one of the other three evacuates we have
20:44:34 mriedem new operation?

Earlier   Later