Skip to content

Commit

Permalink
commit: use race-free RemoveNames instead of SetNames
Browse files Browse the repository at this point in the history
PR containers/storage#1153 added a dedicated API
to remove names assigned image so use `RemoveNames` instead of racy
`SetNames`.

How to verify
```console
printf 'from quay.io/jitesoft/alpine:latest\nrun for i in $(seq 0 10000); do touch /$i; done\n' >Containerfile && for i in `seq 1 25`; do ./buildah build --squash --iidfile id.$i --timestamp 0 . & done; wait; ls -al
```

* Refer to newly added integration test.

Closes: containers/podman#15162

Signed-off-by: Aditya R <arajan@redhat.com>
  • Loading branch information
flouthoc committed Aug 17, 2022
1 parent 298e1a5 commit d895517
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 11 deletions.
15 changes: 4 additions & 11 deletions commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"io"
"io/ioutil"
"os"
"strings"
"time"

"github.com/containers/buildah/pkg/blobcache"
Expand Down Expand Up @@ -374,17 +373,11 @@ func (b *Builder) Commit(ctx context.Context, dest types.ImageReference, options
}
if err == nil {
imgID = img.ID
prunedNames := make([]string, 0, len(img.Names))
for _, name := range img.Names {
if !(nameToRemove != "" && strings.Contains(name, nameToRemove)) {
prunedNames = append(prunedNames, name)
if nameToRemove != "" {
if err = b.store.RemoveNames(imgID, []string{nameToRemove}); err != nil {
return imgID, nil, "", fmt.Errorf("failed to remove temporary name from image %q: %w", imgID, err)
}
}
if len(prunedNames) < len(img.Names) {
if err = b.store.SetNames(imgID, prunedNames); err != nil {
return imgID, nil, "", fmt.Errorf("failed to prune temporary name from image %q: %w", imgID, err)
}
logrus.Debugf("reassigned names %v to image %q", prunedNames, img.ID)
logrus.Debugf("removing %v from assigned names to image %q", nameToRemove, img.ID)
dest2, err := is.Transport.ParseStoreReference(b.store, "@"+imgID)
if err != nil {
return imgID, nil, "", fmt.Errorf("error creating unnamed destination reference for image: %w", err)
Expand Down
19 changes: 19 additions & 0 deletions tests/bud.bats
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,25 @@ _EOF
assert "$output" !~ "unwanted stage"
}

# Note: Please skip this tests in case of podman-remote build
@test "build: test race in updating image name while performing parallel commits" {
# Run 25 parallel builds using the same Containerfile
local count=25
for i in $(seq --format '%02g' 1 $count); do
timeout --foreground -v --kill=10 60 \
${BUILDAH_BINARY} ${BUILDAH_REGISTRY_OPTS} ${ROOTDIR_OPTS} $WITH_POLICY_JSON build --squash --iidfile ${TEST_SCRATCH_DIR}/id.$i --timestamp 0 -f $BUDFILES/check-race/Containerfile &>/dev/null &
done
# Wait for all background builds to complete. Note that this succeeds
# even if some of the individual builds fail! Our actual test is below.
wait
# Number of output bytes must be always same, which confirms that there is no race.
for i in $(seq --format '%02g' 1 $count); do
test $(cat ${TEST_SCRATCH_DIR}/id.$i|wc -c) = 71
done
# clean all images built for this test
run_buildah rmi --all -f
}

# Test skipping images with FROM but stage name also conflicts with additional build context
# so selected stage should be still skipped since it is not being actually used by additional build
# context is being used.
Expand Down
2 changes: 2 additions & 0 deletions tests/bud/check-race/Containerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
FROM alpine
RUN for i in $(seq 0 10000); do touch /$i; done

0 comments on commit d895517

Please sign in to comment.