| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-14 | |||
| 15:45:11 | dansmith | and the column in the db to just dump a blob of data into instance_extra was already merged before this was all discussed, | |
| 15:45:38 | dansmith | so the easiest thing is to just let that become a dumping ground for all this stuff, regardless | |
| 15:46:11 | dansmith | alex_xu: really sorry for ever even involving myself in this, my apologies | |
| 15:46:37 | mriedem | onto tssurya's problem! | |
| 15:46:47 | tssurya | yayy | |
| 15:46:48 | dansmith | I was just goign to say | |
| 15:46:55 | dansmith | tssurya: I missed if there was a reply on the plan | |
| 15:47:10 | tssurya | not yet waiting for mriedem's opinion | |
| 15:47:13 | mriedem | she's just asking if i agree with changing task_state in hte api | |
| 15:47:15 | dansmith | yeah | |
| 15:47:20 | dansmith | didn't see a response on that | |
| 15:47:22 | tssurya | let a comment on the patch: https://review.opendev.org/#/c/645611/ | |
| 15:47:25 | alex_xu | dansmith: sorry, I'm trying my best make it simple and easy. but yea, i still found those issue need help | |
| 15:47:25 | tssurya | left* | |
| 15:47:54 | tssurya | also efried ^ in case you have an opinion | |
| 15:47:59 | mriedem | so if on power-update we don't check or set task_state in the api, we avoid the "instance is stuck witk with non-none task_state b/c it's on a stein compute" issue | |
| 15:48:18 | tssurya | right only to do the same in the manager | |
| 15:48:28 | mriedem | and thhe driver / compute manager would need to handle the UnexpectedTaskStateError | |
| 15:48:36 | dansmith | yep, and it moves the "do we do anything about this" closer to the thing that makes that decision | |
| 15:49:01 | tssurya | why does the driver have to handle UnexpectedTaskStateError ? | |
| 15:49:09 | dansmith | tssurya: it doesn't I don't think | |
| 15:49:13 | tssurya | I would be moving the task_state saving part into the manager | |
| 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 | dansmith | tssurya: meaning ironic may have sent that event, and meanwhile some user tried to reboot the instance at the same time | |
| 15:50:38 | tssurya | dansmith: oh yea true | |
| 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 | imo | |
| 15:54:55 | mriedem | the instance actoin stuff in the api doesn't change | |
| 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 | 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:01 | tssurya | but then again, is it ok for the task_state to still be None when we emit the state notification ? | |
| 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 | |