| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-24 | |||
| 17:33:17 | gibi | priteau: true. if you want to add it then you can by doing the cherry-pick with git and using -x | |
| 17:34:19 | opendevreview | Pierre Riteau proposed openstack/nova stable/wallaby: Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871557 | |
| 17:34:37 | priteau | Yes, I've done this many times. Here we go. | |
| 18:21:51 | gibi | Uggla: I re-reviewd the first half of the manial series mostly OK with it buth johnthetubaguy had some valid questions there so I left -1 to have the visibility | |
| 18:21:58 | gibi | I will continue tomorrow | |
| 18:22:09 | gibi | Uggla: do you have a set of functional tests added to the series? | |
| #openstack-nova - 2023-01-25 | |||
| 01:51:23 | opendevreview | Merged openstack/nova master: Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871612 | |
| 02:20:55 | gmann | gibi: sean-k-mooney: please review the placement RBAC change, https://review.opendev.org/c/openstack/placement/+/865618 | |
| 02:22:06 | 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:34:50 | sean-k-mooney[m] | gmann: i have one question in line regarding listing traits | |
| 02:35:54 | sean-k-mooney[m] | so im +1 but if we dont want to change listing traits im +2 on the change i think | |
| 02:36:00 | sean-k-mooney[m] | illl check back in my morning | |
| 02:36:46 | gmann | sean-k-mooney[m]: thanks. will check and reply | |
| 05:18:15 | gmann | sean-k-mooney[m]: replied https://review.opendev.org/c/openstack/placement/+/865618/2/placement/policies/trait.py#41 | |
| 08:27:15 | Uggla | gibi, hi I will have a look at the comments left by john. Strangely I did not noticed them before. (OO) | |
| 08:28:50 | Uggla | gibi, regarding functional test yes in the latest patches there are some of them. Or do you think about tempest tests ? | |
| 08:34:59 | gibi | Uggla: no, not tempest. But then I will see them as I progress with the review today... | |
| 08:48:34 | zigo | Hi there! | |
| 08:49:34 | 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:50:02 | 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:11 | zigo | gibi: Your opinion? | |
| 08:52:12 | zigo | Oh, I have my answer ... :) | |
| 08:52:16 | frickler | since the fix is for nova, I would keep it restricted to that. just my 0.03€ | |
| 08:52:16 | frickler | since the fix is for nova, I would keep it restricted to that. just my 0.03€ | |
| 08:52:28 | zigo | Latest version has: https://github.com/openstack/nova/blob/master/nova/virt/images.py#L48 (ie: format='json') | |
| 08:52:51 | zigo | So I'll do that ... | |
| 09:02:54 | 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:03:18 | zigo | Yeah ! | |
| 09:03:31 | zigo | Hopefully, I can just take these patches and do -> stein -> rocky, and then I'm good ! :) | |
| 09:05:31 | 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:15:28 | zigo | Shit, other failures ... :/ | |
| 09:18:38 | 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:59 | johnthetubaguy | (i.e. xena seems ready to go now) | |
| 09:20:31 | zigo | I may need all of https://review.opendev.org/c/openstack/nova/+/711679 after all ... | |
| 09:21:22 | gibi | johnthetubaguy: I don't know about any exception from the stable policy for sec patches but maybe elodilles knows | |
| 09:22:11 | 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:30:09 | bauzas | what's the problem with the proposed fix ? | |
| 09:30:51 | 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:31:25 | zigo | bauzas: Parts of nova changed between train and ussuri, but I think I can manage. | |
| 09:31:33 | zigo | Let me finish the backporting ... :) | |
| 09:31:51 | bauzas | johnthetubaguy: I'm rushing to review the stable branches | |
| 09:32:24 | 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:47 | johnthetubaguy | ah, I didn't know that. | |
| 09:33:03 | 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:09 | johnthetubaguy | I see arguments both ways for sure, but I wanted to check. | |
| 09:36:46 | bauzas | so, master is merged | |
| 09:36:59 | bauzas | zed was running on the gate but we got a failure | |
| 09:37:08 | johnthetubaguy | ack | |
| 09:37:09 | bauzas | and I just +2d yoga | |
| 09:37:29 | johnthetubaguy | OK, that was my question really, is that allowed? | |
| 09:37:51 | johnthetubaguy | I guess we just wait for the +W? | |
| 09:38:06 | bauzas | to merge some stable N-x patch before the parents ? | |
| 09:38:45 | bauzas | https://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes | |
| 09:38:53 | 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:39:03 | 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:04 | bauzas | but, there is an exception rule | |
| 09:39:13 | 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:25 | bauzas | gibi: that's what I told to johnthetubaguy | |
| 09:39:42 | 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:44 | gibi | bauzas: so even though the policy allow secu patches to land our tooling did not | |
| 09:39:55 | bauzas | gibi: I was about to write this | |
| 09:39:59 | johnthetubaguy | ah, got you | |
| 09:40:05 | bauzas | somehow we are strictier than the policy | |
| 09:40:45 | johnthetubaguy | so what is the quickest path to merge? in parallel drop that CI job? | |
| 09:40:49 | bauzas | johnthetubaguy: either way, you know we'll need to publish releases | |
| 09:41:07 | bauzas | upstream isn't generally a quick path for remediation | |
| 09:41:14 | bauzas | but we can try to rush | |
| 09:41:21 | johnthetubaguy | its meant to be though, in this particular case. | |
| 09:41:40 | bauzas | johnthetubaguy: that's why distros got notified one week before the disclosure | |
| 09:41:46 | johnthetubaguy | granted the patches are published, which is the important and crucial bit. | |
| 09:42:18 | bauzas | I don't disagree and the security hole is quite dirty | |
| 09:42:41 | bauzas | I would recommend our operators to exceptionnally patch directly | |
| 09:43:00 | bauzas | the patch itself is just about enforcing | |
| 09:46:23 | gibi | one way to rush this is to add [stable-only] to the commit message to silence the backport job | |
| 09:47:41 | bauzas | that's debatable | |
| 09:47:49 | johnthetubaguy | gibi: I like your thinking! | |
| 09:48:11 | bauzas | gibi: your help may be needed on https://review.opendev.org/c/openstack/nova/+/871624 | |
| 09:48:46 | bauzas | johnthetubaguy: which release do you target to ship the security fix ? | |
| 09:49:23 | elodilles | o/ | |
| 09:49:37 | gibi | bauzas: I can approve that but the job will kick it out from the gate | |
| 09:49:44 | elodilles | let me know if any patch needs review | |
| 09:49:50 | gibi | elodilles: it is more like a process thing | |
| 09:49:53 | bauzas | elodilles: we're on the CVE backports | |
| 09:50:03 | elodilles | (i'm not aware of any special handling of security bug fixes) | |
| 09:50:03 | gibi | elodilles: the stable policy allows parallel merge of secu stable fixes | |
| 09:50:06 | bauzas | elodilles: the zed one got kicked out due to some gate failure | |
| 09:50:09 | gibi | elodilles: but the backport job does not | |
| 09:50:43 | elodilles | ok, then [stable-only] is right | |
| 09:50:43 | gibi | elodilles: do you have a magic want to turn off the job or merge the patches against that job? | |
| 09:50:46 | 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:56 | gibi | OK then we have a consensus | |
| 09:51:04 | elodilles | if you want them to merge not in the known order | |
| 09:51:08 | gibi | bauzas: will you add the [stable-only] tags? | |
| 09:51:10 | bauzas | ideally, I'd rather prefer to have a specific tag but we're a bit on a rush | |
| 09:51:12 | 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:32 | gibi | bauzas: then elodilles and me can approve qucikly | |
| 09:51:42 | johnthetubaguy | what about [stable-only][actually-a-cve] ? | |
| 09:52:05 | gibi | johnthetubaguy: adding a sentence why we add stable-only make sense, yes | |
| 09:52:11 | elodilles | :] | |
| 09:52:35 | gibi | and as a follow up we should extend the job to accept [cve] as a tag to silence the landing order check | |