| opendevreview | melanie witt proposed openstack/nova master: pwr mgmt: power down free PCPUS when updating compute node https://review.opendev.org/c/openstack/nova/+/932926 | 02:14 |
|---|---|---|
| *** ralonsoh_ is now known as ralonsoh | 08:08 | |
| opendevreview | Balazs Gibizer proposed openstack/nova-specs master: Extend image-properties-in-server-show spec with rebuild https://review.opendev.org/c/openstack/nova-specs/+/940360 | 08:09 |
| gibi | dansmith, melwitt, sean-k-mooney: thanks for the quick response. Here are the spec update ^^ | 08:12 |
| masahito | gibi: hi, could you review the small bug fixes for the v2.96 or later API bug if you have time? It has been +2ed already so it needs one more W+1. https://review.opendev.org/c/openstack/nova/+/939673 and https://review.opendev.org/c/openstack/nova/+/939658 | 08:17 |
| gibi | masahito: looking... | 08:21 |
| gibi | masahito: I've approved the reproducer and left some API concerns in the fix. We probably need other cores to express opinion about the API impact. | 08:48 |
| gibi | sean-k-mooney: ^^ | 08:49 |
| *** tkajinam is now known as Guest7334 | 09:05 | |
| masahito | gibi: thanks for the quick review :) | 09:33 |
| mikal | sean-k-mooney: that SPICE patch series has now been updated and passes CI. | 09:34 |
| opendevreview | Mark Goddard proposed openstack/nova master: ironic: Let Ironic handle deployment cleanup actions during destroy https://review.opendev.org/c/openstack/nova/+/883411 | 09:57 |
| stephenfin | sean-k-mooney: feck :( | 10:02 |
| stephenfin | sean-k-mooney: Yeah, that checks out. Let me think on it. If you're doing further reviews, maybe just ignore/call out the places I've made those assumption and move on. I'll rejig | 10:04 |
| sean-k-mooney[m] | sure we have merged one already the flavor acess schema | 10:05 |
| sean-k-mooney[m] | we should fix that | 10:05 |
| sean-k-mooney[m] | but there are very few apis that take either a project id or user id | 10:05 |
| stephenfin | sean-k-mooney[m]: Potential proposal. I'm going to drop https://review.opendev.org/c/openstack/nova/+/937013 from the stack and see how clean the rebase is | 10:15 |
| stephenfin | If it's horrific, I might keep it and rejig the commit message to indicate that no, these things aren't UUIDs but they often look like UUIDs so this is a little more realistic?? | 10:15 |
| sean-k-mooney[m] | sure, i would jsut assert that its a stiring between 1 and 64 printable characters | 10:16 |
| stephenfin | Hopefully not an issue but I'd _love_ to avoid a multi-hour rebase if possible š | 10:16 |
| stephenfin | printable or ascii? | 10:16 |
| stephenfin | *alphanumeric | 10:16 |
| stephenfin | (if you know off the top of your head) | 10:17 |
| sean-k-mooney[m] | you can look at the keystone def its effectivly base64 | 10:17 |
| sean-k-mooney[m] | i think | 10:17 |
| sean-k-mooney[m] | alphanumeric and _ perhaps - | 10:17 |
| sean-k-mooney[m] | i dont recall exactly but a subset of ascii | 10:18 |
| sean-k-mooney[m] | stephenfin: https://github.com/openstack/keystone/blob/master/keystone/api/validation/parameter_types.py#L67-L73 | 10:19 |
| stephenfin | ideal, copy-paste job so | 10:21 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server topology API https://review.opendev.org/c/openstack/nova/+/940300 | 10:39 |
| frickler | well this is more explicit, so likely for domains the full answer is "UUID4 without dashes or 'default'"? https://opendev.org/openstack/keystone/src/branch/master/keystone/resource/schema.py#L249 | 10:53 |
| sean-k-mooney | frickler: domain are not used in any nova api | 10:54 |
| sean-k-mooney | project and user ids are and have different rules | 10:54 |
| frickler | ah, ok | 10:55 |
| sean-k-mooney | frickler: user ids for federated keystone are the first 64 bytes of the sha256 of the user objects fields by default btu thte hash is both plugable and configurable | 10:56 |
| sean-k-mooney | frickler: this basically all stems out of the fact that when using ldap/active directory they dont produce uuids for the identifer | 10:56 |
| sean-k-mooney | so orginally keystone pass that through directly | 10:57 |
| sean-k-mooney | the sql backend that most use does produced the hex dump of a uuid4 | 10:57 |
| sean-k-mooney | its one of those things where it will almost alwasy be a uuid in practice but in reality we cant depend on that | 10:59 |
| sean-k-mooney | frickler: my real concer is that the sdk or some other code has made the same assumation | 11:00 |
| sean-k-mooney | quick glance says no, they are constucitng fake ides usign uuid4 but prepeending user-id https://github.com/openstack/openstacksdk/blob/c6459ae0b0983530d8db75178bbfe213c8022396/openstack/tests/unit/identity/v3/test_proxy.py#L38 | 11:04 |
| opendevreview | John Garbutt proposed openstack/nova master: ironic: Fix ConflictException when deleting server https://review.opendev.org/c/openstack/nova/+/883411 | 11:11 |
| johnthetubaguy | sean-k-mooney: I have reworded the commit message on this old patch, because it is now really urgent. We have a big regression on the Ironic driver after merging those convert to SDK patches, basically we lost the ability to retry on HTTP Conflict for the silly unplug_vif code. This old patch looks to fix the problem. It would be ace if you have chance to review this one please (you added some comments saying you wanted to look at | 11:17 |
| johnthetubaguy | this in the past): https://review.opendev.org/c/openstack/nova/+/883411 | 11:17 |
| opendevreview | Merged openstack/nova master: Add ServersViewBuilderTestV296 unit test class https://review.opendev.org/c/openstack/nova/+/939673 | 11:29 |
| sean-k-mooney | johnthetubaguy: so retrying on http conflict is not really a thing we shoudl do in general | 11:32 |
| sean-k-mooney | a 409 means the request is invaild for the current state and we shoudl not expect resending it to work | 11:33 |
| sean-k-mooney | johnthetubaguy: with that said i dont crrently have the context of this patch loaded so i will take a look | 11:34 |
| sean-k-mooney | johnthetubaguy: i had previoulys agreed to allow fixign the current sate even thought he ironci workfow violates our driver api | 11:39 |
| johnthetubaguy | [@_oftc_sean-k-mooney:matrix.org](https://matrix.to/#/@_oftc_sean-k-mooney:matrix.org)I agree with you, we should add more retry on conflict back in. But in this specific case, after we retry for around 15-30 mins, what we get is Bad request, and only then we consider the operation complete. Basically when unprovision has succeded, all the other work has already been done for us. | 11:39 |
| sean-k-mooney | so johnthetubaguy with that in mind i assume you have tested this? | 11:39 |
| sean-k-mooney | we do have a greeen ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa but that boot and deelete like 1 instance | 11:40 |
| johnthetubaguy | [@_oftc_sean-k-mooney:matrix.org](https://matrix.to/#/@_oftc_sean-k-mooney:matrix.org)I haven't tested this on master, but it has been running in some old clouds for a while. I haven't heard back if this works on Caracal yet either I am afraid. Just had this reported to me yesterday evening internally. | 11:41 |
| sean-k-mooney | so this obviouly does nto entrily break everythign but it does not validate that the node can be used again | 11:41 |
| sean-k-mooney | johnthetubaguy: ack | 11:41 |
| johnthetubaguy | Sure, but we call unprovison, and that goes into either error or available, let me find that graph a sec in the ironic docs to check. | 11:42 |
| sean-k-mooney | johnthetubaguy: that not really the issue | 11:42 |
| johnthetubaguy | OK, what bit are you worried about? I could easily be missing something? | 11:43 |
| sean-k-mooney | so there are two conferns one is practically does this work which is what you were answering. and i trust you when you say that unprovision will do all the actions that cleanup_deploy does | 11:43 |
| sean-k-mooney | the issue with that is ironic is not allow to modify neturon ports or cinder volume attachments assocated with a nova instnace. so it doing the cleanup as part of unprovision by makign api calls ironic si not allowed to do | 11:44 |
| sean-k-mooney | so long term that is a probelem that shoudl be fixed | 11:45 |
| johnthetubaguy | Ah, I was missing your concern before. Now I see what you mean. It is a ādifferent danceā with Ironic, for sure. And we do hit issues because of the oddities going on there, e.g.: https://bugs.launchpad.net/nova/+bug/2085432 | 11:46 |
| sean-k-mooney | i agreed that we can move forward with the current state. that does not mean the current state is correct. i condier this to just be a workaround | 11:46 |
| sean-k-mooney | johnthetubaguy: well its not jsut that its also this https://security.openstack.org/ossa/OSSA-2023-003.html | 11:48 |
| johnthetubaguy | I mean, this is Nova better respecting Ironicās API semantics, in my book, but I donāt think we have connects on what we want it to look like. I know Ironic are keen to re-work the networking stack for their standalone cases, and making NGS suck much less would be aceā¦ I should take a look at that soon ish. | 11:49 |
| johnthetubaguy | Ah, so, volumes and Ironic, I mostly hard ignore myself. So that probably is an aspect we need someone to test better. Or at least get some lovely Ironic people to take a look at our reasoning here. | 11:50 |
| sean-k-mooney | form my perspecitve ironci is not allwoed ot make any calls to neutorn or cinder for ports or volumes port are assocated with a nove instance | 11:50 |
| sean-k-mooney | johnthetubaguy: nova does not allow you to add/remove voluvmes or port to/form an instnace via the cidner/neutrion api inluciding createing/modifying volume attachments or port bindings | 11:51 |
| sean-k-mooney | johnthetubaguy: ironic is doign both | 11:51 |
| johnthetubaguy | Yeah, true. | 11:51 |
| sean-k-mooney | so unprovision in ironic should not be cleanign up the neutorn prots ro removing the volume attahcments | 11:52 |
| sean-k-mooney | that shoudl be done by our generic code in the compute manager as part of the instance delete flow | 11:53 |
| sean-k-mooney | johnthetubaguy: anyway what importnt right now is this is impacting actual users | 11:53 |
| sean-k-mooney | so we need to do the best we can in the current state | 11:54 |
| sean-k-mooney | but if we have simialr CVEs in the future ironic integation may break | 11:54 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for hosts APIs https://review.opendev.org/c/openstack/nova/+/937047 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for instance actions https://review.opendev.org/c/openstack/nova/+/937048 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for hypervisors APIs (1/3) https://review.opendev.org/c/openstack/nova/+/937245 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for hypervisors APIs (2/3) https://review.opendev.org/c/openstack/nova/+/937246 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for hypervisors APIs (3/3) https://review.opendev.org/c/openstack/nova/+/937247 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server IPs APIs https://review.opendev.org/c/openstack/nova/+/937608 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for remote consoles https://review.opendev.org/c/openstack/nova/+/940114 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for keypairs APIs https://review.opendev.org/c/openstack/nova/+/940115 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for image metadata APIs https://review.opendev.org/c/openstack/nova/+/940299 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server topology API https://review.opendev.org/c/openstack/nova/+/940300 | 11:58 |
| opendevreview | Stephen Finucane proposed openstack/nova master: api: project/tenant and user IDs are not UUIDs https://review.opendev.org/c/openstack/nova/+/940369 | 11:58 |
| sean-k-mooney | johnthetubaguy: anyway +2 | 11:59 |
| sean-k-mooney | johnthetubaguy: there is another ironci change that you might want to take a look at if you feel like doing a review | 12:00 |
| sean-k-mooney | it fixes a regression added in dalmation so you may not have hit it yet | 12:01 |
| sean-k-mooney | https://bugs.launchpad.net/nova/+bug/2092570 | 12:03 |
| sean-k-mooney | johnthetubaguy: https://review.opendev.org/c/openstack/nova/+/939411 | 12:03 |
| johnthetubaguy | I will take a look now. | 12:40 |
| *** whoami-rajat_ is now known as whoami-rajat | 14:02 | |
| *** Guest7381 is now known as bbezak | 14:56 | |
| johnthetubaguy | [@_oftc_sean-k-mooney:matrix.org](https://matrix.to/#/@_oftc_sean-k-mooney:matrix.org)got distracted by chasing outage fun, but I am +2 that patch. I don't feel in-sync enough to +W right now, but that sure looks broken. I guess we don't test rebuild in that ironic CI you mentioned? | 15:09 |
| johnthetubaguy | (as in we look broken without that patch) | 15:09 |
| sean-k-mooney | we have exactly 1 job and it boot 1 vms and deletes it | 15:09 |
| sean-k-mooney | so ya no rebuild testing | 15:09 |
| sean-k-mooney | we really shoudl fix that... | 15:09 |
| sean-k-mooney | this is a simple case where mypy and type checking would have saved use. we just mixed up the paramater order | 15:10 |
| sean-k-mooney | johnthetubaguy: if no one else has ocmmend ill loop back and +w next week | 15:10 |
| sean-k-mooney | mikal: looking at https://review.opendev.org/c/openstack/nova/+/924844 melwitt comment form pathset 18 have not been adressed | 15:11 |
| kpdev | hi all, need review on https://review.opendev.org/c/openstack/nova/+/927474 | 15:12 |
| kpdev | I had also mentioned at https://etherpad.opendev.org/p/nova-bug-triage-20240904 | 15:12 |
| sean-k-mooney | mikal: sorry https://review.opendev.org/c/openstack/nova/+/924844/24 | 15:13 |
| sean-k-mooney | mikal: i shoudl slow down https://review.opendev.org/c/openstack/nova/+/924844/24/nova/api/openstack/compute/remote_consoles.py the api change relate to the get_spice_console api that was remvoed in 2.6 | 15:14 |
| sean-k-mooney | kpdev: that change is not valid | 15:15 |
| kpdev | @sean-k-mooney: what is missing ? Can you please reply on pull request ? | 15:17 |
| sean-k-mooney | the bug report is invliad | 15:17 |
| sean-k-mooney | if we were to implemnt what you propsoe it would be a feature with a non tivial upgrade impact | 15:17 |
| sean-k-mooney | kpdev: nova assumes that all cpus can be use by default, if that is not the case you are requried to use the [compute]cpu_shared_set and [compute]cpu_dedicated_set or the legacy [default]vcpu_pin_set to specify which cpu can be used | 15:19 |
| sean-k-mooney | if your host cgroups are configure in such a way as they cannot be assigned to the vm becuase of the cpu_exclisve bit or otherwise and you do not use the provided config option that is not a nova bug | 15:20 |
| kpdev | so you mean, using provided config option there is no need of this PR | 15:21 |
| sean-k-mooney | correct i dont think this change is required if you correctly confire nova via the cpu_share_set | 15:22 |
| sean-k-mooney | nova should not be parssing the cgoup configuration to determin which cpu can be used | 15:23 |
| sean-k-mooney | the the installer shoudl be configuring that via the aviabel options | 15:23 |
| kpdev | ok | 15:25 |
| kpdev | I have added this on bug and update after checking with engineer | 15:26 |
| sean-k-mooney | i have tiraged the bug as opipion, if you stongly feel that the ux should be imporved i think we would need to have a disgin dicussion on how before proceedign with this as a new feature | 15:32 |
| sean-k-mooney | i dont think this is in the scope of nova to adress however | 15:32 |
| kpdev | let me check internally as you are saying its not then in scope of nova to address. | 15:37 |
| opendevreview | Rajesh Tailor proposed openstack/nova master: Add support for showing image properties in server show response https://review.opendev.org/c/openstack/nova/+/939649 | 15:43 |
| melwitt | sean-k-mooney: hey I just read your comment on the power management patch. are you saying that you think we should not be offlining CPU for STOPPED instances? | 16:26 |
| opendevreview | Masahito Muroi proposed openstack/nova master: Use defaultdict object for request_specs_dict in the _list_view https://review.opendev.org/c/openstack/nova/+/939658 | 17:35 |
| opendevreview | Masahito Muroi proposed openstack/nova master: Use defaultdict object for request_specs_dict in the _list_view https://review.opendev.org/c/openstack/nova/+/939658 | 17:43 |
| masahito | gibi sean-k-mooney: thank you for the review for the patch. I did the further investigation for the current response value pattern. Please check the result and document update, too. https://review.opendev.org/c/openstack/nova/+/939658 | 17:45 |
| mikal | sean-k-mooney: I think you're saying you just want the changes to that file that you commented on removed? Is that right? | 18:47 |
| sean-k-mooney | so im unsure | 18:47 |
| sean-k-mooney | mikal: melwitt made a comment that that api was remvoed in 2.6 | 18:47 |
| mikal | sean-k-mooney: IIRC Mel was confused by an old removed API and in the end I didn't need to do anything. I am trying to reload state on that file at the moment. | 18:48 |
| sean-k-mooney | so we shoudl not need to update it | 18:48 |
| sean-k-mooney | and as a result we likely dont need that addtional validation schema | 18:48 |
| mikal | sean-k-mooney: yeah, Mel wanted a test for the HTTP GET on that API, which was removed. | 18:48 |
| sean-k-mooney | because if you were to use the 2.5 microversion it shoudl not return the new spice console anyway | 18:49 |
| sean-k-mooney | so it was removed in a micorversion but we did not remove the functionaltiy | 18:49 |
| sean-k-mooney | if we did the funciton woudl be returnign a 410 | 18:49 |
| melwitt | this sounds familiar and maybe I forgot to reply (and don't remember at the moment) | 18:50 |
| sean-k-mooney | melwitt: context is i started reviewing mikal spice console serise again yesterday | 18:50 |
| sean-k-mooney | and noticed your comment did not seam to be adressed | 18:50 |
| mikal | So I can revert the changes to that file. Honestly I'm not across the validation stuff much, but if you think that's correct I believe you. | 18:51 |
| melwitt | ok, I'll take a look | 18:51 |
| mikal | sean-k-mooney: I'm just removing the validation for the HTTP GET right, not the POST? | 18:52 |
| sean-k-mooney | mikal: i think you can revert the changes to that file and also remove the other changes melwitt mention in ps 18 | 18:52 |
| sean-k-mooney | i would also need ot load context | 18:52 |
| sean-k-mooney | im looking for the patch now | 18:52 |
| sean-k-mooney | ok so https://review.opendev.org/c/openstack/nova/+/924844/24/nova/api/openstack/compute/remote_consoles.py | 18:53 |
| sean-k-mooney | so the changes to create are correct | 18:53 |
| sean-k-mooney | schema.get_spice_console_v298 is not needed and the changes to get_spice_console can be reverted | 18:54 |
| mikal | Ok, doing that now. | 18:54 |
| sean-k-mooney | mel mention you can remvoe the scheme here https://review.opendev.org/c/openstack/nova/+/924844/comment/bcfe7166_8d9bb51c/ | 18:55 |
| sean-k-mooney | mikal: i think all the other comments were adress looking over them quickly | 18:56 |
| mikal | Yeah honestly I thought I'd addressed them all but I definitely missed that one. | 18:56 |
| sean-k-mooney | beyond that i dont see anything quickly that i would -1 for | 18:57 |
| sean-k-mooney | i need to finsih reading the release note but i suspect we could proceed ith it as is | 18:57 |
| sean-k-mooney | or adress other issue in a followup | 18:57 |
| mikal | As an aside, I learned the other day that libvirt's equivalent to oVirt VDSM hooks _does_ _not_ allow modifying the domain XML or qemu command line on instance start. There are some times you can change the XML, mostly around live migration, but not for a domain create. | 18:58 |
| sean-k-mooney | oh ya one trivial issue in the release note | 18:58 |
| sean-k-mooney | "spice-direct" shoudl be ``spice-direct`` | 18:58 |
| sean-k-mooney | double backtics to make it bold | 18:58 |
| sean-k-mooney | btu honstly it fine with quotes | 18:58 |
| mikal | I cna fix that now too. | 18:58 |
| sean-k-mooney | same for spice_direct_proxy_base_url | 18:59 |
| mikal | Do you want it bold in the API reelase notes too? | 18:59 |
| sean-k-mooney | you mena the api ref | 18:59 |
| mikal | nova/api/openstack/compute/rest_api_version_history.rst | 18:59 |
| sean-k-mooney | ah the microversion history doc | 19:00 |
| sean-k-mooney | it does not hurt to call it out | 19:00 |
| sean-k-mooney | but up to you | 19:00 |
| sean-k-mooney | i was more commenting on not useing "" for empahsie | 19:00 |
| sean-k-mooney | *emphasis | 19:00 |
| mikal | Ok | 19:00 |
| sean-k-mooney | once that is resovled i think im +2 on the 3 main patches | 19:02 |
| sean-k-mooney | ill try and review the remaing too now while i have context | 19:03 |
| mikal | Thank you. I am in the process of prepping to upload a new version now that address those comments. | 19:04 |
| opendevreview | Michael Still proposed openstack/nova master: libvirt: direct SPICE console object changes https://review.opendev.org/c/openstack/nova/+/926876 | 19:08 |
| opendevreview | Michael Still proposed openstack/nova master: libvirt: direct SPICE console database changes https://review.opendev.org/c/openstack/nova/+/926877 | 19:08 |
| opendevreview | Michael Still proposed openstack/nova master: libvirt: allow direct SPICE connections to qemu https://review.opendev.org/c/openstack/nova/+/924844 | 19:08 |
| opendevreview | Michael Still proposed openstack/nova master: libvirt: Add extra spec for sound device. https://review.opendev.org/c/openstack/nova/+/926126 | 19:08 |
| opendevreview | Michael Still proposed openstack/nova master: libvirt: Add extra specs for USB redirection. https://review.opendev.org/c/openstack/nova/+/927354 | 19:08 |
| sean-k-mooney | mikal: im just finishing for today. im +2 on the first 3 pathces and -1 for the last two, comments in line | 20:17 |
| mikal | Ok cool, I'll take a look at the comments today. | 20:32 |
| opendevreview | Michael Still proposed openstack/os-traits master: Add traits for sound and USB devices on instances. https://review.opendev.org/c/openstack/os-traits/+/940418 | 23:24 |
| mikal | sean-k-mooney: ^--- a first swing at the os-traits change | 23:25 |
| opendevreview | Michael Still proposed openstack/os-traits master: Add traits for sound and USB devices on instances. https://review.opendev.org/c/openstack/os-traits/+/940418 | 23:45 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!