K8SPSMDB-1031: Fix live member calculation with external arbiter

pkg/apis/psmdb/v1/psmdb_types.go

@@ -112,7 +112,7 @@ type PerconaServerMongoDBSpec struct {
 
 type DefaultRWConcern struct {
 	// +kubebuilder:validation:Enum={local,available,majority}
-	ReadConcern  string                  `json:"readConcern,omitempty"`
+	ReadConcern  string                   `json:"readConcern,omitempty"`
 	WriteConcern *DefaultWriteConcernSpec `json:"writeConcern,omitempty"`
 }
 
@@ -645,6 +645,10 @@ type ExternalNode struct {
 }
 
 func (e *ExternalNode) HostPort() string {
+	if strings.Contains(e.Host, ":") {
+		return e.Host
+	}
+
 	return e.Host + ":" + strconv.Itoa(e.Port)
 }
 

pkg/apis/psmdb/v1/psmdb_types_test.go

@@ -648,3 +648,29 @@ func TestCanRestore(t *testing.T) {
 		})
 	}
 }
+
+func TestExternalNode_HostPort(t *testing.T) {
+	tests := map[string]struct {
+		node     ExternalNode
+		expected string
+	}{
+		"host and port set": {
+			node:     ExternalNode{Host: "mongodb.example.com", Port: 27017},
+			expected: "mongodb.example.com:27017",
+		},
+		"host already contains port": {
+			node:     ExternalNode{Host: "mongodb.example.com:27018", Port: 27017},
+			expected: "mongodb.example.com:27018",
+		},
+		"IPv4 host with port": {
+			node:     ExternalNode{Host: "10.0.0.1", Port: 27017},
+			expected: "10.0.0.1:27017",
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, tt.node.HostPort())
+		})
+	}
+}

pkg/controller/perconaservermongodb/mgo.go

@@ -370,11 +370,7 @@ func (r *ReconcilePerconaServerMongoDB) getConfigMemberForExternalNode(id int, e
 		member.Tags = mongo.ReplsetTags{"external": "true"}
 	}
 
-	if strings.Contains(extNode.Host, ":") {
-		member.Host = extNode.Host
-	} else {
-		member.Host = extNode.HostPort()
-	}
+	member.Host = extNode.HostPort()
 
 	for k, v := range extNode.Tags {
 		if member.Tags == nil {
@@ -540,22 +536,37 @@ func (r *ReconcilePerconaServerMongoDB) updateConfigMembers(ctx context.Context,
 		return rsMembers, 0, errors.Wrap(err, "unable to get replset members")
 	}
 
-	liveMembers := 0
+	liveMembers := countLiveMembers(rsStatus, cnf, rs, rsMembers)
+
+	return rsMembers, liveMembers, nil
+}
+
+// countLiveMembers counts the members reported as live (primary, secondary or
+// arbiter) in the replset status, ignoring external members and external
+// arbiters. It also populates rsMembers with the status of every member that
+// maps to an operator-managed pod (identified by the podName tag).
+func countLiveMembers(rsStatus mongo.Status, cnf mongo.RSConfig, rs *api.ReplsetSpec, rsMembers map[string]api.ReplsetMemberStatus) int {
+	count := 0
 	for _, member := range rsStatus.Members {
-		var tags mongo.ReplsetTags
+		var cm mongo.ConfigMember
 
 		for i := range cnf.Members {
 			if member.Id == cnf.Members[i].ID {
-				tags = cnf.Members[i].Tags
+				cm = cnf.Members[i]
 				break
 			}
 		}
 
-		if _, ok := tags["external"]; ok {
+		if _, ok := cm.Tags["external"]; ok {
 			continue
 		}
 
-		if podName, ok := tags["podName"]; ok {
+		// arbiters can't have tags
+		if cm.ArbiterOnly && isExternalArbiter(cm, rs) {
+			continue
+		}
+
+		if podName, ok := cm.Tags["podName"]; ok {
 			rsMembers[podName] = api.ReplsetMemberStatus{
 				Name:     member.Name,
 				State:    member.State,
@@ -565,11 +576,25 @@ func (r *ReconcilePerconaServerMongoDB) updateConfigMembers(ctx context.Context,
 
 		switch member.State {
 		case mongo.MemberStatePrimary, mongo.MemberStateSecondary, mongo.MemberStateArbiter:
-			liveMembers++
+			count++
 		}
 	}
 
-	return rsMembers, liveMembers, nil
+	return count
+}
+
+func isExternalArbiter(cm mongo.ConfigMember, rs *api.ReplsetSpec) bool {
+	for _, extNode := range rs.ExternalNodes {
+		if !extNode.ArbiterOnly {
+			continue
+		}
+
+		if cm.Host == extNode.HostPort() {
+			return true
+		}
+	}
+
+	return false
 }
 
 func inShard(ctx context.Context, client mongo.Client, rsName string) (bool, error) {

pkg/controller/perconaservermongodb/mgo_test.go

@@ -303,3 +303,181 @@ func TestCompareRoles(t *testing.T) {
 		})
 	}
 }
+
+func TestLiveMembers(t *testing.T) {
+	t.Parallel()
+
+	managed := func(id int, host, podName string) mongo.ConfigMember {
+		return mongo.ConfigMember{
+			ID:   id,
+			Host: host,
+			Tags: mongo.ReplsetTags{"podName": podName},
+		}
+	}
+
+	tests := []struct {
+		name              string
+		rsStatus          mongo.Status
+		cnf               mongo.RSConfig
+		rs                *api.ReplsetSpec
+		expectedLive      int
+		expectedRSMembers map[string]api.ReplsetMemberStatus
+	}{
+		{
+			name: "primary secondary secondary all live",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				managed(2, "rs0-2:27017", "rs0-2"),
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				{Id: 2, Name: "rs0-2:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			}},
+			rs:           &api.ReplsetSpec{},
+			expectedLive: 3,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				"rs0-2": {Name: "rs0-2:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			},
+		},
+		{
+			name: "non-live states are not counted",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				managed(2, "rs0-2:27017", "rs0-2"),
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateRecovering, StateStr: "RECOVERING"},
+				{Id: 2, Name: "rs0-2:27017", State: mongo.MemberStateDown, StateStr: "(not reachable/healthy)"},
+			}},
+			rs:           &api.ReplsetSpec{},
+			expectedLive: 1,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateRecovering, StateStr: "RECOVERING"},
+				"rs0-2": {Name: "rs0-2:27017", State: mongo.MemberStateDown, StateStr: "(not reachable/healthy)"},
+			},
+		},
+		{
+			name: "in-cluster arbiter is counted",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				{ID: 2, Host: "rs0-arbiter-0:27017", ArbiterOnly: true},
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				{Id: 2, Name: "rs0-arbiter-0:27017", State: mongo.MemberStateArbiter, StateStr: "ARBITER"},
+			}},
+			rs:           &api.ReplsetSpec{},
+			expectedLive: 3,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			},
+		},
+		{
+			name: "external member is skipped",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				{ID: 2, Host: "external.example.com:27017", Tags: mongo.ReplsetTags{"external": "true"}},
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				{Id: 2, Name: "external.example.com:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			}},
+			rs:           &api.ReplsetSpec{},
+			expectedLive: 2,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			},
+		},
+		{
+			name: "external arbiter is skipped",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				{ID: 2, Host: "arbiter.example.com:27017", ArbiterOnly: true},
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				{Id: 2, Name: "arbiter.example.com:27017", State: mongo.MemberStateArbiter, StateStr: "ARBITER"},
+			}},
+			rs: &api.ReplsetSpec{
+				ExternalNodes: []*api.ExternalNode{
+					{Host: "arbiter.example.com", Port: 27017, ArbiterOnly: true},
+				},
+			},
+			expectedLive: 2,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			},
+		},
+		{
+			name: "non-arbiter external node does not skip in-cluster arbiter",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				{ID: 2, Host: "rs0-arbiter-0:27017", ArbiterOnly: true},
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				{Id: 2, Name: "rs0-arbiter-0:27017", State: mongo.MemberStateArbiter, StateStr: "ARBITER"},
+			}},
+			rs: &api.ReplsetSpec{
+				ExternalNodes: []*api.ExternalNode{
+					{Host: "data.example.com", Port: 27017, ArbiterOnly: false},
+				},
+			},
+			expectedLive: 3,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			},
+		},
+		{
+			name: "arbiter external node with port in host",
+			cnf: mongo.RSConfig{Members: mongo.ConfigMembers{
+				managed(0, "rs0-0:27017", "rs0-0"),
+				managed(1, "rs0-1:27017", "rs0-1"),
+				{ID: 2, Host: "arbiter.example.com:27017", ArbiterOnly: true},
+			}},
+			rsStatus: mongo.Status{Members: []*mongo.Member{
+				{Id: 0, Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				{Id: 1, Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+				{Id: 2, Name: "arbiter.example.com:27017", State: mongo.MemberStateArbiter, StateStr: "ARBITER"},
+			}},
+			rs: &api.ReplsetSpec{
+				ExternalNodes: []*api.ExternalNode{
+					{Host: "arbiter.example.com:27017", Port: 27017, ArbiterOnly: true},
+				},
+			},
+			expectedLive: 2,
+			expectedRSMembers: map[string]api.ReplsetMemberStatus{
+				"rs0-0": {Name: "rs0-0:27017", State: mongo.MemberStatePrimary, StateStr: "PRIMARY"},
+				"rs0-1": {Name: "rs0-1:27017", State: mongo.MemberStateSecondary, StateStr: "SECONDARY"},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rsMembers := make(map[string]api.ReplsetMemberStatus)
+			live := countLiveMembers(tt.rsStatus, tt.cnf, tt.rs, rsMembers)
+			assert.Equal(t, tt.expectedLive, live)
+			assert.Equal(t, tt.expectedRSMembers, rsMembers)
+		})
+	}
+}