-
Notifications
You must be signed in to change notification settings - Fork 145
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.
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
fix(server): return 404 if get backing image and backup not found #2067
fix(server): return 404 if get backing image and backup not found #2067
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle volume/backupvolume for CSI API DeleteSnapshot
?
The idea LGTM.
d80e226
to
872307f
Compare
VolumeSnapshot has 3 types
When deleting
|
872307f
to
9f28b19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@shuo-wu Please help review this. |
@innobead |
NP. Passing the test is the criteria before merging. |
weird, it passed the test in my environment |
ref: longhorn/longhorn 6266 Signed-off-by: Jack Lin <jack.lin@suse.com>
f159724
to
a333f30
Compare
I know the reason why |
We don't have to check by getting the backup or snapshot before deletion longhorn-manager/api/backup.go Lines 125 to 134 in 75d2e77
so removed the check |
…g_and_backup_resource
Sorry... Does the PR contain the removal (of getting backing image/backup volume before requesting deletion)...? BTW, for the snapshot deletion, getting volume is unavoidable. We still need to make sure getting a nil volume should be handled as expected. |
Hi @shuo-wu |
OK. I was thinking you directly remove the backup volume/backing image get calls from the CSI |
I think if from server side it already checks if it exists before deleting it |
In conclusion For
|
So do we need to do any other code change here? |
No, I have tested it manually and through e2e |
I mean, for backing image cleanup, since there is no parent resource, there is no need to invoke |
@shuo-wu Even we delete the BackingImage directly, if it doesn't exist, we still need to handle not found error
And besides this issue, we still need to add 404 not found in the server api for other cases, maybe there are some places in our code also use 404 to determine if api has not found error. |
@mergify backport v1.5.x |
✅ Backports have been created
|
ref: longhorn/longhorn#6266