Wednesday, 2023-01-25

opendevreviewMerged openstack/nova master: Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87161201:51
gmanngibi: sean-k-mooney:  please review the placement RBAC change, https://review.opendev.org/c/openstack/placement/+/86561802:20
gmannI am sure it is in your list, just wanted to get review/merge this soon in case any thing we need to update/test we can do before m-302:22
sean-k-mooney[m]gmann: i have one question in line regarding listing traits02:34
sean-k-mooney[m]so im +1 but if we dont want to change listing traits im +2 on the change i think02:35
sean-k-mooney[m]illl check back in my morning02:36
gmannsean-k-mooney[m]: thanks. will check and reply02:36
gmannsean-k-mooney[m]: replied https://review.opendev.org/c/openstack/placement/+/865618/2/placement/policies/trait.py#4105:18
*** blarnath is now known as d34dh0r5307:01
*** bhagyashris_ is now known as bhagyashris|ruck07:34
Ugglagibi, hi I will have a look at the comments left by john. Strangely I did not noticed them before. (OO)08:27
Ugglagibi, regarding functional test yes in the latest patches there are some of them. Or do you think about tempest tests ?08:28
gibiUggla: no, not tempest. But then I will see them as I progress with the review today...08:34
zigoHi there!08:48
zigoI believe I have a working train patch for CVE-2022-47951, however, I had to change the default value of oslo.utils's QemuImgInfo() from format='human' to format='json'.08:49
zigoWhat should I do, should I make the Nova call add format='json' to the call, or change the default in oslo.utils?08:50
zigogibi: Your opinion?08:50
zigoOh, I have my answer ... :)08:52
fricklersince the fix is for nova, I would keep it restricted to that. just my 0.03€08:52
zigoLatest version has: https://github.com/openstack/nova/blob/master/nova/virt/images.py#L48 (ie: format='json')08:52
zigoSo I'll do that ...08:52
gibizigo: better to call from nova with format='json' to limit the change to that single call. But I see you arrived to that solution anyhow09:02
zigoYeah !09:03
zigoHopefully, I can just take these patches and do -> stein -> rocky, and then I'm good ! :)09:03
zigoContrary to what I wrote yesterday, only backporting https://review.opendev.org/c/openstack/nova/+/706897 was enough to get the VMDK check work in Train (plus that oslo.utils patch...).09:05
zigoShit, other failures ... :/09:15
johnthetubaguyFor security fixes, does the usual branch ordering of merging the fixss apply, I don't remember? i.e. do we just merge each stable patch as it goes green, or we do them in order?09:18
johnthetubaguy(i.e. xena seems ready to go now)09:18
zigoI may need all of https://review.opendev.org/c/openstack/nova/+/711679 after all ...09:20
gibijohnthetubaguy: I don't know about any exception from the stable policy for sec patches but maybe elodilles knows09:21
johnthetubaguyI remember we can't do anything other than +2, by policy, as the review was on the security bug ticket (well for the maintained branches anyways).09:22
* bauzas catches up the conversation09:29
bauzaswhat's the problem with the proposed fix ?09:30
johnthetubaguybauzas: So zigo has issues in train I think (which I am interested in, sadly), but I am more curious if we can merge stable branches out of order for a security fix like this?09:30
zigobauzas: Parts of nova changed between train and ussuri, but I think I can manage.09:31
zigoLet me finish the backporting ... :)09:31
bauzasjohnthetubaguy: I'm rushing to review the stable branches 09:31
bauzasjohnthetubaguy: technically, we have a CI job that prevents merging a patch on a stable branch if the parent isn't merged09:32
johnthetubaguyah, I didn't know that.09:32
bauzasfwiw, ChatGPT is unable to find the typo https://review.opendev.org/c/openstack/oslo.utils/+/706880/4/oslo_utils/imageutils.py despite me giving him clues09:33
johnthetubaguyI see arguments both ways for sure, but I wanted to check.09:33
bauzasso, master is merged09:36
bauzaszed was running on the gate but we got a failure09:36
johnthetubaguyack09:37
bauzasand I just +2d yoga09:37
johnthetubaguyOK, that was my question really, is that allowed?09:37
johnthetubaguyI guess we just wait for the +W?09:37
bauzasto merge some stable N-x patch before the parents ?09:38
* bauzas checks the stable policy09:38
bauzashttps://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes09:38
bauzas" Whether the fix is already on master and all consequent stable branches: a change must be a backport of a change already merged onto master, unless the change simply does not make sense on master. Same applies to N-2 releases, where N is master, in which case both N-1 and N branches should have the patch merged and so on."09:38
gibinova-tox-validate-backport job is voting in the gate queue and I that will prevent merging an older branch before the patch on newer branch lands09:39
bauzasbut, there is an exception rule09:39
bauzas" Some patches may get exception from rule 4 above. These are patches that do not touch production code, like test-only patches, or tox.ini changes that fix major gate breakage, etc.; or security patches that should not take much time to merge once the patches are published. In those cases, stable patches may be pushed into gate without waiting for all consequent branches to be fixed."09:39
bauzasgibi: that's what I told to johnthetubaguy09:39
johnthetubaguyyeah, I think we should just merge them all ASAP: "or security patches that should not take much time to merge once the patches are published"09:39
gibibauzas: so even though the policy allow secu patches to land our tooling did not09:39
bauzasgibi: I was about to write this09:39
johnthetubaguyah, got you09:39
bauzassomehow we are strictier than the policy 09:40
johnthetubaguyso what is the quickest path to merge? in parallel drop that CI job?09:40
bauzasjohnthetubaguy: either way, you know we'll need to publish releases09:40
bauzasupstream isn't generally a quick path for remediation09:41
bauzasbut we can try to rush09:41
johnthetubaguyits meant to be though, in this particular case.09:41
bauzasjohnthetubaguy: that's why distros got notified one week before the disclosure09:41
johnthetubaguygranted the patches are published, which is the important and crucial bit.09:41
bauzasI don't disagree and the security hole is quite dirty09:42
bauzasI would recommend our operators to exceptionnally patch directly09:42
bauzasthe patch itself is just about enforcing 09:43
gibione way to rush this is to add [stable-only] to the commit message to silence the backport job09:46
bauzasthat's debatable09:47
johnthetubaguygibi: I like your thinking!09:47
bauzasgibi: your help may be needed on https://review.opendev.org/c/openstack/nova/+/87162409:48
bauzasjohnthetubaguy: which release do you target to ship the security fix ?09:48
elodilleso/09:49
gibibauzas: I can approve that but the job will kick it out from the gate09:49
elodilleslet me know if any patch needs review09:49
gibielodilles: it is more like a process thing09:49
bauzaselodilles: we're on the CVE backports09:49
elodilles(i'm not aware of any special handling of security bug fixes)09:50
gibielodilles: the stable policy allows parallel merge of secu stable fixes09:50
bauzaselodilles: the zed one got kicked out due to some gate failure09:50
gibielodilles: but the backport job does not09:50
elodillesok, then [stable-only] is right09:50
gibielodilles: do you have a magic want to turn off the job or merge the patches against that job? 09:50
bauzasgibi: I'd say that if the stable policy agrees on that, we could refer to it if we respin the patch to add the [stable-only] tag09:50
gibiOK then we have a consensus09:50
elodillesif you want them to merge not in the known order09:51
gibibauzas: will you add the [stable-only] tags?09:51
bauzasideally, I'd rather prefer to have a specific tag but we're a bit on a rush09:51
johnthetubaguybauzas: we have a zoo of people on different things, I am not worried about them, thats all happening, I am more worried in general about it not landing in the stable branches.09:51
gibibauzas: then elodilles and me can approve qucikly09:51
johnthetubaguywhat about [stable-only][actually-a-cve] ?09:51
gibijohnthetubaguy: adding a sentence why we add stable-only make sense, yes09:52
elodilles:]09:52
gibiand as a follow up we should extend the job to accept [cve] as a tag to silence the landing order check09:52
johnthetubaguygibi: +109:52
bauzasjohnthetubaguy: as a reminder, train requires some oslo.utils bump due to https://review.opendev.org/c/openstack/oslo.utils/+/706880 missing09:53
johnthetubaguybauzas: appreciated, thanks, I saw zigo is working through that.09:53
zigoEither oslo bump, or backport of https://review.opendev.org/c/openstack/oslo.utils/+/70688009:56
zigoI did the later ...09:56
zigoYEAH !!!09:56
zigoGot something that worked ! \o/09:56
bauzasgibi: if we start parallelizing the backports, then we'll break the sha1 information on the commit msgs09:56
bauzassince there is no guarantee that the commit in the gerrit branch will have the same sha1 once merged09:57
gibibauzas: ignore the sha (it does not have it now either) as we disable the check anyhow09:57
bauzasgibi: I'll then mention the gerrit links09:57
gibibauzas: the bug ref is there to make a connection, but yes if you want you can add gerrit change id 09:57
gibior link09:57
bauzasunfortunately, we can't also track all commits easily as they're on different branches09:58
zigoSo, I did:09:58
zigohttps://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/cve-2022-47951-nova-stable-train.patch09:58
zigohttps://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Move_qemu-img_info_calls_into_privsep.patch09:58
zigohttps://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Make_JSON_the_default_output_format_of_calls_to_qemu-img_info.patch09:58
zigoWith these 3 patches, all unit tests are passing.09:58
zigoIs this acceptable (from your Nova upstream point of view) to backport these 3 patches on the train branch?09:59
bauzaszigo: propose the backports10:01
bauzasTrain is EM10:01
zigobauzas: The only issue is that I apply them in the wrong order, so patch 1 and 2 may have failures ...10:02
zigoCan I just merge the 3 patches then?10:02
johnthetubaguyzigo: +1 that looks like a nice solution to me. You can submit them as a patch chain in gerrit, that should work OK I think?10:02
bauzasthen I need to review those patches10:02
bauzasand yeah, this would have to be a gerrit series10:02
zigoOk, doing this.10:02
gibizigo: https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Make_JSON_the_default_output_format_of_calls_to_qemu-img_info.patch#L416 format=output_format ?10:03
zigogibi: That's what is in there: https://review.opendev.org/c/openstack/nova/+/711679/6/nova/virt/images.py#5710:04
opendevreviewSylvain Bauza proposed openstack/nova stable/yoga: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87162410:04
zigoAsk Lee Yarwood I guess? :)10:04
gibizigo: then that is a latent bug, so keep it as is10:04
opendevreviewSylvain Bauza proposed openstack/nova stable/xena: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87162210:04
opendevreviewSylvain Bauza proposed openstack/nova stable/wallaby: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87155710:05
gibizigo: lyarwood is gone from openstack unfortunately10:05
bauzasI'd be honest, I spotted a few bugs on this10:06
bauzas(and ChatGPT as well)10:06
bauzasbut nothing prevents the cve to be fixed10:06
bauzasgibi: did the [stable-only] hack10:07
bauzasenjoy10:07
bauzasjohnthetubaguy: ditto10:07
gibibauzas: I agree, lets fix the cve, the latent bugs can be fixed later / separately10:07
gibibauzas: on it10:07
zigobauzas: Last time, I asked ChatGPT to write an alembic migration env.py for me, using oslo.db, and it did kind of well ... :)10:07
gibibauzas: you missed https://review.opendev.org/c/openstack/nova/+/87161610:07
gibizigo: you live dangerously :)10:07
zigogibi: I didn't use what ChatGPT produced ! :)10:08
gibiahh :)10:08
bauzasgibi: no, this was on purpose, zed is on the check pipeline 10:08
bauzasand was on the gate before10:08
gibibauzas: zed will be kicked out of gate by the backport job as it has no commit hash10:08
bauzasgibi: oh shit, you're right10:08
bauzasdoing then the hack10:08
opendevreviewSylvain Bauza proposed openstack/nova stable/zed: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87161610:09
bauzasthere ^10:09
zigoI really don't feel confident pushing the 3 patches I linked above to Gerrit. I'm sure I'll do some mistakes like commit ID and such ...10:09
zigoCan someone else pick them up ?10:09
johnthetubaguygibi: does that job not run in the check queue?10:10
gibijohnthetubaguy: it is non-voting in check10:10
gibiafaik10:10
johnthetubaguyoh, I see it now: nova-tox-validate-backport https://zuul.opendev.org/t/openstack/build/016594434b19461781e2dbb3688a61cd : FAILURE in 4m 51s (non-voting)10:10
gibiyepp it failed ther nova-tox-validate-backport https://zuul.opendev.org/t/openstack/build/04bf544d0dc44ca48f45ba4d71bb8892 : FAILURE in 5m 45s (non-voting)10:11
bauzasyup, missed that when I rechecked10:12
bauzasI focused on the other failures :D10:12
gibithe important ones :D10:13
bauzasthat's two weeks I devoted to upstream10:13
bauzasand despite this, I nearly progressed on my feature by 5%.10:14
bauzaslovely.10:14
bauzasZuul, I look at you10:14
gibibauzas: keeping Zuul happy is part of the deal unfortunately :)10:14
gibimaybe you should ask ChatGPT to maintain it for us :D10:15
bauzasthat's... dangerous10:15
gibiOK all the patches I know of has +2 +A https://review.opendev.org/q/topic:bug%252F199618810:16
kashyapgibi: Even ChatGPT will throw up in the hands and say "it's a Sisphean task"10:16
gibikashyap: we need better aligned AIs then10:16
kashyapHeh10:16
kashyapAnd this time-out seems unfortunate :-( https://review.opendev.org/c/openstack/nova/+/87079410:17
zigoLooks like doing train -> stein is just a refresh of patches...10:18
bauzaszigo: I've quickly read your patches on your server10:22
bauzaswhy aren't you proposing them upstream ?10:22
bauzasthe problem is that I don't know which objects or methods you've touched 10:23
bauzasI mean, if you need help on how to propose a series on a stable branch, I can surely offer my help10:24
opendevreviewJohn Garbutt proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87169910:30
opendevreviewJohn Garbutt proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87170210:31
johnthetubaguyah, thanks gibi, your cut and paste of the topic was faster10:33
gibijohnthetubaguy: I think there is a merge conflict resolution issue in https://review.opendev.org/c/openstack/nova/+/87169910:35
johnthetubaguyoops, yes10:35
johnthetubaguyI only got as far as pep8 locally10:35
johnthetubaguyfixing that now10:36
gibiaffects the stable/ussuri one too10:36
johnthetubaguyyeah, I cherry-picked that down10:36
sean-k-mooneyi have not read back but why do we have stable only variants10:39
sean-k-mooneydan had proposed cherry picks already10:39
gibisean-k-mooney: we want to land them in parallel10:40
gibisean-k-mooney: and the only way to allow it is to add [stable-only]10:40
sean-k-mooneyhum ok10:40
gibiotherwise the backport job will prevent the merge10:40
gibielodilles: was OK with this from stable :D10:40
sean-k-mooneyi was going to try and land them in sequence10:40
sean-k-mooneyand just hold other patches until they were merged10:41
gibisean-k-mooney: johnthetubaguy expressed time sensitiveness and the stable/policy actually allows it for secu patches10:41
sean-k-mooneyit does but i didnt tink it warrented it10:41
sean-k-mooneybut if other are happy then ok10:41
sean-k-mooneyi just wont try and babysit the patches today10:42
gibiat least bauzas, johnthetubaguy, elodilles and myself was OK with it10:42
johnthetubaguysean-k-mooney: I am curious, why do you think its not urgent enough?10:43
sean-k-mooneyform my perspective there is nothign preventing a downstream form merging them once its proposed. and for anyone that ues a release i.e. form pypi its not going to get it to them faster. those that use source builds can use the gerrit version10:44
opendevreviewJohn Garbutt proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87169910:44
sean-k-mooneyi was still expecting them to all merge today or tommorrow without needing to do it in parallel10:44
gibisean-k-mooney: I assume we will propose a stable release as soon as a stable patch lands10:44
sean-k-mooneyi think anyone who is going to deploy that by the end of the week would jsut take the patch and do it them selevs 10:45
gibisean-k-mooney: given the gate speed parallel is lot faster than sequential when we need to do 7 backports10:45
sean-k-mooneythe gate is fast if this is the only thing we are merging 10:46
gibigate is unstable regardles of the number of patches on it, also we have no power over how other projects using the gate resources 10:46
sean-k-mooneyif we are merging things in parallel less so but as i said im fine with it i just dont think we needed it. i was still expecting all these to be merged by tomrrow either ay10:46
sean-k-mooneygibi: i didnt mean other project i ment lets not merge anything that would put these in merge conflict10:47
sean-k-mooneyanyway ill leave this to ye since ye have a plan10:47
bauzassean-k-mooney: I don't disagree with you, and I expressed the fact that we need anyway need to release so those fixes could be eventually helpful10:48
gibiI think the merge conflict is just a small percent of why serial merge would be slower here. It is more like rechecks and waiting for the patch on the newer branch that slow sequential down10:48
opendevreviewJohn Garbutt proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87170210:48
bauzasbut it occurred from johnthetubaguy that we could rush the things a little bit up10:48
bauzasand eventually I joined the 'rush' team when I saw our stable policy was accepting it10:48
sean-k-mooneywe have a second CVE fix with backprots that need review upstream10:49
bauzassean-k-mooney: just explaining my thoughts journey10:49
sean-k-mooneyif we are going to do a release we proably shoudl also merge that with the same speed10:49
bauzassean-k-mooney: that's understandable, despite that one was pretty nasty10:49
bauzassean-k-mooney: patches ?10:49
sean-k-mooneylooking for them now 10:50
bauzasI only remember the former-embargoed one which is now public10:50
gibihttps://review.opendev.org/q/topic:bug%252F198181310:50
gibiI think sean-k-mooney refers to ^^10:50
gibiit is a lot less sever though10:50
sean-k-mooneyyes10:50
sean-k-mooneywe have a downstream deadlien ot have that merged internally by next week10:50
gibiand we have downstream proactive backport too ;)10:51
sean-k-mooneybut i would prefer to also have it merged upstream10:51
johnthetubaguyI didn't see that one, yes, that should get some love too.10:51
sean-k-mooneyif we are doing a release we should ideally include both or do a second release once both are there10:51
bauzasmmm, OK, I only tho see 'in progress' on ossa10:51
bauzasso I guess that one isn't on mitre10:52
bauzasnevermind me, it is https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-3739410:52
bauzasbut yeah, it's a dos10:52
bauzasnot a compute exposure10:53
sean-k-mooneypreventign the compute agent form stating10:53
bauzasyup, hence the security level10:53
sean-k-mooneyfrom a public cloud perspecivive both are costly in different ways10:53
bauzaswe can try to land them at the same pace, but I don't want to wait more than one day if we can't due to them10:53
bauzassean-k-mooney: I tend to set a different priority on it10:54
gibiI agree to not wait more than a day with a release if the latest cve fix lands10:54
bauzaswe're not talking of direct access to the host which could compromize all the dataplane10:54
sean-k-mooneywell that not quite what the other cve does10:55
sean-k-mooneyit can give you direct access to the file system but not the network10:55
sean-k-mooneyanyway fine lets continue but before i saw the new cve patches yesterday i had planned to ask use to prioites this older one in the team meeting yesterday10:56
bauzasif you have access to the file system, you can access the network later10:56
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (db)  https://review.opendev.org/c/openstack/nova/+/83119310:56
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (objects)  https://review.opendev.org/c/openstack/nova/+/83940110:56
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (manila abstraction)  https://review.opendev.org/c/openstack/nova/+/83119410:56
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (drivers and compute manager part)  https://review.opendev.org/c/openstack/nova/+/83309010:56
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (api)  https://review.opendev.org/c/openstack/nova/+/83683010:56
opendevreviewribaudr proposed openstack/nova master: Check shares support  https://review.opendev.org/c/openstack/nova/+/85049910:56
opendevreviewribaudr proposed openstack/nova master: Add metadata for shares  https://review.opendev.org/c/openstack/nova/+/85050010:56
opendevreviewribaudr proposed openstack/nova master: Add instance.share_attach notification  https://review.opendev.org/c/openstack/nova/+/85050110:56
opendevreviewribaudr proposed openstack/nova master: Add instance.share_detach notification  https://review.opendev.org/c/openstack/nova/+/85102810:56
opendevreviewribaudr proposed openstack/nova master: Add shares to InstancePayload  https://review.opendev.org/c/openstack/nova/+/85102910:56
opendevreviewribaudr proposed openstack/nova master: Add helper methods to attach/detach shares  https://review.opendev.org/c/openstack/nova/+/85208510:56
opendevreviewribaudr proposed openstack/nova master: Add libvirt test to ensure metadata are working.  https://review.opendev.org/c/openstack/nova/+/85208610:57
opendevreviewribaudr proposed openstack/nova master: Add virt/libvirt error test cases  https://review.opendev.org/c/openstack/nova/+/85208710:57
opendevreviewribaudr proposed openstack/nova master: Add share_info parameter to reboot method for each driver (driver part)  https://review.opendev.org/c/openstack/nova/+/85482310:57
opendevreviewribaudr proposed openstack/nova master: Support rebooting an instance with shares (compute and API part)  https://review.opendev.org/c/openstack/nova/+/85482410:57
opendevreviewribaudr proposed openstack/nova master: Add instance.share_attach_error notification  https://review.opendev.org/c/openstack/nova/+/86028210:57
opendevreviewribaudr proposed openstack/nova master: Add instance.share_detach_error notification  https://review.opendev.org/c/openstack/nova/+/86028310:57
opendevreviewribaudr proposed openstack/nova master: Add share_info parameter to resume method for each driver (driver part)  https://review.opendev.org/c/openstack/nova/+/86028410:57
opendevreviewribaudr proposed openstack/nova master: Support resuming an instance with shares (compute and API part)  https://review.opendev.org/c/openstack/nova/+/86028510:57
opendevreviewribaudr proposed openstack/nova master: Add helper methods to rescue/unrescue shares  https://review.opendev.org/c/openstack/nova/+/86028610:57
opendevreviewribaudr proposed openstack/nova master: Support rescuing an instance with shares (driver part)  https://review.opendev.org/c/openstack/nova/+/86028710:57
opendevreviewribaudr proposed openstack/nova master: Support rescuing an instance with shares (compute and API part)  https://review.opendev.org/c/openstack/nova/+/86028810:57
opendevreviewribaudr proposed openstack/nova master: Change microversion to 2.XX  https://review.opendev.org/c/openstack/nova/+/85208810:57
opendevreviewribaudr proposed openstack/nova master: Documentation  https://review.opendev.org/c/openstack/nova/+/87164210:57
* gibi needs to drop for an hour...10:57
sean-k-mooneybauzas: have they confirmed that raw devices special files are actully usable im not sure file access imples network access10:57
sean-k-mooneyit would come down to the speicics of the vmdk impl10:58
sean-k-mooneyanyway we dont need to speculate10:58
bauzassean-k-mooney: I tested it by myself10:58
bauzasand when I say it's nasty, trust me it is10:58
sean-k-mooneyok well since this has been discusled upstream we now also need ot have it downstream by next week at the latest10:58
johnthetubaguybauzas: escation to something nasty should be assume with these things, for sure. Even if we can't see it yet.11:01
sean-k-mooneyjohnthetubaguy: i was just thinking that if it was anythin like virtio-fs special files cant be accesed but i trust dan and bauzas when they say it was nasty11:02
bauzasjohnthetubaguy: sean-k-mooney: read the bug report11:03
bauzasand the first comments, there is a reproducer11:03
sean-k-mooneyi have not reviewed the bug/cve in any detail rather just enduing the patches were moving last night11:03
sean-k-mooneyok so on that reading it does not allow direct network access form the main descrption anyway lets not look for other ways to abuse this11:06
sean-k-mooneyif i try hard enough im sure i can come up with ways to make it worse and i really dont want to do that on a public channel11:06
bauzas+111:09
* bauzas needs to feed himself11:10
zigobauzas: I'll push my patches in a bit...11:18
zigoFYI, I got Stein fixed too... :)11:18
zigoStein -> Rocky backport seems another tricky one...11:18
priteauAnyone know why grenade keeps failing on stable/wallaby with the vmdk patch?12:13
opendevreviewMerged openstack/nova stable/yoga: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/85931212:38
zigoI also got Rocky in shape! :P12:53
opendevreviewMerged openstack/nova stable/yoga: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/85931313:15
bauzassent the xena patches for ^ to the gate 13:22
* bauzas needs to go afk for getting his children's passeports13:22
bauzaspassports* even13:22
* bauzas back in less than 20 mins hopefully13:22
gibibauzas: thanks13:26
sahid0/ bauzas sean-k-mooney https://review.opendev.org/c/openstack/nova/+/858384 when you have a moment if you can double-check the phrasing regarding doc I hope that will be aligned with your thinking13:40
opendevreviewJorge San Emeterio proposed openstack/nova master: WIP: Dividing global privsep profile  https://review.opendev.org/c/openstack/nova/+/87172913:41
sean-k-mooneysahid: sure im on a call but ill check when it wraps13:41
sahidsean-k-mooney: no worries, thanks a lot for your time :-)13:42
opendevreviewJorge San Emeterio proposed openstack/nova master: WIP: Dividing global privsep profile  https://review.opendev.org/c/openstack/nova/+/87172913:44
opendevreviewJorge San Emeterio proposed openstack/nova master: WIP: Dividing global privsep profile  https://review.opendev.org/c/openstack/nova/+/87172913:47
*** dasm|off is now known as dasm13:59
gibiI did a round of rechecks on the vmdk cve 14:04
gibibauzas: bahh, I need to update commit hashes in the https://review.opendev.org/q/topic:bug%252F1981813 series all the way back from xena to train14:16
gibifun14:16
opendevreviewBalazs Gibizer proposed openstack/nova stable/xena: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/85931414:43
opendevreviewBalazs Gibizer proposed openstack/nova stable/xena: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/85931514:43
bauzasgibi: I guess I can review it again ? ^14:52
* bauzas feels today as a fireman14:52
gibibauzas: yeah I hope I did not screw up copying hashes14:52
bauzasgibi: you used the merge patch one ?14:53
gibiyes14:53
gibinow it points to the merged one14:53
bauzasshould work so14:53
gibiin both xena patch14:53
gibies14:53
gibiand I have to do it for the rest of the stable branches too14:53
opendevreviewDan Smith proposed openstack/nova master: WIP: Detect host renames and abort startup  https://review.opendev.org/c/openstack/nova/+/86392014:56
gibia recheckd https://review.opendev.org/c/openstack/nova/+/871622 back to the gate it was kicked due to a slow CI node14:58
bauzasgibi: yup, thanks for having rechecked the changes15:03
gibibauzas: even with trying to land them in parallel I doubt we will land all of them until friday15:06
gibimaybe if the gate is better during the night...15:06
* bauzas won't send a crossfingers emoji but you get it15:06
gibi:D15:06
artom🤞15:23
artomHey, it's unicode!15:23
bauzasartom: tss, wanted to avoid it :p15:38
* artom trolls15:39
bauzas🐈‍⬛15:39
artomThat's basically the extent of my involvement here now15:39
bauzasartom: I'm glad that the litterally first message you write here today is an emoji15:39
bauzasartom: and I'm happy this isn't a poop one15:40
artomI'm like emoji batman15:40
bauzasa wealthy man that hides his emitions behind a mask and wears underwear on top of his pants ?15:43
artomWell, one of those is true15:44
bauzasI hope it's the former, I'm afraid it could be the latter15:45
gibitoo much details :D15:46
bauzasyay15:47
bauzasgibi: you were right, it sounds we have a problem with grenade on xena15:47
gibibauzas: did it fail again?15:47
bauzasyup15:47
gibiwith the same fastener dep issue?15:47
bauzashttps://zuul.opendev.org/t/openstack/build/d8e11efbdc6d43e7b9e27273cf03878615:47
gibi /o\ I have a hunch15:48
gibihttps://review.opendev.org/c/openstack/tempest/+/821732/21/requirements.txt yeah we landed this15:49
bauzas2023-01-25 14:45:10.253 | ERROR: Could not find a version that satisfies the requirement fasteners>=0.16.015:49
bauzashttps://00f8d73ac2d869c11924-90ff9157beec64657d8a46242c5af814.ssl.cf1.rackcdn.com/871557/3/check/nova-grenade-multinode/d8e11ef/controller/logs/grenade.sh_log.txt15:49
bauzasand I was wrong, this is on wallaby, not xena15:50
bauzas-ETOOMANYBRANCHESANDPATCHESTOTRACK15:50
gibiwhat is the last stable branch tempest master supports?15:51
gibiI feel like wallaby is at the boundary15:51
bauzaspossibly15:51
gibigmann: ^^ 15:51
bauzasbut we also have problem with ceph-multistore on yoga15:51
bauzaschanging my focus15:51
bauzashttps://review.opendev.org/c/openstack/nova/+/871624 failed again15:51
gibigmann: it seems https://review.opendev.org/c/openstack/tempest/+/821732 affects stable/wallaby grenade runs15:52
gmanngibi tempest master stopped wallaby support recently. it is stable/xena the last stable it support15:52
gmanngibi: but I have not pinned stable/wallaby with old compatible tempest which I should do15:53
gibigmann: I see so stable/wallaby runs with master tempest and has https://review.opendev.org/c/openstack/tempest/+/821732 but it shoudl not run with master tempest any more15:53
gmanntill now it was running fine but if it is breaking its time to pin tempest there15:53
gibigmann: yes, it breaks now on the fasteners >= 0.16 dependency15:53
gibithat https://review.opendev.org/c/openstack/tempest/+/821732 introduced15:54
gmanngibi: yeah. I will pin it today15:54
gibigmann: thank you!15:54
opendevreviewMerged openstack/nova stable/zed: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87161616:07
bauzassean-k-mooney: +2d sahid's implementation of stopping evacuated instances 16:25
opendevreviewBalazs Gibizer proposed openstack/nova stable/wallaby: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/85932016:35
opendevreviewBalazs Gibizer proposed openstack/nova stable/wallaby: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/85932116:35
opendevreviewBalazs Gibizer proposed openstack/nova stable/victoria: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/86958316:38
opendevreviewBalazs Gibizer proposed openstack/nova stable/victoria: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/86958416:38
opendevreviewBalazs Gibizer proposed openstack/nova stable/ussuri: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/86958516:42
opendevreviewBalazs Gibizer proposed openstack/nova stable/ussuri: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/86958616:42
sahidthank you bauzas ++16:52
opendevreviewBalazs Gibizer proposed openstack/nova stable/train: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/86967316:59
opendevreviewBalazs Gibizer proposed openstack/nova stable/train: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/86967416:59
bauzassahid: I'm really sorry, but I forgot to look at your dependent patch and I found something :(17:00
bauzassahid: https://review.opendev.org/c/openstack/nova/+/858383/2517:00
bauzassahid: tl,dr: you return an exception if a caller asks for a target_state parameter that the compute doesn't know17:01
bauzassahid: thinking out loud, I think this would be better to just *not* provide the target_state parameter if the compute is old17:02
gibithe vmdk cv victoria patch https://review.opendev.org/c/openstack/nova/+/871699/ will be get kicked out of the gate as the commit message has a hash but that hash is not laneded yet https://zuul.opendev.org/t/openstack/build/0e2475a0312d4cdaa0774a0fc20c42ce/log/job-output.txt#1552 it seems the [stable-only] tag only disables the hash check if there is no hash in the commit message17:02
bauzasthis shouldn't be arriving, since you verify that all computes are upgraded, but I'd prefer us to make it clear17:02
bauzasgibi: ack17:02
gibiI will go and remove the hash from the commit message to keep landing the fixes in parallel17:04
opendevreviewBalazs Gibizer proposed openstack/nova stable/wallaby: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87155717:06
opendevreviewBalazs Gibizer proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87169917:07
opendevreviewBalazs Gibizer proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87170217:07
bauzasgibi: sean-k-mooney: I'm actually surprised to see some RPC pattern returning an exception if a compute is too old, instead of just remove the parameter from the call we do17:08
bauzasif we really want to have RPC backwards compat, the RPC client needs to adapt to what the manager supports17:09
sean-k-mooneyif you request something at the api that requries a new rpc version we shoudl not back levle17:12
sean-k-mooneythat should be an api error17:12
sean-k-mooneywe normally use a compute service bump to allow use to detect this in the api17:13
sean-k-mooneybefore getting to the rpc code17:13
bauzasand that's what sahid does17:13
bauzasbut I don't really like us returning exceptions we don't really manage upside17:13
dansmithmaking a call, getting an exception and making it again with different stuff is wasteful *and* wrong,17:14
sean-k-mooneyright and we are not doing that17:14
dansmithokay17:14
sean-k-mooneywhere it can be backleveled we do prepare/version check17:14
bauzasdansmith: tl;dr sahid is adding a service check that verifies all computes are upgraded before adding a parameter17:14
sean-k-mooneybut if you use the new parmater then its not valide to remove it17:14
dansmithack17:15
bauzasby default the param is set to None on the API method17:15
sean-k-mooneybauzas: yep i would break our api microversioning to backlevel in this case17:15
bauzasthen it calls the conductor and then the compute17:15
sean-k-mooneybauzas: only for the new microversion no?17:16
sean-k-mooneywe backlevel if its actully None17:16
sean-k-mooneyi need to pull up the patch again17:16
tobias-urdinis the uuid of a mdev just a random uuidutils.uuid() or is it tracked somewhere in placement?17:16
sean-k-mooneytobias-urdin: totally random17:16
tobias-urdinsean-k-mooney: ack ty17:17
bauzassean-k-mooney: technically with sahid's proposal, we backlevel by the if condition17:17
bauzasbut ok, I see my mistake, I'll clarify my second comment17:18
bauzaseither way, my first comment remains valid, we lack a negative test17:18
sean-k-mooneywe do it by  if not client.can_send_version(version):17:18
sean-k-mooneyso if we cant send the version and target_state is not None we raise17:19
sean-k-mooneyhttps://review.opendev.org/c/openstack/nova/+/858383/25/nova/compute/rpcapi.py#110617:19
sean-k-mooneythis is what you are askign about yes17:20
bauzasyep, you convinced me on the RPC contracty17:20
bauzasmy second comment is not valid17:21
bauzasthat being said, I'm not super happy with those generic exceptions being raised without being properly captured at the API side17:21
bauzasbut that's not worth a -117:21
bauzasmy -1 is just about missing unittests on the RPC checks17:21
sean-k-mooneyim ok with useing pop to do the removal but we woudl need to check the result to determin if we raise or reducet the verison17:21
bauzassean-k-mooney: we're on agreement17:22
bauzasthat's what I mean by the RPC contract17:22
bauzasif this parameter is set to something, we can't backlevel17:22
sean-k-mooneyoh its missing a negitive test17:22
bauzasyou convinced me17:22
bauzassean-k-mooney: yup, that's why I -117:23
sean-k-mooneyya you are right i tough this was unit tested but only the happy path17:23
bauzasand I was +0 on the fact we raise a NovaException without properly capturing it on the API level, but that wasn't a patch blocker17:23
sean-k-mooneyi tought that would be confreted we are raisng nova excption below here too https://review.opendev.org/c/openstack/nova/+/858383/25/nova/compute/rpcapi.py#111617:25
sean-k-mooney*converted17:26
sean-k-mooneywith that said we want this to be a 400 not a 50017:26
sean-k-mooneyoh but this is not hooked up to the api yet17:26
sean-k-mooneyso we have to check the second patch17:26
bauzassean-k-mooney: I checked and this isn't the case17:27
bauzasthat being said, again, not -1ing on this, rather +0ing, because this situation shouldn't arrive thanks to the version check17:28
bauzasthis is just us not being enough prescriptive on the exception handling17:29
sean-k-mooneyright but it would be good to have negitive unit tests in the first patch and a negitive funcitonl test in the secod17:29
bauzas(usually I hate standard exceptions that are meaningless and hard to identify)17:29
sean-k-mooneybauzas: because even if fully upgraded17:29
sean-k-mooneyyou could have an rpc pin set in the config right17:29
bauzastrue17:29
sean-k-mooneythat woudl pass the compute service check17:29
sean-k-mooneyso we dont want this to be a 50017:29
sean-k-mooneyit should be a 400 or maybe 40917:30
bauzasthen I turn my api patch review vote to -117:30
bauzasbecause of the pinset17:30
bauzassean-k-mooney: are you sure that if we pin the rpc versions we hit this ?17:30
bauzasI thought the service check would say "'meh nah"'17:31
bauzasoh17:31
bauzasno, you're right17:31
sean-k-mooneyif we pin to 6.0 the can send version check will fail for 6.217:31
bauzastrue17:31
bauzasthe RPC version check will fail to accept 6.217:31
bauzasbutn,17:31
sean-k-mooneyso we should assert that returns somethign other then a 500 at the api17:31
bauzasthe api version check on check_min_versions() will say 'surely, you can call'17:31
sean-k-mooneyyep17:32
sean-k-mooneybut i dont think this is a probelm in the code nessisarly17:32
sean-k-mooneyjust in the test coverage and perhapse the excpetion raised17:32
sean-k-mooneywe use 409 to comunicate this in other places17:33
sean-k-mooneybauzas: do you want to capture that in the review17:33
sean-k-mooneyor shall i an link to this irc conversation17:34
sean-k-mooneyi think we just need a few more edgecases here https://review.opendev.org/c/openstack/nova/+/858384/34/nova/tests/functional/api_sample_tests/test_evacuate.py17:35
bauzassean-k-mooney: just left comments17:35
bauzasbut I can explain this to sahid later17:35
sean-k-mooneyack17:35
sean-k-mooneythanks for bringing this up17:36
opendevreviewMerged openstack/nova stable/xena: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/85931417:36
bauzasand yeah HTTP409 Conflict makes perfect sense17:36
* bauzas ends his day on this17:37
bauzasI'll just double check the cve patches on fly17:37
gibiUggla: I left some comment in the API patch of the Manial series https://review.opendev.org/c/openstack/nova/+/83683018:03
opendevreviewMerged openstack/nova stable/xena: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87162218:04
opendevreviewMerged openstack/nova stable/xena: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/85931518:05
opendevreviewMerged openstack/nova stable/yoga: [stable-only][cve] Check VMDK create-type against an allowed list  https://review.opendev.org/c/openstack/nova/+/87162420:08
*** dasm is now known as dasm|off21:51
opendevreviewGhanshyam proposed openstack/nova stable/wallaby: DNM: testing tempest pin for stable/wallaby  https://review.opendev.org/c/openstack/nova/+/87179821:56
opendevreviewGhanshyam proposed openstack/nova stable/xena: DNM: testing tempest pin for stable/wallaby  https://review.opendev.org/c/openstack/nova/+/87180022:00
tobias-urdinproposed new releases for xena, yoga and zed to get the CVE-2022-47951 out the door, those backports are merged and no open patches on branches https://review.opendev.org/c/openstack/releases/+/87180222:27

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