Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-04
16:31:04 mriedem b/c ^ doesn't persist the changes, it just updates the object fields
16:31:15 artom mriedem, wait, seriously?
16:31:35 artom mriedem, huh, but instance.save() is still called after in post_live_migration
16:31:36 mriedem don't take my word for it
16:31:43 bauzas gibi: if that's virt-specific, you should document it in https://docs.openstack.org/nova/latest/user/feature-classification.html
16:31:53 gibi bauzas: thanks for the pointer
16:32:16 artom mriedem, ah, you're right, it's not saved
16:32:21 mriedem i'm more worried that we send a server create request to a compute that doesn't support it
16:32:33 mriedem and we don't have any kind of feature capability for this port requested resources stuff
16:32:44 mriedem gibi: bauzas: ^
16:32:48 mriedem and we just drop the request on the floor
16:33:01 dansmith artom: so does that mean some testing gap exists because we never re-migrate the instance a second time or something?
16:33:09 mriedem thinking about https://review.openstack.org/#/c/538498/ coincidentally
16:33:11 bauzas mriedem: that's my -1
16:33:18 dansmith artom: meaning.. how could we be failing to save that and think that this works?
16:33:22 artom But there's an instance.save 5 lines after https://review.openstack.org/#/c/634606/37/nova/compute/manager.py@6925
16:33:23 gibi mriedem: yeah if neutron agent is configured to report bandwidth but the nova virt driver does not support it then it will be NoValidHost after a bunch of reschedule
16:33:35 bauzas mriedem: but the question is : should we leave it as docs, or enforce it ?
16:33:37 artom dansmith, I was just testing the XML is updated correctly
16:33:52 dansmith artom: oh :(
16:34:03 artom dansmith, yeah, testing coverage gap :(
16:34:52 mriedem gibi: bauzas: at this point it's probably best to just doc it and clean it up in train with a capability tag a la https://review.openstack.org/#/c/538498/
16:35:16 bauzas I'm good with that
16:35:19 gibi bauzas, mriedem, sean-k-mooney, stephenfin: this is the cons of the auto detection of the pf_interface_name. It makes the feature virt driver specific. If we go with the a new tag in the passthrough_whitelist then the solution is virt driver agnostic
16:35:19 mriedem aspiers: efried: btw who is on the hook for documenting ^ now that it's going to be codified?
16:35:55 mriedem but then you have to configure more crap
16:36:17 gibi mriedem: exactly
16:36:29 mriedem this is pretty magical unicorn and i expect 99% of openstack users for nfv stuff are using libvirt anyway
16:36:32 openstackgerrit Merged openstack/nova stable/rocky: Fix race in test_volume_swap_server_with_error https://review.openstack.org/640595
16:36:32 gibi mriedem: I'm OK to add the capability in Train
16:38:11 mriedem dansmith: regarding the spot instance stuff i have skimmed some of it in the past (over a month ago) noting some obvious concerns, but haven't been back on it since, nor has anyone else from the core team it looks like, so that one is too risky at this point for me, especially without any integration (tempest) testing
16:38:33 bauzas mriedem: gibi: I'm not opposed to have this feature be virt-driver specific for the moment and provide a new abstract trait later
16:38:49 mdbooth lyarwood: I was looking into your tempest failures but got distracted. Just getting back into it. Did you manage to look by any chance?
16:39:04 dansmith mriedem: yeah, same, I was just skimming over it, thinking that since it was two pieces, maybe half of it was simple stuff we could merge, but it seems all tied together
16:40:43 mdbooth Looking at one of them in particular, I note that the type is deleted after a volume of that type is deleted, but before cinder logs that the delete completed. However, it could be, for eg, that cinder has already deleted the volume from the db but doesn't log until some backend stuff is done? I'm still looking. Looks weird.
16:40:46 dansmith artom: so yeah, I dunno why that save() doesn't get you there, but the migration context stuff is complex, so you might just be tripping over some assumption
16:41:13 dansmith artom: honestly, I'd start trying to reproduce (or disprove) that with a functional test before rattling a fix
16:41:25 artom dansmith, yeah, first order is modifying my tests to catch that
16:41:41 artom (Or writing new func tests for the same purpose)
16:41:42 artom Then debugging
16:41:43 dansmith artom: ++
16:41:59 mriedem btw i do a lot of ^ type stuff in the cross-cell resize functional testing to make sure the db is what i expect it to be
16:42:11 mriedem since i'm farting around with 2 dbs
16:42:17 gibi bauzas: cool, then your current -1 on the patch is just procedural while you reviewing the rest of the patch?
16:43:59 artom mriedem, I think the DB asserting part will be relatively easy, it's the running 2 services part that's finicky
16:44:43 artom dansmith, while I have you, can we context switch to your thoughts on https://review.openstack.org/#/c/639396/?
16:45:57 dansmith artom: ugh, I really don't want to... I know that is going to be ugly because of whatever has moved out of that scope, but I'm really (really) not in favor of changing how that works.. like REALLY.
16:46:04 bauzas gibi: I left a new comment as a soft -1
16:46:18 gibi bauzas: looking
16:46:21 bauzas gibi: but I'm continuing to review so I can +1 once I'm done
16:46:26 gibi bauzas: sure
16:46:30 bauzas gibi: just askign docd
16:46:43 gibi bauzas: I will let you go through it before I start updating the patch
16:46:57 artom dansmith, yeah :/
16:47:20 artom dansmith, as I said in the review, I don't see a way around *some* change, because we race on setting up the event waiter
16:47:21 gibi bauzas: would you accep the documentation in a separate patch?
16:47:32 artom If we don't get to the driver code by the time Neutron sends us the event, it's lost
16:48:11 dansmith artom: yeah, but neutron is sending the event in response to things we're doing, which just means whatever we do to trigger it needs to happen within the context
16:48:26 bauzas gibi: yup
16:48:32 gibi bauzas: coolio
16:48:36 bauzas I can turn into a +1 if you prefer
16:48:55 gibi bauzas: don't worry, I will respin anyhow
16:49:06 artom dansmith, yeah - but we're poking Neutron in the compute manager, and setting up the waiter in the virt driver...
16:49:12 gibi bauzas: just let me know if you spot any other issues
16:49:19 dansmith artom: I understand
16:49:21 artom From the virt driver, I should say. It's a call to the compute manager
16:49:46 artom Hrmm, there's no RPC, mind you
16:49:56 artom Just... a whole bunch of intermediary steps
16:50:07 bauzas gibi: I'm on it
16:50:13 gibi bauzas: thanks a lot
16:50:29 bauzas I wish I could help better, that's it
16:51:45 artom dansmith, anyways, I'll try to come up with a least bad way of doing it, we'll have a strawman to pick apart
16:52:03 artom At least you're on board with the need for change ;)
16:54:06 mriedem artom: so we likely need 2 context managers to wait for separate events right?
16:54:15 mriedem one for the port binding host change, and one for the vif plug
16:54:29 mriedem that port binding host change happens in several move operations we'd care about (at least resize and evacuate)
16:54:37 mriedem and unshelve i think...
16:54:58 artom mriedem, hrmm, you lost me, how would 2 context managers solve this?
16:55:14 mriedem couldn't we create a helper method for that neutronv2.api method that updates the host in the port binding to register an event callback
16:55:22 mriedem artom: we have 2 events to wait for yeah?
16:55:40 mriedem so you wait for the first (port binding host change), then call the driver to spawn or whatever which does the vif plug
16:55:45 mriedem which triggers the event we've always waited for
16:56:14 artom mriedem, uhh, I thought it was Nova that PUT'ed the post binding host change?
16:56:28 mriedem it does
16:56:38 artom And that got us a vif-plugged as a "response"?
16:56:42 mriedem and then we get an event from neutron that we're not waiting for
16:57:08 mriedem and then we actually plug vifs which produces another event yeah?
16:57:11 dansmith artom: was it not updating instance_extra.migration_context, or was it not persisting the applied-from-context instance attributes?
16:57:13 artom We are, sort us, we just start waiting after it gets to us, but yeah, you're basically right
16:57:15 dansmith artom: I had assumed the former
16:57:29 artom dansmith, the latter
16:57:32 dansmith ah
16:57:42 dansmith artom: which attributes of those specifically?
16:58:00 artom dansmith, so I haven't looked into it in detail, but I'm told the new NUMA topology
16:58:02 dansmith numa_topology?
16:58:11 dansmith you probably need to do something to make those dirty,
16:58:16 artom ...
16:58:23 dansmith since they're nested objects, they probably only save if the object itself is dirty
16:58:35 dansmith artom: you need to give them "the talk"
16:59:09 artom Right, tell them they've been naughty and need to be saved from their sins?

Earlier   Later