| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-02-04 | |||
| 19:08:21 | sean-k-mooney | and do a release | |
| 19:22:02 | mikal | Any thoughts on if there is anything missing from that sound patch now? If you think its basically complete I'll go spend some time doing the same thing to the USB one. | |
| 19:24:14 | sean-k-mooney | mikal: im going to try and finsih aroudn the top of the hour ill see if i can review it before i do. | |
| 19:24:44 | mikal | sean-k-mooney: cool, thanks. No pressure. If you don't get to it I'll just move onto the USB one anyways and I can always circle back. | |
| 19:24:58 | sean-k-mooney | actully looking at the change files im not seeing a comptue service bump or min version check | |
| 19:25:32 | mikal | Yeah, that's what I was asking above -- I assume I need those, but I haven't done them before. Where are they? | |
| 19:26:01 | sean-k-mooney | if we go the traits route without those we need a addtional change to enforce the trait request. | |
| 19:26:07 | sean-k-mooney | ah am one sec | |
| 19:26:40 | sean-k-mooney | the service version is defiend here https://github.com/openstack/nova/blob/master/nova/objects/service.py#L34 | |
| 19:27:55 | mikal | Hmmm. Bump twice or bump that at the end after the sound and USB traits have both landed? | |
| 19:28:25 | sean-k-mooney | when we add a new feature tha tneed supprot form the compute manger we add a new constant here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L106-L126 | |
| 19:28:46 | sean-k-mooney | and then add a check on the relevent api action i.e spawn and rebuild in yoru case | |
| 19:29:50 | sean-k-mooney | so we tend to define a check funciton like this _check_support_vnic_accelerator https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1054C9-L1054C40 | |
| 19:30:00 | mikal | Hmmm. How do you define support there? Is that any API request that wants USB or sound, or any API request which needs a compute manager with the SPICE direct code? | |
| 19:30:35 | sean-k-mooney | so you are addign a genic feature to add usb device or sound cards | |
| 19:30:37 | mikal | That is, for the SPICE direct console type to work, you cannot be running an ancient compute manager because it would generate the wrong domain XML. | |
| 19:30:58 | sean-k-mooney | so its not spice specific its server creeate which we validate in _validate_and_build_base_options | |
| 19:31:17 | sean-k-mooney | like this https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1151-L1171 | |
| 19:31:20 | sean-k-mooney | and rebuild | |
| 19:31:29 | mikal | Ahhh, ok. And then the API microversion protects the overall SPICE direct feature I suppose. | |
| 19:31:37 | sean-k-mooney | correct | |
| 19:31:44 | mikal | Ok, that makes sense to me. | |
| 19:32:05 | mikal | In that case I'll take a swing at doing that version bump and so forth today before I move onto the USB traits stuff. | |
| 19:32:10 | sean-k-mooney | the min compute service check it just to make sure we dotn try to create the vm on a compute node that does not know about the new image properties | |
| 19:32:33 | sean-k-mooney | ya so there should be 2 bumps one for sound and one for usb | |
| 19:32:39 | mikal | Yep. | |
| 19:33:09 | sean-k-mooney | the sound traits are really just for schduing to a host that supprot virtio since that is the only one that is not supproted by our min libvirt | |
| 19:33:35 | sean-k-mooney | without the trait we woudl jsut get a late build error with the instance in error | |
| 19:33:58 | mikal | How does the API microversion protect against old compute managers though? Is it simply a case that we require operators upgrade hypervisors before the API servers and that protects against compute managers which don't know how to generate that domain XML? | |
| 19:34:17 | sean-k-mooney | no the api does not | |
| 19:34:30 | sean-k-mooney | for one we now supprot n-2 for slup release | |
| 19:34:43 | sean-k-mooney | so we coudl have an epoxy api and caracal compute | |
| 19:34:56 | sean-k-mooney | the microvsion does not help with that | |
| 19:35:08 | sean-k-mooney | that what the min compute service check in the api is for | |
| 19:35:48 | mikal | Yeah, so I'd need a version check in get_console() (or whatever it was called) as well then right? | |
| 19:35:54 | mikal | Not just spawn and rebuild? | |
| 19:36:02 | sean-k-mooney | not in this case | |
| 19:36:10 | sean-k-mooney | get console does not actully create a spice console | |
| 19:36:17 | opendevreview | Ghanshyam proposed openstack/nova master: Cleanup RBAC jobs in check/gate pipeline https://review.opendev.org/c/openstack/nova/+/940737 | |
| 19:36:17 | opendevreview | Ghanshyam proposed openstack/nova master: [Trivial] Fix the typo error https://review.opendev.org/c/openstack/nova/+/937744 | |
| 19:36:30 | mikal | Oh true, it just wires it up. Its already in the domain XML by that point. | |
| 19:37:06 | sean-k-mooney | i need to check your patch but the sound device is only added if you request it right | |
| 19:37:19 | opendevreview | Ghanshyam proposed openstack/nova master: Cleanup RBAC jobs in check/gate pipeline https://review.opendev.org/c/openstack/nova/+/940737 | |
| 19:37:22 | sean-k-mooney | via the image property | |
| 19:37:40 | sean-k-mooney | well or flavor extra spec | |
| 19:37:46 | mikal | sean-k-mooney: that is my recollection, but I'd have to double check too. | |
| 19:38:09 | sean-k-mooney | ok so your supproting extra specs so we also need to account for resize | |
| 19:38:47 | sean-k-mooney | the server create check prevcent new isntnace usign the fature until everything is upgraded and resize/rebuild checks prevent exisitng instnce form using it until the cloud is fully upgraded | |
| 19:39:03 | mikal | Yeah, if you don't specify a sound model then you don't get one in the domain XML according to _add_sound_device() | |
| 19:39:19 | mikal | Ok. | |
| 19:39:24 | sean-k-mooney | ack thats what i was expecting | |
| 19:39:52 | mikal | Yeah, I'm just old and forgetful. This code is probably over six months old now and I forget some of the finer details sometimes. | |
| 19:40:50 | sean-k-mooney | i dont expect perferct recall, i reviewd it 5 days ago and while i tought that what it did i still had to check | |
| 19:41:26 | sean-k-mooney | its sometimes hard to seperate on irc, what does it actully do, vs what woudl i do, without actully looking at the patch | |
| 19:42:36 | mikal | So yeah, I think I have a plan for today at least then. I'll do the sound compute service bumps and checks and hopefully have something for you to take a look at while I'm asleep. | |
| 19:42:51 | mikal | Thanks again for your help. | |
| 19:42:57 | sean-k-mooney | so other then the version check there is only one other thing that is possibel missing | |
| 19:42:59 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/926126/12/nova/virt/libvirt/driver.py | |
| 19:43:24 | sean-k-mooney | you are not checkign if libvirt suprpot the requested sound device modle for virtio | |
| 19:43:52 | sean-k-mooney | oh | |
| 19:44:00 | sean-k-mooney | never mind that version 12 of 26 | |
| 19:44:30 | sean-k-mooney | ok in the latest veriosn that is still missing | |
| 19:44:34 | mikal | No I think you're still right. I don't think the domain XML generation checks if the hypervisor has virtio sound. | |
| 19:44:50 | mikal | Yeah, I had assumed the trait would protect against incoming requests that couldn't be supported. | |
| 19:45:17 | sean-k-mooney | so the trait can but you missing a addtional change to enforce it | |
| 19:45:45 | sean-k-mooney | oh maybe not | |
| 19:46:00 | mikal | Ok, I can add one there as well. I presume I just raise an exception if I can't support virtio sound there? | |
| 19:46:02 | sean-k-mooney | well you added https://review.opendev.org/c/openstack/nova/+/926126/26/nova/scheduler/request_filter.py | |
| 19:46:29 | sean-k-mooney | mikal: ya i think i suggested one in a previous revision | |
| 19:48:44 | sean-k-mooney | mikal: so looking at https://review.opendev.org/c/openstack/nova/+/926126/26/nova/scheduler/request_filter.py | |
| 19:49:14 | sean-k-mooney | they way you chose to add an uncontintiol prefilter means we coudl skip the min version check | |
| 19:49:30 | sean-k-mooney | as that trait will only be present if the host is upgraded | |
| 19:49:45 | sean-k-mooney | actully not that only covers the virtio cases | |
| 19:49:53 | sean-k-mooney | so we need both | |
| 19:50:16 | sean-k-mooney | it does mean you dont need the check in the xml generation as currently written | |
| 19:50:23 | mikal | All the other sound models have been supported for ages though, so apart from virtio do any of them actually need a check? | |
| 19:50:27 | sean-k-mooney | as you will never get schduled to a host that does not suport virtio | |
| 19:50:51 | sean-k-mooney | no only virtio need to be checked | |
| 19:51:13 | mikal | So doesn't that unconditional prefilter mean I don't need a service bump at all then? | |
| 19:51:14 | sean-k-mooney | we still need the min versin check to protect non upgraded hosts | |
| 19:51:40 | sean-k-mooney | no we need to make sure if you request a diffent model that you land on an upgrade host | |
| 19:51:47 | sean-k-mooney | its almost enouch but not quite | |
| 19:51:59 | mikal | Ok | |
| 19:52:23 | sean-k-mooney | for usb we dont need a prefilter like this since all host that have the correct compute service verion will supprot it | |
| 19:53:18 | sean-k-mooney | mikal: so ya only the comptue service check is missing form the sound change you do not need the xml generation check | |
| 19:53:19 | mikal | So I'm a little confused about why we need USB controller traits at all then? | |
| 19:53:32 | sean-k-mooney | the usb ones are not needed | |
| 19:53:39 | sean-k-mooney | only the virtio one for sound is really required | |
| 19:53:54 | mikal | Yeah, because the others are basically hard coded to true. | |
| 19:53:59 | sean-k-mooney | yep | |
| 19:54:08 | mikal | I guess maybe it matters if another hypervisor wanted to use these extra specs... | |
| 19:54:42 | mikal | That is, I don't think I should remove them from the os-traits patch. | |
| 19:54:43 | sean-k-mooney | technially yes | |
| 19:55:01 | sean-k-mooney | the are ok to keep IMO | |
| 19:55:35 | mikal | I should let you escape. I'll take a swing at the version checks today. | |
| 19:56:01 | sean-k-mooney | ack. i may or may not be back briefly later but if not ill try and review again tomorrow | |
| 19:56:10 | sean-k-mooney | mikal: did you see the nova meeting conversation | |
| 19:56:29 | mikal | About the order of landing the API changes? Yes, that made me happy to be completely honest. | |
| 19:57:03 | sean-k-mooney | cool, ya so hopefully we can get at least the first 3 patches and perhaps all 5 merged this week if others have time to review | |
| 19:57:26 | mikal | I appreciate you handholding me along the way. Thank you. | |