| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-09-01 | |||
| 14:54:19 | sean-k-mooney | dansmith: that wont work ^ but there in the correct order now and i can start on the tests | |
| 14:54:51 | dansmith | sean-k-mooney: okay but we can't land an RPC version that says it does a thing when it doesn't | |
| 14:55:04 | dansmith | sean-k-mooney: so I think your thing needs to be in front with "if False" or something | |
| 14:55:12 | dansmith | and I'll change that to "if the thing is enabled:" | |
| 14:55:46 | sean-k-mooney | dansmith: something like https://review.opendev.org/c/openstack/nova/+/855351/2/nova/virt/libvirt/driver.py#3879 | |
| 14:56:00 | sean-k-mooney | which is never set anywhere to True | |
| 14:56:19 | sean-k-mooney | and this if https://review.opendev.org/c/openstack/nova/+/855351/2/nova/virt/libvirt/driver.py#5011 | |
| 14:56:36 | dansmith | yeah | |
| 14:57:25 | sean-k-mooney | so i was thinking you could start a patch on top of the first one or if you want i can proably do it based on the termbin you provided | |
| 14:57:45 | sean-k-mooney | you will be quicker at that then me since its been a while since i toughed rpc but either way | |
| 14:58:19 | sean-k-mooney | im going to work on the base patch and get it to pass unit and func tests | |
| 14:58:34 | dansmith | yeah, I'm working on the rpc stuff already | |
| 14:58:50 | dansmith | I think that base patch will work now | |
| 14:59:43 | sean-k-mooney | it shoudl work in devstack im expecting the unit test to bitch about the extra paramter to _hard_reboot | |
| 15:04:15 | bauzas | hmm, are configdrivers only supported by libvirt N? | |
| 15:04:20 | bauzas | configdrives | |
| 15:04:30 | bauzas | I'm afraid of https://review.opendev.org/c/openstack/nova/+/816157/16/nova/virt/driver.py | |
| 15:04:36 | sean-k-mooney | techinially they are supproted by other drivers i think | |
| 15:04:38 | bauzas | trampling all our drivers but libvirt | |
| 15:04:39 | gibi | bauzas: nope, it is supported by other drivers too | |
| 15:04:59 | bauzas | that's super late for them to support them | |
| 15:05:07 | bauzas | s/them/this | |
| 15:05:08 | sean-k-mooney | they dont need too | |
| 15:05:13 | sean-k-mooney | they just wont be able to use this feature | |
| 15:05:26 | sean-k-mooney | i think that was in the spec | |
| 15:05:41 | sean-k-mooney | either way this was only planned to be implented by libvirt | |
| 15:05:50 | sean-k-mooney | at least for now | |
| 15:05:58 | bauzas | hmmmm | |
| 15:06:14 | gibi | so we need to detect the capability in the API and reject the reboot request | |
| 15:06:28 | bauzas | yup | |
| 15:06:29 | sean-k-mooney | thats in the patch currentlyyes | |
| 15:06:32 | gibi | if there is user_data update but the virt driver has no capability to regenerate | |
| 15:07:17 | bauzas | it wasn't stated a virt driver dependency in the spec https://review.opendev.org/c/openstack/nova-specs/+/816542/7/specs/zed/approved/update-userdata.rst | |
| 15:07:24 | sean-k-mooney | by the way we can now drop the object changes | |
| 15:07:42 | bauzas | this change seems to me more and more fragile | |
| 15:07:45 | sean-k-mooney | bauzas: well im not going to have time to work on ironic by monday | |
| 15:07:57 | bauzas | sean-k-mooney: I'm not asking you to do such things | |
| 15:08:04 | gibi | (it is not just ironice) | |
| 15:08:05 | sean-k-mooney | so hehe i think we were reving this as libvirt only or at least i was | |
| 15:08:10 | bauzas | but I point out how fragile this is | |
| 15:08:17 | sean-k-mooney | gibi: ya i know | |
| 15:08:33 | bauzas | and again, this wasn't explained in the spec | |
| 15:08:34 | sean-k-mooney | ironic would just be the most painful | |
| 15:08:55 | sean-k-mooney | bauzas: orgianly in the spec they planned to only do it for the metadta api | |
| 15:08:55 | bauzas | I was having concerns with this spec because I thought it wasn't really a needed usecase | |
| 15:09:07 | sean-k-mooney | but then we pointd out config drive exsited and need to also work | |
| 15:09:07 | bauzas | sean-k-mooney: no, they said about configdrives too | |
| 15:09:16 | sean-k-mooney | bauzas: no i asked them to add that | |
| 15:09:25 | sean-k-mooney | they wanted api only | |
| 15:09:35 | bauzas | sean-k-mooney: yes, but I also said it was a problem | |
| 15:09:35 | sean-k-mooney | id did not want this to not work if you used config drive | |
| 15:10:06 | sean-k-mooney | so as it stands any driver that does not supprot the new metond will not report the trait | |
| 15:10:19 | sean-k-mooney | that need to be called out in the api ref | |
| 15:10:26 | sean-k-mooney | or other docs for this | |
| 15:10:39 | sean-k-mooney | we could put it in the driver suport matirx i guess | |
| 15:10:56 | sean-k-mooney | that proably beter long term | |
| 15:14:07 | sean-k-mooney | dansmith: only 6 failure for the extra paramater | |
| 15:14:28 | sean-k-mooney | i actully need the driver api change in the first patch too so ill add that | |
| 15:15:12 | gibi | I'm wondering about the virt driver api change | |
| 15:15:26 | dansmith | but bauzas' point about this not being supported by other drivers is legit | |
| 15:15:37 | dansmith | having features that look like swiss cheese is very confusing for users | |
| 15:15:47 | sean-k-mooney | gibi: i could drop that now actully | |
| 15:15:49 | gibi | I asked for it originally as I thought the reboot + regenerate logic can be orchestrated from the compute manager | |
| 15:15:52 | sean-k-mooney | if we just have the parmater | |
| 15:16:03 | dansmith | even if we reject for the api caller, it's still annoying to say "reboot works, but not reboot with userdata, but only if you're using configdrive" | |
| 15:16:09 | gibi | but the actualy implementation calls the regenerate call from the virt driver | |
| 15:16:22 | dansmith | gibi: reboot is a single call to virt, | |
| 15:16:22 | gibi | so it is less useful to have a virt driver method | |
| 15:16:26 | dansmith | and the regenerate has to happen in the middle | |
| 15:16:31 | gibi | dansmith: I see now | |
| 15:16:50 | dansmith | it's pretty unideal in general for sure | |
| 15:16:59 | dansmith | just saying, I think that's why it was done this way | |
| 15:17:05 | gibi | this is why I pointing out that the separate virt driver method is probably not needed | |
| 15:17:05 | sean-k-mooney | so we dont need a new public methond in the virt driver api | |
| 15:17:20 | sean-k-mooney | its just a new parmater to hard_reboot right | |
| 15:17:22 | dansmith | no, the publicness isn't the concern I think | |
| 15:17:36 | sean-k-mooney | right i know its the uniformatiy of the api | |
| 15:17:43 | sean-k-mooney | and not having it depend on the backend | |
| 15:17:51 | bauzas | agreed with dansmith my concern isn't the publicness | |
| 15:18:05 | bauzas | this is about the fact we depend on virt drivers | |
| 15:18:07 | gibi | it would be nice to do the reboot in 3 generic steps from the compute manager: stop, regenerate, start. But this is a big change | |
| 15:18:16 | sean-k-mooney | yep i get that but we have many things that depend on the virt driver | |
| 15:18:20 | dansmith | I definitely think these kinds of pinpoint features that look generic but only work in libvirt are a problem | |
| 15:19:01 | bauzas | sean-k-mooney: well, do all the virt drivers support to create *or* update the configdrives ? | |
| 15:19:18 | dansmith | create yes | |
| 15:19:23 | dansmith | nobody supports update today AFAIK | |
| 15:19:23 | sean-k-mooney | vmware ironic libvirt yes | |
| 15:19:27 | bauzas | dansmith: that's my point, I wasn't seeing the dependency when reviewing the spec | |
| 15:19:29 | sean-k-mooney | for create | |
| 15:19:35 | sean-k-mooney | not sure about powervm | |
| 15:19:53 | dansmith | gibi: the problem with that is things like vmware where I'm not sure where the config drive actually gets created, or even if generating it on the compute node and passing it to the virt driver as a file would necessarily be reasonable | |
| 15:19:55 | dansmith | since it's a remote thing | |
| 15:20:01 | sean-k-mooney | i can take a look and see what would be required for other driver quickly | |
| 15:20:07 | sean-k-mooney | ironic is the hard one | |
| 15:20:29 | sean-k-mooney | they pass the config drive via ipa | |
| 15:21:02 | gibi | dansmith: I see | |
| 15:21:03 | dansmith | like I think ironic passes the configdrive as a string to the api, | |
| 15:21:07 | dansmith | never generates it on disk | |
| 15:21:43 | sean-k-mooney | ah that might be how nova passes it to ironci but then ironic connects it to the vm via ipa or mounting it | |
| 15:21:50 | dansmith | yeah | |