opendevreview | Merged openstack/nova master: Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871612 | 01:51 |
---|---|---|
gmann | gibi: sean-k-mooney: please review the placement RBAC change, https://review.opendev.org/c/openstack/placement/+/865618 | 02:20 |
gmann | I 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-3 | 02:22 |
sean-k-mooney[m] | gmann: i have one question in line regarding listing traits | 02:34 |
sean-k-mooney[m] | so im +1 but if we dont want to change listing traits im +2 on the change i think | 02:35 |
sean-k-mooney[m] | illl check back in my morning | 02:36 |
gmann | sean-k-mooney[m]: thanks. will check and reply | 02:36 |
gmann | sean-k-mooney[m]: replied https://review.opendev.org/c/openstack/placement/+/865618/2/placement/policies/trait.py#41 | 05:18 |
*** blarnath is now known as d34dh0r53 | 07:01 | |
*** bhagyashris_ is now known as bhagyashris|ruck | 07:34 | |
Uggla | gibi, hi I will have a look at the comments left by john. Strangely I did not noticed them before. (OO) | 08:27 |
Uggla | gibi, regarding functional test yes in the latest patches there are some of them. Or do you think about tempest tests ? | 08:28 |
gibi | Uggla: no, not tempest. But then I will see them as I progress with the review today... | 08:34 |
zigo | Hi there! | 08:48 |
zigo | I 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 |
zigo | What should I do, should I make the Nova call add format='json' to the call, or change the default in oslo.utils? | 08:50 |
zigo | gibi: Your opinion? | 08:50 |
zigo | Oh, I have my answer ... :) | 08:52 |
frickler | since the fix is for nova, I would keep it restricted to that. just my 0.03€ | 08:52 |
zigo | Latest version has: https://github.com/openstack/nova/blob/master/nova/virt/images.py#L48 (ie: format='json') | 08:52 |
zigo | So I'll do that ... | 08:52 |
gibi | zigo: better to call from nova with format='json' to limit the change to that single call. But I see you arrived to that solution anyhow | 09:02 |
zigo | Yeah ! | 09:03 |
zigo | Hopefully, I can just take these patches and do -> stein -> rocky, and then I'm good ! :) | 09:03 |
zigo | Contrary 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 |
zigo | Shit, other failures ... :/ | 09:15 |
johnthetubaguy | For 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 |
zigo | I may need all of https://review.opendev.org/c/openstack/nova/+/711679 after all ... | 09:20 |
gibi | johnthetubaguy: I don't know about any exception from the stable policy for sec patches but maybe elodilles knows | 09:21 |
johnthetubaguy | I 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 conversation | 09:29 | |
bauzas | what's the problem with the proposed fix ? | 09:30 |
johnthetubaguy | bauzas: 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 |
zigo | bauzas: Parts of nova changed between train and ussuri, but I think I can manage. | 09:31 |
zigo | Let me finish the backporting ... :) | 09:31 |
bauzas | johnthetubaguy: I'm rushing to review the stable branches | 09:31 |
bauzas | johnthetubaguy: technically, we have a CI job that prevents merging a patch on a stable branch if the parent isn't merged | 09:32 |
johnthetubaguy | ah, I didn't know that. | 09:32 |
bauzas | fwiw, 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 clues | 09:33 |
johnthetubaguy | I see arguments both ways for sure, but I wanted to check. | 09:33 |
bauzas | so, master is merged | 09:36 |
bauzas | zed was running on the gate but we got a failure | 09:36 |
johnthetubaguy | ack | 09:37 |
bauzas | and I just +2d yoga | 09:37 |
johnthetubaguy | OK, that was my question really, is that allowed? | 09:37 |
johnthetubaguy | I guess we just wait for the +W? | 09:37 |
bauzas | to merge some stable N-x patch before the parents ? | 09:38 |
* bauzas checks the stable policy | 09:38 | |
bauzas | https://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes | 09: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 |
gibi | nova-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 lands | 09:39 |
bauzas | but, there is an exception rule | 09: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 |
bauzas | gibi: that's what I told to johnthetubaguy | 09:39 |
johnthetubaguy | yeah, 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 |
gibi | bauzas: so even though the policy allow secu patches to land our tooling did not | 09:39 |
bauzas | gibi: I was about to write this | 09:39 |
johnthetubaguy | ah, got you | 09:39 |
bauzas | somehow we are strictier than the policy | 09:40 |
johnthetubaguy | so what is the quickest path to merge? in parallel drop that CI job? | 09:40 |
bauzas | johnthetubaguy: either way, you know we'll need to publish releases | 09:40 |
bauzas | upstream isn't generally a quick path for remediation | 09:41 |
bauzas | but we can try to rush | 09:41 |
johnthetubaguy | its meant to be though, in this particular case. | 09:41 |
bauzas | johnthetubaguy: that's why distros got notified one week before the disclosure | 09:41 |
johnthetubaguy | granted the patches are published, which is the important and crucial bit. | 09:41 |
bauzas | I don't disagree and the security hole is quite dirty | 09:42 |
bauzas | I would recommend our operators to exceptionnally patch directly | 09:42 |
bauzas | the patch itself is just about enforcing | 09:43 |
gibi | one way to rush this is to add [stable-only] to the commit message to silence the backport job | 09:46 |
bauzas | that's debatable | 09:47 |
johnthetubaguy | gibi: I like your thinking! | 09:47 |
bauzas | gibi: your help may be needed on https://review.opendev.org/c/openstack/nova/+/871624 | 09:48 |
bauzas | johnthetubaguy: which release do you target to ship the security fix ? | 09:48 |
elodilles | o/ | 09:49 |
gibi | bauzas: I can approve that but the job will kick it out from the gate | 09:49 |
elodilles | let me know if any patch needs review | 09:49 |
gibi | elodilles: it is more like a process thing | 09:49 |
bauzas | elodilles: we're on the CVE backports | 09:49 |
elodilles | (i'm not aware of any special handling of security bug fixes) | 09:50 |
gibi | elodilles: the stable policy allows parallel merge of secu stable fixes | 09:50 |
bauzas | elodilles: the zed one got kicked out due to some gate failure | 09:50 |
gibi | elodilles: but the backport job does not | 09:50 |
elodilles | ok, then [stable-only] is right | 09:50 |
gibi | elodilles: do you have a magic want to turn off the job or merge the patches against that job? | 09:50 |
bauzas | gibi: 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] tag | 09:50 |
gibi | OK then we have a consensus | 09:50 |
elodilles | if you want them to merge not in the known order | 09:51 |
gibi | bauzas: will you add the [stable-only] tags? | 09:51 |
bauzas | ideally, I'd rather prefer to have a specific tag but we're a bit on a rush | 09:51 |
johnthetubaguy | bauzas: 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 |
gibi | bauzas: then elodilles and me can approve qucikly | 09:51 |
johnthetubaguy | what about [stable-only][actually-a-cve] ? | 09:51 |
gibi | johnthetubaguy: adding a sentence why we add stable-only make sense, yes | 09:52 |
elodilles | :] | 09:52 |
gibi | and as a follow up we should extend the job to accept [cve] as a tag to silence the landing order check | 09:52 |
johnthetubaguy | gibi: +1 | 09:52 |
bauzas | johnthetubaguy: as a reminder, train requires some oslo.utils bump due to https://review.opendev.org/c/openstack/oslo.utils/+/706880 missing | 09:53 |
johnthetubaguy | bauzas: appreciated, thanks, I saw zigo is working through that. | 09:53 |
zigo | Either oslo bump, or backport of https://review.opendev.org/c/openstack/oslo.utils/+/706880 | 09:56 |
zigo | I did the later ... | 09:56 |
zigo | YEAH !!! | 09:56 |
zigo | Got something that worked ! \o/ | 09:56 |
bauzas | gibi: if we start parallelizing the backports, then we'll break the sha1 information on the commit msgs | 09:56 |
bauzas | since there is no guarantee that the commit in the gerrit branch will have the same sha1 once merged | 09:57 |
gibi | bauzas: ignore the sha (it does not have it now either) as we disable the check anyhow | 09:57 |
bauzas | gibi: I'll then mention the gerrit links | 09:57 |
gibi | bauzas: the bug ref is there to make a connection, but yes if you want you can add gerrit change id | 09:57 |
gibi | or link | 09:57 |
bauzas | unfortunately, we can't also track all commits easily as they're on different branches | 09:58 |
zigo | So, I did: | 09:58 |
zigo | https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/cve-2022-47951-nova-stable-train.patch | 09:58 |
zigo | https://salsa.debian.org/openstack-team/services/nova/-/blob/debian/train/debian/patches/images_Move_qemu-img_info_calls_into_privsep.patch | 09:58 |
zigo | 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 | 09:58 |
zigo | With these 3 patches, all unit tests are passing. | 09:58 |
zigo | Is this acceptable (from your Nova upstream point of view) to backport these 3 patches on the train branch? | 09:59 |
bauzas | zigo: propose the backports | 10:01 |
bauzas | Train is EM | 10:01 |
zigo | bauzas: The only issue is that I apply them in the wrong order, so patch 1 and 2 may have failures ... | 10:02 |
zigo | Can I just merge the 3 patches then? | 10:02 |
johnthetubaguy | zigo: +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 |
bauzas | then I need to review those patches | 10:02 |
bauzas | and yeah, this would have to be a gerrit series | 10:02 |
zigo | Ok, doing this. | 10:02 |
gibi | zigo: 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 |
zigo | gibi: That's what is in there: https://review.opendev.org/c/openstack/nova/+/711679/6/nova/virt/images.py#57 | 10:04 |
opendevreview | Sylvain Bauza proposed openstack/nova stable/yoga: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871624 | 10:04 |
zigo | Ask Lee Yarwood I guess? :) | 10:04 |
gibi | zigo: then that is a latent bug, so keep it as is | 10:04 |
opendevreview | Sylvain Bauza proposed openstack/nova stable/xena: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871622 | 10:04 |
opendevreview | Sylvain Bauza proposed openstack/nova stable/wallaby: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871557 | 10:05 |
gibi | zigo: lyarwood is gone from openstack unfortunately | 10:05 |
bauzas | I'd be honest, I spotted a few bugs on this | 10:06 |
bauzas | (and ChatGPT as well) | 10:06 |
bauzas | but nothing prevents the cve to be fixed | 10:06 |
bauzas | gibi: did the [stable-only] hack | 10:07 |
bauzas | enjoy | 10:07 |
bauzas | johnthetubaguy: ditto | 10:07 |
gibi | bauzas: I agree, lets fix the cve, the latent bugs can be fixed later / separately | 10:07 |
gibi | bauzas: on it | 10:07 |
zigo | bauzas: 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 |
gibi | bauzas: you missed https://review.opendev.org/c/openstack/nova/+/871616 | 10:07 |
gibi | zigo: you live dangerously :) | 10:07 |
zigo | gibi: I didn't use what ChatGPT produced ! :) | 10:08 |
gibi | ahh :) | 10:08 |
bauzas | gibi: no, this was on purpose, zed is on the check pipeline | 10:08 |
bauzas | and was on the gate before | 10:08 |
gibi | bauzas: zed will be kicked out of gate by the backport job as it has no commit hash | 10:08 |
bauzas | gibi: oh shit, you're right | 10:08 |
bauzas | doing then the hack | 10:08 |
opendevreview | Sylvain Bauza proposed openstack/nova stable/zed: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871616 | 10:09 |
bauzas | there ^ | 10:09 |
zigo | I 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 |
zigo | Can someone else pick them up ? | 10:09 |
johnthetubaguy | gibi: does that job not run in the check queue? | 10:10 |
gibi | johnthetubaguy: it is non-voting in check | 10:10 |
gibi | afaik | 10:10 |
johnthetubaguy | oh, I see it now: nova-tox-validate-backport https://zuul.opendev.org/t/openstack/build/016594434b19461781e2dbb3688a61cd : FAILURE in 4m 51s (non-voting) | 10:10 |
gibi | yepp it failed ther nova-tox-validate-backport https://zuul.opendev.org/t/openstack/build/04bf544d0dc44ca48f45ba4d71bb8892 : FAILURE in 5m 45s (non-voting) | 10:11 |
bauzas | yup, missed that when I rechecked | 10:12 |
bauzas | I focused on the other failures :D | 10:12 |
gibi | the important ones :D | 10:13 |
bauzas | that's two weeks I devoted to upstream | 10:13 |
bauzas | and despite this, I nearly progressed on my feature by 5%. | 10:14 |
bauzas | lovely. | 10:14 |
bauzas | Zuul, I look at you | 10:14 |
gibi | bauzas: keeping Zuul happy is part of the deal unfortunately :) | 10:14 |
gibi | maybe you should ask ChatGPT to maintain it for us :D | 10:15 |
bauzas | that's... dangerous | 10:15 |
gibi | OK all the patches I know of has +2 +A https://review.opendev.org/q/topic:bug%252F1996188 | 10:16 |
kashyap | gibi: Even ChatGPT will throw up in the hands and say "it's a Sisphean task" | 10:16 |
gibi | kashyap: we need better aligned AIs then | 10:16 |
kashyap | Heh | 10:16 |
kashyap | And this time-out seems unfortunate :-( https://review.opendev.org/c/openstack/nova/+/870794 | 10:17 |
zigo | Looks like doing train -> stein is just a refresh of patches... | 10:18 |
bauzas | zigo: I've quickly read your patches on your server | 10:22 |
bauzas | why aren't you proposing them upstream ? | 10:22 |
bauzas | the problem is that I don't know which objects or methods you've touched | 10:23 |
bauzas | I mean, if you need help on how to propose a series on a stable branch, I can surely offer my help | 10:24 |
opendevreview | John Garbutt proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871699 | 10:30 |
opendevreview | John Garbutt proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871702 | 10:31 |
johnthetubaguy | ah, thanks gibi, your cut and paste of the topic was faster | 10:33 |
gibi | johnthetubaguy: I think there is a merge conflict resolution issue in https://review.opendev.org/c/openstack/nova/+/871699 | 10:35 |
johnthetubaguy | oops, yes | 10:35 |
johnthetubaguy | I only got as far as pep8 locally | 10:35 |
johnthetubaguy | fixing that now | 10:36 |
gibi | affects the stable/ussuri one too | 10:36 |
johnthetubaguy | yeah, I cherry-picked that down | 10:36 |
sean-k-mooney | i have not read back but why do we have stable only variants | 10:39 |
sean-k-mooney | dan had proposed cherry picks already | 10:39 |
gibi | sean-k-mooney: we want to land them in parallel | 10:40 |
gibi | sean-k-mooney: and the only way to allow it is to add [stable-only] | 10:40 |
sean-k-mooney | hum ok | 10:40 |
gibi | otherwise the backport job will prevent the merge | 10:40 |
gibi | elodilles: was OK with this from stable :D | 10:40 |
sean-k-mooney | i was going to try and land them in sequence | 10:40 |
sean-k-mooney | and just hold other patches until they were merged | 10:41 |
gibi | sean-k-mooney: johnthetubaguy expressed time sensitiveness and the stable/policy actually allows it for secu patches | 10:41 |
sean-k-mooney | it does but i didnt tink it warrented it | 10:41 |
sean-k-mooney | but if other are happy then ok | 10:41 |
sean-k-mooney | i just wont try and babysit the patches today | 10:42 |
gibi | at least bauzas, johnthetubaguy, elodilles and myself was OK with it | 10:42 |
johnthetubaguy | sean-k-mooney: I am curious, why do you think its not urgent enough? | 10:43 |
sean-k-mooney | form 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 version | 10:44 |
opendevreview | John Garbutt proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871699 | 10:44 |
sean-k-mooney | i was still expecting them to all merge today or tommorrow without needing to do it in parallel | 10:44 |
gibi | sean-k-mooney: I assume we will propose a stable release as soon as a stable patch lands | 10:44 |
sean-k-mooney | i 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 |
gibi | sean-k-mooney: given the gate speed parallel is lot faster than sequential when we need to do 7 backports | 10:45 |
sean-k-mooney | the gate is fast if this is the only thing we are merging | 10:46 |
gibi | gate 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-mooney | if 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 ay | 10:46 |
sean-k-mooney | gibi: i didnt mean other project i ment lets not merge anything that would put these in merge conflict | 10:47 |
sean-k-mooney | anyway ill leave this to ye since ye have a plan | 10:47 |
bauzas | sean-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 helpful | 10:48 |
gibi | I 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 down | 10:48 |
opendevreview | John Garbutt proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871702 | 10:48 |
bauzas | but it occurred from johnthetubaguy that we could rush the things a little bit up | 10:48 |
bauzas | and eventually I joined the 'rush' team when I saw our stable policy was accepting it | 10:48 |
sean-k-mooney | we have a second CVE fix with backprots that need review upstream | 10:49 |
bauzas | sean-k-mooney: just explaining my thoughts journey | 10:49 |
sean-k-mooney | if we are going to do a release we proably shoudl also merge that with the same speed | 10:49 |
bauzas | sean-k-mooney: that's understandable, despite that one was pretty nasty | 10:49 |
bauzas | sean-k-mooney: patches ? | 10:49 |
sean-k-mooney | looking for them now | 10:50 |
bauzas | I only remember the former-embargoed one which is now public | 10:50 |
gibi | https://review.opendev.org/q/topic:bug%252F1981813 | 10:50 |
gibi | I think sean-k-mooney refers to ^^ | 10:50 |
gibi | it is a lot less sever though | 10:50 |
sean-k-mooney | yes | 10:50 |
sean-k-mooney | we have a downstream deadlien ot have that merged internally by next week | 10:50 |
gibi | and we have downstream proactive backport too ;) | 10:51 |
sean-k-mooney | but i would prefer to also have it merged upstream | 10:51 |
johnthetubaguy | I didn't see that one, yes, that should get some love too. | 10:51 |
sean-k-mooney | if we are doing a release we should ideally include both or do a second release once both are there | 10:51 |
bauzas | mmm, OK, I only tho see 'in progress' on ossa | 10:51 |
bauzas | so I guess that one isn't on mitre | 10:52 |
bauzas | nevermind me, it is https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-37394 | 10:52 |
bauzas | but yeah, it's a dos | 10:52 |
bauzas | not a compute exposure | 10:53 |
sean-k-mooney | preventign the compute agent form stating | 10:53 |
bauzas | yup, hence the security level | 10:53 |
sean-k-mooney | from a public cloud perspecivive both are costly in different ways | 10:53 |
bauzas | we 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 them | 10:53 |
bauzas | sean-k-mooney: I tend to set a different priority on it | 10:54 |
gibi | I agree to not wait more than a day with a release if the latest cve fix lands | 10:54 |
bauzas | we're not talking of direct access to the host which could compromize all the dataplane | 10:54 |
sean-k-mooney | well that not quite what the other cve does | 10:55 |
sean-k-mooney | it can give you direct access to the file system but not the network | 10:55 |
sean-k-mooney | anyway 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 yesterday | 10:56 |
bauzas | if you have access to the file system, you can access the network later | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (db) https://review.opendev.org/c/openstack/nova/+/831193 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (objects) https://review.opendev.org/c/openstack/nova/+/839401 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (manila abstraction) https://review.opendev.org/c/openstack/nova/+/831194 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (drivers and compute manager part) https://review.opendev.org/c/openstack/nova/+/833090 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (api) https://review.opendev.org/c/openstack/nova/+/836830 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Check shares support https://review.opendev.org/c/openstack/nova/+/850499 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Add metadata for shares https://review.opendev.org/c/openstack/nova/+/850500 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Add instance.share_attach notification https://review.opendev.org/c/openstack/nova/+/850501 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Add instance.share_detach notification https://review.opendev.org/c/openstack/nova/+/851028 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Add shares to InstancePayload https://review.opendev.org/c/openstack/nova/+/851029 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Add helper methods to attach/detach shares https://review.opendev.org/c/openstack/nova/+/852085 | 10:56 |
opendevreview | ribaudr proposed openstack/nova master: Add libvirt test to ensure metadata are working. https://review.opendev.org/c/openstack/nova/+/852086 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Add virt/libvirt error test cases https://review.opendev.org/c/openstack/nova/+/852087 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Add share_info parameter to reboot method for each driver (driver part) https://review.opendev.org/c/openstack/nova/+/854823 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Support rebooting an instance with shares (compute and API part) https://review.opendev.org/c/openstack/nova/+/854824 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Add instance.share_attach_error notification https://review.opendev.org/c/openstack/nova/+/860282 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Add instance.share_detach_error notification https://review.opendev.org/c/openstack/nova/+/860283 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Add share_info parameter to resume method for each driver (driver part) https://review.opendev.org/c/openstack/nova/+/860284 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Support resuming an instance with shares (compute and API part) https://review.opendev.org/c/openstack/nova/+/860285 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Add helper methods to rescue/unrescue shares https://review.opendev.org/c/openstack/nova/+/860286 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Support rescuing an instance with shares (driver part) https://review.opendev.org/c/openstack/nova/+/860287 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Support rescuing an instance with shares (compute and API part) https://review.opendev.org/c/openstack/nova/+/860288 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Change microversion to 2.XX https://review.opendev.org/c/openstack/nova/+/852088 | 10:57 |
opendevreview | ribaudr proposed openstack/nova master: Documentation https://review.opendev.org/c/openstack/nova/+/871642 | 10:57 |
* gibi needs to drop for an hour... | 10:57 | |
sean-k-mooney | bauzas: have they confirmed that raw devices special files are actully usable im not sure file access imples network access | 10:57 |
sean-k-mooney | it would come down to the speicics of the vmdk impl | 10:58 |
sean-k-mooney | anyway we dont need to speculate | 10:58 |
bauzas | sean-k-mooney: I tested it by myself | 10:58 |
bauzas | and when I say it's nasty, trust me it is | 10:58 |
sean-k-mooney | ok well since this has been discusled upstream we now also need ot have it downstream by next week at the latest | 10:58 |
johnthetubaguy | bauzas: escation to something nasty should be assume with these things, for sure. Even if we can't see it yet. | 11:01 |
sean-k-mooney | johnthetubaguy: 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 nasty | 11:02 |
bauzas | johnthetubaguy: sean-k-mooney: read the bug report | 11:03 |
bauzas | and the first comments, there is a reproducer | 11:03 |
sean-k-mooney | i have not reviewed the bug/cve in any detail rather just enduing the patches were moving last night | 11:03 |
sean-k-mooney | ok so on that reading it does not allow direct network access form the main descrption anyway lets not look for other ways to abuse this | 11:06 |
sean-k-mooney | if 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 channel | 11:06 |
bauzas | +1 | 11:09 |
* bauzas needs to feed himself | 11:10 | |
zigo | bauzas: I'll push my patches in a bit... | 11:18 |
zigo | FYI, I got Stein fixed too... :) | 11:18 |
zigo | Stein -> Rocky backport seems another tricky one... | 11:18 |
priteau | Anyone know why grenade keeps failing on stable/wallaby with the vmdk patch? | 12:13 |
opendevreview | Merged openstack/nova stable/yoga: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859312 | 12:38 |
zigo | I also got Rocky in shape! :P | 12:53 |
opendevreview | Merged openstack/nova stable/yoga: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859313 | 13:15 |
bauzas | sent the xena patches for ^ to the gate | 13:22 |
* bauzas needs to go afk for getting his children's passeports | 13:22 | |
bauzas | passports* even | 13:22 |
* bauzas back in less than 20 mins hopefully | 13:22 | |
gibi | bauzas: thanks | 13:26 |
sahid | 0/ 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 thinking | 13:40 |
opendevreview | Jorge San Emeterio proposed openstack/nova master: WIP: Dividing global privsep profile https://review.opendev.org/c/openstack/nova/+/871729 | 13:41 |
sean-k-mooney | sahid: sure im on a call but ill check when it wraps | 13:41 |
sahid | sean-k-mooney: no worries, thanks a lot for your time :-) | 13:42 |
opendevreview | Jorge San Emeterio proposed openstack/nova master: WIP: Dividing global privsep profile https://review.opendev.org/c/openstack/nova/+/871729 | 13:44 |
opendevreview | Jorge San Emeterio proposed openstack/nova master: WIP: Dividing global privsep profile https://review.opendev.org/c/openstack/nova/+/871729 | 13:47 |
*** dasm|off is now known as dasm | 13:59 | |
gibi | I did a round of rechecks on the vmdk cve | 14:04 |
gibi | bauzas: 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 train | 14:16 |
gibi | fun | 14:16 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/xena: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859314 | 14:43 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/xena: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859315 | 14:43 |
bauzas | gibi: I guess I can review it again ? ^ | 14:52 |
* bauzas feels today as a fireman | 14:52 | |
gibi | bauzas: yeah I hope I did not screw up copying hashes | 14:52 |
bauzas | gibi: you used the merge patch one ? | 14:53 |
gibi | yes | 14:53 |
gibi | now it points to the merged one | 14:53 |
bauzas | should work so | 14:53 |
gibi | in both xena patch | 14:53 |
gibi | es | 14:53 |
gibi | and I have to do it for the rest of the stable branches too | 14:53 |
opendevreview | Dan Smith proposed openstack/nova master: WIP: Detect host renames and abort startup https://review.opendev.org/c/openstack/nova/+/863920 | 14:56 |
gibi | a recheckd https://review.opendev.org/c/openstack/nova/+/871622 back to the gate it was kicked due to a slow CI node | 14:58 |
bauzas | gibi: yup, thanks for having rechecked the changes | 15:03 |
gibi | bauzas: even with trying to land them in parallel I doubt we will land all of them until friday | 15:06 |
gibi | maybe if the gate is better during the night... | 15:06 |
* bauzas won't send a crossfingers emoji but you get it | 15:06 | |
gibi | :D | 15:06 |
artom | 🤞 | 15:23 |
artom | Hey, it's unicode! | 15:23 |
bauzas | artom: tss, wanted to avoid it :p | 15:38 |
* artom trolls | 15:39 | |
bauzas | 🐈⬛ | 15:39 |
artom | That's basically the extent of my involvement here now | 15:39 |
bauzas | artom: I'm glad that the litterally first message you write here today is an emoji | 15:39 |
bauzas | artom: and I'm happy this isn't a poop one | 15:40 |
artom | I'm like emoji batman | 15:40 |
bauzas | a wealthy man that hides his emitions behind a mask and wears underwear on top of his pants ? | 15:43 |
artom | Well, one of those is true | 15:44 |
bauzas | I hope it's the former, I'm afraid it could be the latter | 15:45 |
gibi | too much details :D | 15:46 |
bauzas | yay | 15:47 |
bauzas | gibi: you were right, it sounds we have a problem with grenade on xena | 15:47 |
gibi | bauzas: did it fail again? | 15:47 |
bauzas | yup | 15:47 |
gibi | with the same fastener dep issue? | 15:47 |
bauzas | https://zuul.opendev.org/t/openstack/build/d8e11efbdc6d43e7b9e27273cf038786 | 15:47 |
gibi | /o\ I have a hunch | 15:48 |
gibi | https://review.opendev.org/c/openstack/tempest/+/821732/21/requirements.txt yeah we landed this | 15:49 |
bauzas | 2023-01-25 14:45:10.253 | ERROR: Could not find a version that satisfies the requirement fasteners>=0.16.0 | 15:49 |
bauzas | https://00f8d73ac2d869c11924-90ff9157beec64657d8a46242c5af814.ssl.cf1.rackcdn.com/871557/3/check/nova-grenade-multinode/d8e11ef/controller/logs/grenade.sh_log.txt | 15:49 |
bauzas | and I was wrong, this is on wallaby, not xena | 15:50 |
bauzas | -ETOOMANYBRANCHESANDPATCHESTOTRACK | 15:50 |
gibi | what is the last stable branch tempest master supports? | 15:51 |
gibi | I feel like wallaby is at the boundary | 15:51 |
bauzas | possibly | 15:51 |
gibi | gmann: ^^ | 15:51 |
bauzas | but we also have problem with ceph-multistore on yoga | 15:51 |
bauzas | changing my focus | 15:51 |
bauzas | https://review.opendev.org/c/openstack/nova/+/871624 failed again | 15:51 |
gibi | gmann: it seems https://review.opendev.org/c/openstack/tempest/+/821732 affects stable/wallaby grenade runs | 15:52 |
gmann | gibi tempest master stopped wallaby support recently. it is stable/xena the last stable it support | 15:52 |
gmann | gibi: but I have not pinned stable/wallaby with old compatible tempest which I should do | 15:53 |
gibi | gmann: 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 more | 15:53 |
gmann | till now it was running fine but if it is breaking its time to pin tempest there | 15:53 |
gibi | gmann: yes, it breaks now on the fasteners >= 0.16 dependency | 15:53 |
gibi | that https://review.opendev.org/c/openstack/tempest/+/821732 introduced | 15:54 |
gmann | gibi: yeah. I will pin it today | 15:54 |
gibi | gmann: thank you! | 15:54 |
opendevreview | Merged openstack/nova stable/zed: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871616 | 16:07 |
bauzas | sean-k-mooney: +2d sahid's implementation of stopping evacuated instances | 16:25 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/wallaby: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859320 | 16:35 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/wallaby: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859321 | 16:35 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/victoria: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/869583 | 16:38 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/victoria: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/869584 | 16:38 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/ussuri: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/869585 | 16:42 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/ussuri: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/869586 | 16:42 |
sahid | thank you bauzas ++ | 16:52 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/train: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/869673 | 16:59 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/train: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/869674 | 16:59 |
bauzas | sahid: I'm really sorry, but I forgot to look at your dependent patch and I found something :( | 17:00 |
bauzas | sahid: https://review.opendev.org/c/openstack/nova/+/858383/25 | 17:00 |
bauzas | sahid: tl,dr: you return an exception if a caller asks for a target_state parameter that the compute doesn't know | 17:01 |
bauzas | sahid: thinking out loud, I think this would be better to just *not* provide the target_state parameter if the compute is old | 17:02 |
gibi | the 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 message | 17:02 |
bauzas | this shouldn't be arriving, since you verify that all computes are upgraded, but I'd prefer us to make it clear | 17:02 |
bauzas | gibi: ack | 17:02 |
gibi | I will go and remove the hash from the commit message to keep landing the fixes in parallel | 17:04 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/wallaby: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871557 | 17:06 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871699 | 17:07 |
opendevreview | Balazs Gibizer proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871702 | 17:07 |
bauzas | gibi: 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 do | 17:08 |
bauzas | if we really want to have RPC backwards compat, the RPC client needs to adapt to what the manager supports | 17:09 |
sean-k-mooney | if you request something at the api that requries a new rpc version we shoudl not back levle | 17:12 |
sean-k-mooney | that should be an api error | 17:12 |
sean-k-mooney | we normally use a compute service bump to allow use to detect this in the api | 17:13 |
sean-k-mooney | before getting to the rpc code | 17:13 |
bauzas | and that's what sahid does | 17:13 |
bauzas | but I don't really like us returning exceptions we don't really manage upside | 17:13 |
dansmith | making a call, getting an exception and making it again with different stuff is wasteful *and* wrong, | 17:14 |
sean-k-mooney | right and we are not doing that | 17:14 |
dansmith | okay | 17:14 |
sean-k-mooney | where it can be backleveled we do prepare/version check | 17:14 |
bauzas | dansmith: tl;dr sahid is adding a service check that verifies all computes are upgraded before adding a parameter | 17:14 |
sean-k-mooney | but if you use the new parmater then its not valide to remove it | 17:14 |
dansmith | ack | 17:15 |
bauzas | by default the param is set to None on the API method | 17:15 |
sean-k-mooney | bauzas: yep i would break our api microversioning to backlevel in this case | 17:15 |
bauzas | then it calls the conductor and then the compute | 17:15 |
sean-k-mooney | bauzas: only for the new microversion no? | 17:16 |
sean-k-mooney | we backlevel if its actully None | 17:16 |
sean-k-mooney | i need to pull up the patch again | 17:16 |
tobias-urdin | is the uuid of a mdev just a random uuidutils.uuid() or is it tracked somewhere in placement? | 17:16 |
sean-k-mooney | tobias-urdin: totally random | 17:16 |
tobias-urdin | sean-k-mooney: ack ty | 17:17 |
bauzas | sean-k-mooney: technically with sahid's proposal, we backlevel by the if condition | 17:17 |
bauzas | but ok, I see my mistake, I'll clarify my second comment | 17:18 |
bauzas | either way, my first comment remains valid, we lack a negative test | 17:18 |
sean-k-mooney | we do it by if not client.can_send_version(version): | 17:18 |
sean-k-mooney | so if we cant send the version and target_state is not None we raise | 17:19 |
sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/858383/25/nova/compute/rpcapi.py#1106 | 17:19 |
sean-k-mooney | this is what you are askign about yes | 17:20 |
bauzas | yep, you convinced me on the RPC contracty | 17:20 |
bauzas | my second comment is not valid | 17:21 |
bauzas | that being said, I'm not super happy with those generic exceptions being raised without being properly captured at the API side | 17:21 |
bauzas | but that's not worth a -1 | 17:21 |
bauzas | my -1 is just about missing unittests on the RPC checks | 17:21 |
sean-k-mooney | im ok with useing pop to do the removal but we woudl need to check the result to determin if we raise or reducet the verison | 17:21 |
bauzas | sean-k-mooney: we're on agreement | 17:22 |
bauzas | that's what I mean by the RPC contract | 17:22 |
bauzas | if this parameter is set to something, we can't backlevel | 17:22 |
sean-k-mooney | oh its missing a negitive test | 17:22 |
bauzas | you convinced me | 17:22 |
bauzas | sean-k-mooney: yup, that's why I -1 | 17:23 |
sean-k-mooney | ya you are right i tough this was unit tested but only the happy path | 17:23 |
bauzas | and I was +0 on the fact we raise a NovaException without properly capturing it on the API level, but that wasn't a patch blocker | 17:23 |
sean-k-mooney | i 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#1116 | 17:25 |
sean-k-mooney | *converted | 17:26 |
sean-k-mooney | with that said we want this to be a 400 not a 500 | 17:26 |
sean-k-mooney | oh but this is not hooked up to the api yet | 17:26 |
sean-k-mooney | so we have to check the second patch | 17:26 |
bauzas | sean-k-mooney: I checked and this isn't the case | 17:27 |
bauzas | that being said, again, not -1ing on this, rather +0ing, because this situation shouldn't arrive thanks to the version check | 17:28 |
bauzas | this is just us not being enough prescriptive on the exception handling | 17:29 |
sean-k-mooney | right but it would be good to have negitive unit tests in the first patch and a negitive funcitonl test in the secod | 17:29 |
bauzas | (usually I hate standard exceptions that are meaningless and hard to identify) | 17:29 |
sean-k-mooney | bauzas: because even if fully upgraded | 17:29 |
sean-k-mooney | you could have an rpc pin set in the config right | 17:29 |
bauzas | true | 17:29 |
sean-k-mooney | that woudl pass the compute service check | 17:29 |
sean-k-mooney | so we dont want this to be a 500 | 17:29 |
sean-k-mooney | it should be a 400 or maybe 409 | 17:30 |
bauzas | then I turn my api patch review vote to -1 | 17:30 |
bauzas | because of the pinset | 17:30 |
bauzas | sean-k-mooney: are you sure that if we pin the rpc versions we hit this ? | 17:30 |
bauzas | I thought the service check would say "'meh nah"' | 17:31 |
bauzas | oh | 17:31 |
bauzas | no, you're right | 17:31 |
sean-k-mooney | if we pin to 6.0 the can send version check will fail for 6.2 | 17:31 |
bauzas | true | 17:31 |
bauzas | the RPC version check will fail to accept 6.2 | 17:31 |
bauzas | butn, | 17:31 |
sean-k-mooney | so we should assert that returns somethign other then a 500 at the api | 17:31 |
bauzas | the api version check on check_min_versions() will say 'surely, you can call' | 17:31 |
sean-k-mooney | yep | 17:32 |
sean-k-mooney | but i dont think this is a probelm in the code nessisarly | 17:32 |
sean-k-mooney | just in the test coverage and perhapse the excpetion raised | 17:32 |
sean-k-mooney | we use 409 to comunicate this in other places | 17:33 |
sean-k-mooney | bauzas: do you want to capture that in the review | 17:33 |
sean-k-mooney | or shall i an link to this irc conversation | 17:34 |
sean-k-mooney | i 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.py | 17:35 |
bauzas | sean-k-mooney: just left comments | 17:35 |
bauzas | but I can explain this to sahid later | 17:35 |
sean-k-mooney | ack | 17:35 |
sean-k-mooney | thanks for bringing this up | 17:36 |
opendevreview | Merged openstack/nova stable/xena: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859314 | 17:36 |
bauzas | and yeah HTTP409 Conflict makes perfect sense | 17:36 |
* bauzas ends his day on this | 17:37 | |
bauzas | I'll just double check the cve patches on fly | 17:37 |
gibi | Uggla: I left some comment in the API patch of the Manial series https://review.opendev.org/c/openstack/nova/+/836830 | 18:03 |
opendevreview | Merged openstack/nova stable/xena: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871622 | 18:04 |
opendevreview | Merged openstack/nova stable/xena: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859315 | 18:05 |
opendevreview | Merged openstack/nova stable/yoga: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871624 | 20:08 |
*** dasm is now known as dasm|off | 21:51 | |
opendevreview | Ghanshyam proposed openstack/nova stable/wallaby: DNM: testing tempest pin for stable/wallaby https://review.opendev.org/c/openstack/nova/+/871798 | 21:56 |
opendevreview | Ghanshyam proposed openstack/nova stable/xena: DNM: testing tempest pin for stable/wallaby https://review.opendev.org/c/openstack/nova/+/871800 | 22:00 |
tobias-urdin | proposed 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/+/871802 | 22:27 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!