Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2023-08-22
16:36:13 dansmith yeah, but since it requires an api change,
16:36:44 dansmith I think we should not presumptively merge this because cinder might change their definition of what the event means, when it comes, etc and then we end up with a thing we have to technically support for no reason
16:36:47 sean-k-mooney + compute service change and min version check
16:36:51 kgube It's a bit frustrating because the Cinder change has been ready for review for weeks, and even on top of their feature tracking etherpad: https://etherpad.opendev.org/p/cinder-2023.2-bobcat-features
16:36:57 dansmith I think we've always expected these to merge in the dependent service first
16:37:10 kgube but I guess they have been very busy
16:37:16 dansmith sean-k-mooney: right, there are REST API changes, implicit RPC ones, service versions, etc
16:37:18 sean-k-mooney certenly with neutron we have
16:37:35 dansmith kgube: yeah, I definitely understand your frustration
16:37:40 gibi kgube: yes, this is frustrating
16:37:58 sean-k-mooney kgube: do you have depends on or tempest/devestack covergae to show this working by any chance
16:38:33 sean-k-mooney this is feeling more like an early pre milestone 1 type of change then the next two weeks
16:39:00 sean-k-mooney but if you have already staged the patches with depends on and can show this working in ci then that more compleing
16:39:37 kgube sean-k-mooney: It is covered by the existing online volume extend tests if you run them on an affected driver
16:39:47 dansmith no devstack change that I see
16:40:05 sean-k-mooney we do not have a cinder nfs job in novas gate
16:40:06 dansmith kgube: right, but we need to see them running against the proper driver and this change
16:40:46 sean-k-mooney the depend on relation ship si not quite right althoguh its close
16:40:52 dansmith I guess the nfs job on this patch maybe? https://review.opendev.org/c/openstack/cinder/+/873889/7?tab=change-view-tab-header-zuul-results-summary
16:40:54 kgube There is a devstack-nfs job
16:40:58 sean-k-mooney https://review.opendev.org/c/openstack/cinder/+/873686/9 woudl be the patch to make the nfs driver work
16:41:23 dansmith that patch has depends-on relationships to cinder itself, which is weird
16:41:38 sean-k-mooney ya so that patch should depend on either the nova patch
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)

Earlier   Later