cmd/runtimetest/main.go: Unify validateMountsExist and validateMountsOrder

wking · wking · commit bcb5379855cd · 2017-08-31T11:32:47.000-07:00
Also increase the error message detail and continue through the remaining mounts instead of breaking on the first missing/misordered mount. Based on previous discussion in [1,2]. With this commit, a configuration like: "mounts": [ { "destination": "/tmp", "type": "tmpfs", "source": "none" }, { "destination": "/tmp", "type": "tmpfs", "source": "none" }, { "destination": "/dev", "type": "devtmpfs", "source": "devtmpfs" } ] and mountinfo like: $ grep -n '/dev \|/tmp ' /proc/self/mountinfo 2:19 17 0:6 / /dev rw,nosuid,relatime - devtmpfs devtmpfs rw,size=10240k,nr_inodes=2043951,mode=755 25:41 17 0:38 / /tmp rw,relatime - tmpfs none rw will generate errors like: * mounts[1] {/tmp tmpfs none []} does not exist * mounts[2] {/dev devtmpfs devtmpfs []} mounted before mounts[0] {/tmp tmpfs none []} Before this commit, the error was just: * Mounts[1] /tmp is not mounted in order I'd prefer errors like: * mounts[1] {/tmp tmpfs none []} does not exist * mounts[2] {/dev devtmpfs devtmpfs []} is system mount 1, while mounts[0] {/tmp tmpfs none []} is system mount 24 where grep reports 2 and 25 because it's counting from one, and runtimetest reports 1 and 24 because it's counting from zero, but Ma prefers to not mention the system-mount order [3]. [1]: #444 (comment) [2]: #444 (comment) [3]: #456 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
diff --git a/cmd/runtimetest/main.go b/cmd/runtimetest/main.go
@@ -576,113 +576,91 @@ func validateGIDMappings(spec *rspec.Spec) error {
 	return validateIDMappings(spec.Linux.GIDMappings, "/proc/self/gid_map", "linux.gidMappings")
 }
 
-func mountMatch(configMount rspec.Mount, sysMount rspec.Mount) error {
-	if filepath.Clean(configMount.Destination) != sysMount.Destination {
-		return fmt.Errorf("mount destination expected: %v, actual: %v", configMount.Destination, sysMount.Destination)
+func mountMatch(configMount rspec.Mount, sysMount *mount.Info) error {
+	sys := rspec.Mount{
+		Destination: sysMount.Mountpoint,
+		Type:        sysMount.Fstype,
+		Source:      sysMount.Source,
 	}
 
-	if configMount.Type != sysMount.Type {
-		return fmt.Errorf("mount %v type expected: %v, actual: %v", configMount.Destination, configMount.Type, sysMount.Type)
+	if filepath.Clean(configMount.Destination) != sys.Destination {
+		return fmt.Errorf("mount destination expected: %v, actual: %v", configMount.Destination, sys.Destination)
 	}
 
-	if filepath.Clean(configMount.Source) != sysMount.Source {
-		return fmt.Errorf("mount %v source expected: %v, actual: %v", configMount.Destination, configMount.Source, sysMount.Source)
+	if configMount.Type != sys.Type {
+		return fmt.Errorf("mount %v type expected: %v, actual: %v", configMount.Destination, configMount.Type, sys.Type)
+	}
+
+	if filepath.Clean(configMount.Source) != sys.Source {
+		return fmt.Errorf("mount %v source expected: %v, actual: %v", configMount.Destination, configMount.Source, sys.Source)
 	}
 
 	return nil
 }
 
-func validateMountsExist(spec *rspec.Spec) error {
+func validateMounts(spec *rspec.Spec) error {
+	if runtime.GOOS == "windows" {
+		logrus.Warnf("mounts validation not yet implemented for OS %q", runtime.GOOS)
+		return nil
+	}
+
 	mountInfos, err := mount.GetMounts()
 	if err != nil {
 		return err
 	}
 
-	mountsMap := make(map[string][]rspec.Mount)
-	for _, mountInfo := range mountInfos {
-		m := rspec.Mount{
-			Destination: mountInfo.Mountpoint,
-			Type:        mountInfo.Fstype,
-			Source:      mountInfo.Source,
-		}
-		mountsMap[mountInfo.Mountpoint] = append(mountsMap[mountInfo.Mountpoint], m)
-	}
-
-	for _, configMount := range spec.Mounts {
+	var mountErrs error
+	var consumedSys = make(map[int]bool)
+	highestMatchedConfig := -1
+	highestMatchedSystem := -1
+	var j = 0
+	for i, configMount := range spec.Mounts {
 		if configMount.Type == "bind" || configMount.Type == "rbind" {
 			// TODO: add bind or rbind check.
 			continue
 		}
 
 		found := false
-		for _, sysMount := range mountsMap[filepath.Clean(configMount.Destination)] {
+		for k, sysMount := range mountInfos[j:] {
 			if err := mountMatch(configMount, sysMount); err == nil {
 				found = true
+				j += k + 1
+				consumedSys[j-1] = true
+				if j > highestMatchedSystem {
+					highestMatchedSystem = j - 1
+					highestMatchedConfig = i
+				}
 				break
 			}
 		}
 		if !found {
-			return fmt.Errorf("Expected mount %v does not exist", configMount)
-		}
-	}
-
-	return nil
-}
-
-func validateMountsOrder(spec *rspec.Spec) error {
-	if runtime.GOOS == "windows" {
-		logrus.Warnf("mounts order validation not yet implemented for OS %q", runtime.GOOS)
-		return nil
-	}
-
-	mountInfos, err := mount.GetMounts()
-	if err != nil {
-		return err
-	}
-
-	type mountOrder struct {
-		Order  int
-		Root   string
-		Dest   string
-		Source string
-	}
-	mountsMap := make(map[string][]mountOrder)
-	for i, mountInfo := range mountInfos {
-		m := mountOrder{
-			Order:  i,
-			Root:   mountInfo.Root,
-			Dest:   mountInfo.Mountpoint,
-			Source: mountInfo.Source,
-		}
-		mountsMap[mountInfo.Mountpoint] = append(mountsMap[mountInfo.Mountpoint], m)
-	}
-	current := -1
-	for i, configMount := range spec.Mounts {
-		mounts := mountsMap[configMount.Destination]
-		if len(mounts) == 0 {
-			return fmt.Errorf("Mounts[%d] %s is not mounted in order", i, configMount.Destination)
-		}
-		for j, mount := range mounts {
-			source := mount.Source
-			for _, option := range configMount.Options {
-				if option == "bind" || option == "rbind" {
-					source = mount.Root
-					break
+			if j > 0 {
+				for k, sysMount := range mountInfos[:j-1] {
+					if _, ok := consumedSys[k]; ok {
+						continue
+					}
+					if err := mountMatch(configMount, sysMount); err == nil {
+						found = true
+						break
+					}
 				}
 			}
-			if source == configMount.Source {
-				if current > mount.Order {
-					return fmt.Errorf("Mounts[%d] %s is not mounted in order", i, configMount.Destination)
-				}
-				current = mount.Order
-				// in order to deal with dup mount elements
-				mountsMap[configMount.Destination] = append(mountsMap[configMount.Destination][:j], mountsMap[configMount.Destination][j+1:]...)
-				break
+			if found {
+				mountErrs = multierror.Append(
+					mountErrs,
+					fmt.Errorf(
+						"mounts[%d] %v mounted before mounts[%d] %v",
+						i,
+						configMount,
+						highestMatchedConfig,
+						spec.Mounts[highestMatchedConfig]))
+			} else {
+				mountErrs = multierror.Append(mountErrs, fmt.Errorf("mounts[%d] %v does not exist", i, configMount))
 			}
 		}
 	}
 
-	return nil
+	return mountErrs
 }
 
 func run(context *cli.Context) error {
@@ -711,13 +689,9 @@ func run(context *cli.Context) error {
 			description: "hostname",
 		},
 		{
-			test:        validateMountsExist,
+			test:        validateMounts,
 			description: "mounts",
 		},
-		{
-			test:        validateMountsOrder,
-			description: "mounts order",
-		},
 	}
 
 	linuxValidations := []validation{
