Repository: ceph/ceph-csi
Pull request #3222 (2022-06-28 13:17:28)

revert: rbd: consider remote image health for primary (backport #3216)
This is an automatic backport of pull request #3216 done by [Mergify](https://mergify.com).


---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the [documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>



Patch:
diff --git a/internal/rbd/replicationcontrollerserver.go b/internal/rbd/replicationcontrollerserver.go
index 90c885632..57c0b7bf8 100644
--- a/internal/rbd/replicationcontrollerserver.go
+++ b/internal/rbd/replicationcontrollerserver.go
@@ -580,7 +580,12 @@ func (rs *ReplicationServer) PromoteVolume(ctx context.Context,
 		}
 	}
 
-	err = checkHealthyPrimary(ctx, rbdVol)
+	mirrorStatus, err := rbdVol.getImageMirroringStatus()
+	if err != nil {
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
+	err = checkHealthyPrimary(ctx, rbdVol, mirrorStatus)
 	if err != nil {
 		return nil, status.Error(codes.Internal, err.Error())
 	}
@@ -615,14 +620,9 @@ func (rs *ReplicationServer) PromoteVolume(ctx context.Context,
 }
 
 // checkHealthyPrimary checks if the image is a healhty primary or not.
-// healthy primary image will be in up+stopped state in local cluster and
-// up+replaying in the remote clusters, for states other than this it returns
-// an error message.
-func checkHealthyPrimary(ctx context.Context, rbdVol *rbdVolume) error {
-	mirrorStatus, err := rbdVol.getImageMirroringStatus()
-	if err != nil {
-		return err
-	}
+// healthy primary image will be in up+stopped state, for states other
+// than this it returns an error message.
+func checkHealthyPrimary(ctx context.Context, rbdVol *rbdVolume, mirrorStatus *librbd.GlobalMirrorImageStatus) error {
 	localStatus, err := mirrorStatus.LocalStatus()
 	if err != nil {
 		// LocalStatus can fail if the local site status is not found in
@@ -633,7 +633,7 @@ func checkHealthyPrimary(ctx context.Context, rbdVol *rbdVolume) error {
 		return fmt.Errorf("failed to get local status: %w", err)
 	}
 
-	if !localStatus.Up && localStatus.State != librbd.MirrorImageStatusStateStopped {
+	if !localStatus.Up || localStatus.State != librbd.MirrorImageStatusStateStopped {
 		return fmt.Errorf("%s %w. State is up=%t, state=%q",
 			rbdVol,
 			ErrUnHealthyMirroredImage,
@@ -641,26 +641,6 @@ func checkHealthyPrimary(ctx context.Context, rbdVol *rbdVolume) error {
 			localStatus.State)
 	}
 
-	// Remote image should be in up+replaying state.
-	for _, s := range mirrorStatus.SiteStatuses {
-		log.UsefulLog(
-			ctx,
-			"peer site mirrorUUID=%q, daemon up=%t, mirroring state=%q, description=%q and lastUpdate=%d",
-			s.MirrorUUID,
-			s.Up,
-			s.State,
-			s.Description,
-			s.LastUpdate)
-		if s.MirrorUUID != "" {
-			if !s.Up && s.State != librbd.MirrorImageStatusStateReplaying {
-				return fmt.Errorf("remote image %s is not healthy. State is up=%t, state=%q",
-					rbdVol,
-					s.Up,
-					s.State)
-			}
-		}
-	}
-
 	return nil
 }
 


Test patch:
diff --git a/internal/rbd/replicationcontrollerserver_test.go b/internal/rbd/replicationcontrollerserver_test.go
index 401292587..87f60a38f 100644
--- a/internal/rbd/replicationcontrollerserver_test.go
+++ b/internal/rbd/replicationcontrollerserver_test.go
@@ -279,3 +279,75 @@ func TestCheckVolumeResyncStatus(t *testing.T) {
 		})
 	}
 }
+
+func Test_checkHealthyPrimary(t *testing.T) {
+	t.Parallel()
+	rbdVol := &rbdVolume{
+		rbdImage: rbdImage{
+			RbdImageName: "test",
+			Pool:         "test-pool",
+		},
+	}
+	tests := []struct {
+		name         string
+		ctx          context.Context
+		rbdVol       *rbdVolume
+		mirrorStatus *librbd.GlobalMirrorImageStatus
+		wantErr      bool
+	}{
+		{
+			name:   "when image is in up+stopped state",
+			ctx:    context.TODO(),
+			rbdVol: rbdVol,
+			mirrorStatus: &librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "",
+						State:      librbd.MirrorImageStatusStateStopped,
+						Up:         true,
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name:   "when image is in up+error state",
+			ctx:    context.TODO(),
+			rbdVol: rbdVol,
+			mirrorStatus: &librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "",
+						State:      librbd.MirrorImageStatusStateError,
+						Up:         false,
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name:   "when image is in up+replaying state",
+			ctx:    context.TODO(),
+			rbdVol: rbdVol,
+			mirrorStatus: &librbd.GlobalMirrorImageStatus{
+				SiteStatuses: []librbd.SiteMirrorImageStatus{
+					{
+						MirrorUUID: "",
+						State:      librbd.MirrorImageStatusStateReplaying,
+						Up:         true,
+					},
+				},
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		ts := tt
+		t.Run(ts.name, func(t *testing.T) {
+			t.Parallel()
+			if err := checkHealthyPrimary(ts.ctx, ts.rbdVol, ts.mirrorStatus); (err != nil) != ts.wantErr {
+				t.Errorf("checkHealthyPrimary() error = %v, wantErr %v", err, ts.wantErr)
+			}
+		})
+	}
+}