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

[flouthoc]

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.

[flouthoc]

@mtrmac @vrothberg PTAL

[flouthoc]

@mtrmac Dropped version commits

[mtrmac]

The keyring tests are 576c8db , that can be back ported.

The missing VMs I don’t know. On the main branch they were replaced in f920b5c , but I’m not at all sure that’s right for this branch. @cevich can you help, please? Note that this is for a 5.16 branch, corresponding to Podman‘s v3.4.2-rhel.

(If we ended up needing to just disable/ignore those tests, and/or run them locally, that’s fine, but let’s make an explicit record of why and what.)

[mtrmac]

@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?

[flouthoc]

@mtrmac Sure.

[mtrmac]

Alternatively, if Podman has a working CI on that branch (and there are e2e tests for the race that this is trying to fix, and those are intended to also be backported), it might be ideal to prepare a Podman PR with replace directives in go.mod demonstrating 1) that the relevant libraries can be integrated as expected, and 2) that the resulting Podman binary fixes the problem.

Given that Podman exercises c/storage much more than Skopeo, if that worked, I would feel very comfortable with skipping the Skopeo parts of c/image CI.

[mtrmac]

Code LGTM.

Now, the testing…

[mtrmac]

@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?

go mod edit -replace github.com/containers/image/v5=github.com/flouthoc/image/v5@bbe074bff0e81fbf61f317c0319d24aae8d6efd5

works as expected. (References to the v5.16.0-rhel name don’t work, but after using a commit reference, make vendor creates the expected 5.16.0-… pseudo-version.)

[mtrmac]

LGTM if either the Skopeo tests pass, or a Podman PR with replace directives passes.

[cevich]

On the main branch they were replaced...but I’m not at all sure that’s right for this branch.

Correct, release branches shouldn't have their images updated except in some very limited extreme situations.

If we ended up needing to just disable/ignore those tests, and/or run them locally, that’s fine, but let’s make an explicit record of why and what.

This is likely the direction you'll need to go. We don't have Cirrus-CI cron-jobs set up for any branches on this repo. to keep the images alive (via the meta task). That means after 60 days of inactivity, the VM images will be deleted (which is what happened here).

Are there other branches I should add keep-alive cron-jobs on (assuming they're also not already FUBAR)? It's a bit of a pain for me to do (the first time), but if functional CI is beneficial for backports, I'll set it up.

[mtrmac]

@cevich Thanks for looking into this. For the most part I think this is a one-off that doesn’t indicate we need to systemically allocate resources for this specific branch (if we haven’t identified it at any prior time as relevant).

@flouthoc is already working on testing the final outcome in Podman (including testing that the race is actually fixed), that’s going to be far more representative than Skopeo tests.

[flouthoc]

I think we can merge this once this is green: containers/podman#13793 , although i have tested this locally as well.

[mtrmac]

I think we can merge this once this is green: containers/podman#13793 , although i have tested this locally as well.

Yup, please do.

[flouthoc]

I think we can merge this once this is green: containers/podman#13793 , although i have tested this locally as well.

Yup, please do.

This PR is good to merge. If anybody spots this PR before @mtrmac please merge me.

[rhatdan]

LGTM