| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-02-04 | |||
| 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. | |
| 19:57:47 | mikal | I think that gaming company probably still want the USB one at least, so it would be nice to get all of them in, but I understand we're starting to run out of time. | |
| 19:58:29 | sean-k-mooney | we still ahve 3 weeks so it think its doabel if we get the first 3 this week | |
| 19:58:46 | mikal | This makes me happy. | |
| 19:58:57 | mikal | Well, I'll let you get on with your life. | |
| 19:58:59 | sean-k-mooney | i think there name was shadow https://shadow.tech/ i think | |
| 19:59:55 | mikal | Yeah, that's them. | |
| 20:00:21 | mikal | Maxime Lubin was commenting on the USB patch at one point, but I haven't seen anything recently. | |