| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 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, | |
| 21:53:23 | dansmith | we notify delete.started..delete.ended in the api, or soft_delete.started..ended, | |
| 21:53:34 | dansmith | and then again in compute in both cases? | |
| 21:53:46 | dansmith | so if you normal delete, you get two delete.started notifications/ | |
| 21:53:59 | dansmith | and if soft delete then you get the soft one first and the regular one later? | |
| 21:59:02 | melwitt | the notifys should be limited to the local delete cases, right? | |
| 21:59:08 | melwitt | the new ones, I mean | |
| 21:59:43 | mriedem | melwitt: i think so https://review.openstack.org/#/c/437222/7/nova/compute/api.py@1866 | |
| 21:59:53 | melwitt | so they wouldn't be going on to compute to get a second notify | |
| 22:00:01 | mriedem | right | |
| 22:00:09 | mriedem | and where gibi has made this change, it's a local delete | |
| 22:00:45 | dansmith | not if we fail the last race check, but yeah | |
| 22:01:52 | dansmith | so do we ever emit a soft-delete notification normally? | |
| 22:02:00 | dansmith | meaning before all of this changed.. | |
| 22:02:12 | dansmith | only in the local delete, which is totally not a soft delete, is that the only time we emit soft-deleted.start? | |
| 22:03:43 | mriedem | it looks like it... | |
| 22:04:34 | dansmith | how stupid is that? | |
| 22:04:44 | dansmith | the only time we emit soft-deleted is when .. we didn't | |
| 22:04:48 | mriedem | down in _delete_instance in the compute manager, even if it was a soft_delete, we issue instance.delete.start | |
| 22:06:01 | melwitt | I wonder how soft-deleted notify ever came about then. because like you said, there isn't really a soft-delete from the user perspective | |
| 22:06:02 | mriedem | dansmith: that's a good point | |
| 22:06:21 | dansmith | melwitt: right, I can't think of any reason it makes sense to emit soft-deleted | |
| 22:06:39 | dansmith | except maybe in the case where we're deferring it, which we _don't_ notify about | |
| 22:08:15 | mriedem | we'll also issue soft_delete.start/end in the case that the instance is shelved_offloaded, because that's another local delete case | |
| 22:08:26 | dansmith | yeah | |
| 22:08:28 | mriedem | so yeah, seems totally pointless to even distinguish | |
| 22:08:34 | mriedem | since you can't restore it anyway | |
| 22:08:35 | dansmith | we totally cover all the cases where we're not actually soft deleting | |