Cherry-picked docker#268

integration/image/remove_unix_test.go

@@ -0,0 +1,93 @@
+// +build !windows
+
+package image // import "github.com/docker/docker/integration/image"
+
+import (
+	"context"
+	"io"
+	"io/ioutil"
+	"os"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+	"unsafe"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/internal/test/daemon"
+	"github.com/docker/docker/internal/test/fakecontext"
+	"gotest.tools/assert"
+	"gotest.tools/skip"
+)
+
+// This is a regression test for #38488
+// It ensures that orphan layers can be found and cleaned up
+// after unsuccessful image removal
+func TestRemoveImageGarbageCollector(t *testing.T) {
+	// This test uses very platform specific way to prevent
+	// daemon for remove image layer.
+	skip.If(t, testEnv.DaemonInfo.OSType != "linux")
+	skip.If(t, os.Getenv("DOCKER_ENGINE_GOARCH") != "amd64")
+
+	// Create daemon with overlay2 graphdriver because vfs uses disk differently
+	// and this test case would not work with it.
+	d := daemon.New(t, daemon.WithStorageDriver("overlay2"), daemon.WithImageService)
+	d.Start(t)
+	defer d.Stop(t)
+
+	ctx := context.Background()
+	client := d.NewClientT(t)
+	i := d.ImageService()
+
+	img := "test-garbage-collector"
+
+	// Build a image with multiple layers
+	dockerfile := `FROM busybox
+	RUN echo echo Running... > /run.sh`
+	source := fakecontext.New(t, "", fakecontext.WithDockerfile(dockerfile))
+	defer source.Close()
+	resp, err := client.ImageBuild(ctx,
+		source.AsTarReader(t),
+		types.ImageBuildOptions{
+			Remove:      true,
+			ForceRemove: true,
+			Tags:        []string{img},
+		})
+	assert.NilError(t, err)
+	_, err = io.Copy(ioutil.Discard, resp.Body)
+	resp.Body.Close()
+	assert.NilError(t, err)
+	image, _, err := client.ImageInspectWithRaw(ctx, img)
+	assert.NilError(t, err)
+
+	// Mark latest image layer to immutable
+	data := image.GraphDriver.Data
+	file, _ := os.Open(data["UpperDir"])
+	attr := 0x00000010
+	fsflags := uintptr(0x40086602)
+	argp := uintptr(unsafe.Pointer(&attr))
+	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, file.Fd(), fsflags, argp)
+	assert.Equal(t, "errno 0", errno.Error())
+
+	// Try to remove the image, it should generate error
+	// but marking layer back to mutable before checking errors (so we don't break CI server)
+	_, err = client.ImageRemove(ctx, img, types.ImageRemoveOptions{})
+	attr = 0x00000000
+	argp = uintptr(unsafe.Pointer(&attr))
+	_, _, errno = syscall.Syscall(syscall.SYS_IOCTL, file.Fd(), fsflags, argp)
+	assert.Equal(t, "errno 0", errno.Error())
+	assert.Assert(t, err != nil)
+	errStr := err.Error()
+	if !(strings.Contains(errStr, "permission denied") || strings.Contains(errStr, "operation not permitted")) {
+		t.Errorf("ImageRemove error not an permission error %s", errStr)
+	}
+
+	// Verify that layer remaining on disk
+	dir, _ := os.Stat(data["UpperDir"])
+	assert.Equal(t, "true", strconv.FormatBool(dir.IsDir()))
+
+	// Run imageService.Cleanup() and make sure that layer was removed from disk
+	i.Cleanup()
+	dir, err = os.Stat(data["UpperDir"])
+	assert.ErrorContains(t, err, "no such file or directory")
+}

internal/test/daemon/daemon.go

@@ -16,6 +16,7 @@ import (
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/events"
 	"github.com/docker/docker/client"
+	"github.com/docker/docker/daemon/images"
 	"github.com/docker/docker/internal/test"
 	"github.com/docker/docker/internal/test/request"
 	"github.com/docker/docker/opts"
@@ -77,6 +78,7 @@ type Daemon struct {
 	init          bool
 	dockerdBinary string
 	log           logT
+	imageService  *images.ImageService
 
 	// swarm related field
 	swarmListenAddr string
@@ -720,3 +722,8 @@ func cleanupRaftDir(t testingT, rootPath string) {
 		}
 	}
 }
+
+// ImageService returns the Daemon's ImageService
+func (d *Daemon) ImageService() *images.ImageService {
+	return d.imageService
+}

internal/test/daemon/ops.go

@@ -63,3 +63,10 @@ func WithEnvironment(e environment.Execution) func(*Daemon) {
 		}
 	}
 }
+
+// WithStorageDriver sets store driver option
+func WithStorageDriver(driver string) func(d *Daemon) {
+	return func(d *Daemon) {
+		d.storageDriver = driver
+	}
+}

internal/test/daemon/ops_unix.go

@@ -0,0 +1,34 @@
+// +build !windows
+
+package daemon
+
+import (
+	"path/filepath"
+	"runtime"
+
+	"github.com/docker/docker/daemon/images"
+	"github.com/docker/docker/layer"
+
+	// register graph drivers
+	_ "github.com/docker/docker/daemon/graphdriver/register"
+	"github.com/docker/docker/pkg/idtools"
+)
+
+// WithImageService sets imageService options
+func WithImageService(d *Daemon) {
+	layerStores := make(map[string]layer.Store)
+	os := runtime.GOOS
+	layerStores[os], _ = layer.NewStoreFromOptions(layer.StoreOptions{
+		Root:                      d.Root,
+		MetadataStorePathTemplate: filepath.Join(d.RootDir(), "image", "%s", "layerdb"),
+		GraphDriver:               d.storageDriver,
+		GraphDriverOptions:        nil,
+		IDMapping:                 &idtools.IdentityMapping{},
+		PluginGetter:              nil,
+		ExperimentalEnabled:       false,
+		OS:                        os,
+	})
+	d.imageService = images.NewImageService(images.ImageServiceConfig{
+		LayerStores: layerStores,
+	})
+}

layer/filestore.go

@@ -14,7 +14,7 @@ import (
 
 	"github.com/docker/distribution"
 	"github.com/docker/docker/pkg/ioutils"
-	"github.com/opencontainers/go-digest"
+	digest "github.com/opencontainers/go-digest"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -305,6 +305,55 @@ func (fms *fileMetadataStore) GetMountParent(mount string) (ChainID, error) {
 	return ChainID(dgst), nil
 }
 
+func (fms *fileMetadataStore) getOrphan() ([]roLayer, error) {
+	var orphanLayers []roLayer
+	for _, algorithm := range supportedAlgorithms {
+		fileInfos, err := ioutil.ReadDir(filepath.Join(fms.root, string(algorithm)))
+		if err != nil {
+			if os.IsNotExist(err) {
+				continue
+			}
+			return nil, err
+		}
+
+		for _, fi := range fileInfos {
+			if !fi.IsDir() || !strings.HasSuffix(fi.Name(), "-removing") {
+				continue
+			}
+			// At this stage, fi.Name value looks like <digest>-<random>-removing
+			// Split on '-' to get the digest value.
+			nameSplit := strings.Split(fi.Name(), "-")
+			dgst := digest.NewDigestFromEncoded(algorithm, nameSplit[0])
+			if err := dgst.Validate(); err != nil {
+				logrus.WithError(err).WithField("digest", string(algorithm)+":"+nameSplit[0]).Debug("ignoring invalid digest")
+				continue
+			}
+
+			chainFile := filepath.Join(fms.root, string(algorithm), fi.Name(), "cache-id")
+			contentBytes, err := ioutil.ReadFile(chainFile)
+			if err != nil {
+				if !os.IsNotExist(err) {
+					logrus.WithError(err).WithField("digest", dgst).Error("failed to read cache ID")
+				}
+				continue
+			}
+			cacheID := strings.TrimSpace(string(contentBytes))
+			if cacheID == "" {
+				logrus.Error("invalid cache ID")
+				continue
+			}
+
+			l := &roLayer{
+				chainID: ChainID(dgst),
+				cacheID: cacheID,
+			}
+			orphanLayers = append(orphanLayers, *l)
+		}
+	}
+
+	return orphanLayers, nil
+}
+
 func (fms *fileMetadataStore) List() ([]ChainID, []string, error) {
 	var ids []ChainID
 	for _, algorithm := range supportedAlgorithms {
@@ -346,8 +395,39 @@ func (fms *fileMetadataStore) List() ([]ChainID, []string, error) {
 	return ids, mounts, nil
 }
 
-func (fms *fileMetadataStore) Remove(layer ChainID) error {
-	return os.RemoveAll(fms.getLayerDirectory(layer))
+// Remove layerdb folder if that is marked for removal
+func (fms *fileMetadataStore) Remove(layer ChainID, cache string) error {
+	dgst := digest.Digest(layer)
+	files, err := ioutil.ReadDir(filepath.Join(fms.root, string(dgst.Algorithm())))
+	if err != nil {
+		return err
+	}
+	for _, f := range files {
+		if !strings.HasSuffix(f.Name(), "-removing") || !strings.HasPrefix(f.Name(), dgst.String()) {
+			continue
+		}
+
+		// Make sure that we only remove layerdb folder which points to
+		// requested cacheID
+		dir := filepath.Join(fms.root, string(dgst.Algorithm()), f.Name())
+		chainFile := filepath.Join(dir, "cache-id")
+		contentBytes, err := ioutil.ReadFile(chainFile)
+		if err != nil {
+			logrus.WithError(err).WithField("file", chainFile).Error("cannot get cache ID")
+			continue
+		}
+		cacheID := strings.TrimSpace(string(contentBytes))
+		if cacheID != cache {
+			continue
+		}
+		logrus.Debugf("Removing folder: %s", dir)
+		err = os.RemoveAll(dir)
+		if err != nil && !os.IsNotExist(err) {
+			logrus.WithError(err).WithField("name", f.Name()).Error("cannot remove layer")
+			continue
+		}
+	}
+	return nil
 }
 
 func (fms *fileMetadataStore) RemoveMount(mount string) error {

layer/filestore_test.go

@@ -10,6 +10,7 @@ import (
 	"syscall"
 	"testing"
 
+	"github.com/docker/docker/pkg/stringid"
 	"github.com/opencontainers/go-digest"
 )
 
@@ -102,3 +103,50 @@ func TestStartTransactionFailure(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestGetOrphan(t *testing.T) {
+	fms, td, cleanup := newFileMetadataStore(t)
+	defer cleanup()
+
+	layerRoot := filepath.Join(td, "sha256")
+	if err := os.MkdirAll(layerRoot, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	tx, err := fms.StartTransaction()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	layerid := randomLayerID(5)
+	err = tx.Commit(layerid)
+	if err != nil {
+		t.Fatal(err)
+	}
+	layerPath := fms.getLayerDirectory(layerid)
+	if err := ioutil.WriteFile(filepath.Join(layerPath, "cache-id"), []byte(stringid.GenerateRandomID()), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	orphanLayers, err := fms.getOrphan()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(orphanLayers) != 0 {
+		t.Fatalf("Expected to have zero orphan layers")
+	}
+
+	layeridSplit := strings.Split(layerid.String(), ":")
+	newPath := filepath.Join(layerRoot, fmt.Sprintf("%s-%s-removing", layeridSplit[1], stringid.GenerateRandomID()))
+	err = os.Rename(layerPath, newPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	orphanLayers, err = fms.getOrphan()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(orphanLayers) != 1 {
+		t.Fatalf("Expected to have one orphan layer")
+	}
+}