Skip to content

Conversation

amisevsk
Copy link
Collaborator

@amisevsk amisevsk commented Jun 15, 2022

What does this PR do?

Fix a regression caused by commit cbd3fd1. When deleting workspaces, if
there are no non-terminating workspaces left in the namespace, we delete
the common PVC rather than running cleanup jobs. However, the way the
check was written assumed that we were counting all workspaces, and
would delete the common PVC if there were two workspaces in a namespace
and one was deleted.

What issues does this PR fix or reference?

Mentioned in review for #846 (review) -- I'm not sure how I didn't catch it in the original PR; test case was likely missed by me testing

  • delete a single workspace
  • delete 3-5 workspaces at once

Closes #871

Is it tested? How?

kubectl apply -f samples/theia-next.yaml
yq '.metadata.name="theia-next-2"' samples/theia-next.yaml | kubectl apply -f -
# wait for workspaces to enter running state
kubectl delete dw theia-next
kubectl get pvc # should not be terminating.

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@amisevsk amisevsk requested a review from AObuchow June 15, 2022 18:54
@amisevsk amisevsk requested a review from ibuziuk as a code owner June 15, 2022 18:54
@amisevsk
Copy link
Collaborator Author

Once merged, this PR should be cherry-picked to the 0.15.x branch.

Fix a regression caused by commit cbd3fd1. When deleting workspaces, if
there are no non-terminating workspaces left in the namespace, we delete
the common PVC rather than running cleanup jobs. However, the way the
check was written assumed that we were counting _all_ workspaces, and
would delete the common PVC if there were two workspaces in a namespace
and one was deleted.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk amisevsk force-pushed the fix-deleting-common-pvc branch from f36f052 to c46d7e5 Compare June 15, 2022 20:49
Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

This fixes the issue that the PR aims to fix, but prevents the PVC from being deleted when there are no workspaces present (
i.e. delete both workspaces and see that the common PVC is not deleted). I'm going to think a bit more of another potential fix.

@amisevsk
Copy link
Collaborator Author

@AObuchow I had made a mistake (>= 0 vs > 0) -- should be fixed now.

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

@amisevsk Works as expected now :)

@openshift-ci
Copy link

openshift-ci bot commented Jun 15, 2022

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@amisevsk amisevsk merged commit f5d1d31 into devfile:main Jun 16, 2022
@amisevsk amisevsk deleted the fix-deleting-common-pvc branch June 16, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[regression] Deleting a DevWorkspace when there are only two workspaces in a namespace results in the common PVC being deleted.

3 participants