| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 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 | |
| 21:42:59 | melwitt | if they're doing log collecting and correlating. anyway, that's the thing I could think of it mattering | |
| 21:43:25 | dansmith | mriedem: I guess my point is, this is demonstrably better than what we have now, I think, especially considering the quota thing behind it | |
| 21:43:46 | dansmith | in some very teensy windows of a less-mainstream feature, you might get a delete type you didn't expect | |
| 21:43:49 | melwitt | at my last company, use used the old notification driver that emits notifications to the logs. and we had those in splunk and correlated things like that, to look at when people opened bugs etc | |
| 21:45:00 | melwitt | I think what I'm saying is what the patch is doing, so it makes sense to me to keep everything saying "soft delete" if the user issued "soft delete" | |
| 21:45:57 | dansmith | well, it's not in the first case he brought up | |
| 21:46:05 | mriedem | the user doesn't technically request "soft" delete | |
| 21:46:11 | mriedem | it's all driven by config | |
| 21:46:17 | mriedem | it's totally not interoperable | |
| 21:46:55 | melwitt | erg, right. notifications probably have the request-id too right? so that's what anyone would use to correlate anyway | |
| 21:47:02 | dansmith | yeah | |
| 21:47:40 | melwitt | okay, that addresses what I was thinking | |
| 21:48:42 | mriedem | so i think i might still pull this down, add some comments, and add a test for the soft-delete case where there isn't a host but it's in a cell (any cell) and the build request was already gone | |
| 21:49:07 | mriedem | because i'm pretty sure i'm right here https://review.openstack.org/#/c/437222/7/nova/tests/unit/utils.py | |
| 21:49:17 | mriedem | then i'll rebase my stuff on top of it | |
| 21:49:23 | mriedem | and then it's pants off dance off | |
| 21:53:05 | dansmith | so, wait, | |