Correct updated return value in UpdateVolumeAnnotations.

`UpdateVolumeAnnotations` was returning `updated = true` even when a
mount type was already correct. This avoids unnecessarily overwriting the spec
when there are no changes. Also simplify tests.

PiperOrigin-RevId: 766710817

Author: ayushr2

pkg/shim/v1/utils/BUILD

@@ -27,5 +27,8 @@ go_test(
     size = "small",
     srcs = ["volumes_test.go"],
     library = ":utils",
-    deps = ["@com_github_opencontainers_runtime_spec//specs-go:go_default_library"],
+    deps = [
+        "@com_github_mohae_deepcopy//:go_default_library",
+        "@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
+    ],
 )

pkg/shim/v1/utils/volumes.go

@@ -195,8 +195,9 @@ func UpdateVolumeAnnotations(s *specs.Spec) (bool, error) {
 					}
 					// Container mount type must match the mount type specified by
 					// admission controller. See note about EmptyDir.
-					specutils.ChangeMountType(&s.Mounts[i], v)
-					updated = true
+					if specutils.ChangeMountType(&s.Mounts[i], v) {
+						updated = true
+					}
 				}
 			}
 		}
@@ -275,14 +276,17 @@ func configureShm(s *specs.Spec) (bool, error) {
 				// inside the sandbox anyways) and apply options to subcontainers as
 				// they bind mount individually.
 				s.Annotations[volumeKeyPrefix+devshmName+".options"] = "rw"
+				updated = true
 			}
 
-			specutils.ChangeMountType(m, devshmType)
-			updated = true
+			if specutils.ChangeMountType(m, devshmType) {
+				updated = true
+			}
 
 			// Remove the duplicate entry now that we found the shared /dev/shm mount.
 			if duplicate >= 0 {
 				s.Mounts = append(s.Mounts[:duplicate], s.Mounts[duplicate+1:]...)
+				updated = true
 			}
 			break
 		}

pkg/shim/v1/utils/volumes_test.go

@@ -20,6 +20,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/mohae/deepcopy"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
@@ -52,11 +53,10 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 	}
 
 	for _, test := range []struct {
-		name         string
-		spec         *specs.Spec
-		expected     *specs.Spec
-		expectErr    bool
-		expectUpdate bool
+		name      string
+		spec      *specs.Spec
+		expected  *specs.Spec // If nil, the spec is expected to be unchanged.
+		expectErr bool
 	}{
 		{
 			name: "volume annotations for sandbox",
@@ -79,7 +79,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testVolumeName + ".source":  testVolumePath,
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "volume annotations for sandbox with legacy log path",
@@ -102,7 +101,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testVolumeName + ".source":  testVolumePath,
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "tmpfs: volume annotations for container",
@@ -150,7 +148,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testVolumeName + ".options": "ro",
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "non-empty volume for sandbox",
@@ -173,7 +170,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testNonEmptyVolumeName + ".source":  testNonEmptyVolumePath,
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "non-empty volume for container",
@@ -193,22 +189,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testNonEmptyVolumeName + ".options": "ro",
 				},
 			},
-			expected: &specs.Spec{
-				Mounts: []specs.Mount{
-					{
-						Destination: "/test",
-						Type:        "bind",
-						Source:      testNonEmptyVolumePath,
-						Options:     []string{"ro"},
-					},
-				},
-				Annotations: map[string]string{
-					ContainerTypeAnnotation:                               ContainerTypeContainer,
-					volumeKeyPrefix + testNonEmptyVolumeName + ".share":   "pod",
-					volumeKeyPrefix + testNonEmptyVolumeName + ".type":    "tmpfs",
-					volumeKeyPrefix + testNonEmptyVolumeName + ".options": "ro",
-				},
-			},
 		},
 		{
 			name: "bind: volume annotations for sandbox",
@@ -231,7 +211,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testVolumeName + ".source":  testVolumePath,
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "bind: volume annotations for container",
@@ -251,23 +230,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testVolumeName + ".options": "ro",
 				},
 			},
-			expected: &specs.Spec{
-				Mounts: []specs.Mount{
-					{
-						Destination: "/test",
-						Type:        "bind",
-						Source:      testVolumePath,
-						Options:     []string{"ro"},
-					},
-				},
-				Annotations: map[string]string{
-					ContainerTypeAnnotation:                       ContainerTypeContainer,
-					volumeKeyPrefix + testVolumeName + ".share":   "container",
-					volumeKeyPrefix + testVolumeName + ".type":    "bind",
-					volumeKeyPrefix + testVolumeName + ".options": "ro",
-				},
-			},
-			expectUpdate: true,
 		},
 		{
 			name: "should not return error without pod log directory",
@@ -279,14 +241,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					volumeKeyPrefix + testVolumeName + ".options": "ro",
 				},
 			},
-			expected: &specs.Spec{
-				Annotations: map[string]string{
-					ContainerTypeAnnotation:                       containerTypeSandbox,
-					volumeKeyPrefix + testVolumeName + ".share":   "pod",
-					volumeKeyPrefix + testVolumeName + ".type":    "tmpfs",
-					volumeKeyPrefix + testVolumeName + ".options": "ro",
-				},
-			},
 		},
 		{
 			name: "should return error if volume path does not exist",
@@ -309,12 +263,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					ContainerTypeAnnotation: containerTypeSandbox,
 				},
 			},
-			expected: &specs.Spec{
-				Annotations: map[string]string{
-					sandboxLogDirAnnotation: testLogDirPath,
-					ContainerTypeAnnotation: containerTypeSandbox,
-				},
-			},
 		},
 		{
 			name: "no volume annotations for container",
@@ -337,25 +285,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					ContainerTypeAnnotation: ContainerTypeContainer,
 				},
 			},
-			expected: &specs.Spec{
-				Mounts: []specs.Mount{
-					{
-						Destination: "/test",
-						Type:        "bind",
-						Source:      "/test",
-						Options:     []string{"ro"},
-					},
-					{
-						Destination: "/random",
-						Type:        "bind",
-						Source:      "/random",
-						Options:     []string{"ro"},
-					},
-				},
-				Annotations: map[string]string{
-					ContainerTypeAnnotation: ContainerTypeContainer,
-				},
-			},
 		},
 		{
 			name: "bind options removed",
@@ -393,7 +322,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					},
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "shm-sandbox",
@@ -429,7 +357,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					},
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "shm-container",
@@ -459,7 +386,6 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					},
 				},
 			},
-			expectUpdate: true,
 		},
 		{
 			name: "shm-duplicate",
@@ -505,10 +431,13 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 					},
 				},
 			},
-			expectUpdate: true,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
+			expectUpdate := test.expected != nil
+			if test.expected == nil && !test.expectErr {
+				test.expected = deepcopy.Copy(test.spec).(*specs.Spec)
+			}
 			updated, err := UpdateVolumeAnnotations(test.spec)
 			if test.expectErr {
 				if err == nil {
@@ -519,8 +448,8 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
 			if err != nil {
 				t.Fatalf("UpdateVolumeAnnotations(spec): %v", err)
 			}
-			if test.expectUpdate != updated {
-				t.Errorf("want: %v, got: %v", test.expectUpdate, updated)
+			if expectUpdate != updated {
+				t.Errorf("want: %v, got: %v", expectUpdate, updated)
 			}
 			if !reflect.DeepEqual(test.expected, test.spec) {
 				t.Fatalf("want: %+v, got: %+v", test.expected, test.spec)

runsc/specutils/specutils.go

@@ -301,9 +301,13 @@ func ReadMounts(f *os.File) ([]specs.Mount, error) {
 }
 
 // ChangeMountType changes m.Type to the specified type. It may do necessary
-// amends to m.Options.
-func ChangeMountType(m *specs.Mount, newType string) {
-	m.Type = newType
+// amends to m.Options. It returns true if m was modified.
+func ChangeMountType(m *specs.Mount, newType string) bool {
+	updated := false
+	if m.Type != newType {
+		m.Type = newType
+		updated = true
+	}
 
 	// OCI spec allows bind mounts to be specified in options only. So if new type
 	// is not bind, remove bind/rbind from options.
@@ -315,10 +319,13 @@ func ChangeMountType(m *specs.Mount, newType string) {
 		for _, opt := range m.Options {
 			if opt != "rbind" && opt != "bind" {
 				newOpts = append(newOpts, opt)
+			} else {
+				updated = true
 			}
 		}
 		m.Options = newOpts
 	}
+	return updated
 }
 
 // Capabilities takes in spec and returns a TaskCapabilities corresponding to