strip-volatile-option-tmp-mounts

KodieGlosserIBM · fuweid · commit bb80bd7681ce · 2024-06-13T15:39:44.000+08:00
Signed-off-by: krglosse <krglosse@us.ibm.com> do not alter original slice Signed-off-by: krglosse <krglosse@us.ibm.com> Update core/mount/temp.go makes sense, thank you! Co-authored-by: Derek McGowan <derek@mcg.dev> Signed-off-by: KodieGlosserIBM <39170759+KodieGlosserIBM@users.noreply.github.com> do not copy mount structure unless conditional is met and adding a test case for it Signed-off-by: krglosse <krglosse@us.ibm.com> copy option slice when removing the element instead of giving the element an empty string remove unneeded block Signed-off-by: krglosse <krglosse@us.ibm.com> simplify Signed-off-by: krglosse <krglosse@us.ibm.com> (cherry picked from commit cfe8321) Signed-off-by: Wei Fu <fuweid89@gmail.com>
diff --git a/mount/mount_test.go b/mount/mount_test.go
@@ -148,3 +148,114 @@ func TestReadonlyMounts(t *testing.T) {
 		}
 	}
 }
+
+func TestRemoveVolatileTempMount(t *testing.T) {
+	testCases := []struct {
+		desc     string
+		input    []Mount
+		expected []Mount
+	}{
+		{
+			desc: "remove volatile option from overlay mounts, ignore non overlay",
+			input: []Mount{
+				{
+					Type:   "overlay",
+					Source: "overlay",
+					Options: []string{
+						"index=off",
+						"workdir=/path/to/snapshots/4/work",
+						"upperdir=/path/to/snapshots/4/fs",
+						"lowerdir=/path/to/snapshots/1/fs",
+						"volatile",
+					},
+				},
+				{
+					Type:   "underlay",
+					Source: "underlay",
+					Options: []string{
+						"index=on",
+						"lowerdir=/another/path/to/snapshots/2/fs",
+						"volatile",
+					},
+				},
+			},
+			expected: []Mount{
+				{
+					Type:   "overlay",
+					Source: "overlay",
+					Options: []string{
+						"index=off",
+						"workdir=/path/to/snapshots/4/work",
+						"upperdir=/path/to/snapshots/4/fs",
+						"lowerdir=/path/to/snapshots/1/fs",
+					},
+				},
+				{
+					Type:   "underlay",
+					Source: "underlay",
+					Options: []string{
+						"index=on",
+						"lowerdir=/another/path/to/snapshots/2/fs",
+						"volatile",
+					},
+				},
+			},
+		},
+		{
+			desc: "return original slice since no volatile options on overlay",
+			input: []Mount{
+				{
+					Type:   "overlay",
+					Source: "overlay",
+					Options: []string{
+						"index=off",
+						"workdir=/path/to/snapshots/4/work",
+						"upperdir=/path/to/snapshots/4/fs",
+						"lowerdir=/path/to/snapshots/1/fs",
+					},
+				},
+				{
+					Type:   "underlay",
+					Source: "underlay",
+					Options: []string{
+						"index=on",
+						"lowerdir=/another/path/to/snapshots/2/fs",
+						"volatile",
+					},
+				},
+			},
+			expected: []Mount{
+				{
+					Type:   "overlay",
+					Source: "overlay",
+					Options: []string{
+						"index=off",
+						"workdir=/path/to/snapshots/4/work",
+						"upperdir=/path/to/snapshots/4/fs",
+						"lowerdir=/path/to/snapshots/1/fs",
+					},
+				},
+				{
+					Type:   "underlay",
+					Source: "underlay",
+					Options: []string{
+						"index=on",
+						"lowerdir=/another/path/to/snapshots/2/fs",
+						"volatile",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		original := copyMounts(tc.input)
+		actual := removeVolatileTempMount(tc.input)
+		if !reflect.DeepEqual(actual, tc.expected) {
+			t.Fatalf("incorrectly modified mounts: %s.\n\n Expected: %v\n\n, Actual: %v", tc.desc, tc.expected, actual)
+		}
+		if !reflect.DeepEqual(original, tc.input) {
+			t.Fatalf("modified original mounts: %s.\n\n Expected: %v\n\n, Actual: %v", tc.desc, original, tc.input)
+		}
+	}
+}
diff --git a/mount/temp.go b/mount/temp.go
@@ -28,6 +28,7 @@ var tempMountLocation = getTempDir()
 
 // WithTempMount mounts the provided mounts to a temp dir, and pass the temp dir to f.
 // The mounts are valid during the call to the f.
+// The volatile option of overlayfs doesn't allow to mount again using the same upper / work dirs. Since it's a temp mount, avoid using that option here if found.
 // Finally we will unmount and remove the temp dir regardless of the result of f.
 func WithTempMount(ctx context.Context, mounts []Mount, f func(root string) error) (err error) {
 	root, uerr := os.MkdirTemp(tempMountLocation, "containerd-mount")
@@ -58,7 +59,8 @@ func WithTempMount(ctx context.Context, mounts []Mount, f func(root string) erro
 			}
 		}
 	}()
-	if uerr = All(mounts, root); uerr != nil {
+
+	if uerr = All(removeVolatileTempMount(mounts), root); uerr != nil {
 		return fmt.Errorf("failed to mount %s: %w", root, uerr)
 	}
 	if err := f(root); err != nil {
@@ -67,6 +69,42 @@ func WithTempMount(ctx context.Context, mounts []Mount, f func(root string) erro
 	return nil
 }
 
+// removeVolatileTempMount The volatile option of overlayfs doesn't allow to mount again using the
+// same upper / work dirs. Since it's a temp mount, avoid using that
+// option here. Reference: https://docs.kernel.org/filesystems/overlayfs.html#volatile-mount
+// TODO: Make this logic conditional once the kernel supports reusing
+// overlayfs volatile mounts.
+func removeVolatileTempMount(mounts []Mount) []Mount {
+	var out []Mount
+	for i, m := range mounts {
+		if m.Type != "overlay" {
+			continue
+		}
+		for j, opt := range m.Options {
+			if opt == "volatile" {
+				if out == nil {
+					out = copyMounts(mounts)
+				}
+				out[i].Options = append(out[i].Options[:j], out[i].Options[j+1:]...)
+				break
+			}
+		}
+	}
+
+	if out != nil {
+		return out
+	}
+
+	return mounts
+}
+
+// copyMounts creates a copy of the original slice to allow for modification and not altering the original
+func copyMounts(in []Mount) []Mount {
+	out := make([]Mount, len(in))
+	copy(out, in)
+	return out
+}
+
 // WithReadonlyTempMount mounts the provided mounts to a temp dir as readonly,
 // and pass the temp dir to f. The mounts are valid during the call to the f.
 // Finally we will unmount and remove the temp dir regardless of the result of f.
