Merge pull request #888 from bitoku/fix-false-layer-missing-ro-parent

Fix false ErrLayerMissing for RW layers with RO parents

Author: mtrmac

storage/check.go

@@ -414,32 +414,33 @@ func (s *store) Check(options *CheckOptions) (CheckReport, error) {
 				}
 			}
 		}
-		// Check that we don't have any dangling parent layer references.
-		for id, parent := range report.layerParentsByLayerID {
-			// If this layer doesn't have a parent, no problem.
-			if parent == "" {
-				continue
-			}
-			// If we've already seen a layer with this parent ID, skip it.
-			if _, checked := referencedLayers[parent]; checked {
-				continue
-			}
-			if _, checked := referencedROLayers[parent]; checked {
-				continue
-			}
-			// We haven't seen a layer with the ID that this layer's record
-			// says is its parent's ID.
-
-			// Not recordError2 because we are recording for layer "id"
-			// but the text talks about layer "parent".
-			report.recordLayerErrors(id,
-				fmt.Errorf("%slayer %s: %w", readWriteDesc, parent, ErrLayerMissing))
-		}
 		return struct{}{}, false, nil
 	}); err != nil {
 		return CheckReport{}, err
 	}
 
+	// Check that we don't have any dangling parent layer references.
+	// This runs after all layer stores (RW and RO) have been visited,
+	// so both referencedLayers and referencedROLayers are fully populated.
+	for id, parent := range report.layerParentsByLayerID {
+		if parent == "" {
+			continue
+		}
+		if _, checked := referencedLayers[parent]; checked {
+			continue
+		}
+		if _, checked := referencedROLayers[parent]; checked {
+			continue
+		}
+		if _, isRO := referencedROLayers[id]; isRO {
+			report.recordROLayerErrors(id,
+				fmt.Errorf("read-only layer %s: %w", parent, ErrLayerMissing))
+		} else {
+			report.recordLayerErrors(id,
+				fmt.Errorf("layer %s: %w", parent, ErrLayerMissing))
+		}
+	}
+
 	// This map will track examined images.  If we have multiple stores, read-only ones can
 	// contain copies of images that are also in the read-write store, or the read-write store
 	// may contain a duplicate entry that refers to layers in the read-only stores, but when

storage/check_test.go

@@ -2,6 +2,7 @@ package storage
 
 import (
 	"archive/tar"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -66,6 +67,33 @@ func TestCheckDirectory(t *testing.T) {
 	}
 }
 
+func TestCheckParentLayerInROStore(t *testing.T) {
+	roStore := newTestStore(t, StoreOptions{})
+	_, err := roStore.CreateLayer("ro-parent-layer", "", nil, "", false, nil)
+	require.NoError(t, err)
+	roStoreRoot := roStore.GraphRoot()
+	_, err = roStore.Shutdown(false)
+	require.NoError(t, err)
+
+	// Copy the graph root to a fresh path so the global lock file cache
+	// doesn't conflict (the first store registered these locks as RW).
+	roStoreCopy := t.TempDir()
+	require.NoError(t, os.CopyFS(roStoreCopy, os.DirFS(roStoreRoot)))
+
+	rwStore := newTestStore(t, StoreOptions{
+		GraphDriverOptions: []string{"imagestore=" + roStoreCopy},
+	})
+	t.Cleanup(func() { _, _ = rwStore.Shutdown(true) })
+
+	_, err = rwStore.CreateLayer("rw-child-layer", "ro-parent-layer", nil, "", true, nil)
+	require.NoError(t, err)
+
+	report, err := rwStore.Check(nil)
+	require.NoError(t, err)
+	assert.Empty(t, report.Layers, "RW child layer with RO parent should not be flagged")
+	assert.Empty(t, report.ROLayers, "RO parent layer should not be flagged")
+}
+
 func TestCheckDetectWriteable(t *testing.T) {
 	var sawRWlayers, sawRWimages bool
 	stoar, err := GetStore(StoreOptions{