Uh oh!
There was an error while loading. Please reload this page.
feat: cleanup shared PVC when no longer in use - #834
Conversation
Hi @AObuchow. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| return nil | ||
| } else { | ||
| return runCommonPVCCleanupJob(workspace, clusterAPI) |
There was a problem hiding this comment.
In terms of balancing conditional branches, it probably makes sense to move this one up to the top, i.e. have
iftotalWorkspaces>1 {
returnrunCommonPVCCleanupJob(workspace, clusterAPI)
} else {
// delete PVC
}| err = clusterAPI.Client.Delete(clusterAPI.Ctx, existingPVC) | ||
| if err != nil && !k8sErrors.IsNotFound(err) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Similarly here, need to handle the IsNotFound error case (it's not an error, the object has already been deleted).
There was a problem hiding this comment.
Oh man, I was tired yesterday, ignore this one :D
| if workspace.Spec.Started { | ||
| started++ | ||
| } |
There was a problem hiding this comment.
If started isn't currently needed, may as well not count it for now.
Uh oh!
There was an error while loading. Please reload this page.
| if totalWorkspaces > 1 { | ||
| return runCommonPVCCleanupJob(workspace, clusterAPI) | ||
| } else { | ||
| existingPVC := &corev1.PersistentVolumeClaim{} |
There was a problem hiding this comment.
maybe rename to sharedPVC?
| if totalWorkspaces > 1 { | ||
| return runCommonPVCCleanupJob(workspace, clusterAPI) | ||
| } else { | ||
| existingPVC := &corev1.PersistentVolumeClaim{} |
| err = clusterAPI.Client.Delete(clusterAPI.Ctx, existingPVC) | ||
| if err != nil && !k8sErrors.IsNotFound(err) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Oh man, I was tired yesterday, ignore this one :D
| if err != nil && !k8sErrors.IsNotFound(err) { | ||
| if err != nil { | ||
| if k8sErrors.IsNotFound(err) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I apologize for my review here yesterday...
iferr!=nil&&!k8sErrors.IsNotFound(err) {
returnerr
}
returnniland
err=clusterAPI.Client.Delete(clusterAPI.Ctx, existingPVC)
iferr!=nil {
ifk8sErrors.IsNotFound(err) {
returnnil
}
returnerr
}
returnnilare equivalent, with the original being a little cleaner. Sorry for the confusion!
amisevsk
left a comment
There was a problem hiding this comment.
LGTM!
Tested on minikube.
Please autosquash commits before merging
Fixdevfile#826 Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
24985c5 to
1378757Compare[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, AObuchow, ibuziuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does this PR do?
When the common PVC is cleaned up after workspace deletion, a check occurs to see if other workspaces using the
commonorsharedstorage-class attribute exist in the same namespace (i.e. other workspaces are using the same shared PVC). If there are no workspaces using the shared PVC, then the PVC is marked for deletion.What issues does this PR fix or reference?
Fix#826
Is it tested? How?
async,common, or unset)claim-devworkspacePVC is createdclaim-devworkspacePVC is deleted shortly afterwards.PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che