rbd: remove grpc and csi-addons import from rbd/replication.go #3884

riya-singhal31 · 2023-06-05T16:21:06Z

this commit removes grpc import from replication.go and replaced it with usual errors

updates - #3314

yati1998 · 2023-06-05T18:07:55Z

internal/rbd/errors.go

+	// This is a most likely a transient condition and may be corrected
+	// by retrying with a backoff. Note that it is not always safe to retry
+	// non-idempotent operations.
+	ErrUnavailable = errors.New("service is currently unavailable")


ErrUnavailable could be ErrUnavailableService for better readability.

Updated, thanks!

yati1998 · 2023-06-05T18:08:13Z

internal/rbd/errors.go

+	// ErrFailedPrecondition is returned when operation is rejected because the system is not in a state 
+	// required for the operation's execution.
+	ErrFailedPrecondition = errors.New("operation is rejected because the system is not in a state required for the operation's execution.")
+    // ErrUnavailable is returned when the service is currently unavailable.


please check the indentation.

Rakshith-R

The linked issue does not require this change.
Can you provide the reason for making this change ?
grpc codes & status are used for a reason.

riya-singhal31 · 2023-06-06T05:42:38Z

The linked issue does not require this change. Can you provide the reason for making this change ? grpc codes & status are used for a reason.

This is the update for - https://github.com/ceph/ceph-csi/pull/3608/files#r1092953730
This commit involves cleanup of grpc package and will be adding another commit to remove csi-addons.

Rakshith-R · 2023-06-06T05:55:29Z

The linked issue does not require this change. Can you provide the reason for making this change ? grpc codes & status are used for a reason.

This is the update for - https://github.com/ceph/ceph-csi/pull/3608/files#r1092953730 This commit involves cleanup of grpc package and will be adding another commit to remove csi-addons.

The main devel branch should get only complete prs going from one stable state to another.
Please mark the pr ready for review (draft state otherwise) until all commits are in.
Make sure the functions(in csiaddons package) which in turn handle these errors definitely need to return the correct corresponding grpc status and codes.

nixpanic

This is a good start for cleaning up, thanks!

You'll need to check the callers of the functions, and map the new errors to some gRPC codes. The CSI protocol expects a gRPC code+error to be returned from the upper API functions. Have a look at the CSI spec to see what code is suitable for the error in the particular API (CreateVolume or DeleteVolume may return a different error to its caller for the same internal failure).

riya-singhal31 · 2023-06-13T09:01:45Z

The linked issue does not require this change. Can you provide the reason for making this change ? grpc codes & status are used for a reason.

This is the update for - https://github.com/ceph/ceph-csi/pull/3608/files#r1092953730 This commit involves cleanup of grpc package and will be adding another commit to remove csi-addons.

The main devel branch should get only complete prs going from one stable state to another. Please mark the pr ready for review (draft state otherwise) until all commits are in. Make sure the functions(in csiaddons package) which in turn handle these errors definitely need to return the correct corresponding grpc status and codes.

Yes, I have used the similar error definitions as the corresponding grpc status and codes.

nixpanic · 2023-06-13T10:46:46Z

internal/csi-addons/rbd/replication.go

-		return corerbd.DisableVolumeReplication(rbdVol, mirroringInfo, force)
+		err = rbdVol.DisableVolumeReplication(mirroringInfo, force)
+		if err != nil {
+			return nil, err


this should not return err directly, but a gRPC error with a suitable code. DisableVolumeReplication() did not seem to have done that in all cases, that was not correct. With this refactoring it becomes clear, and you should address that.

nixpanic · 2023-06-14T11:54:00Z

internal/rbd/replication.go

 )

 func (rv *rbdVolume) ResyncVol(localStatus librbd.SiteMirrorImageStatus, force bool) error {
 	if resyncRequired(localStatus) {
 		// If the force option is not set return the error message to retry
 		// with Force option.
 		if !force {
-			return status.Errorf(codes.FailedPrecondition,
-				"image is in %q state, description (%s). Force resync to recover volume",
+			fmt.Errorf("image is in %q state, description (%s). Force resync to recover volume",


this does not look righ, you may want to assign it to err and return that?

internal/rbd/replication.go:32:14: unusedresult: result of fmt.Errorf call not used (govet) fmt.Errorf("image is in %q state, description (%s). Force resync to recover volume", ^

riya-singhal31 · 2023-06-15T06:31:51Z

This now removes the replication service from internal/rbd and returns gRPC code+error in internal/csi-addons/rbd.
Thanks, PTAL.
@nixpanic @Rakshith-R

Madhu-1 · 2023-06-15T07:11:58Z

internal/rbd/errors.go

+	// ErrFailedPrecondition is returned when operation is rejected because the system is not in a state
+	// required for the operation's execution.
+	ErrFailedPrecondition = errors.New("system is not in a state required for the operation's execution")
+	// ErrServiceUnavailable is returned when the service is currently unavailable.
+	// This is a most likely a transient condition and may be corrected
+	// by retrying with a backoff. Note that it is not always safe to retry
+	// non-idempotent operations.
+	ErrServiceUnavailable = errors.New("service is currently unavailable")
+	// ErrAborted is returned when the operation is aborted.
+	ErrAborted = errors.New("operation got aborted")
+	// ErrInvalidArgument is returned when the client specified an invalid argument.
+	ErrInvalidArgument = errors.New("invalid arguments provided")


where are these error messages are used?

Madhu-1 · 2023-06-15T07:12:36Z

internal/csi-addons/rbd/replication.go

+		switch {
+		case strings.Contains(err.Error(), "failed to get local state"):
+			return nil, status.Error(codes.Internal, err.Error())
+		case strings.Contains(err.Error(), "secondary image status is"):
+			return nil, status.Error(codes.InvalidArgument, err.Error())
+		case strings.Contains(err.Error(), "failed to disable image mirroring"):
+			return nil, status.Error(codes.Internal, err.Error())
+		case strings.Contains(err.Error(), "failed to get mirroring info of image"):
+			return nil, status.Error(codes.Internal, err.Error())
+		case strings.Contains(err.Error(), "in disabling state"):
+			return nil, status.Error(codes.Aborted, err.Error())
+		}


You should define an error and return it and use errors.IS/As for comparison instead of string comparison.

nixpanic · 2023-06-15T07:13:09Z

internal/rbd/replication.go

-			return status.Errorf(codes.FailedPrecondition,
-				"image is in %q state, description (%s). Force resync to recover volume",
-				localStatus.State, localStatus.Description)
+		 	fmt.Errorf("image is in %q state, description (%s). Force resync to recover volume", localStatus.State, localStatus.Description)


These fmt.Errorf(....) statements return an error, and a lot of them are not used/returned.

You can probably do something like this which will be better:

return fmt.Errorf("%w: image is in %q state, description (%s). Force resync to recover volume", ErrFailedPrecondition, localStatus.State, localStatus.Description)

or

err = fmt.Errorf("%w: image is in %q state, description (%s). Force resync to recover volume", ErrFailedPrecondition, localStatus.State, localStatus.Description) return err

By using %w and pasing ErrFailedPrecondition for the formatting, you get the error message in the error string, and the type of the error will still be ErrFailedPrecondition.

Yes Niels, I have already done this here (in 2nd commit),
return fmt.Errorf("image is in %q state, description (%s). Force resync to recover volume: %w",

nixpanic · 2023-06-15T07:16:20Z

internal/rbd/replication.go

 		}
 		err := rv.resyncImage()
 		if err != nil {
-			return status.Error(codes.Internal, err.Error())
+			fmt.Errorf("failed to resync image")
+			return err


What type of error is returned here? Maybe it makes sense to provide a comment at the top of the function that describes the different errors? The caller of the function can then easily figure out what errors need to be caught and handled differently.

nixpanic · 2023-06-15T07:17:23Z

internal/csi-addons/rbd/replication.go

+			return &replication.DisableVolumeReplicationResponse{}, nil
+		}
+		switch {
+		case strings.Contains(err.Error(), "failed to get local state"):


Instead of checking for contents of the string, use errors.Is() as function to check if an error is of a particular type.

ceph-csi/internal/rbd/clone.go

Lines 58 to 88 in 40888f0

switch {

case errors.Is(err, ErrSnapNotFound):

// as the snapshot is not present, create new snapshot,clone and

// delete the temporary snapshot

err = createRBDClone(ctx, tempClone, rv, snap)

if err != nil {

return false, err

}

return true, nil

case errors.Is(err, ErrImageNotFound):

// as the temp clone does not exist,check snapshot exists on parent volume

// snapshot name is same as temporary clone image

snap.RbdImageName = tempClone.RbdImageName

err = parentVol.checkSnapExists(snap)

if err == nil {

// the temp clone exists, delete it lets reserve a new ID and

// create new resources for a cleaner approach

err = parentVol.deleteSnapshot(ctx, snap)

}

if errors.Is(err, ErrSnapNotFound) {

return false, nil

}

return false, err

default:

// any error other than the above return error

return false, err

}

riya-singhal31 · 2023-06-15T08:54:20Z

Thanks for the comments, have addressed all the suggested modification, requesting you to PTAL,
@Madhu-1 @nixpanic

nixpanic

You can squash both commits into one, as the 2nd almost redoes everything from the 1st.

Applying only the 1st will likely not result in a functional ceph-csi executable. Only if the commits are in such an order and independent enough so that applying one at the time, run tests, apply the next, run tests, keeps working as it should, commits can be split. (That is the best way, but sometimes difficult to get right. It is required for git blame which can be used for bug hunting.)

nixpanic · 2023-06-15T09:04:48Z

internal/csi-addons/rbd/replication.go

+				return nil, status.Error(codes.Internal, err.Error())
+			case errors.Is(err, corerbd.ErrAborted):
+				return nil, status.Error(codes.Aborted, err.Error())
+			}


please add a default case, otherwise uncaught errors will get ignored

internal/csi-addons/rbd/replication.go

riya-singhal31 · 2023-06-15T09:25:58Z

You can squash both commits into one, as the 2nd almost redoes everything from the 1st.

Applying only the 1st will likely not result in a functional ceph-csi executable. Only if the commits are in such an order and independent enough so that applying one at the time, run tests, apply the next, run tests, keeps working as it should, commits can be split. (That is the best way, but sometimes difficult to get right. It is required for git blame which can be used for bug hunting.)

Updated, thanks.

riya-singhal31 · 2023-06-15T10:24:18Z

Update: To reduce the cyclomatic complexity, changed the switch statements to storing the values in map.

nixpanic · 2023-06-16T07:20:31Z

internal/csi-addons/rbd/replication.go

@@ -649,6 +665,17 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
 	return resp, nil
 }

+func getError(err error, errorStatusMap map[error]codes.Code) error {


New helper functions like this require a unit-test to prevent them from breaking in the future. Please add one.

Added the unit test, thank you!

nixpanic · 2023-06-16T07:22:50Z

internal/csi-addons/rbd/replication.go

@@ -627,9 +639,13 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,

 	err = rbdVol.ResyncVol(localStatus, req.Force)
 	if err != nil {
-		log.ErrorLog(ctx, err.Error())
+		errorStatusMap := map[error]codes.Code{


There could be a signle errorStatusMap internally to the getError() function. You use it twice, and there is no overlap in the keys.

nixpanic · 2023-06-19T09:53:48Z

internal/csi-addons/rbd/replication.go

@@ -326,7 +326,12 @@ func (rs *ReplicationServer) DisableVolumeReplication(ctx context.Context,
 	case librbd.MirrorImageDisabling:
 		return nil, status.Errorf(codes.Aborted, "%s is in disabling state", volumeID)
 	case librbd.MirrorImageEnabled:
-		return corerbd.DisableVolumeReplication(rbdVol, mirroringInfo, force)
+		err = rbdVol.DisableVolumeReplication(mirroringInfo, force)
+		if err == nil {


nit: the usual check is for err != nil, and have the success path continue after the if-statement

Thanks Niels, updated. PTAL

internal/rbd/errors.go

Rakshith-R · 2023-06-19T10:38:35Z

internal/rbd/replication.go

 	}
 	if mirroringInfo.State == librbd.MirrorImageDisabling {
-		return nil, status.Errorf(codes.Aborted, "%s is in disabling state", rbdVol.VolID)
+		return fmt.Errorf("%w: %s is in disabling state", ErrAborted, rv.VolID)


Suggested change

return fmt.Errorf("%w: %s is in disabling state", ErrAborted, rv.VolID)

return fmt.Errorf("%w: %q is in disabling state", ErrAborted, rv.VolID)

This is still pending.

internal/rbd/replication.go

internal/csi-addons/rbd/replication_test.go

internal/csi-addons/rbd/replication.go

Rakshith-R

lgtm

Madhu-1 · 2023-06-22T05:56:14Z

internal/csi-addons/rbd/replication.go

+	if err == nil {
+		return status.Error(codes.OK, codes.OK.String())
+	}


This should be the first check, move this to line 656

Done, thanks!

Pull request has been modified.

Madhu-1 · 2023-06-22T08:18:26Z

internal/csi-addons/rbd/replication.go

+		}
+	}
+
+	// Handle any other error nol nil error not listed in the map


Suggested change

// Handle any other error nol nil error not listed in the map

// Handle any other error non nil error not listed in the map

Rakshith-R · 2023-06-22T10:03:45Z

@Mergifyio queue

mergify · 2023-06-22T10:03:51Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at cdaa926

this commit removes grpc import from replication.go and replaced it with usual errors and passed gRPC responses in csi-addons Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

ceph-csi-bot · 2023-06-22T10:04:24Z

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

ceph-csi-bot · 2023-06-22T10:04:25Z

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

ceph-csi-bot · 2023-06-22T10:04:26Z

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

ceph-csi-bot · 2023-06-22T10:04:27Z

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

ceph-csi-bot · 2023-06-22T10:04:28Z

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

ceph-csi-bot · 2023-06-22T10:04:29Z

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

ceph-csi-bot · 2023-06-22T10:04:30Z

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

ceph-csi-bot · 2023-06-22T10:04:31Z

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

ceph-csi-bot · 2023-06-22T10:04:32Z

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

ceph-csi-bot · 2023-06-22T10:04:33Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-06-22T10:04:34Z

/test ci/centos/upgrade-tests-rbd

riya-singhal31 requested a review from nixpanic June 5, 2023 16:21

mergify bot added the component/rbd Issues related to RBD label Jun 5, 2023

yati1998 reviewed Jun 5, 2023

View reviewed changes

Rakshith-R requested changes Jun 6, 2023

View reviewed changes

nixpanic requested changes Jun 6, 2023

View reviewed changes

riya-singhal31 changed the title ~~rbd: remove grpc import from rbd/replication.go~~ rbd: remove grpc and csi-addons import from rbd/replication.go Jun 13, 2023

nixpanic reviewed Jun 13, 2023

View reviewed changes

nixpanic reviewed Jun 14, 2023

View reviewed changes

riya-singhal31 requested review from nixpanic and Rakshith-R June 15, 2023 06:30

Madhu-1 requested changes Jun 15, 2023

View reviewed changes

nixpanic reviewed Jun 15, 2023

View reviewed changes

riya-singhal31 requested review from Madhu-1, nixpanic and yati1998 June 15, 2023 08:54

nixpanic requested changes Jun 15, 2023

View reviewed changes

riya-singhal31 requested a review from nixpanic June 15, 2023 10:28

nixpanic requested changes Jun 16, 2023

View reviewed changes

riya-singhal31 requested a review from nixpanic June 16, 2023 09:24

riya-singhal31 mentioned this pull request Jun 16, 2023

rbd: complete migration of volrep to csi-addons #3314

Closed

5 tasks

nixpanic previously approved these changes Jun 19, 2023

View reviewed changes

Rakshith-R reviewed Jun 19, 2023

View reviewed changes

riya-singhal31 requested a review from Madhu-1 June 21, 2023 14:56

nixpanic previously approved these changes Jun 21, 2023

View reviewed changes

riya-singhal31 requested a review from Rakshith-R June 21, 2023 20:03

Rakshith-R reviewed Jun 22, 2023

View reviewed changes

Madhu-1 reviewed Jun 22, 2023

View reviewed changes

riya-singhal31 requested review from Madhu-1, nixpanic and Rakshith-R June 22, 2023 08:14

Madhu-1 reviewed Jun 22, 2023

View reviewed changes

nixpanic approved these changes Jun 22, 2023

View reviewed changes

nixpanic requested a review from Madhu-1 June 22, 2023 08:37

Madhu-1 approved these changes Jun 22, 2023

View reviewed changes

rbd: migration of replication service to csi-addon

76faeb3

this commit removes grpc import from replication.go and replaced it with usual errors and passed gRPC responses in csi-addons Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

mergify bot added the ok-to-test Label to trigger E2E tests label Jun 22, 2023

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

mergify bot merged commit cdaa926 into ceph:devel Jun 22, 2023

	switch {
	case errors.Is(err, ErrSnapNotFound):
	// as the snapshot is not present, create new snapshot,clone and
	// delete the temporary snapshot
	err = createRBDClone(ctx, tempClone, rv, snap)
	if err != nil {
	return false, err
	}

	return true, nil

	case errors.Is(err, ErrImageNotFound):
	// as the temp clone does not exist,check snapshot exists on parent volume
	// snapshot name is same as temporary clone image
	snap.RbdImageName = tempClone.RbdImageName
	err = parentVol.checkSnapExists(snap)
	if err == nil {
	// the temp clone exists, delete it lets reserve a new ID and
	// create new resources for a cleaner approach
	err = parentVol.deleteSnapshot(ctx, snap)
	}
	if errors.Is(err, ErrSnapNotFound) {
	return false, nil
	}

	return false, err

	default:
	// any error other than the above return error
	return false, err
	}

	return fmt.Errorf("%w: %s is in disabling state", ErrAborted, rv.VolID)
	return fmt.Errorf("%w: %q is in disabling state", ErrAborted, rv.VolID)

	// Handle any other error nol nil error not listed in the map
	// Handle any other error non nil error not listed in the map

rbd: remove grpc and csi-addons import from rbd/replication.go #3884

rbd: remove grpc and csi-addons import from rbd/replication.go #3884

Conversation

riya-singhal31 commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R left a comment

Choose a reason for hiding this comment

riya-singhal31 commented Jun 6, 2023

Rakshith-R commented Jun 6, 2023

nixpanic left a comment

Choose a reason for hiding this comment

riya-singhal31 commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented Jun 15, 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

riya-singhal31 commented Jun 15, 2023 • edited Loading

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented Jun 15, 2023

riya-singhal31 commented Jun 15, 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

Rakshith-R left a 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

Rakshith-R commented Jun 22, 2023

mergify bot commented Jun 22, 2023 • edited Loading

✅ The pull request has been merged automatically

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

ceph-csi-bot commented Jun 22, 2023

riya-singhal31 commented Jun 15, 2023 •

edited

Loading

mergify bot commented Jun 22, 2023 •

edited

Loading