Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-04-01
07:27:24 gibi bauzas: hi! I've propose and RC2 patch and but a -W on it until next week https://review.opendev.org/c/openstack/releases/+/784201
08:28:24 gibi lyarwood: I put a heavy -1 to https://review.opendev.org/c/openstack/nova/+/784130
08:28:45 sean-k-mooney gibi: we could encode if its the flavorid/port id in the string
08:28:57 sean-k-mooney i do not belive its defined as a uuid
08:29:18 sean-k-mooney so we could do flavor/<uuid> and port/<uuid> if we needed too
08:29:45 sean-k-mooney normally i would say just add a requester_type
08:29:53 sean-k-mooney but that would be a db change
08:29:54 gibi sean-k-mooney: it wasn't filled not becasue we don't know what to put there, we did not put flavor there as nobody uses that information
08:30:19 sean-k-mooney ah yes
08:30:27 sean-k-mooney that makes sense i guess
08:30:32 gibi sean-k-mooney: and there later some source information is also added to the ovo in I292a0e2d840bbf657ba6d0932f9a3decbcb2778f
08:30:37 gibi https://review.opendev.org/c/openstack/nova/+/659101/4/nova/objects/instance_pci_requests.py
08:31:04 sean-k-mooney yes i remember that
08:31:19 gibi I have no issue if somebody starts adding flavorid tehre
08:31:20 gibi there
08:31:41 sean-k-mooney ya i think we can use store the uuid directly
08:31:49 gibi yepp
08:31:54 sean-k-mooney and use that source property to figure it out if we need too
08:32:09 gibi and we can add a fallback to the ovo that if the requester_id is None then it looks up the flavor stored in the instance
08:33:28 sean-k-mooney have you seen artoms change by the way https://review.opendev.org/c/openstack/nova/+/784168
08:33:54 sean-k-mooney i still need to properly review it but at first glance this seams like a workable solution going forward
08:34:20 sean-k-mooney im still debating if we want to add somethign to populate the requeter id for exsiting recoreds
08:34:57 sean-k-mooney possibly as a nova manage command or something on init_host as a seperate patch
08:36:35 sean-k-mooney what i was thinkng was loop over all the claimed devices without requester_id set, then lookup the port info form the network info cache and try and use the pci slot to corralte the pci devices to the ports
08:39:16 gibi sean-k-mooney: haven't looked that review yet
08:42:35 sean-k-mooney its still WIP so not quite ready for review yet but the core chagne is there and the previous patch add a functional repoducer which passes after the change
08:45:20 lyarwood gibi: replied, not sure I agree tbh, seems like a trivial requirement to codify to avoid operators borking things
08:45:43 lyarwood gibi: we had an example downstream of evacuations from 2019 being cleaned up recently in 2021 that spawned this btw
08:46:01 lyarwood gibi: somehow in the iterim the instance(s) had been migrated back to the original host
08:46:19 lyarwood gibi: I assume with manual operator intervention to cleanup instance dirs etc
08:46:36 lyarwood gibi: thankfully no data was lost but I'm just trying to avoid a future situation where we don't get as lucky
08:47:02 lyarwood gibi: and again it's specifically for the force down, evacuate and force up usecase. Outside of that I agree that a periodic or something would be useful
08:47:11 gibi lyarwood: this is a slippery slope as then when the next bug cames that after force_down + evac the VM volume became corrupted due to two VMs writing it then somebody can say that we should not evacuate a VM if we cannot pervent VM duplication
08:47:58 gibi as soon as the admin used force_down we should stop caring
08:48:04 lyarwood gibi: this isn't trying to resolve any issues during the initial force down
08:48:13 lyarwood gibi: *only* when the operator forces the service back up
08:48:28 lyarwood gibi: and we are given the chance to check the state of the migrations
08:48:44 sean-k-mooney so the alternitive here would be to make the action for what happens when we discover vms in this state configureable
08:48:48 lyarwood gibi: anything that happens before the force up is out of scope for this
08:48:55 sean-k-mooney like the running_delete_instance_action
08:49:20 sean-k-mooney our current behavior is hardcoded reap the instance basically
08:49:22 lyarwood yeah as I said yesterday sean-k-mooney you would force up the compute service first before moving them
08:49:33 lyarwood moving them back*
08:49:45 lyarwood so checking during force up makes more sense tbh
08:50:19 gibi lyarwood: so the goal is to check if the system is in consistent state before we allow forecing the compute up
08:50:39 sean-k-mooney ya im not really against that check but i do kind fo agree with gibi that once they do force down then they are assuming resposiblity to some degree
08:50:47 sean-k-mooney gibi: yes
08:51:00 lyarwood gibi: correct, evacuation migration records in done with a source host pointing at the localhost meaning a service restart is required
08:51:24 lyarwood sean-k-mooney: yup agree but docs have been proven time and time again not to be enough to avoid PEBKAC
08:51:56 sean-k-mooney lyarwood: would an alternitive be to do an rpc on force up?
08:52:12 sean-k-mooney basically to trigger what woudl be done on service restart
08:52:32 lyarwood sean-k-mooney: yeah we could do that moving forward and backport this check?
08:52:47 sean-k-mooney well i dont know if that was a good idea or not
08:52:48 lyarwood sean-k-mooney: and/or have a periodic that triggers this
08:53:04 sean-k-mooney just tinking things through
08:53:06 gibi sean-k-mooney: while you could trigger the evac cleanup during force up we could think through what else needs to be done. Like clean up local deleted instances.
08:53:25 lyarwood we have a periodic for that right
08:53:32 sean-k-mooney local deleted yes
08:54:15 sean-k-mooney i tought we did for evacuated instance but stephenfin did not find one when he quickly search on the triage call
08:54:26 sean-k-mooney it might exist already but proably not
08:54:37 gibi lyarwood: yepp we have _cleanup_running_deleted_instances
08:54:58 gibi sean-k-mooney: there is no periodic for evac cleanup I've checked
08:56:03 gibi we have _error_out_instances_whose_build_was_interrupted during init_host as well
08:56:22 gibi that also someting that is only done in init_host
08:56:29 gibi as might need to be done after forece up
08:57:27 sean-k-mooney on a related note there was also https://review.opendev.org/c/openstack/nova/+/627765 before
08:58:00 sean-k-mooney in this case looking at orpahned instance which are deleted instance that have been purged from the database before that periodic runs
08:58:30 gibi anyhow we are divergin. Lee only wants a backportable thing now
08:58:43 lyarwood Lee always wants a backportable thing ;)
08:59:09 sean-k-mooney yes to block more latent vms in thsi state
08:59:32 lyarwood but yeah I think this API change is backportable, a periodic could also be but adding a RPC call from the API during force up wouldn't be
08:59:53 sean-k-mooney lyarwood: i assume we woudl return a 409
08:59:58 lyarwood yup
09:00:04 lyarwood that's how I've written it at the moment
09:00:10 sean-k-mooney proably with that is its a new return code
09:00:16 gibi I still don't like trying to build in safety into a workflow that we marked unsafe at the beginning. But if you frame the whole thing in a way that before nova assumes responsibility again about this compute node after a force_down we do some consistency check, then I can be convinced to remove my -1
09:00:22 sean-k-mooney which is normally a microverison bump
09:00:29 lyarwood yeah but AFAIK we don't require microversion bumps for new error codes
09:00:39 sean-k-mooney i think we do
09:00:40 lyarwood I've asked gmann about this before
09:00:56 sean-k-mooney it think that only if we are converting from a 500
09:01:09 sean-k-mooney to the flow chart
09:02:10 lyarwood sean-k-mooney: I was just looking for that, can you share the link?
09:02:18 sean-k-mooney https://docs.openstack.org/nova/latest/contributor/microversions.html
09:02:23 lyarwood ta
09:02:33 sean-k-mooney so yes we need a microversion if you use 409
09:02:37 sean-k-mooney so 400?
09:02:38 gibi I think we need to keep it 400 for a backportable fix
09:02:50 gibi current clients are not prepared for 409
09:02:58 gibi so it is breaking them
09:03:08 sean-k-mooney yep
09:04:03 lyarwood kk understood, with a TODO to move to 409 with a new microversion in Xena?
09:05:01 sean-k-mooney am sure we could
09:05:18 gibi hm
09:05:19 sean-k-mooney as long as we have a good error message
09:05:24 gibi An obvious regression bug in an admin-only API where the bug can still be fixed upstream on active stable branches. Admin-only APIs are less of a concern for interoperability and generally a regression in behavior can be dealt with as a bug fix when the documentation clearly shows the API behavior was unexpectedly regressed. See 3 for an example. Intentional behavior changes to an admin-only
09:05:24 sean-k-mooney i dont know if its needed
09:05:24 gibi we have have this as well
09:05:30 gibi API do require a microversion, like the 2.53 microversion for example.
09:05:59 gibi nvm, this bug is not a regression

Earlier   Later