Merge pull request #1311 from jcechace/PBM-1675-incompatible-bcps

PBM-1675 Fix backup compatibility checks

Author: jcechace

cmd/pbm/backup.go

@@ -577,25 +577,50 @@ func bcpsMatchCluster(
 	ver string,
 	fcv string,
 	shards []topo.Shard,
-	confsrv string,
 	rsMap map[string]string,
 ) {
-	sh := make(map[string]bool, len(shards))
-	for _, s := range shards {
-		sh[s.RS] = s.RS == confsrv
-	}
+	sh := backupRequiredRSMap(shards)
 
 	mapRS, mapRevRS := util.MakeRSMapFunc(rsMap), util.MakeReverseRSMapFunc(rsMap)
 	for i := 0; i < len(bcps); i++ {
 		bcpMatchCluster(&bcps[i], ver, fcv, sh, mapRS, mapRevRS)
 	}
 }
 
+// backupRequiredRSMap marks the RS whose absence makes the backup incompatible:
+// the config RS for sharded clusters, or the sole RS otherwise.
+func backupRequiredRSMap(shards []topo.Shard) map[string]bool {
+	sh := make(map[string]bool, len(shards))
+	requiredRS := backupRequiredRSName(shards)
+
+	for _, s := range shards {
+		sh[s.RS] = s.RS == requiredRS
+	}
+
+	return sh
+}
+
+// backupRequiredRSName returns the RS whose absence makes the backup incompatible:
+// the config RS for sharded clusters, or the sole RS otherwise.
+func backupRequiredRSName(shards []topo.Shard) string {
+	if len(shards) == 1 {
+		return shards[0].RS
+	}
+
+	for _, s := range shards {
+		if s.ID == "config" {
+			return s.RS
+		}
+	}
+
+	return ""
+}
+
 func bcpMatchCluster(
 	bcp *backup.BackupMeta,
 	ver string,
 	fcv string,
-	shards map[string]bool,
+	rs map[string]bool,
 	mapRS util.RSMapFunc,
 	mapRevRS util.RSMapFunc,
 ) {
@@ -618,26 +643,26 @@ func bcpMatchCluster(
 	}
 
 	var nomatch []string
-	hasconfsrv := false
+	hasMandatoryRS := false
 	for i := range bcp.Replsets {
 		name := mapRS(bcp.Replsets[i].Name)
 
-		isconfsrv, ok := shards[name]
+		isMandatoryRS, ok := rs[name]
 		if !ok {
 			nomatch = append(nomatch, name)
 		} else if mapRevRS(name) != bcp.Replsets[i].Name {
 			nomatch = append(nomatch, name)
 		}
 
-		if isconfsrv {
-			hasconfsrv = true
+		if isMandatoryRS {
+			hasMandatoryRS = true
 		}
 	}
 
-	if len(nomatch) != 0 || !hasconfsrv {
+	if len(nomatch) != 0 || !hasMandatoryRS {
 		names := make([]string, len(nomatch))
 		copy(names, nomatch)
-		bcp.SetRuntimeError(missedReplsetsError{names: names, configsrv: !hasconfsrv})
+		bcp.SetRuntimeError(missedReplsetsError{names: names, configsrv: !hasMandatoryRS})
 	}
 }
 

cmd/pbm/backup_test.go

@@ -20,14 +20,12 @@ func TestBcpMatchCluster(t *testing.T) {
 		expect defs.Status
 	}
 	cases := []struct {
-		confsrv string
-		shards  []topo.Shard
-		bcps    []bcase
+		shards []topo.Shard
+		bcps   []bcase
 	}{
 		{
-			confsrv: "config",
 			shards: []topo.Shard{
-				{RS: "config"},
+				{ID: "config", RS: "config"},
 				{RS: "rs1"},
 				{RS: "rs2"},
 			},
@@ -104,7 +102,6 @@ func TestBcpMatchCluster(t *testing.T) {
 			},
 		},
 		{
-			confsrv: "rs1",
 			shards: []topo.Shard{
 				{RS: "rs1"},
 			},
@@ -171,7 +168,7 @@ func TestBcpMatchCluster(t *testing.T) {
 				b.meta.Status = defs.StatusDone
 				m = append(m, b.meta)
 			}
-			bcpsMatchCluster(m, "", "", c.shards, c.confsrv, nil)
+			bcpsMatchCluster(m, "", "", c.shards, nil)
 			for i := 0; i < len(c.bcps); i++ {
 				if c.bcps[i].expect != m[i].Status {
 					t.Errorf("wrong status for %s, expect %s, got %s", m[i].Name, c.bcps[i].expect, m[i].Status)
@@ -316,6 +313,31 @@ func TestBcpMatchRemappedCluster(t *testing.T) {
 	}
 }
 
+func TestBackupRequiredRSMap(t *testing.T) {
+	t.Run("sharded cluster uses config shard rs", func(t *testing.T) {
+		sh := backupRequiredRSMap([]topo.Shard{
+			{ID: "config", RS: "cfg"},
+			{ID: "rs1", RS: "rs1"},
+			{ID: "rs2", RS: "rs2"},
+		})
+
+		if !sh["cfg"] {
+			t.Fatal("expected config RS to be marked as required")
+		}
+		if sh["rs1"] || sh["rs2"] {
+			t.Fatal("expected only config RS to be marked as required")
+		}
+	})
+
+	t.Run("sole rs cluster uses only replset", func(t *testing.T) {
+		sh := backupRequiredRSMap([]topo.Shard{{RS: "rs1"}})
+
+		if !sh["rs1"] {
+			t.Fatal("expected sole RS to be marked as required")
+		}
+	})
+}
+
 func checkBcpMatchClusterError(err, target error) string {
 	if err == nil && target == nil {
 		return ""
@@ -354,7 +376,7 @@ func checkBcpMatchClusterError(err, target error) string {
 
 func BenchmarkBcpMatchCluster3x10(b *testing.B) {
 	shards := []topo.Shard{
-		{RS: "config"},
+		{ID: "config", RS: "config"},
 		{RS: "rs1"},
 		{RS: "rs2"},
 	}
@@ -374,13 +396,13 @@ func BenchmarkBcpMatchCluster3x10(b *testing.B) {
 	}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }
 
 func BenchmarkBcpMatchCluster3x100(b *testing.B) {
 	shards := []topo.Shard{
-		{RS: "config"},
+		{ID: "config", RS: "config"},
 		{RS: "rs1"},
 		{RS: "rs2"},
 	}
@@ -400,13 +422,13 @@ func BenchmarkBcpMatchCluster3x100(b *testing.B) {
 	}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }
 
 func BenchmarkBcpMatchCluster17x100(b *testing.B) {
 	shards := []topo.Shard{
-		{RS: "config"},
+		{ID: "config", RS: "config"},
 		{RS: "rs1"},
 		{RS: "rs12"},
 		{RS: "rs112"},
@@ -454,13 +476,13 @@ func BenchmarkBcpMatchCluster17x100(b *testing.B) {
 	}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }
 
 func BenchmarkBcpMatchCluster3x1000(b *testing.B) {
 	shards := []topo.Shard{
-		{RS: "config"},
+		{ID: "config", RS: "config"},
 		{RS: "rs1"},
 		{RS: "rs2"},
 	}
@@ -480,12 +502,12 @@ func BenchmarkBcpMatchCluster3x1000(b *testing.B) {
 	}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }
 
 func BenchmarkBcpMatchCluster1000x1000(b *testing.B) {
-	shards := []topo.Shard{{RS: "config"}}
+	shards := []topo.Shard{{ID: "config", RS: "config"}}
 	rss := []backup.BackupReplset{{Name: "config"}}
 
 	for i := 0; i < 1000; i++ {
@@ -503,13 +525,13 @@ func BenchmarkBcpMatchCluster1000x1000(b *testing.B) {
 	}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }
 
 func BenchmarkBcpMatchCluster3x10Err(b *testing.B) {
 	shards := []topo.Shard{
-		{RS: "config"},
+		{ID: "config", RS: "config"},
 		{RS: "rs2"},
 	}
 
@@ -543,12 +565,12 @@ func BenchmarkBcpMatchCluster3x10Err(b *testing.B) {
 		})
 	}
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }
 
 func BenchmarkBcpMatchCluster1000x1000Err(b *testing.B) {
-	shards := []topo.Shard{{RS: "config"}}
+	shards := []topo.Shard{{ID: "config", RS: "config"}}
 	rss := []backup.BackupReplset{{Name: "config"}}
 
 	for i := 0; i < 1000; i++ {
@@ -567,6 +589,6 @@ func BenchmarkBcpMatchCluster1000x1000Err(b *testing.B) {
 	}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		bcpsMatchCluster(bcps, "", "", shards, "config", nil)
+		bcpsMatchCluster(bcps, "", "", shards, nil)
 	}
 }

cmd/pbm/list.go

@@ -338,11 +338,6 @@ func getSnapshotList(
 		return nil, errors.Wrap(err, "get cluster members")
 	}
 
-	inf, err := topo.GetNodeInfoExt(ctx, conn.MongoClient())
-	if err != nil {
-		return nil, errors.Wrap(err, "define cluster state")
-	}
-
 	ver, err := version.GetMongoVersion(ctx, conn.MongoClient())
 	if err != nil {
 		return nil, errors.Wrap(err, "get mongo version")
@@ -352,9 +347,7 @@ func getSnapshotList(
 		return nil, errors.Wrap(err, "get featureCompatibilityVersion")
 	}
 
-	// PBM agent is always connected either to config server or to the sole (hence main) RS
-	// which the `confsrv` param in `bcpMatchCluster` is all about
-	bcpsMatchCluster(bcps, ver.VersionString, fcv, shards, inf.SetName, rsMap)
+	bcpsMatchCluster(bcps, ver.VersionString, fcv, shards, rsMap)
 
 	var s []snapshotStat
 	for i := len(bcps) - 1; i >= 0; i-- {
@@ -389,11 +382,6 @@ func getPitrList(
 	unbacked bool,
 	rsMap map[string]string,
 ) ([]pitrRange, map[string][]pitrRange, error) {
-	inf, err := topo.GetNodeInfoExt(ctx, conn.MongoClient())
-	if err != nil {
-		return nil, nil, errors.Wrap(err, "define cluster state")
-	}
-
 	shards, err := topo.ClusterMembers(ctx, conn.MongoClient())
 	if err != nil {
 		return nil, nil, errors.Wrap(err, "get cluster members")
@@ -431,10 +419,7 @@ func getPitrList(
 		rstlines = append(rstlines, tlns)
 	}
 
-	sh := make(map[string]bool, len(shards))
-	for _, s := range shards {
-		sh[s.RS] = s.RS == inf.SetName
-	}
+	sh := backupRequiredRSMap(shards)
 
 	ranges := []pitrRange{}
 	for _, tl := range oplog.MergeTimelines(rstlines...) {

cmd/pbm/status.go

@@ -601,9 +601,7 @@ func getStorageStat(
 		return s, errors.Wrap(err, "get cluster members")
 	}
 
-	// pbm.PBM is always connected either to config server or to the sole (hence main) RS
-	// which the `confsrv` param in `bcpMatchCluster` is all about
-	bcpsMatchCluster(bcps, ver.VersionString, fcv, shards, inf.SetName, rsMap)
+	bcpsMatchCluster(bcps, ver.VersionString, fcv, shards, rsMap)
 
 	stg, err := util.GetStorage(ctx, conn, inf.Me,
 		log.FromContext(ctx).NewEvent("", "", "", primitive.Timestamp{}))