Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rbd: use internal as default error code in getGRPCError() #4671

Merged
merged 1 commit into from
Jun 11, 2024
Merged

rbd: use internal as default error code in getGRPCError() #4671

merged 1 commit into from
Jun 11, 2024

Conversation

Rakshith-R
Copy link
Contributor

Describe what this PR does

This commit replaces codes.Unknown with codes.Internal as the default error code in getGRPCError()
since it much better suited.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes
    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

@Rakshith-R Rakshith-R added component/rbd Issues related to RBD ci/skip/e2e skip running e2e CI jobs labels Jun 10, 2024
@Rakshith-R Rakshith-R requested review from nixpanic, iPraveenParihar and a team June 10, 2024 09:59
@nixpanic
Copy link
Member

Hi @Rakshith-R,

Can you explain why Internal is much better suited than Unknown for an unrecognized error?
Does this influence the behavior of CSI callers on these errors?

@Rakshith-R
Copy link
Contributor Author

Hi @Rakshith-R,

Can you explain why Internal is much better suited than Unknown for an unrecognized error? Does this influence the behavior of CSI callers on these errors?

Unknown and Internal both will get retries from the callers.

We don't use Unknown anywhere else.
Most command execution failures are treated as internal and not unknown elsewhere in cephcsi.
We should default to that instead of Unknown.

@Rakshith-R
Copy link
Contributor Author

Hi @Rakshith-R,
Can you explain why Internal is much better suited than Unknown for an unrecognized error? Does this influence the behavior of CSI callers on these errors?

Unknown and Internal both will get retries from the callers.

We don't use Unknown anywhere else. Most command execution failures are treated as internal and not unknown elsewhere in cephcsi. We should default to that instead of Unknown.

cdaa926

This commit introduced unknown grpc error code.
Without using internal as default, we would need to export each error message and add it to the list every time a new code block it added.

@nixpanic
Copy link
Member

Hi @Rakshith-R,
Can you explain why Internal is much better suited than Unknown for an unrecognized error? Does this influence the behavior of CSI callers on these errors?

Unknown and Internal both will get retries from the callers.
We don't use Unknown anywhere else. Most command execution failures are treated as internal and not unknown elsewhere in cephcsi. We should default to that instead of Unknown.

cdaa926

This commit introduced unknown grpc error code. Without using internal as default, we would need to export each error message and add it to the list every time a new code block it added.

Except that it is easy to forget to add a new error code (we should have unit tests for that), considering a matching gRPC error code is probably better than always returning Internal?

This change is not bad, but I am not sure how it improves things. In order to prevent the Unknown error from being returned, maybe log a warning marked as bug with call-stack so that it needs to be addressed?

@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Jun 11, 2024 •
edited
Loading

Hi @Rakshith-R,
Can you explain why Internal is much better suited than Unknown for an unrecognized error? Does this influence the behavior of CSI callers on these errors?

Unknown and Internal both will get retries from the callers.
We don't use Unknown anywhere else. Most command execution failures are treated as internal and not unknown elsewhere in cephcsi. We should default to that instead of Unknown.

cdaa926
This commit introduced unknown grpc error code. Without using internal as default, we would need to export each error message and add it to the list every time a new code block it added.

Except that it is easy to forget to add a new error code (we should have unit tests for that), considering a matching gRPC error code is probably better than always returning Internal?

This change is not bad, but I am not sure how it improves things. In order to prevent the Unknown error from being returned, maybe log a warning marked as bug with call-stack so that it needs to be addressed?

I think the whole way of creating constants of errors in rbd package, exporting them and using them in csiaddons package should be used only when you need a grpc error code that is not internal.

Adopting the export and use method for internal code too would be way too tedious according to me.

Most of the places we error out with internal error code. We don't use unknown code at all in other places.
Please take a look, I've replaced all the error messages which used internal error code.

nixpanic
nixpanic approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error constants that are used only once don't make much sense to me either.

@Rakshith-R
Copy link
Contributor Author

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 11, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at ec80175

@Rakshith-R
rbd: use internal as default error code in getGRPCError()
d0566c3 
This commit replaces codes.Unknown with codes.Internal
as the default error code in getGRPCError().

Signed-off-by: Rakshith R <rar@redhat.com>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jun 11, 2024
@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jun 11, 2024
@mergify mergify bot merged commit ec80175 into ceph:devel Jun 11, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@iPraveenParihar iPraveenParihar iPraveenParihar approved these changes

Assignees
No one assigned
Labels
ci/skip/e2e skip running e2e CI jobs component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Rakshith-R @nixpanic @ceph-csi-bot @iPraveenParihar