| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 21:09:03 | dansmith | because if we deleted that, then we're going to end up with no instance record when the conductor realizes that we won | |
| 21:09:07 | dansmith | L1712 | |
| 21:10:57 | dansmith | well, wait, | |
| 21:11:18 | mriedem | it seems like we just don't support soft_delete for the case that the instance isn't in a cell yet | |
| 21:11:19 | dansmith | yeah | |
| 21:11:28 | dansmith | right | |
| 21:11:38 | mriedem | soft_delete is also totally dependent on the compute deleting the instance | |
| 21:11:44 | mriedem | and if it never landed in a cell, there is no compute to clean it up | |
| 21:12:06 | mriedem | seems like a good comment to add in the code as a reminder of that | |
| 21:12:33 | mriedem | we could do that in a follow up, like my ask for a comment about why we avoid the lazy-load in the context manager for sys meta | |
| 21:12:45 | mriedem | cburgess: done, and the bp in lp too | |
| 21:13:28 | cburgess | cool | |
| 21:13:30 | mriedem | this patch still doesn't have a test to make sure that the soft_delete case is handled when the instance is in cell0 | |
| 21:13:33 | dansmith | mriedem: yeah | |
| 21:13:55 | dansmith | mriedem: I can do that, or you can throw it in while you're stacking your others on top here | |
| 21:14:14 | mriedem | let me think about what i just said, | |
| 21:14:19 | dansmith | yeah, that... | |
| 21:14:22 | dansmith | would be a problem | |
| 21:14:27 | mriedem | if it's in cell0, again, it's we can't soft delete it | |
| 21:14:29 | dansmith | but there's also no resources to clean up | |
| 21:15:19 | mriedem | https://review.openstack.org/#/c/437222/7/nova/compute/api.py@2126 | |
| 21:16:39 | dansmith | that cast is just going to go into the ether | |
| 21:16:59 | dansmith | if you delete it again, it hard deletes right? | |
| 21:18:03 | openstackgerrit | Sarafraj Singh proposed openstack/nova-specs master: WIP: Replace live migration steps and delays with memory iteration logic https://review.openstack.org/443339 | |
| 21:20:25 | dansmith | mriedem: from the api perspective, what we have _might_ make the most sense | |
| 21:20:36 | dansmith | you soft delete, it stays around half deleted, | |
| 21:20:56 | dansmith | but never gets culled because there is no compute for it | |
| 21:20:57 | dansmith | then you hard delete it to make it go away | |
| 21:20:58 | dansmith | on the other hand, | |
| 21:21:09 | mriedem | you mean force-delete? | |
| 21:21:28 | dansmith | just making it kill the instance immediately if it's in cell0 also makes sense, since there's no reason to keep a dead instance around | |
| 21:21:40 | dansmith | soft-delete being for "oops, I deleted a thing that I wanted to keep" | |
| 21:21:57 | mriedem | i don't think force-delete would fix anytihng here | |
| 21:22:04 | openstackgerrit | Sarafraj Singh proposed openstack/nova-specs master: Spec to add per instance timeout for LM https://review.openstack.org/442809 | |
| 21:22:04 | mriedem | but i'm not following the half-deleted case | |
| 21:22:09 | mriedem | i put some comments in there if you want to read those and reply | |
| 21:22:27 | dansmith | mriedem: https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/servers.py#L782-L785 | |
| 21:22:40 | mriedem | basically i think the only issue now is the notification is lying if it says we do a soft-delete but we actually hard-delete in cell0 | |
| 21:23:07 | mriedem | what triggers that InstanceInvalidState though? | |
| 21:23:18 | dansmith | mriedem: lies how? because it doesn't call it a soft delete? | |
| 21:23:22 | openstackgerrit | Sarafraj Singh proposed openstack/nova-specs master: Add live-migration-force-after-timeout spec https://review.openstack.org/438467 | |
| 21:23:27 | mriedem | must_have_launched=True | |
| 21:23:31 | dansmith | mriedem: must_have_launched | |
| 21:23:32 | dansmith | yeah | |
| 21:24:10 | mriedem | what sets launched_at? compute? | |
| 21:24:24 | mriedem | yup _update_instance_after_spawn | |
| 21:24:26 | dansmith | yeah | |
| 21:25:18 | mriedem | see https://review.openstack.org/#/c/437222/7/nova/compute/api.py@1794 | |
| 21:25:20 | mriedem | for the lie | |
| 21:26:01 | dansmith | oh, calls it a soft delete when it's not | |
| 21:26:04 | mriedem | yeah | |
| 21:26:18 | mriedem | i mean that's very minor | |
| 21:26:23 | mriedem | but i think it's technically wrong | |
| 21:26:55 | dansmith | if we do that, | |
| 21:26:56 | dansmith | hmm | |
| 21:27:07 | dansmith | you can get there if you have a non-cell0 cell too right? | |
| 21:27:22 | mriedem | but no host? | |
| 21:27:28 | dansmith | if you race with compute owning the instance | |
| 21:27:35 | dansmith | the instance_mapping has a cell, | |
| 21:27:38 | dansmith | because scheduler decided, | |
| 21:27:44 | dansmith | but compute hasn't instance.host=$me yet | |
| 21:29:34 | mriedem | what sets instance.host in the compute? | |
| 21:29:38 | mriedem | for spawn i mean, i'm not seeing that | |
| 21:29:41 | dansmith | yeah, hang on | |
| 21:30:10 | dansmith | I always have to dig for this I dunno why | |
| 21:31:31 | melwitt | it's in instance_claim in the RT | |
| 21:31:40 | dansmith | oh, that's probably why | |
| 21:31:56 | mriedem | i figured that gd low down RT was involved | |
| 21:31:59 | dansmith | https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L336-L344 | |
| 21:32:12 | melwitt | haha, yeah. it's hidden there, for fun! | |
| 21:32:46 | mriedem | RT is like a leprachaun | |
| 21:32:56 | mriedem | full of tricks | |
| 21:33:50 | mriedem | dansmith: ok so back to https://review.openstack.org/#/c/437222/7/nova/compute/api.py@1794 if it's not in cell0 and it doesn't have a host yet, but let's say it's in cell1, | |
| 21:34:00 | mriedem | we still delete the instance | |
| 21:34:27 | mriedem | so i think that just means, when the reclaim task (or whatever it's called) in the compute runs, it just won't see that instance | |
| 21:34:32 | mriedem | and that's fine, it's already gone | |
| 21:34:39 | mriedem | so it was an immediate soft delete :) | |
| 21:34:46 | mriedem | right? | |
| 21:35:17 | dansmith | unless we got here because instance wasn't created in the database | |
| 21:35:28 | dansmith | but I _think_ that means it's gone-gone too | |
| 21:35:37 | mriedem | which database? | |
| 21:35:42 | mriedem | cell db? | |
| 21:36:17 | dansmith | yeah the only place instances get created | |
| 21:36:31 | dansmith | I think we can also fail to delete there if host changes, right? | |
| 21:36:38 | dansmith | we're in a try..except there | |
| 21:36:55 | dansmith | and I think that's for the "if host changed" case, | |
| 21:36:56 | dansmith | where we can either fall through or bail | |
| 21:37:12 | dansmith | I think we might be over-obsessing here | |
| 21:37:16 | mriedem | yeah if the host changes we're here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1816 | |
| 21:37:26 | mriedem | and then it falls through to the normal delete path | |
| 21:37:36 | dansmith | I'm not sure whether notifications consumers will do something useful with the soft/hard indication | |
| 21:37:54 | mriedem | i agree this is probably not the end of the world | |
| 21:38:23 | dansmith | the missing notifications, and the quota bug behind this are definitely more important | |
| 21:38:37 | dansmith | gibi might have a better handle on how much this does or does not matter | |
| 21:39:27 | melwitt | I was thinking my main concern would be whether the start/end notifications match, and also that they match the request that was issued from the user | |
| 21:39:45 | melwitt | for the first, I saw the context manager makes sure the start/end match | |
| 21:39:47 | dansmith | the context manager makes sure they match I think | |
| 21:40:45 | melwitt | for the last, I guess that's where the delete_type comes in. I was just trying to think, how someone might use this and if they're listening to notifications, they may also be looking at what requests were issued and correlating notifications with those | |
| 21:41:43 | melwitt | to get like a bundle of "what happened" which would be 1) user issued delete_type X 2) nova emitted delete_type X start 3) nova emitted delete_type X end | |
| 21:42:55 | dansmith | I'm not sure what the point of correlating the requests and the notifications really is, but we don't really know what (all) people use these for | |