| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-08-22 | |||
| 16:41:45 | sean-k-mooney | ro the nova patch should ideally depend on it | |
| 16:41:53 | dansmith | well, not reall,y | |
| 16:42:01 | dansmith | because you need to run a cinder patch to see that job | |
| 16:42:11 | sean-k-mooney | the nova patch depens on the cinder client patch which depends on https://review.opendev.org/c/openstack/cinder/+/873557/8 | |
| 16:42:13 | dansmith | it looks like every patch in the stack depends on the thing below it or something? | |
| 16:42:16 | dansmith | confusing | |
| 16:42:31 | dansmith | this one depends on the nova patch: https://review.opendev.org/c/openstack/nova/+/873560 | |
| 16:42:32 | sean-k-mooney | ya that will work but its not how its ment to be used | |
| 16:42:37 | dansmith | and the top cinder one depends on that one | |
| 16:42:48 | sean-k-mooney | oh the netapp one | |
| 16:42:48 | dansmith | so it's a bit convoluted | |
| 16:42:55 | dansmith | anyway kgube disconnected | |
| 16:42:58 | sean-k-mooney | ya so the issue is that the api change patch is first | |
| 16:43:03 | dansmith | so we can probably end it here and discuss later | |
| 16:43:05 | sean-k-mooney | and then the driver patches ar after that | |
| 16:43:10 | sean-k-mooney | sure | |
| 16:43:40 | gibi | OK | |
| 16:43:47 | gibi | is the any other topic before we close? | |
| 16:44:03 | kgube | sorry, I got disconnected | |
| 16:44:35 | dansmith | kgube: we were just deciphering the patch dependency network to figure out how to see a job running the whole stack | |
| 16:44:45 | dansmith | which I think is there, but it's convoluted | |
| 16:44:51 | dansmith | either way I think the answer is the same :/ | |
| 16:45:17 | kgube | yeah, I was afraid of that | |
| 16:45:23 | kgube | but I can understand it | |
| 16:45:58 | sean-k-mooney | i dont know if cinder has the same policy as nova | |
| 16:46:11 | sean-k-mooney | but for nova we merge the api change last in a series | |
| 16:46:22 | sean-k-mooney | so the fact that is first in the cidner one is also a little odd | |
| 16:47:10 | kgube | Well the nova change needs the new volume action | |
| 16:47:10 | gibi | I don't see any other topic being raised. So I will close the meeting and you can continue untangling the dep tree on the channel | |
| 16:47:15 | opendevmeet | Log: https://meetings.opendev.org/meetings/nova/2023/nova.2023-08-22-16.00.log.html | |
| 16:47:15 | opendevmeet | Minutes (text): https://meetings.opendev.org/meetings/nova/2023/nova.2023-08-22-16.00.txt | |
| 16:47:15 | opendevmeet | Minutes: https://meetings.opendev.org/meetings/nova/2023/nova.2023-08-22-16.00.html | |
| 16:47:15 | opendevmeet | Meeting ended Tue Aug 22 16:47:15 2023 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) | |
| 16:47:15 | gibi | #endmeeting | |
| 16:47:36 | kgube | and everything else is dependent on the support in nova | |
| 16:47:42 | sean-k-mooney | also an fyi every time you use depends on it makes zuul clone an addtional copy of the relevent repo then it does a merge of all repos with the same name | |
| 16:48:23 | sean-k-mooney | so while you can use a depend on in the same repo in some cases its generally not a good idea as it causes a much more complex git merge to be done | |
| 16:49:20 | kgube | oh, okay, so I don't need them if the changes are already ordered in a branch | |
| 16:49:45 | kgube | ? | |
| 16:49:58 | dansmith | kgube: right, a single stack of patches are necessarily dependent and ordered | |
| 16:50:15 | dansmith | kgube: you do need them cross-repo (i.e. cinder->nova) but not within a single branch in cinder | |
| 16:50:30 | kgube | dansmith: alright, thanks! | |
| 16:50:40 | dansmith | also, the newer way to depends-on is to use a URL to the actual change, which also makes it easier for us to see "this is the nova dependency" | |
| 16:50:58 | dansmith | I thought zuul actually stopped supporting the change-id way, but I guess it still works (or at least the tagging does) | |
| 16:51:29 | kgube | that is also good to know. I kinda just copied it from other changes that I saw | |
| 16:52:10 | sean-k-mooney | the reason i mentioned the depend on mechanics is i have seen it cause merge conflict in the ci jobs that done repoduce locally | |
| 16:52:15 | sean-k-mooney | which can be very hard to debug | |
| 16:52:23 | dansmith | kgube: here's an example of how it should work these days: https://review.opendev.org/c/openstack/nova/+/650172 | |
| 16:52:27 | sean-k-mooney | i.e. using depend on in the same repo | |
| 16:53:19 | kgube | dansmith: thanks! | |
| 16:59:26 | spencerh` | sean-k-mooney: I am curious about the history with nova and libvirt tsc frequency. Is there a link to a discussion or is it possible to summarize concerns? Not expecting to get this in this cycle, but this patch has been critical for us. | |
| 16:59:35 | sean-k-mooney | kgube: the VolumesExtendAttachedTest test in tempest in the nfs job is skiped by hte way | |
| 16:59:49 | sean-k-mooney | kgube: so its not actully being tested by that job | |
| 17:00:13 | sean-k-mooney | spencerh`: bascially it comes done to the fact it will break live migration | |
| 17:00:30 | sean-k-mooney | we do not have the ablity to schdule based on the clock rate fo the hsot cpus | |
| 17:01:10 | sean-k-mooney | and if we allwo you to set the tsc_frequency via a host config option then we need rpc change to check comaptiablity | |
| 17:01:19 | spencerh` | The tsc frequency doesn't have to match the host capabilities on our cpus. Maybe that's vendor specific? | |
| 17:01:33 | sean-k-mooney | this is also not somethign that should chagne over the life time of the vm so it should not be a host level opation but rather an image property | |
| 17:01:58 | spencerh` | In my experience, it's the opposite: when invtsc is enabled, not setting tsc frequency will break live migration. | |
| 17:02:01 | sean-k-mooney | as far as ia am aware it cant exceed the host frequency on intel | |
| 17:02:29 | sean-k-mooney | a requicemnet if invtsc is that live migration is only supproted between hosts with the same frequesncy yes | |
| 17:02:37 | sean-k-mooney | and seting it in the xml is the workaroudn for aht | |
| 17:02:51 | sean-k-mooney | https://blueprints.launchpad.net/nova/+spec/config-tsc-freq | |
| 17:03:22 | sean-k-mooney | that is the blueprint that was propsed for adding invTSC orginally | |
| 17:03:53 | dansmith | spencerh`: I imagine anything like this will need a spec and discussion where we can make sure the experts in a given area have time to opine about such a change | |
| 17:04:06 | dansmith | spencerh`: so that's a good place to document the how and why, the impacts and other considerations | |
| 17:04:44 | spencerh` | Sounds good! Sorry, I'm not familiar with the spec process, but I'll be happy to talk about our environment and why we need this. Thanks for the background! | |
| 17:05:12 | kgube | sean-k-mooney, yeah, I will try to make a change that enables it in the job and make this dependent on the NFS driver support change. | |
| 17:05:25 | dansmith | spencerh`: https://specs.openstack.org/openstack/nova-specs/ | |
| 17:06:01 | spencerh` | dansmith: ty! | |
| 17:06:05 | sean-k-mooney | spencerh`: i started reviewing your code change by the way. you have a lot of random formatingin changes in it that should be remvoed | |
| 17:06:40 | spencerh` | Oh no. Probably because I enabled autopep8. I will fix | |
| 17:07:15 | sean-k-mooney | autopep8 shoudl not make these changes | |
| 17:07:22 | sean-k-mooney | we have it enabeld in ci | |
| 17:08:48 | spencerh` | Strange. Maybe my local instance is using different settings. There were fewer lines of diff before I messed it up this morning. I'll get that fixed. | |
| 17:21:24 | opendevreview | Spencer Harmon proposed openstack/nova master: libvirt tsc_frequency https://review.opendev.org/c/openstack/nova/+/891924 | |
| 17:23:09 | spencerh` | Looks better now. Sorry about that. (But no rush to review) | |
| 18:35:47 | sean-k-mooney | spencerh`: what is your usecase for enabeling the invtsc by the way | |
| 18:36:39 | sean-k-mooney | im just wonderign if the hpet provided by https://review.opendev.org/c/openstack/nova/+/605902 is enouch for your usecase of if you really need the tsc time source | |
| 18:38:31 | spencerh` | Haha it is very similar. I work at blizzard and we're running game servers on that have invtsc as a requirement (for lower latency/better performance). It's not windows specific, though. We're running various flavors of linux for the guests. | |
| 18:39:36 | spencerh` | Oh sorry. I conflated what you said with the spec. | |
| 18:40:02 | spencerh` | No, I don't think this will provide what's required for libvirt to live migrate with invtsc enabled. | |
| 18:40:18 | sean-k-mooney | yes we added invtsc and htpe for blizzard specicifcally in the past | |
| 18:40:41 | sean-k-mooney | right it does not i was asking can invtsc be disabled and use the hpet instead | |
| 18:41:17 | sean-k-mooney | the invtsc flag tells the guest tha tthe TSC is stable and wont change frequency | |
| 18:41:31 | spencerh` | I'll have to benchmark that to see if it solves the same issues that invtsc does. AFAIK, these are not fungible as far as the workload is concerned. | |
| 18:42:04 | sean-k-mooney | they gerneally shoudl be if the workload is not direfctly calling the TSC | |
| 18:42:17 | spencerh` | The workload does directly call the tsc. | |
| 18:42:21 | sean-k-mooney | the tsc is often used because its really fast to read | |
| 18:42:39 | sean-k-mooney | but its also generally not the most stable time source espcially in a vm | |
| 18:42:41 | sean-k-mooney | ack | |
| 19:34:57 | sean-k-mooney | dansmith: the lazy load serise all looks good overal. i had one of two nits inlien but they are all approved now | |
| 19:35:46 | sean-k-mooney | do you know off the top of your head what "trusted_certs" is used for in the instance. is that related to trusted boot? | |
| 19:51:44 | melwitt | sean-k-mooney: it's for this https://specs.openstack.org/openstack/nova-specs/specs/rocky/implemented/nova-validate-certificates.html | |
| 19:52:29 | sean-k-mooney | possibly but why would nova have the certs | |
| 19:53:11 | sean-k-mooney | i would expect that to be a propry of the image and there for in the instnace_system_metadata rather then the instance table | |
| 19:54:08 | sean-k-mooney | oh its form a join | |
| 19:54:11 | sean-k-mooney | ya its that | |
| 19:54:14 | sean-k-mooney | """This change will update the InstanceExtra schema to support a trusted_certs column, """ | |
| 20:04:02 | opendevreview | sean mooney proposed openstack/nova master: introduce global greenpool https://review.opendev.org/c/openstack/nova/+/873061 | |