storage: fix some bugs in the additional layer store

storage/additional_layer_test.go

@@ -0,0 +1,83 @@
+//go:build linux
+
+package storage
+
+import (
+	"encoding/base64"
+	"os"
+	"path/filepath"
+	"testing"
+
+	digest "github.com/opencontainers/go-digest"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// setupAdditionalLayerStore creates a temporary additional layer store directory
+// with the expected structure for the given tocDigest and image reference.
+// The info file contains the provided content.
+func setupAdditionalLayerStore(t *testing.T, tocDigest digest.Digest, imageRef string, infoContent string) string {
+	t.Helper()
+	alsRoot := t.TempDir()
+
+	refDir := base64.StdEncoding.EncodeToString([]byte(imageRef))
+	layerDir := filepath.Join(alsRoot, refDir, tocDigest.String())
+	require.NoError(t, os.MkdirAll(layerDir, 0o755))
+
+	require.NoError(t, os.MkdirAll(filepath.Join(layerDir, "diff"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(layerDir, "info"), []byte(infoContent), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(layerDir, "blob"), []byte{}, 0o644))
+
+	return alsRoot
+}
+
+func TestLookupAdditionalLayerSuccess(t *testing.T) {
+	prevLevel := logrus.GetLevel()
+	logrus.SetLevel(logrus.ErrorLevel)
+	t.Cleanup(func() { logrus.SetLevel(prevLevel) })
+
+	tocDigest := digest.FromString("test-layer")
+	imageRef := "fedora"
+
+	info := Layer{
+		ID:             "test-id",
+		CompressedSize: 42,
+		TOCDigest:      tocDigest,
+	}
+	infoJSON, err := json.Marshal(info)
+	require.NoError(t, err)
+
+	alsPath := setupAdditionalLayerStore(t, tocDigest, imageRef, string(infoJSON))
+	store := newTestStore(t, StoreOptions{
+		GraphDriverName:    "overlay",
+		GraphDriverOptions: []string{"additionallayerstore=" + alsPath + ":ref"},
+	})
+	t.Cleanup(func() { _, _ = store.Shutdown(true) })
+
+	al, err := store.LookupAdditionalLayer(tocDigest, imageRef)
+	require.NoError(t, err)
+	defer al.Release()
+
+	assert.Equal(t, tocDigest, al.TOCDigest())
+	assert.Equal(t, int64(42), al.CompressedSize())
+}
+
+func TestLookupAdditionalLayerDecodeError(t *testing.T) {
+	prevLevel := logrus.GetLevel()
+	logrus.SetLevel(logrus.ErrorLevel)
+	t.Cleanup(func() { logrus.SetLevel(prevLevel) })
+
+	tocDigest := digest.FromString("test-layer")
+	imageRef := "fedora"
+
+	alsPath := setupAdditionalLayerStore(t, tocDigest, imageRef, "not valid json")
+	store := newTestStore(t, StoreOptions{
+		GraphDriverName:    "overlay",
+		GraphDriverOptions: []string{"additionallayerstore=" + alsPath + ":ref"},
+	})
+	t.Cleanup(func() { _, _ = store.Shutdown(true) })
+
+	_, err := store.LookupAdditionalLayer(tocDigest, imageRef)
+	assert.Error(t, err, "should fail on invalid JSON in info file")
+}

storage/drivers/overlay/overlay.go

@@ -648,10 +648,10 @@ func SupportsNativeOverlay(home, runhome string) (bool, error) {
 		// Do nothing.
 	default:
 		needsMountProgram, err := scanForMountProgramIndicators(home)
-		if err != nil && !os.IsNotExist(err) {
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
 			return false, err
 		}
-		if err := os.WriteFile(getMountProgramFlagFile(home), []byte(fmt.Sprintf("%t", needsMountProgram)), 0o600); err != nil && !os.IsNotExist(err) {
+		if err := os.WriteFile(getMountProgramFlagFile(home), []byte(fmt.Sprintf("%t", needsMountProgram)), 0o600); err != nil && !errors.Is(err, fs.ErrNotExist) {
 			return false, err
 		}
 		if needsMountProgram {
@@ -1193,7 +1193,7 @@ func (d *Driver) getLower(parent string) (string, error) {
 	// Read Parent link fileA
 	parentLink, err := os.ReadFile(path.Join(parentDir, "link"))
 	if err != nil {
-		if !os.IsNotExist(err) {
+		if !errors.Is(err, fs.ErrNotExist) {
 			return "", err
 		}
 		logrus.Warnf("Can't read parent link %q because it does not exist. Going through storage to recreate the missing links.", path.Join(parentDir, "link"))
@@ -1266,7 +1266,7 @@ func (d *Driver) getLowerDirs(id string) ([]string, error) {
 			// if the link does not exist, we lost the symlinks during a sudden reboot.
 			// Let's go ahead and recreate those symlinks.
 			if err != nil {
-				if os.IsNotExist(err) {
+				if errors.Is(err, fs.ErrNotExist) {
 					logrus.Warnf("Can't read link %q because it does not exist. A storage corruption might have occurred, attempting to recreate the missing symlinks. It might be best wipe the storage to avoid further errors due to storage corruption.", lower)
 					if err := d.recreateSymlinks(); err != nil {
 						return nil, fmt.Errorf("recreating the missing symlinks: %w", err)
@@ -1282,7 +1282,7 @@ func (d *Driver) getLowerDirs(id string) ([]string, error) {
 			}
 			lowersArray = append(lowersArray, path.Clean(d.dir(path.Join("link", lp))))
 		}
-	} else if !os.IsNotExist(err) {
+	} else if !errors.Is(err, fs.ErrNotExist) {
 		return nil, err
 	}
 	return lowersArray, nil
@@ -1335,7 +1335,7 @@ func (d *Driver) removeCommon(id string, cleanup func(string) error) error {
 
 	d.releaseAdditionalLayerByID(id)
 
-	if err := cleanup(dir); err != nil && !os.IsNotExist(err) {
+	if err := cleanup(dir); err != nil && !errors.Is(err, fs.ErrNotExist) {
 		return err
 	}
 	if d.quotaCtl != nil {
@@ -1430,7 +1430,7 @@ func (d *Driver) recreateSymlinks() error {
 			// Check if the symlink exists, and if it doesn't, create it again with the
 			// name we got from the "link" file
 			err = fileutils.Lexists(linkPath)
-			if err != nil && os.IsNotExist(err) {
+			if err != nil && errors.Is(err, fs.ErrNotExist) {
 				if err := os.Symlink(path.Join("..", dir.Name(), "diff"), linkPath); err != nil {
 					errs = errors.Join(errs, err)
 					continue
@@ -1464,7 +1464,7 @@ func (d *Driver) recreateSymlinks() error {
 				errs = errors.Join(errs, fmt.Errorf("link target of %q looks weird: %q", link, target))
 				// force the link to be recreated on the next pass
 				if err := os.Remove(filepath.Join(linkDirFullPath, link.Name())); err != nil {
-					if !os.IsNotExist(err) {
+					if !errors.Is(err, fs.ErrNotExist) {
 						errs = errors.Join(errs, fmt.Errorf("removing link %q: %w", link, err))
 					} // else don’t report any error, but also don’t set madeProgress.
 					continue
@@ -1590,7 +1590,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
 	}
 
 	lowers, err := os.ReadFile(path.Join(dir, lowerFile))
-	if err != nil && !os.IsNotExist(err) {
+	if err != nil && !errors.Is(err, fs.ErrNotExist) {
 		return "", err
 	}
 	splitLowers := strings.Split(string(lowers), ":")
@@ -1645,7 +1645,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
 	maybeAddComposefsMount := func(lowerID string, i int, readWrite bool) (string, error) {
 		composefsBlob := d.getComposefsData(lowerID)
 		if err := fileutils.Exists(composefsBlob); err != nil {
-			if os.IsNotExist(err) {
+			if errors.Is(err, fs.ErrNotExist) {
 				return "", nil
 			}
 			return "", err
@@ -1706,7 +1706,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
 			// if it is a "not found" error, that means the symlinks were lost in a sudden reboot
 			// so call the recreateSymlinks function to go through all the layer dirs and recreate
 			// the symlinks with the name from their respective "link" files
-			if lower == "" && os.IsNotExist(err) {
+			if lower == "" && errors.Is(err, fs.ErrNotExist) {
 				logrus.Warnf("Can't stat lower layer %q because it does not exist. Going through storage to recreate the missing symlinks.", newpath)
 				if err := d.recreateSymlinks(); err != nil {
 					return "", fmt.Errorf("recreating the missing symlinks: %w", err)
@@ -1983,7 +1983,7 @@ func (d *Driver) Put(id string) error {
 	if count := d.ctr.Decrement(mountpoint); count > 0 {
 		return nil
 	}
-	if err := fileutils.Exists(path.Join(dir, lowerFile)); err != nil && !os.IsNotExist(err) {
+	if err := fileutils.Exists(path.Join(dir, lowerFile)); err != nil && !errors.Is(err, fs.ErrNotExist) {
 		return err
 	}
 
@@ -2028,7 +2028,7 @@ func (d *Driver) Put(id string) error {
 	}
 
 	if !unmounted {
-		if err := unix.Unmount(mountpoint, unix.MNT_DETACH); err != nil && !os.IsNotExist(err) {
+		if err := unix.Unmount(mountpoint, unix.MNT_DETACH); err != nil && !errors.Is(err, fs.ErrNotExist) {
 			logrus.Debugf("Failed to unmount %s overlay: %s - %v", id, mountpoint, err)
 			if !errors.Is(err, unix.EINVAL) {
 				return fmt.Errorf("unmounting %q: %w", mountpoint, err)
@@ -2205,7 +2205,7 @@ func (d *Driver) DiffGetter(id string) (_ graphdriver.FileGetCloser, Err error)
 		if Err != nil {
 			for _, f := range composefsMounts {
 				f.Close()
-				if err := unix.Rmdir(f.Name()); err != nil && !os.IsNotExist(err) {
+				if err := unix.Rmdir(f.Name()); err != nil && !errors.Is(err, fs.ErrNotExist) {
 					logrus.Warnf("Failed to remove %s: %v", f.Name(), err)
 				}
 			}
@@ -2378,7 +2378,7 @@ func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *gr
 			return err
 		}
 	}
-	if err := os.RemoveAll(diffPath); err != nil && !os.IsNotExist(err) {
+	if err := os.RemoveAll(diffPath); err != nil && !errors.Is(err, fs.ErrNotExist) {
 		return err
 	}
 
@@ -2750,7 +2750,7 @@ func (d *Driver) getAdditionalLayerPath(tocDigest digest.Digest, ref string) (st
 func (d *Driver) releaseAdditionalLayerByID(id string) {
 	if al, err := d.getAdditionalLayerPathByID(id); err == nil {
 		notifyReleaseAdditionalLayer(al)
-	} else if !os.IsNotExist(err) {
+	} else if !errors.Is(err, fs.ErrNotExist) {
 		logrus.Warnf("Unexpected error on reading Additional Layer Store pointer %v", err)
 	}
 }
@@ -2827,7 +2827,7 @@ func notifyUseAdditionalLayer(al string) {
 	}
 	useFile := path.Join(al, "use")
 	f, err := os.Create(useFile)
-	if os.IsNotExist(err) {
+	if errors.Is(err, fs.ErrNotExist) {
 		return
 	} else if err == nil {
 		f.Close()
@@ -2849,7 +2849,7 @@ func notifyReleaseAdditionalLayer(al string) {
 	}
 	// tell the additional layer store that we don't use this layer anymore.
 	err := unix.Rmdir(al)
-	if os.IsNotExist(err) {
+	if errors.Is(err, fs.ErrNotExist) {
 		return
 	}
 	logrus.Warnf("Unexpected error by Additional Layer Store %v during release; GC doesn't seem to be supported", err)

storage/store.go

@@ -3499,6 +3499,12 @@ func (s *store) LookupAdditionalLayer(tocDigest digest.Digest, imageref string)
 		}
 		return nil, err
 	}
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			al.Release()
+		}
+	}()
 	info, err := al.Info()
 	if err != nil {
 		return nil, err
@@ -3508,6 +3514,7 @@ func (s *store) LookupAdditionalLayer(tocDigest digest.Digest, imageref string)
 	if err := json.NewDecoder(info).Decode(&layer); err != nil {
 		return nil, err
 	}
+	succeeded = true
 	return &additionalLayer{&layer, al, s}, nil
 }