rbd: Update return error message when delete volume failed #5138

ecosysbin · 2025-02-12T04:36:54Z

Describe what this PR does

Issue: When delete pv failed, error message shows 'volume deletion failed: rpc error:
code = Internal desc = rbd: ret=-39, Directory not empty', the actual failed reason is 'access denied'

Fixes: #5132

This commit ensures ceph-csi return right error massage.

github-actions · 2025-02-12T05:01:19Z

Thanks for taking this issue! Let us know if you have any questions!

nixpanic · 2025-02-13T08:49:52Z

@Mergifyio rebase

The build failures have been addressed. If all is well, the CI jobs should pass now.

mergify · 2025-02-13T08:50:07Z

rebase

✅ Branch has been successfully rebased

nixpanic

Hi @ecosysbin, there are two minor CI failures that you need to address before we can get this merged:

line length in the commit message should be < 80 characters, please add some line-breaks
before each return statemens, golangci-lint expects an empty line

Please correct these things, and force-push your branch again. Ideally rebase your branch on the latest master, so that an unrelated build failure is prevented.

Thanks!

internal/rbd/rbd_util.go

Rakshith-R · 2025-02-14T07:05:42Z

internal/rbd/rbd_util.go

 		err = librbd.TrashRemove(ri.ioctx, ri.ImageID, true)
 		if err != nil {
-			log.ErrorLog(ctx, "failed to delete rbd image: %s, %v", ri, err)
-
-			return err


Thanks for catching this and the pr !

Present code hides trash remove task error while the current pr changes hide trash remove error.

Let's just return both error when librbd.TrashRemove cmd fails like suggested below:

trashRemoveErr = librbd.TrashRemove(ri.ioctx, ri.ImageID, true) if err != nil { log.ErrorLog(ctx, "failed to delete rbd image: %s, %v", ri, trashRemoveErr) return fmt.Errorf("failed to add task to remove image: %w, failed to trash remove image: %w",err, trashRemoveError)

I would be more comfortable with this kind of simple change.

Ok， it's a good idea. I have done it.

Rakshith-R · 2025-02-14T07:06:59Z

internal/rbd/rbd_util.go

 			}
 		}
+
+		return knownErr


This will involve similar changes as suggested with trash remove error.

nixpanic · 2025-02-14T09:06:30Z

internal/rbd/rbd_util.go

 		}
+
+		return knownErr


Why is it required to return the knownErr here? librbd.TrashRemove() was a success, so the goal of the function (removing the image from the trash) was achieved.

Ok, I have modify it.

Rakshith-R

one more comment.

Rakshith-R · 2025-02-14T09:05:45Z

internal/rbd/rbd_util.go

+
+		return knownErr


Suggested change

return knownErr

This is not required since trashremove has done the job?

ok，I have modified it

Rakshith-R · 2025-02-14T09:06:02Z

internal/rbd/rbd_util.go

+
+		return knownErr


Suggested change

return knownErr

This is not required since trashremove has done the job?

Ok, I have done it.

nixpanic

Thanks! Looks good to me 👍

nixpanic · 2025-02-14T16:14:55Z

internal/rbd/rbd_util.go


-			return err
+			return fmt.Errorf("failed to add task to remove image: %v, failed to trash remove image: %v", knownErr, trashRemoveError)


Need to use %w for error formatting, not %v. This causes golangci-lint to fail:

At the begin, It was set %w and it also show golangci-lint to fail, so I want to try %v. Now, I recovered to %w.

Pull request has been modified.

Rakshith-R · 2025-02-17T07:21:23Z

hey @ecosysbin
There's golangci lint failure
https://github.com/ceph/ceph-csi/actions/runs/13340136953/job/37320372109?pr=5138

internal/rbd/rbd_util.go:724: the line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
level=info msg="[runner] Processors filtering stat (in/out): diff: 1/1, max_same_issues: 1/1, path_prefixer: 1/1, cgo: 381/294, path_prettifier: 294/294, skip_files: 294/294, max_per_file_from_linter: 1/1, path_shortener: 1/1, severity-rules: 1/1, invalid_issue: 294/294, exclude: 294/294, uniq_by_line: 1/1, nolint: 228/1, max_from_linter: 1/1, fixer: 1/1, skip_dirs: 294/294, autogenerated_exclude: 294/294, exclude-rules: 294/228, sort_results: 1/1, filename_unadjuster: 294/294, identifier_marker: 294/294, source_code: 1/1"
			return fmt.Errorf("failed to add task to remove image: %w, failed to trash remove image: %w", knownErr, trashRemoveError)

Please take a look.

mergify · 2025-02-19T09:28:26Z

queue

🛑 The pull request has been removed from the queue `default`

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

ceph-csi-bot · 2025-02-19T09:28:40Z

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

ceph-csi-bot · 2025-02-19T09:28:40Z

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

ceph-csi-bot · 2025-02-19T09:28:41Z

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

ceph-csi-bot · 2025-02-19T09:28:41Z

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

ceph-csi-bot · 2025-02-19T09:28:41Z

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

ceph-csi-bot · 2025-02-19T09:28:42Z

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

ceph-csi-bot · 2025-02-19T09:28:42Z

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

ceph-csi-bot · 2025-02-19T09:28:42Z

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

ceph-csi-bot · 2025-02-19T09:28:42Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-02-19T09:28:43Z

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

ceph-csi-bot · 2025-02-19T09:28:43Z

/test ci/centos/upgrade-tests-rbd

mergify · 2025-02-19T09:29:48Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ ci/centos/k8s-e2e-external-storage/1.32
❌ ci/centos/mini-e2e/k8s-1.30
❌ ci/centos/mini-e2e/k8s-1.32
❌ ci/centos/upgrade-tests-cephfs
❌ ci/centos/upgrade-tests-rbd

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

ecosysbin · 2025-02-19T11:39:51Z

@nixpanic I see the CI error shows 'Error cloning remote repo 'origin'', what can we do next step?
ERROR: Error cloning remote repo 'origin'
ERROR: Maximum checkout retry attempts reached, aborting

Rakshith-R · 2025-02-19T13:27:49Z

/retest ci/centos/k8s-e2e-external-storage/1.32

Rakshith-R · 2025-02-19T13:28:15Z

@nixpanic I see the CI error shows 'Error cloning remote repo 'origin'', what can we do next step? ERROR: Error cloning remote repo 'origin' ERROR: Maximum checkout retry attempts reached, aborting

I think its a CI issue, not due to this pr.
We'll take care of this.

ceph-csi-bot · 2025-02-19T13:36:05Z

/retest ci/centos/mini-e2e/k8s-1.32

ceph-csi-bot · 2025-02-19T13:36:06Z

@ecosysbin "ci/centos/mini-e2e/k8s-1.32" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-02-19T13:36:06Z

/retest ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-02-19T13:36:07Z

@ecosysbin "ci/centos/upgrade-tests-rbd" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-02-19T13:36:08Z

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

ceph-csi-bot · 2025-02-19T13:36:08Z

@ecosysbin "ci/centos/mini-e2e/k8s-1.30" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-02-19T13:36:09Z

/retest ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-02-19T13:36:09Z

@ecosysbin "ci/centos/upgrade-tests-cephfs" test failed. Logs are available at location for debugging

ceph-csi-bot · 2025-02-19T13:36:10Z

@Mergifyio requeue

mergify · 2025-02-19T13:36:47Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added the component/rbd Issues related to RBD label Feb 12, 2025

ecosysbin force-pushed the devel branch from 9cac4fb to 39c66ff Compare February 12, 2025 04:44

ecosysbin changed the title ~~rbd: Update return error massage when delete volume failed and the fu…~~ rbd: Update return error massage when delete volume failed Feb 12, 2025

github-actions bot assigned ecosysbin Feb 12, 2025

ecosysbin force-pushed the devel branch from 39c66ff to ec057d5 Compare February 12, 2025 05:04

nixpanic force-pushed the devel branch from ec057d5 to a773bdd Compare February 13, 2025 08:50

nixpanic changed the title ~~rbd: Update return error massage when delete volume failed~~ rbd: Update return error message when delete volume failed Feb 13, 2025

nixpanic reviewed Feb 13, 2025

View reviewed changes

internal/rbd/rbd_util.go Outdated Show resolved Hide resolved

ecosysbin force-pushed the devel branch 3 times, most recently from 0a1b9cd to a441f0e Compare February 14, 2025 02:24

Rakshith-R requested changes Feb 14, 2025

View reviewed changes

ecosysbin force-pushed the devel branch from a441f0e to 689186a Compare February 14, 2025 08:59

nixpanic reviewed Feb 14, 2025

View reviewed changes

Rakshith-R requested changes Feb 14, 2025

View reviewed changes

ecosysbin force-pushed the devel branch 2 times, most recently from 1b63839 to e9251fc Compare February 14, 2025 09:12

ecosysbin requested a review from Rakshith-R February 14, 2025 09:18

ecosysbin force-pushed the devel branch 2 times, most recently from fce8eef to 4160eb2 Compare February 14, 2025 10:17

nixpanic previously approved these changes Feb 14, 2025

View reviewed changes

nixpanic reviewed Feb 14, 2025

View reviewed changes

ecosysbin force-pushed the devel branch from 4160eb2 to 016fb3e Compare February 15, 2025 00:57

ecosysbin force-pushed the devel branch from 016fb3e to 7fd905a Compare February 17, 2025 07:26

nixpanic approved these changes Feb 17, 2025

View reviewed changes

mergify bot added the ok-to-test Label to trigger E2E tests label Feb 19, 2025

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Feb 19, 2025

Rakshith-R added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Feb 19, 2025

mergify bot merged commit 0907ba9 into ceph:devel Feb 19, 2025
40 checks passed


		return err
		return fmt.Errorf("failed to add task to remove image: %v, failed to trash remove image: %v", knownErr, trashRemoveError)

rbd: Update return error message when delete volume failed #5138

rbd: Update return error message when delete volume failed #5138

Conversation

ecosysbin commented Feb 12, 2025 • edited by iPraveenParihar Loading

Describe what this PR does

github-actions bot commented Feb 12, 2025

nixpanic commented Feb 13, 2025

mergify bot commented Feb 13, 2025

✅ Branch has been successfully rebased

nixpanic left a comment

Choose a reason for hiding this comment

Rakshith-R Feb 14, 2025 • edited Loading

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

Choose a reason for hiding this comment

nixpanic left a 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 Feb 17, 2025

mergify bot commented Feb 19, 2025 • edited Loading

🛑 The pull request has been removed from the queue default

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

mergify bot commented Feb 19, 2025

ecosysbin commented Feb 19, 2025

Rakshith-R commented Feb 19, 2025

Rakshith-R commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

ceph-csi-bot commented Feb 19, 2025

mergify bot commented Feb 19, 2025

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

ecosysbin commented Feb 12, 2025 •

edited by iPraveenParihar

Loading

Rakshith-R Feb 14, 2025 •

edited

Loading

mergify bot commented Feb 19, 2025 •

edited

Loading

🛑 The pull request has been removed from the queue `default`