| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-04-01 | |||
| 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 | we have have this as well | |
| 09:05:24 | sean-k-mooney | i dont know if its needed | |
| 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: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 | |
| 09:06:12 | gibi | it is a new behavior | |
| 09:06:17 | sean-k-mooney | correct and the other exemption https://docs.openstack.org/nova/latest/contributor/microversions.html#id3 | |
| 09:06:26 | sean-k-mooney | also does not apply to 409 | |
| 09:06:43 | sean-k-mooney | also this is not a 500 | |
| 09:07:04 | gibi | lyarwood: yeah, let's keep a todo for Xena | |
| 09:07:50 | sean-k-mooney | lyarwood: you coudl just write both patches now. technially master is now xena | |
| 09:08:15 | sean-k-mooney | although we might want to hold off api microverion bumps until the release is actully done | |
| 09:08:35 | lyarwood | yup I'll get it posted later today before the break (./me is off until Tuesday after today). | |
| 09:08:37 | gibi | yepp, master is not fully open to Xena yet | |
| 09:08:48 | lyarwood | thanks both :) | |
| 09:09:19 | sean-k-mooney | ya ill be on pto till tuesday too | |
| 09:11:48 | gibi | I guess it is true for most of us | |
| 09:12:22 | sean-k-mooney | lyarwood: by the way partly while im off and partly early next week i plann to dismantel my home cloud and reinstall it and do some hardware tweeks | |
| 09:12:41 | sean-k-mooney | do you need any data form your vms | |
| 09:12:50 | lyarwood | sean-k-mooney: ack, nope I don't nuke away | |
| 09:13:29 | sean-k-mooney | cool ill be backing up a few thign but one of the change ill be doing is swaping my current cinder lvm sotrage for ceph so ill be easing most of the stroage | |
| 09:14:15 | sean-k-mooney | also doing an os reinstall moving form cenots 8 to stream wroked for a while but now im getting some repo conflictis so doing a reinstall to fix that | |
| 09:14:41 | lyarwood | huh I had assumed this was all running on Ubuntu tbh | |
| 09:14:51 | lyarwood | but cool | |
| 09:14:57 | sean-k-mooney | hehe well it will be soon | |
| 09:15:17 | sean-k-mooney | stephenfin: convicned me to try centos 8 for it then it lifecycle changed | |
| 09:15:42 | stephenfin | don't dare try to shift the blame to me - this is all on you :P | |
| 09:15:53 | sean-k-mooney | it is | |