rbd: Dont depend on image state to issue resync #4076

Madhu-1 · 2023-08-24T11:30:01Z

Previously we were dependent on the image mirror state to issue the resync command, What we came to know is that we cannot depend on the image mirror state as the state can change anytime from one to another, We are following the below steps to fix this problem.

During the Demote volume store the image creation timestamp.
During Resync do the below operation
- Check the image creation timestamp stored during the Demote operation and the current creation timestamp during Resync and check both are equal and it for force resync then issue resync
- If the image on both sides is not in the unknown state, check last_snapshot_timestamp on the local mirror description, if it's present send volumeReady as false, or else return an error message.
- If both the images are in up+unknown the send volumeReady as true.

Storing the image creation timestamp on the rbd image as metadata and it gets cleaned up when the image is recreated as it gets synced up from the primary cluster.

Madhu-1 · 2023-08-24T11:44:32Z

Looks like yum update on base image is broken :(

nixpanic · 2023-08-24T11:50:10Z

internal/csi-addons/rbd/replication.go

@@ -637,14 +658,28 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
 		ready = checkRemoteSiteStatus(ctx, mirrorStatus)
 	}

-	err = rbdVol.ResyncVol(localStatus, req.Force)
+	err = rbdVol.GetImageInfo()


GetImageInfo() only returns an error? Can you not return whatever information you need? Checking attributes like rbdVol.CreatedAt is dangerous, specially because of the required calling of GetImageInfo() (that probably was a reason the function was not public).

Maybe introduce rbdVol.GetCreationTime() that internally calls getImageInfo() if the creation time is not set yet?

@nixpanic yes it internally updates the image details in local structure and returns an error if that fails. am not sure we should start introduces functions to get a field of the rbdVol.

Maybe rename GetImageInfo() to UpdateImageInfo(), but accessing attributes of rbdVolume outside of the rbs package is not very clean. There is no guarantee that the attribute has been set (unless you remember to call getImageInfo(), which is rather error prone.

internal/csi-addons/rbd/replication.go

nixpanic

GetImgeInfo() really should be an internal function of rbdVolume. A Get...() function should return something in addition to an error.

Using attributes from rbdVolume outside of its own package is not very clean. Use functions that return requires attributes instead (so that the can be populated/checked in rbdVolume functions).

Madhu-1 · 2023-08-24T12:07:55Z

GetImgeInfo() really should be an internal function of rbdVolume. A Get...() function should return something in addition to an error.

Using attributes from rbdVolume outside of its own package is not very clean. Use functions that return requires attributes instead (so that the can be populated/checked in rbdVolume functions).

Added one extra function to return CreationTimestamp, @nixpanic PTAL

nixpanic · 2023-08-24T12:24:11Z

internal/csi-addons/rbd/replication.go

+		return false, errors.New("failed to get image creation time")
+	}
+
+	log.UsefulLog(ctx, "savedImageTime=%v, currentImageTime=%v", savedImageTime, currentImageTime)


nit: remove this, or phrase it a little nicer, or move it to debug level.

nixpanic · 2023-08-24T12:29:50Z

internal/rbd/replication.go

@@ -126,3 +98,23 @@ func (rv *rbdVolume) DisableVolumeReplication(

 	return nil
 }
+
+func (rv *rbdVolume) CheckImageIsPrimary() error {


I am confused by the naming of the function. I expected a true/false return in case the image is (not) primary...

Instead, if the image is primary, it returns a ErrInvalidArgument?

What is the intention of this function, and can it be made clearer by renaming and/or returning a bool? A comment with a description of the function would also help.

things got messed up when I tried to refractor, for now removed refractoring as it looks complicated to do as part of this PR.

yati1998 · 2023-08-25T05:58:21Z

internal/csi-addons/rbd/replication.go

+	if len(strings.Split(parts[0], ":")) != 2 || len(strings.Split(parts[1], ":")) != 2 {
+		return nil, status.Errorf(codes.Internal, "failed to parse image creation time: %s", savedImageTime)
+	}
+	secondsStr := strings.Split(parts[0], ":")[1]


In my opinion it would be to good to have a different function to parse the creation time and get the required format, we can also further add unit test for it. What do you think @Madhu-1 ?

yes, definitely a good suggestion!

nixpanic · 2023-08-25T07:32:02Z

internal/csi-addons/rbd/replication.go

+	if len(strings.Split(parts[0], ":")) != 2 || len(strings.Split(parts[1], ":")) != 2 {
+		return nil, status.Errorf(codes.Internal, "failed to parse image creation time: %s", savedImageTime)
+	}
+	secondsStr := strings.Split(parts[0], ":")[1]


yes, definitely a good suggestion!

nixpanic · 2023-08-25T07:35:36Z

internal/csi-addons/rbd/replication.go

+		// store the image creation time for resync
+		_, err = rbdVol.GetMetadata(imageCreationTimeKey)
+		if err != nil && errors.Is(err, librbd.ErrNotFound) {
+			err = rbdVol.SetMetadata(imageCreationTimeKey, creationTime.String())


Consider using your own timestampToString() and timestampFromString() functions. There is no guarantee that creationTime.String() produces the same string format across package versions.

nixpanic · 2023-08-29T13:53:35Z

internal/rbd/rbd_util.go

@@ -1558,6 +1558,19 @@ func (rv *rbdVolume) setImageOptions(ctx context.Context, options *librbd.ImageO
 	return nil
 }

+// getImageCreationTime returns the creation time of the image. if the image


getImageCreationTime -> GetImageCreationTime

nixpanic · 2023-08-29T13:57:18Z

internal/csi-addons/rbd/replication.go

 	if err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, status.Errorf(codes.Internal, "failed to get image creation time: %s", err.Error())


consider including the image name and key in the error message, if something goes wrong it gives a hint to check while troubleshooting

nixpanic · 2023-08-29T13:58:37Z

internal/csi-addons/rbd/replication.go

 	if err != nil {
-		return nil, getGRPCError(err)
+		return nil, status.Errorf(codes.Internal, "failed to get image info: %s", err.Error())


update the error message to include the name of the image?

ceph-csi-bot · 2023-08-29T18:51:27Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-08-29T18:51:27Z

/test ci/centos/mini-e2e/k8s-1.27

ceph-csi-bot · 2023-08-29T18:51:27Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-08-29T18:51:28Z

/test ci/centos/k8s-e2e-external-storage/1.28

ceph-csi-bot · 2023-08-29T18:51:28Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2023-08-29T18:51:29Z

/test ci/centos/mini-e2e-helm/k8s-1.28

ceph-csi-bot · 2023-08-29T18:51:30Z

/test ci/centos/mini-e2e/k8s-1.28

mergify · 2023-08-29T18:52:54Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at e013cfe

Not sure why but go-lint is failing with below error and this fix is required to make it pass ``` directive `//nolint:staticcheck // See comment above.` is unused for linter "staticcheck" (nolintlint) ``` Signed-off-by: Madhu Rajanna <[email protected]>

During the Demote volume store the image creation timestamp. During Resync do below operation * Check image creation timestamp stored during Demote operation and current creation timestamp during Resync and check both are equal and its for force resync then issue resync * If the image on both sides is not in unknown state, check last_snapshot_timestamp on the local mirror description, if its present send volumeReady as false or else return error message. If both the images are in up+unknown the send volumeReady as true. Signed-off-by: Madhu Rajanna <[email protected]>

ceph-csi-bot · 2023-08-29T18:55:21Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-08-29T18:55:21Z

/test ci/centos/k8s-e2e-external-storage/1.28

ceph-csi-bot · 2023-08-29T18:55:21Z

/test ci/centos/k8s-e2e-external-storage/1.26

ceph-csi-bot · 2023-08-29T18:55:22Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2023-08-29T18:55:22Z

/test ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-08-29T18:55:22Z

/test ci/centos/mini-e2e-helm/k8s-1.28

ceph-csi-bot · 2023-08-29T18:55:22Z

/test ci/centos/k8s-e2e-external-storage/1.27

ceph-csi-bot · 2023-08-29T18:55:23Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-08-29T18:55:23Z

/test ci/centos/mini-e2e/k8s-1.28

ceph-csi-bot · 2023-08-29T18:55:23Z

/test ci/centos/mini-e2e-helm/k8s-1.27

ceph-csi-bot · 2023-08-29T18:55:24Z

/test ci/centos/mini-e2e/k8s-1.27

Madhu-1 · 2023-08-30T07:26:43Z

/test ci/centos/mini-e2e/k8s-1.27

Madhu-1 · 2023-08-30T07:26:51Z

/test ci/centos/mini-e2e/k8s-1.26

Madhu-1 · 2023-08-30T07:27:07Z

/test ci/centos/k8s-e2e-external-storage/1.27

Madhu-1 requested review from idryomov, ShyamsundarR and a team August 24, 2023 11:30

mergify bot added the component/rbd Issues related to RBD label Aug 24, 2023

Madhu-1 force-pushed the fix-rbd-resync branch 2 times, most recently from b765827 to 6f3af4d Compare August 24, 2023 11:41

Madhu-1 force-pushed the fix-rbd-resync branch from 6f3af4d to 3e4dfd0 Compare August 24, 2023 11:50

nixpanic reviewed Aug 24, 2023

View reviewed changes

internal/csi-addons/rbd/replication.go Outdated Show resolved Hide resolved

nixpanic requested changes Aug 24, 2023

View reviewed changes

Madhu-1 force-pushed the fix-rbd-resync branch from 3e4dfd0 to 812b97b Compare August 24, 2023 12:03

Madhu-1 requested a review from nixpanic August 24, 2023 12:04

Madhu-1 force-pushed the fix-rbd-resync branch from 812b97b to 72d5df4 Compare August 24, 2023 12:07

Madhu-1 force-pushed the fix-rbd-resync branch from 72d5df4 to bad71a4 Compare August 24, 2023 12:13

nixpanic reviewed Aug 24, 2023

View reviewed changes

Madhu-1 force-pushed the fix-rbd-resync branch 3 times, most recently from f32f641 to daa2550 Compare August 24, 2023 13:15

Madhu-1 requested a review from nixpanic August 24, 2023 13:15

Madhu-1 force-pushed the fix-rbd-resync branch from daa2550 to f27714e Compare August 24, 2023 13:25

yati1998 reviewed Aug 25, 2023

View reviewed changes

nixpanic requested changes Aug 25, 2023

View reviewed changes

Madhu-1 force-pushed the fix-rbd-resync branch from f27714e to 3c79b78 Compare August 28, 2023 07:43

Madhu-1 requested review from nixpanic and yati1998 August 28, 2023 07:43

nixpanic reviewed Aug 29, 2023

View reviewed changes

Madhu-1 force-pushed the fix-rbd-resync branch from 3c79b78 to 9facccc Compare August 29, 2023 14:07

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 29, 2023

Madhu-1 added 2 commits August 29, 2023 18:53

cephfs: fix make go-lint error

db3a554

Not sure why but go-lint is failing with below error and this fix is required to make it pass ``` directive `//nolint:staticcheck // See comment above.` is unused for linter "staticcheck" (nolintlint) ``` Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 force-pushed the fix-rbd-resync branch from 10b0f79 to 2b6cf40 Compare August 29, 2023 18:53

mergify bot added the ok-to-test Label to trigger E2E tests label Aug 29, 2023

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 29, 2023

mergify bot merged commit e013cfe into ceph:devel Aug 30, 2023
23 checks passed

Madhu-1 mentioned this pull request Aug 30, 2023

BUG 2165941: rbd: Dont depend on image state to issue resync red-hat-storage/ceph-csi#185

Merged

rbd: Dont depend on image state to issue resync #4076

rbd: Dont depend on image state to issue resync #4076

Conversation

Madhu-1 commented Aug 24, 2023 • edited Loading

Madhu-1 commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Madhu-1 commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

mergify bot commented Aug 29, 2023 • edited Loading

✅ The pull request has been merged automatically

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

ceph-csi-bot commented Aug 29, 2023

Madhu-1 commented Aug 30, 2023

Madhu-1 commented Aug 30, 2023

Madhu-1 commented Aug 30, 2023

Madhu-1 commented Aug 24, 2023 •

edited

Loading

mergify bot commented Aug 29, 2023 •

edited

Loading