| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-14 | |||
| 15:39:47 | sean-k-mooney | so if the only schduling for vpmem is done via placmeent then the assignment could be track via the xml | |
| 15:40:13 | mriedem | or pmem affinity | |
| 15:40:18 | sean-k-mooney | mriedem: we would either need to call placement for the data or we cant | |
| 15:40:55 | mriedem | btw, are there a fair number of rhosp users using vgpus now that you're on queens? | |
| 15:41:02 | sean-k-mooney | pmem affinity(i assume numa affinity) could be modeled in the RP tree | |
| 15:41:19 | sean-k-mooney | mriedem: not that im aware of | |
| 15:41:41 | sean-k-mooney | most are using full GPU passthough when they need gpus | |
| 15:41:46 | mriedem | was just going to say that | |
| 15:41:58 | sean-k-mooney | nvida licening is $$$$ | |
| 15:42:00 | dansmith | I'll be really honest here, I think this is a very niche, very libvirt-specific, very unlikely-to-be-widely-used feature, and I think that adding a bunch of nova persistence for these things brings more impact to operations and upgrades than we need, | |
| 15:42:24 | dansmith | and storing this information purely in the place where it matters (in libvirt) limits that impact and scope a lot | |
| 15:43:10 | sean-k-mooney | dansmith: im not against that just wanted to point out we have always assumed the xmls are not required until now | |
| 15:43:11 | dansmith | if an operator changes hardware after a maint cycle that changes the ordering of these devices or something, I worry about handing persistent devices to the wrong instances, and I think keeping the mapping(s) in one place that is visible and accessible to the operators if they need to remap is also a good idea | |
| 15:43:20 | sean-k-mooney | e.g. that we can jsut regenerate them | |
| 15:43:29 | dansmith | sean-k-mooney: no, that's not true I don't think | |
| 15:43:43 | dansmith | sean-k-mooney: if we delete the instance from a guest and we restart nova I think it will freak | |
| 15:44:10 | sean-k-mooney | yes but if an operator change the xml with virsh and we do a hard rebot we jsut regenerate it | |
| 15:44:11 | dansmith | sean-k-mooney: regenerating the xml all the time does not mean that the xml is not useful data.. we use it to determine which instances are actually on this host, vs just assigned | |
| 15:44:22 | sean-k-mooney | if the domain is missing i dont knwo what happens | |
| 15:44:25 | dansmith | sean-k-mooney: not if we don't store that detail ourselves | |
| 15:44:48 | dansmith | anyway, I think I've already spent way more time on this than this feature is worth, | |
| 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 ? | |