[WIP] imagebuildah: prevent race cause c/storage does not persist names with layers.
#3794
Conversation
When running concurrent builds references to layers are changed and c/storage does not accounts for names so use ID where possible. [NO NEW TESTS NEEDED] [NO TESTS NEEDED] Signed-off-by: Aditya R <[email protected]>
flouthoc
force-pushed
the
tag-race-storage
branch
from
5e75fdd to
544e75e
Compare
|
@giuseppe @nalind PTAL. I am sure something could be done at Issue is easy to reporduce with |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| img, err = is.Transport.GetStoreImage(b.store, dest) | ||
| logrus.Debugf("assigned names %v to image %q", img.Names, img.ID) | ||
|
|
||
| _, img, err = util.FindImage(b.store, "", b.systemContext, imageID) |
There was a problem hiding this comment.
Isn't this the same line that was called in 747?
There was a problem hiding this comment.
@rhatdan It is called again after we perform tagging of current name and any additionalTags if they were present. So we can return a success message for each one of them.
There was a problem hiding this comment.
Is this still a draft or ready to merge?
There was a problem hiding this comment.
I'll just test it with few more edge cases just to be sure and waiting for @nalind to also take a quick look.
| } | ||
| logrus.Debugf("assigned names %v to image %q", img.Names, img.ID) | ||
|
|
||
| b.additionalTags = append(b.additionalTags, dest.DockerReference().String()) |
There was a problem hiding this comment.
That name was assigned to the image when it was committed, wasn't it?
There was a problem hiding this comment.
@nalind There seems be a race in c/storage and layers don't have name as the property while they get stored, so in concurrent build somehow the reference for name gets altered by other builds using the same blob.
I can drop this particular line if its bothering and add a tight locking while tagging as well. But motivation behind this was just a additional safety check, this might not be needed. I'll verify and remove this if its not needed.
|
We might not need this PR anymore, |
|
This PR is not needed after containers/image#1480 and containers/storage#1153 |
Invoking parallel/concurrent builds from podman race against each other following behviour was fixed in containers/storage#1153 and containers/image#1480 Test verifies if following bug is fixed in new race-free API or not. Read more about this issue, see bz 2055487 for more details. More details here: containers/buildah#3794 and containers#13339 Co-authored-by: Ed Santiago <[email protected]> Signed-off-by: Aditya R <[email protected]>
Invoking parallel/concurrent builds from podman race against each other following behviour was fixed in containers/storage#1153 and containers/image#1480 Test verifies if following bug is fixed in new race-free API or not. Read more about this issue, see bz 2055487 for more details. More details here: containers/buildah#3794 and containers#13339 Co-authored-by: Ed Santiago <[email protected]> Signed-off-by: Aditya R <[email protected]>
When running concurrent builds references to layers are changed and
c/storage does not accounts for names so use ID where possible.
Builds fail with
image not known.Reproducer
Dockerfile
FROM quay.io/jitesoft/alpineNote:
This PR is just for discussion to confirm if we can do better solutions or not. I believe underlying problem still persists either in
c/imageorc/storage. See bz 2055487