Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-03-09
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,
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

Earlier   Later