search

containers / storage

Container Storage Library
Apache License 2.0
539 stars 234 forks source link

add patch for imagestore from 1.53 #1859

Closed kannon92 closed 3 months ago

kannon92 commented 3 months ago

Bring https://github.com/containers/storage/pull/1784 to 1.51.

This fixes the test failures in https://github.com/kubernetes/kubernetes/pull/123518 where we discovered some issues with imagestore.

Also fixes https://github.com/containers/storage/issues/1779

openshift-ci[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kannon92 Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/containers/storage/blob/release-1.51/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
giuseppe commented 3 months ago

the git commit message doesn't say anything about the patch.

The way to correctly cherry-pick it is to use the git cherry-pick command.

kannon92 commented 3 months ago

the git commit message doesn't say anything about the patch.

The way to correctly cherry-pick it is to use the git cherry-pick command.

I didn't use cherry pick in this case. I manually moved the changes to the previous branch as it seemed a bit difficult to cherry-pick that patch.

giuseppe commented 3 months ago

the git commit message doesn't say anything about the patch. The way to correctly cherry-pick it is to use the git cherry-pick command.

I didn't use cherry pick in this case. I manually moved the changes to the previous branch as it seemed a bit difficult to cherry-pick that patch.

This patch changes some critical parts in the storage and it is not trivial to backport.

that is why I suggested we either use a newer c/storage, or just postpone the feature to the next release.

giuseppe commented 3 months ago

in any case, if we really want to consider it for a backport, the backport must be done properly with git cherry-pick and maintain the original patch commit message

TomSweeneyRedHat commented 3 months ago

@cevich can you take a look at the lint error please? Do we need to add a "1.51" tag somewhere? The Makefile seems to be looking at the main branch rather than the "1.51" branch.

kannon92 commented 3 months ago

the git commit message doesn't say anything about the patch. The way to correctly cherry-pick it is to use the git cherry-pick command.

I didn't use cherry pick in this case. I manually moved the changes to the previous branch as it seemed a bit difficult to cherry-pick that patch.

This patch changes some critical parts in the storage and it is not trivial to backport.

that is why I suggested we either use a newer c/storage, or just postpone the feature to the next release.

Is is possible to have a simpler patch without the refactor? To be honest, it isn't clear to me why this PR fixes the problem that I was seeing.

kannon92 commented 3 months ago

So given @giuseppe concerns, @mrunalp, @haircommander and I talked about this. We were hoping to get this into 1.30 so that we can have working e2e tests for KEP-4191. But since the patch is a bit more involved, we are fine with fixing this in 1.53 and using the updated crio in 1.31 once podman v5 is released.

It doesn't impact our timeline too much as we were planning on promoting this feature to beta in 1.31 anyway. We will just make sure that the e2e tests are working before promotion to beta.

cevich commented 3 months ago

@cevich can you take a look at the lint error please? Do we need to add a "1.51" tag somewhere? The Makefile seems to be looking at the main branch rather than the "1.51" branch.

I'm assuming you're referring to the message:

fatal: Not a valid object name main

Looks like the flow goes through the test/tools Makefile, so I'm not quite sure where that message is coming from. Still looking...

cevich commented 3 months ago

...found it, it's in hack/git-validation.sh and DEST_BRANCH is missing from .cirrus.yml. Somebody must have thought that setting unimportant for some reason :disappointed: I'll open a PR to put it back...

cevich commented 3 months ago

...wait wait wait, this is containers/storage. I just double-checked my memory, there's no CI-longevity setup on this repo. Last commit was 4-months ago. Abandon all hope, CI will not work on this release branch, not without a TON of effort.

I've asked many times if "No releease-branch CI" is what we want, and the answer was always "not worth your time to setup" on this repo. But someone please tell me if that stance is changing, and we need CI to work on future release branches (current ones are all mostly doomed).

kannon92 commented 3 months ago

It sounds like we don't want to go this route. We will aim to update c/storage to 1.53 in CRI-O once 1.30 is released and podman 5.0 is released.