Tuesday, 2021-07-27

opendevreviewTakashi Kajinami proposed openstack/python-novaclient master: Use Block Storage API v3 instead of API v2  https://review.opendev.org/c/openstack/python-novaclient/+/80241702:00
*** rpittau|afk is now known as rpittau07:29
MrClayPoleTime appropriate greetings, we have a company policy for all network and compute devices have their timezone set to UTC time. The issue we have is that during the summer in the UK we have our clocks set to UTC+1. Our Windows servers are currently booting with clock 1 hour in the past. Is there a way for libvirt/nova to track our timezone while keeping Ubuntu 18.04 set to UTC time?08:40
lyarwoodMrClayPole: are you using os_type=windows for the images?08:43
lyarwoodhttps://bugs.launchpad.net/nova/+bug/1231254 a slightly old bug but I wonder if it still applies08:43
lyarwoodhttps://libvirt.org/formatdomain.html#time-keeping would be how libvirt controls this FWIW08:45
lyarwoodhttps://github.com/openstack/nova/blob/1c490ecd7be5722c6cfdaddfacc8d7a5539dd035/nova/virt/libvirt/driver.py#L5792-L5806 is where we switch to localtime when os_type=windows08:46
lyarwoodinstead of utc08:46
lyarwoodah right but your hosts remain on UTC so that isn't useful08:47
MrClayPolelyarwood: We are running OpenStack rocky, as you said the os_type didn't help but it's interesting that libvirt xml supports a timezone. Can this be set from Nova?08:56
lyarwoodMrClayPole: not at the moment AFAICT, we have all of the config code just no way for a user to request it08:57
MrClayPoleI wondering if the best way forward might just be to have an exception for our compute nodes so they can bet set to "Europe/London" rather than UTC08:59
lyarwoodYeah for your env on Rocky that would be for the best08:59
lyarwoodFor the Yoga OpenStack release we could easily add an image property to control the timezone directly09:00
MrClayPoleThat would defo be useful to us so we can revert the compute nodes back to UTC.09:02
stephenfinbauzas: gate fix for novaclient here if you have 209:15
stephenfinhttps://review.opendev.org/c/openstack/python-novaclient/+/80241709:15
bauzasstephenfin: we have a gate issue ? 09:16
stephenfinYes, a very slight one due to cinderclient's removal of cinder API v209:16
stephenfinSpotted after reading through Takashi Kajinami's email to openstack-discuss ("[all] Broken gate caused by Block Storage API v2 removal")09:16
bauzasstephenfin: any bug y'know ?09:16
bauzasahah09:16
stephenfinlots of related patches available also https://review.opendev.org/q/topic:%22volumev2-removal%22+(status:open%20OR%20status:merged)09:17
bauzasI just read the email, I don't see failing jobs09:18
bauzashah09:18
lyarwoodstephenfin: ACK'd the novaclient change09:19
stephenfinty!09:19
bauzasare we sure that the v3 client supports the same than for the v2 ?09:20
bauzaslyarwood: ^09:21
lyarwoodbauzas: yeah it does09:21
bauzasok, I was a bit afraid of just using the new version without making sure it wasn't creating problems for us09:22
bauzasbut if it doesn't change our client API, fair enough09:22
lyarwoodhttps://docs.openstack.org/api-ref/block-storage/api_microversion_history.html#maximum-in-mitaka09:22
lyarwoodThe 3.0 Cinder API includes all v2 core APIs existing prior to the introduction of microversions. The /v3 URL is used to call 3.0 APIs. This is the initial version of the Cinder API which supports microversions.09:22
sean-k-mooneyit looks like in most cases the convertion to v3 is trivial as a result09:26
bauzaslyarwood: thanks09:26
sean-k-mooneyMrClayPole: im not sure th at nova should really provide a facility to set a timezone09:29
sean-k-mooneyat least not at the host level provbly not ant the image or flaovr level. although im more open to the image idea09:29
sean-k-mooneyif we were to suprot settign a time zone i think i would want it to be per instance hosetly but really i think that woule be better handeles withine the workload09:30
sean-k-mooneyjust always set your host to utc09:30
sean-k-mooneyand in windows set the timezone appropreiately if needed09:31
CeeMacsean-k-mooney: thats exactly the issues we have at the moment though.  The host is set to utc and windows seems locked to that, so during DST the guest still runs -1 hour instead of updating.09:32
CeeMaci must confess I'm a little perplexed as I was expecting the os_type=windows to resolve the issue, but this doesn't appear to be the case09:32
sean-k-mooneyCeeMac: you should be able to set the time in windows  the clock source will by utc09:35
sean-k-mooneybut in windows itself you can change the local09:35
CeeMacsean-k-mooney: yes, "should" appears to be the main issues here as that was my expectation, but the results don't appear to support this :(09:36
sean-k-mooneyare you saying if you go into the region settign in windows it does not work?09:36
sean-k-mooneyCeeMac: that sound like a windows bug you should report to microsoft09:36
CeeMacour guest VMs are all set to BST (UTC+1) and the system clock is reporting UTC+0 times, so they're all an hour in the past09:36
sean-k-mooneyin a cloud enviornemt you cannot know the timezone of the tenant09:37
sean-k-mooneyso you shoudl always be abel to set it indepenently of the host09:37
CeeMacit appears to be a known issue around how windows handles time from the bios clock, which i believe is the entire premise on why the os_type=windows patch was ported from hyper-v hypervisor to kvm hypervisor in nova09:37
sean-k-mooneyCeeMac: for what its worth i hate daylight saving time so im glad ill by on utc permently next year09:37
CeeMacsean-k-mooney: yeah, thats the dream!09:38
CeeMacyeah, can't say I'm a big fan either.  its those pesky customers who are complaining :)09:38
sean-k-mooneywell os_type=windows does other things too09:38
sean-k-mooneybut that patch yes09:38
sean-k-mooneyalthough when we use os_type=windows it shoudl enabel the hyperv clock09:39
CeeMacfrom what I gather, there is a reg hack that resolves the problem, but microsoft wont support it and say it is "buggy", whatever that means09:39
sean-k-mooneywhich clock is this changing09:39
sean-k-mooneywe have multipel clock source i think in the vm09:39
sean-k-mooneyCeeMac: why not set the windows vms to utc09:40
CeeMaci've seen the <timer name='hypervclock' present='yes'/>09:40
CeeMac get added to the clock offset stanza for instances with os_type=windows09:40
sean-k-mooneyyou can disable DST in them09:40
sean-k-mooneyCeeMac: ill pretend your not altering the xml :)09:41
CeeMacsean-k-mooney: its ok, i'm not, i'm just looking at it with dumpxml09:41
CeeMac:)09:41
sean-k-mooneyso we have offset and timezone https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L703-L70509:42
CeeMacso, the issue we're looking at currently, is a hosted platform for a customer clocking / time management systems.  Presumably they want/need the clock in / clock out times to register against the correct regional time for accurate reporting09:42
sean-k-mooneyand then we add the hyperv clock source  https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L5840-L5844 in addation to the pit, rtc and optionally the hpet09:44
sean-k-mooneyCeeMac: right but the normal way to do that is to use UTC and then do the conversion client side09:45
sean-k-mooneyyou should never store data in local time09:45
CeeMactbh, until we came across this issues it was always my belief that the windows kernel ran in utc natively and just adusted 'user' time based on the regional settings09:46
sean-k-mooneyyou enither use unix time or some other utc reference time like isotime format09:46
CeeMacbut windows09:46
CeeMacappears to be the issues09:46
sean-k-mooneyCeeMac: i belive it does with a different epoc09:46
CeeMacas its chose to use the 'localtime' standard, if you can call it a standard09:46
CeeMacstupid windows09:46
sean-k-mooneyCeeMac: i think windows expect the hardware time to be in utc09:46
CeeMaci think more stick poking is required09:47
CeeMacthanks sean-k-mooney we'll have a ponder and see if we can see whats happening, or not happening as the case may be09:48
sean-k-mooneyhttps://libvirt.org/formatdomain.html#time-keeping09:49
CeeMacthanks, thats an interesting read.09:51
sean-k-mooneyso reading that localtime should give you what you wanted i think09:52
CeeMacyeah, that was our theory. Perhaps we're missing something in the os09:52
sean-k-mooneywe can use timezone since we can only update that on reboot so you woudl have to reboot your vms evey 6 months09:52
CeeMaci presume there is no easy way to insert the additional track and catchup information?09:53
sean-k-mooneywe could maybe use variable09:53
sean-k-mooneyits not currently exposed in nova no09:53
sean-k-mooneythis is getting more speicic then im comfortable exposing in a cloud env by the way09:54
CeeMacall the data suggests that it should work, as you say09:54
sean-k-mooneywe might expsoe it but im not sure we should in general09:54
CeeMacno thats fine, i get that.  It must be working for other people, otherwise I'm sure there would be more chat out there for it not working09:54
sean-k-mooney<clock offset='varible' basis='localtime'>09:54
sean-k-mooneywe cold maybe try that09:55
sean-k-mooney*could09:55
sean-k-mooneyCeeMac: if we were to expose the timer settign i think it woudl ahve to be at teh image level rghat then host by the way09:57
CeeMacsean-k-mooney: yes, that would make sense09:57
sean-k-mooneyCeeMac: host level config that modify the xml are basicaly terribel form a live migration standpoint as we havne to schdule on it or pass the data or both 09:57
sean-k-mooneyso this type of info really needt to live with the instnace for its lifetime hence flavor or image09:58
sean-k-mooneywith that said i woudl be tempeted to start using server metadata for this personally09:59
sean-k-mooneysince this is a very pet like tuning09:59
CeeMacit makes sense that it should follow the instance09:59
CeeMacwindows is all about the pet sadly09:59
CeeMacserver metadata would be useful i think10:01
sean-k-mooneywe dont currently allow it to modify xml exctra but i have always wondered if we should allow it to set anything that is setabel via image metadata10:02
sean-k-mooneythat would require a spec and some semi invasive changes to parts of the libvirt driver10:03
CeeMacin some scenarios it could be beneficial, especially if its discovered you need to retrofit a value that would normally only be doable through an image10:03
sean-k-mooneyso not sure its worth it10:03
CeeMacits still a very pet mentality granted10:03
CeeMachmm, cost/benefit are skewed i guess 10:04
sean-k-mooneyCeeMac: ya we are adding a nova-manage command that operators can use for a limit set of image properties10:04
CeeMacoh, that sounds interesting10:04
sean-k-mooneythe intent is for operators taht need to change things for upgrades10:05
sean-k-mooneye.g. move to q35 which means you have to remove use of ide 10:05
sean-k-mooneyectra10:05
sean-k-mooneywe approved https://github.com/openstack/nova-specs/blob/master/specs/newton/approved/virt-image-props-boot-override.rst in the past but then decieded to no porceed with it when we came to implemenation10:06
sean-k-mooneyim still somewhat open to that idea  but i dont fully recal what the main objects were10:07
CeeMaci imagine its tricky finding a decent balance point between stability and the ability to make dynamic changes10:07
sean-k-mooneyya and stricking a blance between upstream and downstream10:08
sean-k-mooneydownstream our hands are forced a bit by change made by other teams 10:08
sean-k-mooneyfor example qxl graphic is going away  downstream so we have to provide a way to move instances off it which is what propmeted the change10:09
sean-k-mooneyCeeMac: this is the new nova-magage command by the way https://github.com/openstack/nova-specs/blob/master/specs/xena/approved/nova-manage-commands-to-update-libvirt-device-models.rst10:25
sean-k-mooneyCeeMac: lyarwood  is currently workign on it for xena10:25
CeeMaclooks good10:28
opendevreviewMerged openstack/python-novaclient master: Use Block Storage API v3 instead of API v2  https://review.opendev.org/c/openstack/python-novaclient/+/80241710:46
opendevreviewMerged openstack/nova master: Bump os-resource-classes to 1.1.0  https://review.opendev.org/c/openstack/nova/+/80097611:13
opendevreviewMerged openstack/os-vif master: add configurable per port bridges  https://review.opendev.org/c/openstack/os-vif/+/79805511:50
*** akekane_ is now known as abhishekk12:31
brinzhanggibi,sean-k-moonkey,stephenfin,alex_xu: we submited the PoC code for allowing PMEM's data wihle migrate vm, but it is not completely implemented in accordance with the spec, because some irrationality in the spec was discovered during the implementation process, so we hope you can review the PoC code.12:34
brinzhangah, gibi has a holday(pto), gibi_pto12:35
brinzhangthe PoC code link: https://review.opendev.org/c/openstack/nova/+/80222512:36
sean-k-mooneybrinzhang: do you have a patch to update the spec whith what diverged12:36
sean-k-mooneybrinzhang: what exactly was teh "irrationality in the spec" it may have been stated that way for a reason which the impleation misses12:37
brinzhangsean-k-mooney: in spec we want to ask ensure which pmem we can copy in the target host when run migtate_disk_and_power_off, but this time we cannt know the target pmem's path in the context12:40
sean-k-mooneyyes we can12:41
brinzhanghttps://review.opendev.org/c/openstack/nova-specs/+/785563/14/specs/xena/approved/allow-migrate-pmem-data.rst#6512:41
sean-k-mooney its includeded in the instance claim if i recall correctly12:41
brinzhangwe test and write the logs, and we cannt get the target PMEM's path from the context12:41
sean-k-mooneyill have to review but i was pretty sure i pointed to the code where you could get it at one point12:43
brinzhangnot exectly, it's cannot claim it now, now we just get the pmem's path when we were started if I wasn's missed some key info12:43
sean-k-mooneyno12:43
sean-k-mooneyit must claim it before you do the data copy12:44
sean-k-mooneyyou cannot pass back paths to unclaimed devices12:44
sean-k-mooneythat is a potential securrity bug12:44
brinzhangyes, hope we were wrong, you can review the PoC code and give some points, thanks 12:46
sean-k-mooneyprep_resize should have claimed the remote pmemdevices and store them in the migration context12:46
sean-k-mooneybrinzhang: i can but since it deviates form the spec im -1 untill you can show why the spec wont work12:46
sean-k-mooneywhy did you not save the pmem device in the migration context in prep_resize12:47
sean-k-mooneywe create a resize claim here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L5153-L515612:48
brinzhangyeah, it looks like he jumped the prep_resize, so he cannot get the target pmem's info12:52
sean-k-mooneyright now the claims dont claim the pci devices12:53
sean-k-mooney* pmem devcies12:53
sean-k-mooneybut the intent of the spec was to extend it to do that 12:53
sean-k-mooneywhich can be sotre either in the claim direclty or we can update the migration or migration context12:53
sean-k-mooneywe hvae access to both12:54
brinzhangas I know, the VM only knows the path information of the PMEM after it is started, otherwise the VM does not know what the PMEM patch used inside the virtual machine is (e.g. /dev/pmem0)12:56
brinzhangmay this is my confusing12:56
sean-k-mooneyi dont belive that is correct12:56
sean-k-mooneywe must know the path before we start the vm since we need to specify the path in the vm xml12:57
brinzhangright12:57
sean-k-mooneyand we cannot copy any data untile we have claimed the devcie because other wise we coudl race with a differnte vm and that could be a security bug or at least a data loss bug12:58
brinzhangyes, if it's right we will lost the datas in pmem12:59
brinzhangI will check the prep_resize tomorrow13:00
brinzhangmay it need to claim the pci devices in resource tracker13:01
sean-k-mooneyim currenlty trying to figure out why it does not appear to be doign that although i may have missed it13:02
sean-k-mooneyi tought it was doing that already13:02
sean-k-mooneybut yes when we aquire the instnace clam or move claim it should for cold migration at least13:03
sean-k-mooneyfor live migration we explitly claim the neutron pci devices in the pci tracker 13:03
sean-k-mooneywe do not use move claims for that13:03
sean-k-mooneyfor pmems though you will need to cliam them in the resouce table in the db13:04
brinzhangyes, we shuold claims it firstly before cold migrate the instance13:05
sean-k-mooneyyep because we want to abort the migration if we cant claim13:07
sean-k-mooneythis is why we insited the spec be written the way it currently is13:08
sean-k-mooneybrinzhang: the poc wont work by the way13:12
sean-k-mooneybrinzhang: ill push comments in a second but its using processutiles incorrectly13:13
sean-k-mooneyhttps://review.opendev.org/c/openstack/nova/+/802225/1/nova/virt/libvirt/driver.py#1075713:13
brinzhangit's ok, I agree your point, and I think firstly we should claim the pmem devices in prep_resize interface, and then check it when we want to execute migrage_server then to copy date form source pmem to the target pmem devices13:13
sean-k-mooneywell if the claim fails then the migration shoudl go to error and the vm will stay in active13:14
opendevreviewPavlo Shchelokovskyy proposed openstack/nova stable/queens: libvirt: Skip encryption metadata lookups if secret already exists on host  https://review.opendev.org/c/openstack/nova/+/76577413:15
brinzhangsean-k-mooney:ok, hope you can review if you find the error in that patch, we will update and try to update in next patch13:16
sean-k-mooneythe issue is the 3 commands in _migrate_vpmem_data will all execute on the same host. to fix it you need to pipe the output of the first commadn into the input of the second and the second and third commands should be combined13:19
brinzhangagree, I saw your comment, it's useful to improve efficiency13:21
sean-k-mooneywell it wont work the way it is now13:28
sean-k-mooneyit might work on the same host but both daxio command woudl execute on the local host13:28
brinzhangack, it should be update, in local we have tested the CLI, it works fine13:32
sean-k-mooneyif you use those comand on the cli it would wokr but only because the second comamdn will put you in a new shell on the dest13:33
sean-k-mooneyto emulate this on the cli you need to rune each exec in a new terminal13:33
brinzhangack13:33
*** akekane_ is now known as abhishekk14:19
bauzasreminder : nova meeting in 47 mins-ish here in #openstack-nova15:12
opendevreviewMerged openstack/nova stable/ussuri: Reject open redirection in the console proxy  https://review.opendev.org/c/openstack/nova/+/79180615:17
opendevreviewStephen Finucane proposed openstack/nova master: Cleanup 'drop_move_claim' and '_drop_move_claim'  https://review.opendev.org/c/openstack/nova/+/74774715:29
opendevreviewStephen Finucane proposed openstack/nova master: manager: Address TODO  https://review.opendev.org/c/openstack/nova/+/74774815:29
opendevreviewStephen Finucane proposed openstack/nova master: manager: Move context manager up one level  https://review.opendev.org/c/openstack/nova/+/74567415:29
opendevreviewStephen Finucane proposed openstack/nova master: compute: Add type hints for resize functions  https://review.opendev.org/c/openstack/nova/+/74534115:29
opendevreviewStephen Finucane proposed openstack/nova master: WIP: compute: Add more type hints for resize functions  https://review.opendev.org/c/openstack/nova/+/74567515:29
opendevreviewStephen Finucane proposed openstack/nova master: objects: Remove 'NovaObjectDictCompat' from 'Migration'  https://review.opendev.org/c/openstack/nova/+/72357215:48
opendevreviewStephen Finucane proposed openstack/nova master: objects: Remove 'NovaObjectDictCompat' from 'InstancePCIRequest'  https://review.opendev.org/c/openstack/nova/+/72357315:48
opendevreviewStephen Finucane proposed openstack/nova master: Remove use of pkg_resources  https://review.opendev.org/c/openstack/nova/+/74066115:50
bauzaslast reminder : nova meeting in 10 mins here in this chan. Sharpen your pencils.15:50
opendevreviewStephen Finucane proposed openstack/nova master: api: Improve extra spec validator help texts  https://review.opendev.org/c/openstack/nova/+/78241215:56
stephenfinlyarwood: Can you look at https://review.opendev.org/c/openstack/nova/+/773640/ and https://review.opendev.org/c/openstack/nova/+/797513/ seeing as you're +2 on patches later in the series15:57
stephenfinalso addressed your comments on https://review.opendev.org/c/openstack/nova/+/78241215:58
bauzas#startmeeting nova16:00
opendevmeetMeeting started Tue Jul 27 16:00:05 2021 UTC and is due to finish in 60 minutes.  The chair is bauzas. Information about MeetBot at http://wiki.debian.org/MeetBot.16:00
opendevmeetUseful Commands: #action #agreed #help #info #idea #link #topic #startvote.16:00
opendevmeetThe meeting name has been set to 'nova'16:00
stephenfino/16:00
bauzashowdy folks I'll be your chair for this meeting given our Supreme Leader is on vacations16:00
bauzas\o16:00
sean-k-mooneyo/16:00
bauzasawesome, one more people from the last meeting I chaired \o/16:01
bauzasagenda is up at https://wiki.openstack.org/wiki/Meetings/Nova16:01
elodilleso/16:01
bauzasfeel free to add items you wanna discuss in the last section above ^ 16:02
bauzasmoving on now16:02
bauzas #topic Bugs (stuck/critical) 16:02
bauzas No Critical bugs16:02
bauzas #link 11 new untriaged bugs (+1 since the last meeting): #link https://bugs.launchpad.net/nova/+bugs?search=Search&field.status=New16:02
bauzasI'll try to look at some of them tomorrow16:02
bauzasany other bugs people wanna raise ?16:03
stephenfinnope, we had a gate issue due to Sphinx 4.x but sean-k-mooney fixed that for us16:03
bauzaswe could have had a cinderclient bug, but the v3 change is now merged, right?16:04
bauzasstephenfin: excellent, thanks sean-k-mooney16:04
sean-k-mooneyya i think that is merged now16:04
stephenfinYes, the nova one landed last week and the novaclient one went in earlier today16:04
bauzasoki doki16:04
sean-k-mooneyhttps://review.opendev.org/c/openstack/nova/+/80233416:04
bauzaswere we limiting the cinderclient version ?16:04
sean-k-mooneythat was the nova one16:05
sean-k-mooneybauzas: no we jsut had a refernce to v216:05
bauzasok16:05
sean-k-mooneyreplced it with v316:05
bauzasanyway, moving16:05
bauzas #topic Gate status 16:05
bauzas Nova gate bugs #link https://bugs.launchpad.net/nova/+bugs?field.tag=gate-failure16:05
bauzas#topic Gate status16:06
bauzasmeh, maybe the meetbot works with the #topic section16:07
bauzasanyway16:07
bauzasnothing to say about any gate issue ?16:07
stephenfinnope, not beyond the above16:07
bauzasI can see a new one from lyarwood https://bugs.launchpad.net/nova/+bug/193802116:07
sean-k-mooneywe are still using the tempoary workaround for the ovsdb issue. ill try and find out how the ovs change is comming before m3 but no other update on that16:08
sean-k-mooneyhum interesting16:08
melwittI have noticed while working on placement consumer types that a generation conflict gets hit on my patches, let me find the (old) gate bug16:08
sean-k-mooneywas tehre a new olo release16:08
bauzassean-k-mooney: good question16:09
melwittthis one http://bugs.launchpad.net/bugs/183675416:09
sean-k-mooneybauzas: the messging issue might be related to something moving and we are nolonger mocking properly in the func tests16:10
bauzasmelwitt: heh, who is working on this one ?16:10
melwittit occurs in general too but while working on placement to do more during a PUT it makes it happen a lot more. so a heads up that I think we'll need to address that before placement consumer types will be usable16:10
sean-k-mooneyalthough perhaps not it is corectly using the fake implementation.16:10
bauzasmelwitt: oh it's you16:11
sean-k-mooneyhuh i could see that conflict happening alright16:11
melwittbauzas: I have restored mriedem's old patch about the bug and will add tests to it for review16:11
bauzasdo we have race conditions for this a lot ? (the conflict)16:11
melwittyeah, it was originallly from tssurya and cdent but both moved off of openstack before it was finished so I've been working on finishing it16:11
bauzasor is it just for a few job runs ?16:12
melwittbauzas: I have seen it on other patches yes, but not nearly as often as I do on the placement patches. on the placement patches it looks pretty much guaranteed16:12
sean-k-mooneywell im not sure the frequency matteers with the scale we run at its goign to block patches at least temporaly and require a recheck16:12
sean-k-mooneyso i think we should try an fix it sooner rather then later16:13
bauzasmelwitt: so we get a conflict when deleting the allocation but why are we getting an exception ?16:13
bauzasthe allocation should just be orphaned, that's it16:13
melwittjust wanted to give everyone a heads up about it because back when the fix was proposed, it there was a lot of discussion on the review. so if anyone has concerns about DELETE for most allocations cases rather than PUT, comment on the review16:13
bauzasmelwitt: sure, will review your change if you want16:14
melwittbecause it was changed to a PUT instead of a DELETE when consumer generations were added16:14
bauzasah shit, I see16:14
melwittit's mriedem's change that I'm going to complete16:14
bauzasmelwitt: thanks for working on it either way16:14
sean-k-mooneymelwitt: wait deleting an allocation was chgange to a put?16:14
sean-k-mooneybecause what delete dont have a body and we did not want to include the consomer generation in the query arg?16:15
bauzassean-k-mooney: https://review.opendev.org/c/openstack/nova/+/688802/2/nova/scheduler/client/report.py#b210716:15
melwitthere's the review https://review.opendev.org/c/openstack/nova/+/68880216:15
melwittsean-k-mooney: yes, https://review.opendev.org/c/openstack/nova/+/59159716:15
melwittsean-k-mooney: I don't know, tbh16:15
bauzassean-k-mooney: we now call put()16:15
sean-k-mooneywell that by itself is a bug16:16
melwittI tend to agree16:16
sean-k-mooneywe should not use put for delete and im not convince we need to even include the generation version16:16
bauzaslet's discuss this after the meeting, if people want 16:16
sean-k-mooneysure16:16
bauzasbut I tend to agree too16:16
bauzasI need to understand the *why* for put16:17
bauzasso looking at the original change16:17
bauzasanyway16:17
bauzasmoving on16:17
bauzasPlacement periodic job status #link https://zuul.openstack.org/builds?project=openstack%2Fplacement&pipeline=periodic-weekly16:17
melwittthe only thing you get with PUT is if you issue a delete of your instance, if someone else updates it while it's deleting, you have a chance to reconsider your decision to delete it. afaik that might be the reasoning16:17
bauzasmelwitt: yeah, that's what I think too 16:18
bauzasfor a race 16:18
bauzasanyway16:18
melwittyeah sorry, can move on16:18
bauzasabout the placement periodic job, well, we merged stuff 16:18
sean-k-mooneyya that is what i assuem too but i dont think that is the right design choice if we delete it we shoudl just delete it16:18
bauzaslast week16:18
bauzasnow the job looks to work16:18
bauzas(we merged a now o-r-c)16:18
bauzas*version16:18
sean-k-mooneyo-r-c16:19
sean-k-mooney??16:19
sean-k-mooneyoh os-resouce-classes16:19
sean-k-mooneyyes16:19
sean-k-mooneyplacment was updated to account for the new os-resource-class release16:20
bauzasyeah, sorry was triying to find the patch16:20
bauzashttps://review.opendev.org/c/openstack/placement/+/79659516:20
bauzasanyway, nothing to tell more16:20
sean-k-mooneyone thing we might want to consider it preparing the patch when we are preparing the release16:21
bauzasexcept maybe https://zuul.openstack.org/build/0e135bb912b240c8bc2aa96049727a1a16:21
sean-k-mooneywe know we have to do this every time we release it so we can prementivly submit the placment patch with a depens on the releases repo patch16:21
bauzasoh nevermind, was fixed by the above16:21
bauzassean-k-mooney: you mean the placement release patch ?16:22
sean-k-mooneyyep16:22
bauzasfor m-3 ?16:22
sean-k-mooneyyes16:22
sean-k-mooneywell16:22
sean-k-mooneywhen we go to release o-r-c again16:22
sean-k-mooneywe can prepare a patch to placment for it 16:23
sean-k-mooneyto update the canary test and have it ready to go by depending on the pathch to the release repo16:23
bauzaswell, generally this is made by the release mgmt team but we can surely prepare it16:23
sean-k-mooneyim not sure if we will have anothger o-r-c release at m316:23
sean-k-mooneythey will open the patch if we dont but they ask the ptl to approve16:24
sean-k-mooneyso at that point we can just do the house keeping patch for placnement and preappove ti so it will merge wehen the release patch does16:24
bauzasthey ask either the release folk or the PTL, yup :)16:24
sean-k-mooneyanyway we can move on just a tought 16:24
bauzassean-k-mooney: keep your thought for next week when we get our ptl back16:25
bauzasmoving on16:25
bauzastime is flying16:25
bauzasPlease look at the gate failures, file a bug, and add an elastic-recheck  signature in the opendev/elastic-recheck repo (example: #link https://review.opendev.org/#/c/759967)16:25
bauzas#topic Release Planning 16:25
bauzasWe past M2 and spec freeze. M3 is in 5 weeks.16:26
bauzasWe have 21 approved an open blueprints and we have 5 weeks to finish  them. Please focus review effort on bps in Needs Code Review state.16:26
bauzasthat reminds me, people have to make sure their blueprint is on Needs Code Review 16:26
bauzas#link https://launchpad.net/nova/+milestone/xena-316:26
bauzasthe delivery status doesn't really mean anything but that can help reviewers to know which series to look at 16:27
bauzasso, if you love reviews, you know what to do16:27
bauzasNext deadline is non-client library freeze at 16th of August16:28
bauzasthink about it for os-resource-class ;) 16:28
bauzasmoving on16:28
bauzas#topic PTG Planning 16:29
bauzasPTG timeslots booked by gibi, see #link http://lists.openstack.org/pipermail/openstack-discuss/2021-July/023787.html16:29
bauzasThe PTG etherpad is ready to be filled with topics: #link https://etherpad.opendev.org/p/nova-yoga-ptg16:29
bauzasIf you see a need for a specific cross project section then please let gibi know16:29
bauzasI'm pretty sure this etherpad will be filled before we have the PTG :)16:30
bauzas#topic Stable Branches16:30
bauzaselodilles: flood is yours16:30
bauzasfloor*16:31
bauzas(oh man)16:31
elodillesstable gates are not blocked16:31
elodilles:)16:31
elodillesat least as far as I can tell16:31
bauzasexcellent, excellent :D16:31
bauzastbh, this is not really time of the cycle when I look at stable changes16:31
elodillesnot so much activity around stable branches (M2, M3, vacations, etc...)16:31
bauzasyup, most of the team efforts are focused on feature delivery as we speak, I guess 16:32
elodillesbauzas: true :)16:32
bauzasmoving on16:32
bauzas#topic Sub/related team Highlights 16:32
bauzasLibvirt (bauzas) 16:32
bauzasbauzas: floor is your16:32
bauzasbauzas: thanks16:32
bauzasbauzas: nothing to report, sir.16:32
bauzasbauzas: thanks.16:33
bauzasmoving on.16:33
bauzas#topic Open discussion 16:33
bauzasI refreshed and nothing popped in the wikipage while we were speaking16:33
bauzasso, nothing to say on this today, unless someone wanna raise something now16:34
stephenfinnope16:34
bauzas(I guess my fake dialog frightened a lof of people who disappeared)16:34
bauzasoh wow, at least someone stayed \o/16:34
bauzasI'm not that bad actor16:35
sean-k-mooneycan we ever really leave16:35
bauzasI could just pretend I'll keep the stick for the whole hour and prevent you to use this channel for the last 25 mins16:35
sean-k-mooneyi dont have anything more for today16:35
bauzasprivilege of the power, whahahah16:35
bauzasbut, heh,16:36
bauzas#stopmeeting16:36
bauzas#endmeeting 16:36
opendevmeetMeeting ended Tue Jul 27 16:36:53 2021 UTC.  Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)16:36
opendevmeetMinutes:        https://meetings.opendev.org/meetings/nova/2021/nova.2021-07-27-16.00.html16:36
opendevmeetMinutes (text): https://meetings.opendev.org/meetings/nova/2021/nova.2021-07-27-16.00.txt16:36
opendevmeetLog:            https://meetings.opendev.org/meetings/nova/2021/nova.2021-07-27-16.00.log.html16:36
bauzaseven16:36
sean-k-mooneymelwitt: bauzas  do you want to chat about the allocation delete issue16:37
bauzassean-k-mooney: I guess I need to look at the original change16:37
sean-k-mooneyit didnt really contain much more motivation then we surmised already16:38
sean-k-mooneyhttps://review.opendev.org/c/openstack/nova/+/59159716:38
sean-k-mooneyit looks like the current intent was to put the instance in error if there was a conflict16:39
sean-k-mooneywhich you would preumable fix by deleting it again16:39
sean-k-mooneyso based on that https://bugs.launchpad.net/nova/+bug/1836754 is invlid16:40
sean-k-mooneysince that is the expect behavior16:40
*** rpittau is now known as rpittau|afk16:40
sean-k-mooneyand presumable tempest does not handel the fact delete can fail and it shoudl retry16:40
melwittno it does not. I debated whether that should be the fix, to retry on 409 during resource cleanup16:41
melwittbut then I found the WIP change and thought it didn't seem like good UX to ever reject a delete request from a user16:41
sean-k-mooneyright16:42
melwittthat was in fact one of the things customers within yahoo when I worked there were vehement about, delete should never fail16:42
sean-k-mooneybut if we decied its not a good ux to delete it form the user instead of https://review.opendev.org/c/openstack/nova/+/68880216:42
sean-k-mooneywe should revert the previous patch and start callign delete again on plamcnet16:42
melwittI'm cool with that too. I wasn't 100% sure whether there's internal cases where we would want the chance to know about a conflict16:44
sean-k-mooneymelwitt: basiclaly  i think we have 2 options. mark the bug as invilad and adapt tempset for the 409, or always call placemtn with DELETE16:44
sean-k-mooneyi dont think we need a new arguement that default to ture16:44
bauzasI see two different things here16:45
sean-k-mooneymelwitt: do we know of any internal cases this would protect form 16:45
bauzas1/ a delete should always work and never return an exception, for sure16:45
bauzas2/ a racing delete could tho leave orphaned allocations and that's fine16:45
melwittsean-k-mooney: they're in the WIP patch, one of them was during a soft-delete reclaim periodic. I need to look again to see the other cases where we would pass force=False16:45
sean-k-mooneymelwitt: ack16:46
melwittbut yeah for sure when it's an end user requesting delete we should ensure that cannot fail16:46
melwittbauzas: only adjacent to how this discussion came about, I would love for you to be the second reviewer on the placement consumer types series, if you might have interest16:48
bauzasmelwitt: I can help16:48
melwitt\o/16:48
bauzasI mostly work on the generic mdev stuff but I'm not sure I'll be able to put all the changes before I leave for 3 weeks, so I'll maybe turn into reviews next week16:49
melwittgibi has gone through it thoroughly already (thank you gibi!) so it's in a nice shape now16:50
sean-k-mooneyi can try and take a look too but while i dicuss the api of placment alot i never really look at the code so millage will vary 16:51
bauzasmelwitt: ping me your series so I'll open a tab and make it a priority16:52
melwittsean-k-mooney: ++ 16:53
melwittbauzas: it's 3 patches starting here https://review.opendev.org/c/openstack/placement/+/669170 I will remove the -W, I put up a DNM nova change to run with the new stuff and it all worked well _except_ for hitting the generation conflict seemingly guaranteed16:54
opendevreviewStephen Finucane proposed openstack/nova master: mypy: Add nova.cmd, nova.conf, nova.console  https://review.opendev.org/c/openstack/nova/+/70565716:55
opendevreviewStephen Finucane proposed openstack/nova master: mypy: Add type annotations to top-level modules  https://review.opendev.org/c/openstack/nova/+/70565816:55
opendevreviewStephen Finucane proposed openstack/nova master: trivial: Clean manager.Manager, service.Service signatures  https://review.opendev.org/c/openstack/nova/+/76480616:55
opendevreviewStephen Finucane proposed openstack/nova master: WIP: Expand type hints for nova.block_device  https://review.opendev.org/c/openstack/nova/+/74217016:55
bauzasmelwitt: cool, will read tomorrow16:57
melwittthanks bauzas++16:57
sean-k-mooneymelwitt: bauzas  this is a summary of my view on the WIP patch https://review.opendev.org/c/openstack/nova/+/688802/2#message-70a28bde99719c723c954c947d0af17227a344e916:59
melwittthanks sean-k-mooney 17:00
sean-k-mooneymelwitt: there are other reasonce lients can recive a 409 namely locked instances so we can go either way with it17:00
melwittI feel strongly that delete (by an end user) should never fail17:00
melwittinternals can handle 409s if there is some reason to17:00
sean-k-mooneyright but the api contract is it must fail if the instance is locked17:00
melwittwell, yeah, agree that is expected17:01
melwittthat's the point (or one of the points) of locking17:01
sean-k-mooneyhttps://docs.openstack.org/api-ref/compute/?expanded=force-delete-server-forcedelete-action-detail#force-delete-server-forcedelete-action im kind of suprised force delete can also return a 40917:02
melwittyeah :\17:02
sean-k-mooneyforce-delet i do expect to never fail17:02
sean-k-mooneywell 401/403/404 sire17:03
sean-k-mooney*sure17:03
sean-k-mooneybut not 40917:03
melwittyeah17:03
melwittfyi there's a new spec proposed around migrating instances between projects that I had -2ed the implementation proposal as this majorly needs a spec https://review.opendev.org/c/openstack/nova-specs/+/80203417:08
sean-k-mooneymelwitt: oh i forgot to bring this up in the meeting 17:20
sean-k-mooneymelwitt: bauzas  how do you feel about approving/reviewing yoga specs now?17:21
sean-k-mooneywe normally wait untill around m3 but ^ and any other specs can be targeted again the yoga release now17:21
sean-k-mooneywe have the folder created17:21
sean-k-mooneymelwitt: and ya that needs a spec17:23
melwittI personally think review is welcome at any time. approval I think is natural post PTG at least but obviously it doesn't have to be17:23
sean-k-mooneywe sould also need support in every other serivce to move the ownership of the resouces17:23
sean-k-mooneywe have approved spec pre ptg before but mostly onse that missed the previous cycle17:24
melwittyeah. we've talked about it many times as we know, and some of the proposal were to only support the project move within nova and then let other tools call all the different projects' APIs17:24
sean-k-mooneyos-chown was the latest iteration of that right17:24
sean-k-mooneyhttps://github.com/kk7ds/oschown17:25
sean-k-mooneyi think there was a sepereate repo created form dansmith poc17:26
melwittyeah there is that and I didn't know about a separate repo17:26
sean-k-mooneythe only one i can find is os-migrate17:26
sean-k-mooneybut that is different17:27
sean-k-mooneythat is inter cloud migration17:27
sean-k-mooneymelwitt: so i think my issue with nova just doing the nova bits is we end up in a situration where for a time the ownser ship of each resouce is split between multipel porojects/users/domains17:28
opendevreviewStephen Finucane proposed openstack/placement master: tests: Silence noisy tests  https://review.opendev.org/c/openstack/placement/+/80110117:28
opendevreviewStephen Finucane proposed openstack/placement master: tox: Remove psycopg2 warning filter  https://review.opendev.org/c/openstack/placement/+/80091117:28
opendevreviewStephen Finucane proposed openstack/placement master: setup: Replace dashes with underscores  https://review.opendev.org/c/openstack/placement/+/80110217:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Replace implicit conversion of SELECT into FROM  https://review.opendev.org/c/openstack/placement/+/80091017:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Replace 'as_scalar()' with 'scalar_subquery()'  https://review.opendev.org/c/openstack/placement/+/80110017:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Update 'select()' calls  https://review.opendev.org/c/openstack/placement/+/80110317:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Remove use of non-integer/slice indices  https://review.opendev.org/c/openstack/placement/+/80110417:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Replace deprecated 'FromClause.select().whereclause' parameter  https://review.opendev.org/c/openstack/placement/+/80110517:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Use explicit transactions  https://review.opendev.org/c/openstack/placement/+/80110617:28
opendevreviewStephen Finucane proposed openstack/placement master: db: Remove unnecessary use of '_mapping'  https://review.opendev.org/c/openstack/placement/+/80110717:28
opendevreviewStephen Finucane proposed openstack/placement master: tox: Enable SQLAlchemy 2.0 warnings  https://review.opendev.org/c/openstack/placement/+/80110817:28
sean-k-mooneyfor example what happens if you move the ownwer ship of the neutron port of cinder volumes first before the nova instance17:29
sean-k-mooneyif we created the port and volume and delete on terminate is set17:29
sean-k-mooneythen you delete the instance17:29
melwittyeah... I think that's why we've nacked it in the past. if we add something like that, it needs a big fat warning on it. and maybe a dragon icon17:29
sean-k-mooneywhat do we do then17:29
sean-k-mooneybut dragons are cool17:30
sean-k-mooneywe should put a honey badger on it instead17:30
melwitt"if you do this and it messes stuff up, you are on your own"17:30
melwitthehe17:30
dansmithhow about we only allow the transition if the instance is locked?17:31
dansmithI don't remember how much lock really restricts, and we've probably not done a good job of honoring lock everywhere,17:31
melwittyeah true dragons might attract people to call the API17:31
dansmithbut it would at least be something procedural17:31
sean-k-mooneydansmith: admin bypass lock by default apparently which is fun17:31
dansmithsean-k-mooney: sure, but this change ownership call could ... not17:32
sean-k-mooneydansmith: but i get your point we could take some of the rough edges off with lock17:32
melwittconceptually I think that's a good idea. require some kind of freezing of it before allowing project migration17:32
dansmithif a compute rebooted in the middle or something I think you'd still get magic smoke17:32
sean-k-mooneyi assume this woudl ba an admin only api by default too17:33
dansmithfor sure17:33
sean-k-mooneyalthough a domain admin might have a usecase for it to not be17:33
melwittyes, haha I can't imagine it being open to normal users by default17:34
sean-k-mooneymelwitt: well not project member anyway or likely even project admin17:34
sean-k-mooneybut in new rbac world i coudl see domain_admin or system_admin having a use case for it17:34
dansmithnot project admin either, I wouldn't think17:35
sean-k-mooneyunless you did it liek cinder? i think it cinder that has a volume transfer api17:35
sean-k-mooneythat allows you to transfer volumes between tenatns17:35
melwittyeah that would seem weird to be able to move something into another project if you're not admin in that project. I guess if you were project admin in the source and destination projects?17:35
sean-k-mooneythat has a request and accpet model17:36
dansmithwell, if you did that, I guess you could also freeze all operations on the instance if it had a pending transfer,17:36
dansmithwhich maybe would be good17:36
dansmithI dunno17:36
sean-k-mooneyif i rememebr correct both side have to agree to the transfer17:36
melwittthat sounds familiar17:36
dansmithI dunno, I have a hard time ever seeing this being not fraught with pain17:36
sean-k-mooneyso i  think the work flow is project a start the tansfer then project b acclets it 17:36
dansmithtransferring a volume in isolation is a lot simpler than an instance with tentacles17:37
sean-k-mooneyyes true17:37
sean-k-mooneybut i like the idea of the transfering state as you said and potentally blocking all or most operation when its in that state17:37
sean-k-mooneywe might still want to supprot apport or delete17:38
sean-k-mooney*abort17:38
sean-k-mooneyaborting and roling back though is anohter can of worms17:38
dansmithyeah, for serious17:38
sean-k-mooneyso the current spec is for instance that dont have volumes only but honestly im not sure how useful it is at that point17:40
dansmithwell, you will always have a port, maybe more, so the problem is already bad without volumes17:40
dansmithmight as well include the volumes of course17:40
sean-k-mooneyalso they are adding that stiplulation to avoid the cinder interaction but its still there for neutron and cyborg so really unless we have a solution for all the things im not sure i see the point17:40
sean-k-mooneyyep17:41
dansmithwhat about backups? aren't those somehow linked to the instance?17:41
sean-k-mooneyhehe i was just thinking about snapshots in glance17:41
sean-k-mooneywhich might be store in swift or cinder on the backend17:41
melwitt+1 there are so many worms. but I'm not completely closed off to the idea if other people think it's worth the trouble17:41
sean-k-mooneyalthough i think the volumes / objecst are technialy onwed by glance in that case not the ownere of the image but still17:42
dansmithcorrect17:42
dansmiththe backend wouldn't be a thing I don't think17:42
sean-k-mooneywe do store tpm data in swift for shelved instance though17:42
sean-k-mooneyand secrets in barbican17:43
sean-k-mooneymelwitt: im not agaisnt the idea either just the spec ist trivialising what is a very hard thing to do17:43
sean-k-mooneywell to do right17:43
melwittyeah agree17:44
opendevreviewMerged openstack/nova master: docs: Fold in MDS security flaw doc  https://review.opendev.org/c/openstack/nova/+/78241118:24
opendevreviewMerged openstack/nova master: docs: Change formatting of hypervisor config guides  https://review.opendev.org/c/openstack/nova/+/78143918:24
opendevreviewMerged openstack/nova master: docs: Add libvirt misc doc  https://review.opendev.org/c/openstack/nova/+/78144018:24
opendevreviewMerged openstack/placement master: Add periodic-stable-jobs template  https://review.opendev.org/c/openstack/placement/+/77538418:38
melwittzzzeek: do you know what  TypeError("Boolean value of this clause is not defined") means in the context of trying to add a column with nullable=True? (I'm trying the statement you suggested on the review) https://pastebin.com/cVRZjhLk20:02
zzzeekit means the wrong kind of input is either being passed to op.add_column() or something internal in alembic20:04
zzzeekthat looks like an older version of alembic at least20:04
zzzeekmelwitt: ^^20:04
melwittok thanks for the hint20:05
zzzeekcould be a bug woudl have to see the calling code20:06
melwittthis is the20:08
melwittwith op.batch_alter_table('consumers') as batch_op:20:09
melwitt    batch_op.add_column('consumers', sa.Column('consumer_type_id', sa.Integer(), sa.ForeignKey('consumer_types.id'), nullable=True))20:09
zzzeekmelwitt: can you run the migration directly?  does it only fail when running the test suite?20:09
zzzeekohhh20:09
zzzeekwait20:09
zzzeektake the table name out20:09
zzzeekbatch_op.add_column(sa.Column(...))20:09
zzzeek^^^^20:10
melwittzzzeek: that looks to have worked, thank you!20:12
zzzeekyup20:13
opendevreviewLee Yarwood proposed openstack/nova master: trivial: Cleanup a comment about a now removed libvirt version check  https://review.opendev.org/c/openstack/nova/+/80261720:51
opendevreviewLee Yarwood proposed openstack/nova master: Add functional test for bug 1937375  https://review.opendev.org/c/openstack/nova/+/80201120:59
opendevreviewLee Yarwood proposed openstack/nova master: compute: Avoid duplicate BDMs during reserve_block_device_name  https://review.opendev.org/c/openstack/nova/+/80199020:59
opendevreviewMerged openstack/nova master: scheduler: Remove 'USES_ALLOCATION_CANDIDATES'  https://review.opendev.org/c/openstack/nova/+/77364021:21
opendevreviewLee Yarwood proposed openstack/nova master: fup: Move _wait_for_volume_attach into InstanceHelperMixin  https://review.opendev.org/c/openstack/nova/+/80262321:21
opendevreviewMerged openstack/nova master: scheduler: 'USES_ALLOCATION_CANDIDATES' removal cleanup  https://review.opendev.org/c/openstack/nova/+/79751321:25
opendevreviewmelanie witt proposed openstack/placement master: Add consumer_types migration, database and object changes  https://review.opendev.org/c/openstack/placement/+/66917021:58
opendevreviewmelanie witt proposed openstack/placement master: Microversion 1.38: API support for consumer types  https://review.opendev.org/c/openstack/placement/+/67944121:58
opendevreviewmelanie witt proposed openstack/placement master: Switch ConsumerType to use an AttributeCache  https://review.opendev.org/c/openstack/placement/+/67948621:58
opendevreviewMerged openstack/nova master: scheduler: Remove 'hosts_up'  https://review.opendev.org/c/openstack/nova/+/77364121:59
opendevreviewMerged openstack/nova master: trivial: Remove FakeScheduler (for realz)  https://review.opendev.org/c/openstack/nova/+/77364222:00

Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!