| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-09-04 | |||
| 14:05:02 | bauzas | this is honestly super weird to have that in our API | |
| 14:05:23 | sean-k-mooney | ya that was a pebkac, my keyboard was not aligent so i was typing off center | |
| 14:05:26 | bauzas | I don't disagree with sean-k-mooney when he said we should have stopped to provide it | |
| 14:05:53 | bauzas | but now we have it in detailed-list instead of a specific API call... so that's even worst | |
| 14:06:11 | dansmith | yep, just got to fix it, not the end of the world | |
| 14:08:44 | gibi | ^^ +1 | |
| 14:09:51 | bauzas | dansmith: using a periodic that would cache some raw value ? | |
| 14:10:33 | dansmith | bauzas: just use whatever periodic we use to update the metrics and/or RT IMHO | |
| 14:12:04 | bauzas | I guess we would need to modify the API docs to say that the uptime value can be a bit stale | |
| 14:12:57 | dansmith | does the api guarantee or suggest that it's live? | |
| 14:13:28 | jkulik | with the call taking 40s, I guess it wasn't live already for a lot of them :D | |
| 14:13:39 | dansmith | jkulik: excellent point :D | |
| 14:19:26 | bauzas | dansmith: good point, /me looks at the existing docs | |
| 14:21:35 | bauzas | well, nothing is specifically said, so I guess we can just fix it silently | |
| 14:21:44 | dansmith | :) | |
| 14:23:12 | bauzas | "stats" can be a good CN object field candidate for persisting the value | |
| 14:23:22 | bauzas | that's a dict of strings | |
| 14:23:31 | bauzas | sean-k-mooney: ^ | |
| 14:24:33 | dansmith | IIRC both stats and metrics are actually used for something, but I don't remember all the details | |
| 14:25:11 | opendevreview | Rajesh Tailor proposed openstack/nova stable/2024.2: Fix case sensitive comparison https://review.opendev.org/c/openstack/nova/+/959534 | |
| 14:27:29 | bauzas | dansmith: I just remember now where stats is called | |
| 14:27:49 | sean-k-mooney | bauzas: you realise i litrally said that many many times | |
| 14:27:59 | bauzas | first good news, that's a periodically updated field using the RT update_avail_resources() | |
| 14:28:55 | sean-k-mooney | dansmith: the api refe just say "The total uptime of the hypervisor and information about average load." | |
| 14:29:02 | bauzas | sean-k-mooney: well, then I missed the many times when you said that | |
| 14:29:11 | sean-k-mooney | and everythin else is cached at the period of the update_aviable_resouces perodic | |
| 14:30:01 | dansmith | sean-k-mooney: yeah, it's unfortunate that's in our api ref at all, especially since all that is very os-specific | |
| 14:31:22 | bauzas | anyway, sounds like we have a plan | |
| 14:31:33 | sean-k-mooney | that what the deprecate api says and thsi is what the hypervior deail api say ```The total uptime of the hypervisor and information about average load. Only reported for active hosts where the virt driver supports this feature``` | |
| 14:31:40 | bauzas | we can fix 2.88 and drop that field in a later microversion | |
| 14:32:07 | bauzas | if ops complain about that missing field, then we can tell them to use whatever tool for calling the OS by themselves | |
| 14:32:19 | sean-k-mooney | bauzas: just for extra context gibi and i talk about this a few weeks ago and before that i talk to others about this too. | |
| 14:32:22 | bauzas | ansible, prometheus or nagios, I don't care | |
| 14:32:36 | sean-k-mooney | so this is not the first converation we have had upstream or downstream on how to fix this | |
| 14:32:37 | bauzas | few weeks ago, I was on PTO :) | |
| 14:32:42 | sean-k-mooney | you were on pto for the prior ones | |
| 14:33:12 | bauzas | so I had no context, but the good thing is that we went to the same solution, isn't that good ? | |
| 14:33:16 | sean-k-mooney | yes im aware but we were not activly fixing it because of time pressuer for FF | |
| 14:33:29 | bauzas | I think this is a reasonable bugfix for RC1 | |
| 14:33:54 | bauzas | not RC2 of course as that's a not a Flamingo regression per say | |
| 14:34:00 | bauzas | but I know you know | |
| 14:34:01 | dansmith | bauzas: ++ for rc1 | |
| 14:34:03 | sean-k-mooney | maybe ill see if i can fix it tomrrwo or at least ahve a draft for review | |
| 14:34:07 | dansmith | if it's reasonable to do so | |
| 14:34:43 | bauzas | extending the Stats class by stuffing a new keypair sounds a reasonable solution | |
| 14:35:05 | bauzas | but sean already found the solution, I leave him tell whether he's OK | |
| 14:36:00 | sean-k-mooney | lets just review the patch and we can backprot as normal. if its ready for rc1 cool if not we chan cherry pick after we release 2025.2 | |
| 14:38:01 | dansmith | sean-k-mooney: there's no patch up to review yet, right? just one in your head? | |
| 14:38:21 | sean-k-mooney | exactly im still chekcing if any other virtdriver ever supproted this rpc | |
| 14:39:28 | sean-k-mooney | basicly im not sure i want to stash a sentenal value in the stats for other virt driver | |
| 14:40:08 | sean-k-mooney | if i can show its only libvirt by inspecting the code i can skip that and gard the rpc call behidn both the virt type adn the absence of the key in the dict | |
| 14:40:37 | sean-k-mooney | that way i can aovid stashign garbage values for ironic or vmeare compute nodes | |
| 14:45:33 | sean-k-mooney | looks like zvm also suports it | |
| 14:46:58 | sean-k-mooney | although if your using zvm the contet of the respocne i think is diffent based on the unit tests | |
| 14:48:51 | sean-k-mooney | so zvm and libvirt the rest all raise not implemtned ok that good to know | |
| 14:49:13 | sean-k-mooney | i will probaly put the zvm fix in a spereate commit but it wil be effectivly the same | |
| 14:51:12 | dansmith | super expensive to make that rpc call for every hypervisor only to have them raise NotImplemented :/ | |
| 14:53:05 | sean-k-mooney | yep that why i dont want to even try :) | |
| 14:54:41 | bauzas | stats are already provided by the virt driver and we stuff them in the Stats object using update_stats() | |
| 14:55:03 | bauzas | so we can just return the uptime as part of the virt resources | |
| 14:55:38 | sean-k-mooney | yep thats the plan | |
| 14:56:03 | sean-k-mooney | but the upstream is ment to be the uptime of the node which is drivder depenent. its not the uptime of the comptue service | |
| 14:56:11 | bauzas | if the driver doesn't provide that uptime value, fine, we already provide None | |
| 14:56:26 | bauzas | that's why I don't see the problem | |
| 14:56:34 | sean-k-mooney | for ironic if we chosse to implement it it would be the uptime of the ironic instnace. but im not suggesting we do that | |
| 14:58:07 | opendevreview | ribaudr proposed openstack/nova stable/2024.1: Reproducer for bug 2114951 https://review.opendev.org/c/openstack/nova/+/959540 | |
| 14:58:08 | opendevreview | ribaudr proposed openstack/nova stable/2024.1: Fix bug 2114951 https://review.opendev.org/c/openstack/nova/+/959541 | |
| 14:58:42 | bauzas | I don't see the problem | |
| 14:58:54 | bauzas | (sorry) | |
| 14:59:19 | bauzas | each of the virt drivers can have get_host_uptime() defined | |
| 14:59:48 | bauzas | the simple fix seems for me to call get_host_uptime() if defined directly in the virt driver method than returns the resources | |
| 15:00:11 | bauzas | and we could later remove the RPC API and make that method private | |
| 15:00:13 | dansmith | _is_ there a problem? | |
| 15:01:36 | bauzas | I dunno hence my question | |
| 15:02:03 | bauzas | AFAICS, only zvm and libvirt do implement the method | |
| 15:02:10 | bauzas | vmware is not providing it | |
| 15:02:11 | dansmith | right, so no problem | |
| 15:02:38 | dansmith | honestly, | |
| 15:02:48 | dansmith | the API also provides for =None for all these drivers | |
| 15:03:08 | dansmith | and the "does this specific driver provide this data" couldn't possibly be part of the API contract, IMHO | |
| 15:03:27 | dansmith | like, if we started remotely managing libvirt nodes from a central one, we wouldn't be able to provide this anymore | |
| 15:03:30 | bauzas | that API field has to be gone IMHO | |
| 15:03:40 | bauzas | but for the moment, let's fix the performance issue | |
| 15:03:53 | dansmith | I'm not saying we shouldn't just solve this the way we've prescribed here, but... :) | |
| 15:04:43 | bauzas | in a cloud world, I'm still surprised to see that we continue to provide hypervisor specific fields | |
| 15:09:25 | sean-k-mooney | bauzas: well https://review.opendev.org/c/openstack/nova-specs/+/765797/comment/8926d415_9c5a7022/ we all said we woudl prefer to remove it but you wanted to discuss it with operators first. | |
| 15:10:22 | sean-k-mooney | we tried to remove it once before and we decied to revers cource becuase peopel ask to keep it | |
| 15:10:39 | sean-k-mooney | that does nto mean we cant revisit that and fix it going forward | |
| 15:10:49 | sean-k-mooney | but we shoudl nto rewrite history | |
| 15:11:02 | bauzas | LOL | |
| 15:11:16 | bauzas | I love when something pops up out of context :) | |
| 15:11:32 | bauzas | 1/ I was only talking about hypervisor show | |
| 15:11:52 | bauzas | 2/ I was just trying to explain that we should remove that but by another spec, not that one | |
| 15:12:08 | sean-k-mooney | well that was update 3 to the sepc | |
| 15:12:12 | bauzas | and AFAIK, the spec you mentioned wasn't the one we had for 2.88, right? | |
| 15:12:26 | sean-k-mooney | we rogainlly agreed to remvoe it then we updated it after the fact to readd it | |
| 15:12:58 | sean-k-mooney | it was the spec for 2.88 it jsut went through many iteration | |
| 15:14:36 | sean-k-mooney | you can see the iteration if you really care https://github.com/openstack/nova-specs/commits/abfb91875c7d6b1e2df45d5bc2244956bc662884/specs/wallaby/approved/modernize-os-hypervisors-api.rst | |
| 15:16:56 | sean-k-mooney | i alwasy rememebr this spec because stephen use a 💩emoji | |
| 16:33:46 | gmaan | ++, I agree to continue showing it in same API instead of separate (separate API even in old API will be microversion bump). reporting cache value is better solution and backportable. | |