Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-08-14
15:49:14 mriedem the downside is losing some race between the api and the compute manager where the sync power states task has turned off your bm instance b/c the nova db said it should be off but it's actually on again in ironic, right?
15:49:20 tssurya and since it has a lock it should be fine
15:49:23 dansmith when Ioriginally suggested this, I was imaging the driver wholly owning the "what do we do"
15:49:51 tssurya dansmith: yea I remember you telling it in the spec design phase
15:49:52 dansmith so only ironic will be anything other than a no-op in this case, and all it needs to do is do its poweroff
15:50:00 dansmith tssurya: the lock only works within a compute node,
15:50:04 tssurya its just that the notifications/action stuff happens in the upper level
15:50:25 dansmith tssurya: so you can still race with the compute node processing this and something else trying to take action on the instance
15:50:37 mriedem something needs to handle UnexpectedTaskStateError for power-update otherwise we fail to process all of the events in the same request on the same host
15:50:38 tssurya dansmith: oh yea true
15:50:38 dansmith tssurya: meaning ironic may have sent that event, and meanwhile some user tried to reboot the instance at the same time
15:51:08 mriedem and you could lose a race with the bug you're trying to fix with this, i think, right?
15:51:09 tssurya so what's the point of moving it to the manager again ?
15:51:12 dansmith mriedem: all we need to do is handle, in the ironic case, what happens if I do instance.save(expected_Task_state=None) right?
15:51:19 mriedem tssurya: rolling upgrades for one
15:51:34 tssurya ah yes
15:51:36 dansmith tssurya: and it makes it so we don't touch the instance at all for any drivers that don't care about this
15:52:10 tssurya mriedem: and yea the downside point is valid
15:52:13 mriedem dansmith: i think so, but if the driver raises the nthe compute manager code has to handle it and not barf for the other events in the same request
15:52:53 dansmith mriedem: I think the driver should just not raise, I think the driver should handle all of this, because it's only one
15:53:29 mriedem so if the driver gets UnexpectedTaskStateError, it just logs and returns?
15:53:34 dansmith yeah
15:53:38 mriedem wfm
15:53:48 dansmith it will be contextually relevant,
15:53:52 tssurya dansmith, mriedem: wait the instance.save(expected_Task_state=None) is happening in the manager no ?
15:53:56 dansmith where compute manager won't know what the eff it means
15:54:00 tssurya and the instance.save(expected_task_state=task_states.POWERING_ON)
15:54:05 tssurya will happen in the driver
15:54:13 mriedem tssurya: no, your code is doing that in the driver
15:54:14 dansmith tssurya: I don't think so
15:54:21 mriedem it's like the only thing the driver does
15:54:30 mriedem https://review.opendev.org/#/c/645611/12/nova/virt/ironic/driver.py
15:54:38 tssurya ok so we record an instance action start before setting the task_state to POWERING_ON ?
15:54:55 mriedem the instance actoin stuff in the api doesn't change
15:54:55 mriedem imo
15:55:10 dansmith I dunno
15:55:13 mriedem that's how you know from the api if the event failed or not
15:55:14 tssurya so isn't it confusing to the user that the task_state hasn't changed
15:55:22 tssurya but there is an instance-action record/notification
15:55:25 tssurya emitted
15:55:26 dansmith mriedem: if I send this event for libvirt instance, it will record an action but nothing really happened
15:55:45 mriedem dansmith: and i'll say, you shouldn't really do that
15:55:57 mriedem same with trying to swap volumes on anything other than libvirt
15:56:29 dansmith :/
15:57:06 mriedem or extend an attached volume, or do host-assised snapshot, or anything with pmems :)
15:58:13 mriedem so https://review.opendev.org/#/c/645611/12/nova/compute/api.py@250 would move to the compute manager, or the ironic driver?
15:58:25 dansmith yeah, well, this is a minor detail
15:58:32 mriedem i'm cool with that moving to the driver too
15:58:40 mriedem until some other driver needs this same thing for whatever reason
15:58:52 dansmith I would think it moves to the driver, and really doesn't need to be all that detailed I would think, but yeah
15:59:02 dansmith I mean, I guess it could stay, I don't really care that much
15:59:04 tssurya ok I can move that whole chunk with the no-op
15:59:09 tssurya all into the driver
15:59:27 tssurya only the instance action and notification stuff remain in place in the api and manager right ?
15:59:49 mriedem the state checking in the api could save you some rpc traffic i guess, but you could be racing either way
16:00:01 tssurya but then again, is it ok for the task_state to still be None when we emit the state notification ?
16:00:01 dansmith the thing I think makes the biggest difference, is changing the power state before we know if we're going to do anything
16:00:07 tssurya start*
16:00:45 dansmith I don't know the semantics of any expectations around that
16:01:10 tssurya ok yea then maybe its fine :)
16:01:12 dansmith I didn't really think notifications are required to be realtime and synchronous, so I don't think it'd matter
16:01:35 mriedem tssurya: you can set the task_state on the instance in the compute manager before calling the driver method if we care - but then you have to handle UnexpectedTaskStateError there as well
16:01:49 dansmith but if we do that,
16:01:50 dansmith we
16:02:01 dansmith are changing it for non-ironic instances,
16:02:12 tssurya hmm yea
16:02:13 dansmith then catch the NotImplementedError, and then change it right back, yeah?
16:02:18 mriedem which won't actually power off or on the guest
16:02:33 mriedem @reverts_task_state would set the task_state back to None yes
16:03:08 mriedem but i'm not too worried about non-ironic instances because this is admin-only api stuff and if someone is abusing it for non-ironic instances and it doesn't work as expected....meh?
16:03:18 dansmith okay
16:03:45 tssurya so what't it going to be: manager or driver ?
16:03:55 tssurya at this point I don't mind either
16:04:12 dansmith "at this point" .. translation: "at this point I'm about to shoot both of you guys"
16:04:13 mriedem i forgot the question
16:04:26 tssurya dansmith: ;)
16:04:35 dansmith tssurya: I think it doesn't matter that much, figure it out when you move the code and it's probably okay
16:04:44 tssurya mriedem: meaning we move the task state and https://review.opendev.org/#/c/645611/12/nova/compute/api.py@250
16:04:46 dansmith tssurya: also when are you actually leaving?
16:05:13 mriedem right now :)
16:05:32 dansmith lol
16:05:36 dansmith #ragequit
16:05:37 mriedem (11:04:47 AM) dansmith: tssurya: also when are you actually leaving?
16:05:38 tssurya sorry bad connection
16:05:49 tssurya I leave on Friday
16:05:53 dansmith okay
16:05:56 tssurya so I can work on it this evening and tomorrow
16:06:02 dansmith okay cool
16:06:24 dansmith tssurya: so I said I think manager vs. driver probably doesn't matter that much, you choose while moving the code and it'll probably be okay
16:06:37 dansmith I don't have a strong opinion without seeing it, so maybe just have to pick one and see
16:06:41 tssurya ok awesome thanks
16:06:48 dansmith mriedem: sound okay?
16:06:49 tssurya so first I'll try with the manager and then we can see
16:06:50 mriedem i'll leave a couple of comments
16:07:22 tssurya thanks a lot dansmith and mriedem :D
16:10:48 mriedem done https://review.opendev.org/#/c/645611/12
16:13:10 tssurya ty
16:13:49 dansmith mriedem: yeah, so assuming your "don't do this for libvirt" assertion I think what you said in there is fine
16:13:49 openstackgerrit Merged openstack/nova stable/stein: Fix misuse of nova.objects.base.obj_equal_prims https://review.opendev.org/676289
16:42:22 alex_xu dansmith: mriedem sean-k-mooney anyway thanks for your time today, it is indeed I use a lot today :)

Earlier   Later