[7.0.x] loosen same OS requirement (#1484) (#1530)

* Loosen the same OS requirement in the following way: * consider OS distributions to be same if name and version are an exact match * consider OS distributions to be compatible if name is an exact match and versions match on prefix including the first dot * otherwise consider OS distributions to be incompatible Updates #1112.
gravitational · May 12, 2020 · 988d767 · 988d767
1 parent a12f831
commit 988d767
Show file tree

Hide file tree

Showing 2 changed files with 103 additions and 42 deletions.
diff --git a/lib/checks/checks.go b/lib/checks/checks.go
@@ -772,28 +772,58 @@ func checkRAM(info ServerInfo, ram schema.RAM) error {
 	return nil
 }
 
-// checkSameOS makes sure all servers have the same OS/version
+// checkSameOS verifies the OS distribution requirement for the specified set of servers.
+// The check will pass if all nodes in the cluster are based on the same OS distribution and major version.
+// Variance in minor/patch versions is acceptable.
 func checkSameOS(servers []Server) error {
-	osToNodes := make(map[string][]string)
+	// distros maps distribution name to list of versions
+	distros := make(map[string][]string)
 	for _, server := range servers {
-		os := systeminfo.OS(server.GetOS()).Name()
-		osToNodes[os] = append(osToNodes[os], fmt.Sprintf("%v (%v)",
-			server.ServerInfo.GetHostname(), server.AdvertiseAddr))
+		info := server.GetOS()
+		distros[info.ID] = append(distros[info.ID], info.Version)
 	}
+	if len(distros) != 1 {
+		return trace.BadParameter("servers have different OS distributions: %v", formatKeysAsList(distros))
+	}
+	// Version verification is purposely simply and will compare the prefixes
+	// up to to either the first '.' or end of line
+	for _, versions := range distros {
+		if !verifyCommonVersionPrefix(versions...) {
+			return trace.BadParameter("servers have different OS versions: %v", formatAsList(distros))
+		}
+	}
+	log.Infof("Servers passed check for same OS: %v.", formatAsList(distros))
+	return nil
+}
 
-	if len(osToNodes) > 1 {
-		var formatted []string
-		for os, nodes := range osToNodes {
-			formatted = append(formatted, fmt.Sprintf(
-				"%v: %v", os, strings.Join(nodes, ", ")))
+func verifyCommonVersionPrefix(versions ...string) bool {
+	if len(versions) <= 1 {
+		return true
+	}
+	for i := 0; i < len(versions)-1; i += 1 {
+		if !strings.EqualFold(
+			strings.Split(versions[i], ".")[0],
+			strings.Split(versions[i+1], ".")[0]) {
+			return false
 		}
-		return trace.BadParameter(
-			"servers have different OSes/versions:\n%v",
-			strings.Join(formatted, "\n"))
 	}
+	return true
+}
+
+func formatAsList(m map[string][]string) (result []string) {
+	result = make([]string, 0, len(m))
+	for k, v := range m {
+		result = append(result, fmt.Sprintf("%v (%v)", k, v))
+	}
+	return result
+}
 
-	log.Infof("Servers passed check for the same OS: %v.", osToNodes)
-	return nil
+func formatKeysAsList(m map[string][]string) (result []string) {
+	result = make([]string, 0, len(m))
+	for k := range m {
+		result = append(result, k)
+	}
+	return result
 }
 
 // checkTime checks if time it out of sync between servers

diff --git a/lib/checks/checks_test.go b/lib/checks/checks_test.go
@@ -118,41 +118,72 @@ func (s *ChecksSuite) TestTime(c *check.C) {
 }
 
 func (s *ChecksSuite) TestCheckSameOS(c *check.C) {
-	infos := []Server{
+	var testCases = []struct {
+		comment string
+		servers []Server
+		err     string
+	}{
 		{
-			ServerInfo: ServerInfo{
-				System: storage.NewSystemInfo(storage.SystemSpecV2{
-					Hostname: "node-1",
-					OS: storage.OSInfo{
-						ID:      "centos",
-						Version: "7.1",
-					},
-				}),
+			comment: "servers match on the major version",
+			servers: []Server{
+				newServer("node-1", "centos", "7.1"),
+				newServer("node-2", "centos", "7.2"),
+				newServer("node-3", "centos", "7.2"),
 			},
 		},
 		{
-			ServerInfo: ServerInfo{
-				System: storage.NewSystemInfo(storage.SystemSpecV2{
-					Hostname: "node-2",
-					OS: storage.OSInfo{
-						ID:      "centos",
-						Version: "7.2",
-					},
-				}),
+			comment: "servers do not match on the major version",
+			servers: []Server{
+				newServer("node-1", "centos", "7.1"),
+				newServer("node-2", "centos", "6.2"),
+				newServer("node-3", "centos", "7.2"),
 			},
+			err: "servers have different OS versions.*",
 		},
 		{
-			ServerInfo: ServerInfo{
-				System: storage.NewSystemInfo(storage.SystemSpecV2{
-					Hostname: "node-3",
-					OS: storage.OSInfo{
-						ID:      "centos",
-						Version: "7.2",
-					},
-				}),
+			comment: "match on the whole version",
+			servers: []Server{
+				newServer("node-1", "centos", "7"),
+				newServer("node-2", "centos", "7"),
 			},
 		},
+		{
+			comment: "no match on the whole version",
+			servers: []Server{
+				newServer("node-1", "centos", "7"),
+				newServer("node-2", "centos", "6"),
+			},
+			err: "servers have different OS versions.*",
+		},
+		{
+			comment: "no match on distribution",
+			servers: []Server{
+				newServer("node-1", "centos", "7"),
+				newServer("node-2", "rhel", "7"),
+			},
+			err: "servers have different OS distributions.*",
+		},
+	}
+	for _, tc := range testCases {
+		comment := check.Commentf(tc.comment)
+		if tc.err != "" {
+			c.Assert(checkSameOS(tc.servers), check.ErrorMatches, tc.err, comment)
+		} else {
+			c.Assert(checkSameOS(tc.servers), check.IsNil, comment)
+		}
+	}
+}
+
+func newServer(hostname, os, version string) Server {
+	return Server{
+		ServerInfo: ServerInfo{
+			System: storage.NewSystemInfo(storage.SystemSpecV2{
+				Hostname: hostname,
+				OS: storage.OSInfo{
+					ID:      os,
+					Version: version,
+				},
+			}),
+		},
 	}
-	c.Assert(checkSameOS(infos[:2]), check.NotNil)
-	c.Assert(checkSameOS(infos[1:]), check.IsNil)
 }