Repository: ceph/ceph-csi
Pull request #3291 (2022-08-08 18:06:13)

rbd: Report remote peer readiness if Up and status.Unknown
Current code uses an !A && !B condition incorrectly to
test A:Up and B:status for a remote peer image.

This should be !A || !B as we require both conditions to
be in the specified state (Up: true, and status Unknown).

This is corrected by this commit, and further fixes:
- check and return ready only when a remote site is
found in the status output
- check if all peer sites are ready, if multiple are found
and return ready appropriately

Signed-off-by: Shyamsundar Ranganathan <srangana@redhat.com>

ShyamsundarR: Executed e2e disaster recovery testing, including ramen orchestration and this code change across deploy->failover->relocate->delete of an application with multiple PVs and also when one of the clusters is down. The change does not impact the regular workflow.

Adding this note here for confidence level on the patch during reviews.

Patch:
diff --git a/internal/rbd/replicationcontrollerserver.go b/internal/rbd/replicationcontrollerserver.go
index 7393c2fdd..4e428082a 100644
--- a/internal/rbd/replicationcontrollerserver.go
+++ b/internal/rbd/replicationcontrollerserver.go
@@ -680,6 +680,7 @@ func (rs *ReplicationServer) DemoteVolume(ctx context.Context,
 // It returns true if the state of the remote cluster is up and unknown.
 func checkRemoteSiteStatus(ctx context.Context, mirrorStatus *librbd.GlobalMirrorImageStatus) bool {
 	ready := true
+	found := false
 	for _, s := range mirrorStatus.SiteStatuses {
 		log.UsefulLog(
 			ctx,
@@ -690,13 +691,16 @@ func checkRemoteSiteStatus(ctx context.Context, mirrorStatus *librbd.GlobalMirro
 			s.Description,
 			s.LastUpdate)
 		if s.MirrorUUID != "" {
-			if s.State != librbd.MirrorImageStatusStateUnknown && !s.Up {
+			found = true
+			// If ready is already "false" do not flip it based on another remote peer status
+			if ready && (s.State != librbd.MirrorImageStatusStateUnknown || !s.Up) {
 				ready = false
 			}
 		}
 	}
 
-	return ready
+	// Return readiness only if at least one remote peer status was processed
+	return found && ready
 }
 
 // ResyncVolume extracts the RBD volume information from the volumeID, If the


Test patch:
diff --git a/internal/rbd/replicationcontrollerserver_test.go b/internal/rbd/replicationcontrollerserver_test.go
index e10d35249..a4b042bb8 100644
--- a/internal/rbd/replicationcontrollerserver_test.go
+++ b/internal/rbd/replicationcontrollerserver_test.go
@@ -295,3 +295,140 @@ func TestCheckVolumeResyncStatus(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckRemoteSiteStatus(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name      string
+		args      librbd.GlobalMirrorImageStatus
+		wantReady bool
+	}{
+		{
+			name: "Test a single peer in sync",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+				},
+			},
+			wantReady: true,
+		},
+		{
+			name: "Test a single peer in sync, including a local instance",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+					{
+						MirrorUUID: "",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+				},
+			},
+			wantReady: true,
+		},
+		{
+			name: "Test a multiple peers in sync",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote1",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+					{
+						MirrorUUID: "remote2",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+				},
+			},
+			wantReady: true,
+		},
+		{
+			name: "Test no remote peers",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{},
+			},
+			wantReady: false,
+		},
+		{
+			name: "Test single peer not in sync",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote",
+						State:      librbd.MirrorImageStatusStateReplaying,
+						Up:         true,
+					},
+				},
+			},
+			wantReady: false,
+		},
+		{
+			name: "Test single peer not up",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         false,
+					},
+				},
+			},
+			wantReady: false,
+		},
+		{
+			name: "Test multiple peers, when first peer is not in sync",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote1",
+						State:      librbd.MirrorImageStatusStateStoppingReplay,
+						Up:         true,
+					},
+					{
+						MirrorUUID: "remote2",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+				},
+			},
+			wantReady: false,
+		},
+		{
+			name: "Test multiple peers, when second peer is not up",
+			args: librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "remote1",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         true,
+					},
+					{
+						MirrorUUID: "remote2",
+						State:      librbd.MirrorImageStatusStateUnknown,
+						Up:         false,
+					},
+				},
+			},
+			wantReady: false,
+		},
+	}
+	for _, tt := range tests {
+		ts := tt
+		t.Run(ts.name, func(t *testing.T) {
+			t.Parallel()
+			if ready := checkRemoteSiteStatus(context.TODO(), &ts.args); ready != ts.wantReady {
+				t.Errorf("checkRemoteSiteStatus() ready = %v, expect ready = %v", ready, ts.wantReady)
+			}
+		})
+	}
+}