Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v5.16, backport] storage: use race-free AddNames instead of SetNames #1512

Closed

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Apr 1, 2022

Backport of: #1480
Similar to: #1503

Commits from parallel builds using SetNames removes names from
storage for other builds.

Use race-free atomic AddNames to prevent breaking of parallel builds.

Podman `v3.4.2-rhel` and buildah release `1.23` branch uses `c/image
5.16.0` which needs `v1.36.3` of `c/storage`

Signed-off-by: Aditya R <arajan@redhat.com>
Commits from parallel builds using `SetNames` removes `names` from
storage for other builds.

Use race-free atomic `AddNames` to prevent breaking of parallel builds.

Signed-off-by: Aditya R <arajan@redhat.com>
Signed-off-by: Aditya R <arajan@redhat.com>
Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc
Copy link
Contributor Author

flouthoc commented Apr 1, 2022

@mtrmac PTAL. I am just concerned about why c/storage was pointing to v1.37.0 on previous branch when both podman and buildah using this release are using c/storage v1.36.1 https://github.com/containers/podman/blob/v3.4.2-rhel/go.mod

@@ -8,7 +8,7 @@ require (
github.com/containerd/containerd v1.5.4 // indirect
github.com/containers/libtrust v0.0.0-20190913040956-14b96171aa3b
github.com/containers/ocicrypt v1.1.2
github.com/containers/storage v1.37.0
github.com/containers/storage v1.36.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not acceptable.

(Alternative approaches are being discussed, just noting this here so that it doesn’t get merged as is.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomSweeneyRedHat @mheon FYI this is sort of a trouble where we are stuck and discussing an alternative approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flouthoc looks like a good Cabal topic. Could you add it to the list please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomSweeneyRedHat I created this PR for v3.4.2-rhel but afaik the backport for v3.4.2-rhel is not decided so we might not need this PR anymore.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 6, 2022

@flouthoc Could you, please, as a sanity check, try just building/vendoring Podman with a replace directive pointing at your exact commit, to make sure that Go doesn’t somehow fall over because of the out-of-branch commit reference, before we commit to that approach?

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 6, 2022

Closing in favor of #1519 ; please speak up if that’s incorrect.

@mtrmac mtrmac closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants