| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-09-29 | |||
| 16:31:16 | lyarwood | I wouldn't say so for this case where the instance was running on a host | |
| 16:31:25 | dansmith | delete is and always has been pretty much "I want this to complete and stop charging me immediately", so... we're really not wrong here, IMHO | |
| 16:31:28 | lyarwood | that makes perfect sense | |
| 16:31:30 | opendevreview | Balazs Gibizer proposed openstack/nova master: Enable min pps tempest testing in nova-next https://review.opendev.org/c/openstack/nova/+/811748 | |
| 16:32:26 | dansmith | lyarwood: wouldn't say what, that relying on the lock is unsafe? we can do stuff in the api to delete things (in the local case) which has nothing to do with the instance lock, | |
| 16:32:46 | dansmith | and I'm not sure that relying on the ordering of two calls is really safe either unless we have perfectly fair locks | |
| 16:32:50 | bauzas | sorry, I was afk | |
| 16:33:13 | lyarwood | dansmith: we can't force the delete in the API of an instance still associated with a host can we? | |
| 16:33:18 | dansmith | like, if you issue three calls on that instance and the stop is #2, delete is #3, the lock has to be fair to make sure we don't happen to choose the waiting #3 thread when we stop doing #1 | |
| 16:33:32 | dansmith | lyarwood: of course we can | |
| 16:33:43 | dansmith | if we think the host is down, or we missed the last service update, etc | |
| 16:34:12 | lyarwood | right I'm stuck in the happy path here | |
| 16:34:38 | melwitt | we have the ability to add fair=True to the locks, I wonder if that would work? | |
| 16:34:48 | bauzas | my biggest concern here is that we would make destroy holding for a graceful shutdown | |
| 16:35:01 | dansmith | bauzas: yeah I don't think we should do that for sure | |
| 16:35:23 | dansmith | bauzas: I think at most, we should try to make sure a delete doesn't preempt a stop operation in progress | |
| 16:35:31 | bauzas | yup, agreed | |
| 16:35:41 | bauzas | if a stop is occuring, destroy should wait | |
| 16:35:58 | melwitt | how can we make it wait without rejecting it? | |
| 16:36:08 | bauzas | excellent questioin | |
| 16:36:21 | melwitt | I do wonder if the fair=True would help | |
| 16:36:28 | bauzas | I'd say we would reject synchronously by looking at the VM state | |
| 16:36:39 | melwitt | I'll try it in a devstack if I can repro the problem | |
| 16:37:09 | bauzas | destroy is synchronous, right? | |
| 16:37:20 | bauzas | even if the host is down, destroy will occur | |
| 16:37:43 | dansmith | melwitt: I'm just suggesting that lock inversion is *one* possible way that we might not be able to depend on the ordering | |
| 16:37:48 | bauzas | that's the guest destroy which is async, right? | |
| 16:38:19 | dansmith | anything else that happens on the compute host before we block on the lock could cause it, as well as if conductor is involved or something like that | |
| 16:38:31 | bauzas | dansmith: I missed your proposal | |
| 16:38:43 | bauzas | you're telling we could lock on stop ? | |
| 16:38:58 | bauzas | hence preventing the delete ? | |
| 16:39:15 | melwitt | we do lock on stop on the compute node | |
| 16:39:21 | melwitt | but lock waiters would not be in order because they are not fair locks | |
| 16:39:42 | melwitt | *would not necessarily be in order | |
| 16:39:59 | dansmith | bauzas: I haven't proposed anything | |
| 16:40:14 | bauzas | ok, melwitt explained | |
| 16:41:03 | dansmith | yeah, so when we go to do delete, | |
| 16:41:06 | melwitt | dansmith: ack, I was just thinking that would be such a small and simple way to fix it if the fair locks would do that. but like you said if we have an issue with two async things arriving on the compute node in the "wrong order" then it wouldn't help | |
| 16:41:12 | dansmith | we create an instance event before we run terminate_instance and grab the lock, | |
| 16:41:22 | dansmith | which means we're doing network IO to conductor | |
| 16:41:32 | dansmith | so even fair locks won't prevent order inversion there | |
| 16:41:53 | melwitt | yeah... | |
| 16:42:13 | dansmith | so if stop and delete arrive in the right order, but each call out to conductor, we hit any conductor in the cluster, each one tries to create records in the db, who knows which one will finish first, grab the lock, etc | |
| 16:42:35 | dansmith | that's what I mean by assuming this sort of super tight ordering is unsafe, even though we *think* we're on the compute and largely single-threaded | |
| 16:42:46 | bauzas | I see | |
| 16:43:03 | bauzas | stupid idea, can't we rely on the state of the instance ? | |
| 16:43:13 | dansmith | delete is anything-goes I think | |
| 16:43:27 | bauzas | yeah, that's the original problem | |
| 16:43:32 | dansmith | because that's what we want... | |
| 16:43:41 | bauzas | yup... | |
| 16:44:15 | dansmith | and anything else we build into there is going to be pretty obscure.. "delete always deletes, except stop but not shelve... and either waits or refuses or ..." | |
| 16:44:24 | dansmith | makes the "delete always works" contract a little less clear | |
| 16:44:27 | bauzas | honestly I don't know how to move on with this ask | |
| 16:44:38 | dansmith | tell them that delete means pulling the plug, | |
| 16:44:41 | bauzas | "ask your orchestration to be smarter ?" | |
| 16:44:41 | lyarwood | how about making stop more graceful just for the libvirt driver? | |
| 16:44:46 | gibi | can we add a flag to the delete api saying I-want-a-shutdown-first? | |
| 16:45:00 | dansmith | and you wouldn't do that while waiting for start->shutdown on a physical server, so they should wait for vm_state=STOPPED before delete | |
| 16:45:24 | dansmith | gibi: that makes it a little less obscure, but doesn't eliminate the need to make that orchestration bit work, of course | |
| 16:45:37 | bauzas | yeah | |
| 16:45:39 | lyarwood | artom: there main issue with stop was that it eventually destroys the instance right? | |
| 16:45:47 | bauzas | that doesn't solve the ordering problem | |
| 16:46:07 | lyarwood | artom: and in your change you've suggested that we call shutdown on the domain as an initial step to make this more graceful | |
| 16:46:35 | dansmith | lyarwood: can we call quiesce or something else constant-time before libvirt destroy? | |
| 16:46:43 | dansmith | instead of shutdown, which the guest can block or ignore? | |
| 16:47:31 | bauzas | the root problem is the I/O flushes, right? | |
| 16:47:40 | dansmith | novaclient has a --poll option for some things. "nova stop server --poll; nova delete server" would solve this pretty easy :) | |
| 16:49:01 | melwitt | they're using tripleo/heat but I'm pretty sure heat has dependency or waiting ability | |
| 16:49:09 | dansmith | surely | |
| 16:49:33 | lyarwood | dansmith: quiesce before $domain.shutdown() might help flush things if that's where the guestOS is getting hung up | |
| 16:49:52 | melwitt | their argument has been that stop + insta delete should not break the volume, IIUC | |
| 16:50:00 | lyarwood | waiting assumes we don't kill it before it's finished shutting down | |
| 16:50:14 | dansmith | lyarwood: I meant to make sure journal buffers are written before we nuke the guest | |
| 16:50:36 | dansmith | melwitt: that argument is fine as long as you wait between stop and delete :) | |
| 16:51:01 | dansmith | melwitt: because again, start->shutdown and then pulling the plug before it finishes yields a corrupted disk :) | |
| 16:51:21 | melwitt | dansmith: that was the first thing I said on the bug report but it's gone on for a long time now and gotten into the weeds | |
| 16:52:04 | bauzas | melwitt: I haven't seen any bug report against artom's patch | |
| 16:52:12 | bauzas | I guess you're talking internally | |
| 16:52:19 | melwitt | bauzas: yes internally | |
| 16:52:42 | bauzas | yeah, because that's what worried me originally | |
| 16:52:51 | bauzas | technically, destroy works like expected | |
| 16:52:52 | dansmith | we also really need to do a better job of making this a sanitized bug externally if we're going to claim this is a bug in nova | |
| 16:53:25 | bauzas | I'm not happy with claiming this as an upstream bug | |
| 16:53:37 | dansmith | me either, fwiw :) | |
| 16:53:40 | bauzas | a blueprint or a wishlist bug | |
| 16:54:08 | dansmith | we could add a feature as gibi said, but destroy is doing the right thing here | |
| 16:54:21 | bauzas | (18:52:51) bauzas: technically, destroy works like expected | |
| 16:54:28 | melwitt | +1 to all of that | |
| 16:54:30 | dansmith | an alternative to gibi's idea, would be a delete flag that says "assuming task_state=None" meaning "delete this if nothing else is going on" | |
| 16:54:30 | bauzas | strong agreement here | |
| 16:54:47 | dansmith | but it would require the client to retry, which they could currently do by just waiting for the stop to finish | |
| 16:54:50 | dansmith | so, meh | |
| 16:55:22 | bauzas | dansmith: yeah, that's why I was considering the vm state or the task state | |
| 16:55:38 | bauzas | if we really want to do *something* | |
| 16:55:39 | bauzas | but, | |
| 16:55:49 | bauzas | this can't be done with the current destroy API | |
| 16:55:52 | dansmith | it doesn't make it do what they want, and honestly it's kinda weird since they could just wait for the stop just as well, but it's less new orchestration stuff | |
| 16:56:09 | dansmith | [09:47:37] <dansmith> novaclient has a --poll option for some things. "nova stop server --poll; nova delete server" would solve this pretty easy :) | |
| 16:56:11 | dansmith | ^ :) | |
| 16:56:12 | bauzas | it would be a 'destroy++" API | |