Fix volume cache issue with buildah bud --layers

buildah bud would fail at the next step after a VOLUME command
due to buildah trying to cache the volume contents and restore it
later, but the intermediate container used at the VOLUME step didn't
exist anymore.
Changed functionality to delete successful intermediate containers at
the end of the whole build process.

Signed-off-by: umohnani8 <umohnani@redhat.com>

Closes: #892
Approved by: rhatdan

imagebuildah/build.go

@@ -215,6 +215,7 @@ type Executor struct {
 	noCache                        bool
 	removeIntermediateCtrs         bool
 	forceRmIntermediateCtrs        bool
+	containerIDs                   []string // Stores the IDs of the successful intermediate containers used during layer build
 }
 
 // withName creates a new child executor that will be used whenever a COPY statement uses --from=NAME.
@@ -684,6 +685,7 @@ func (b *Executor) Prepare(ctx context.Context, ib *imagebuilder.Builder, node *
 	// Add the top layer of this image to b.topLayers so we can keep track of them
 	// when building with cached images.
 	b.topLayers = append(b.topLayers, builder.TopLayer)
+	logrus.Debugln("Container ID:", builder.ContainerID)
 	return nil
 }
 
@@ -811,12 +813,8 @@ func (b *Executor) Execute(ctx context.Context, ib *imagebuilder.Builder, node *
 			// it is used to create the container for the next step.
 			imgID = cacheID
 		}
-		// Delete the intermediate container if b.removeIntermediateCtrs is true.
-		if b.removeIntermediateCtrs {
-			if err := b.Delete(); err != nil {
-				return errors.Wrap(err, "error deleting intermediate container")
-			}
-		}
+		// Add container ID of successful intermediate container to b.containerIDs
+		b.containerIDs = append(b.containerIDs, b.builder.ContainerID)
 		// Prepare for the next step with imgID as the new base image.
 		if i != len(children)-1 {
 			if err := b.Prepare(ctx, ib, node, imgID); err != nil {
@@ -1122,11 +1120,14 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) error
 	if len(stages) == 0 {
 		errors.New("error building: no stages to build")
 	}
-	var stageExecutor *Executor
+	var (
+		stageExecutor *Executor
+		lastErr       error
+	)
 	for _, stage := range stages {
 		stageExecutor = b.withName(stage.Name, stage.Position)
 		if err := stageExecutor.Prepare(ctx, stage.Builder, stage.Node, ""); err != nil {
-			return err
+			lastErr = err
 		}
 		// Always remove the intermediate/build containers, even if the build was unsuccessful.
 		// If building with layers, remove all intermediate/build containers if b.forceRmIntermediateCtrs
@@ -1135,8 +1136,18 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) error
 			defer stageExecutor.Delete()
 		}
 		if err := stageExecutor.Execute(ctx, stage.Builder, stage.Node); err != nil {
-			return err
+			lastErr = err
 		}
+
+		// Delete the successful intermediate containers if an error in the build
+		// process occurs and b.removeIntermediateCtrs is true.
+		if lastErr != nil {
+			if b.removeIntermediateCtrs {
+				stageExecutor.deleteSuccessfulIntermediateCtrs()
+			}
+			return lastErr
+		}
+		b.containerIDs = append(b.containerIDs, stageExecutor.containerIDs...)
 	}
 
 	if !b.layers && !b.noCache {
@@ -1154,7 +1165,9 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) error
 	// the removal of intermediate/build containers will be handled by the
 	// defer statement above.
 	if b.removeIntermediateCtrs && (b.layers || b.noCache) {
-		return stageExecutor.Delete()
+		if err := b.deleteSuccessfulIntermediateCtrs(); err != nil {
+			return errors.Errorf("Failed to cleanup intermediate containers")
+		}
 	}
 	return nil
 }
@@ -1225,3 +1238,16 @@ func BuildDockerfiles(ctx context.Context, store storage.Store, options BuildOpt
 	stages := imagebuilder.NewStages(mainNode, b)
 	return exec.Build(ctx, stages)
 }
+
+// deleteSuccessfulIntermediateCtrs goes through the container IDs in b.containerIDs
+// and deletes the containers associated with that ID.
+func (b *Executor) deleteSuccessfulIntermediateCtrs() error {
+	var lastErr error
+	for _, ctr := range b.containerIDs {
+		if err := b.store.DeleteContainer(ctr); err != nil {
+			logrus.Errorf("error deleting build container %q: %v\n", ctr, err)
+			lastErr = err
+		}
+	}
+	return lastErr
+}

tests/bud.bats

@@ -5,11 +5,11 @@ load helpers
 @test "bud with --layers and --no-cache flags" {
   buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t test1 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 5 ]
+  [ $(wc -l <<< "$output") -eq 6 ]
   [ "${status}" -eq 0 ]
   buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t test2 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 6 ]
+  [ $(wc -l <<< "$output") -eq 7 ]
   [ "${status}" -eq 0 ]
   run buildah inspect --format "{{.Docker.ContainerConfig.Env}}" test2
   echo "$output"
@@ -18,23 +18,23 @@ load helpers
 
   buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t test3 -f Dockerfile.2 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 8 ]
+  [ $(wc -l <<< "$output") -eq 9 ]
   [ "${status}" -eq 0 ]
 
   mkdir -p ${TESTSDIR}/bud/use-layers/mount/subdir
   buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t test4 -f Dockerfile.3 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 10 ]
+  [ $(wc -l <<< "$output") -eq 11 ]
   [ "${status}" -eq 0 ]
   touch ${TESTSDIR}/bud/use-layers/mount/subdir/file.txt
   buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t test5 -f Dockerfile.3 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 12 ]
+  [ $(wc -l <<< "$output") -eq 13 ]
   [ "${status}" -eq 0 ]
 
   buildah bud --signature-policy ${TESTSDIR}/policy.json --no-cache -t test6 -f Dockerfile.2 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 15 ]
+  [ $(wc -l <<< "$output") -eq 16 ]
   [ "${status}" -eq 0 ]
 
   buildah rmi -a -f
@@ -49,7 +49,7 @@ load helpers
 
   buildah bud --signature-policy ${TESTSDIR}/policy.json --rm=false --layers -t test2 ${TESTSDIR}/bud/use-layers
   run buildah --debug=false containers
-  [ $(wc -l <<< "$output") -eq 4 ]
+  [ $(wc -l <<< "$output") -eq 5 ]
   [ "${status}" -eq 0 ]
 
   buildah rm -a

tests/bud/use-layers/Dockerfile

@@ -1,4 +1,5 @@
 FROM alpine
 RUN mkdir /hello
+VOLUME /var/lib/testdata
 RUN touch file.txt
 ENV foo=bar

tests/images.bats

@@ -18,7 +18,7 @@ load helpers
   [ $(wc -l <<< "$output") -eq 3 ]
   [ "${status}" -eq 0 ]
   run buildah --debug=false images -a
-  [ $(wc -l <<< "$output") -eq 5 ]
+  [ $(wc -l <<< "$output") -eq 6 ]
 
   # create a no name image which should show up when doing buildah images without the --all flag
   buildah bud --signature-policy ${TESTSDIR}/policy.json ${TESTSDIR}/bud/use-layers