stop reconciling workspace when PVC cleanup job fails by AObuchow · Pull Request #851 · devfile/devworkspace-operator

AObuchow · 2022-06-02T05:42:21Z

What does this PR do?

Currently, in the reconcile loop, there is a bug with marking a workspace as having an error after the common PVC cleanup job fails.

From my understanding, after detecting that the cleanup job has failed and returning a ProvisionError from storageProvisioner.CleanupWorkspaceStorage(), the workspace's status phase is set to Error. However, the reconcile function checks for deleted workspaces before checking for workspaces with errors/failures. The existing logic in the finalize function (which is called when reconciling deleted workspaces) then overwrites the workspace's status phase (setting it to Terminating) and eventually runs storageProvisioner.CleanupWorkspaceStorage() again, leading to an endless loop.

My current fix simply modifies the finalize function to check if the workspace's status phase is set to Error, and if so, it does not overwrite the workspace's status phase and returns early.

What issues does this PR fix or reference?

Fix#845

Is it tested? How?

Start up DWO
Create 2 workspaces that use the common PVC storage-class strategy
Delete one of the workspaces so that the common PVC cleanup job will be run, eg. kubectl delete dw theia-next -n $NAMESPACE
Wait for all the PVC cleanup job-related pods to fail
DWO will now log an error similar to the following, only once:

{"level":"error","ts":1654116778.779755,"logger":"controllers.DevWorkspace","msg":"Failed to clean up DevWorkspace storage","Request.Namespace":"devworkspace-controller","Request.Name":"theia-next","devworkspace_id":"workspace10db2004baac400d","error":"DevWorkspace PVC cleanup job failed: see logs for job \"cleanup-workspace10db2004baac400d\" for details","stacktrace":"github.com/devfile/devworkspace-operator/controllers/workspace.(*DevWorkspaceReconciler).finalize\n\t/home/aobuchow/git/devworkspace-operator/controllers/workspace/finalize.go:64\ngithub.com/devfile/devworkspace-operator/controllers/workspace.(*DevWorkspaceReconciler).Reconcile\n\t/home/aobuchow/git/devworkspace-operator/controllers/workspace/devworkspace_controller.go:130\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:298\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:253\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:214"}

Ensure that the workspace which was deleted is set to the Error phase by doing kubectl get devworkspace -n $NAMESPACE:

NAME DEVWORKSPACE ID PHASE INFO
theia-next workspace10db2004baac400d Error DevWorkspace PVC cleanup job failed: see logs for job "cleanup-workspace10db2004baac400d" for details
theia-next2 workspace9e7aa15f7aa44c92 Running https://workspace9e7aa15f7aa44c92-theia-3100.192.168.39.100.nip.io/

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

AObuchow · 2022-06-02T05:43:49Z

-	if err != nil && !k8sErrors.IsConflict(err) {
- return reconcile.Result{}, err
-	}
+	if workspace.Status.Phase != dw.DevWorkspaceStatusError {


There are potentially other places where the workspace status phase may be set to Error that do not concern the common PVC cleanup job.
For example, finalizeServiceAccount() may set the workspace status phase to Error. Furthermore, it is possible that in the future, other unrelated conditions may set the workspace's status phase to Error.
Thus, it might be best to also check the workspaces condition message for "Failed to clean up DevWorkspace storage" in the finalize function (unless this bug also occurs with finalizeServiceAccount(), which may be likely? 🤔 )

AObuchow · 2022-06-02T05:44:14Z

 Args: []string{
 "-c",
- fmt.Sprintf(cleanupCommandFmt, path.Join(pvcClaimMountPath, workspaceId)),
+ "exit 1",


This commit/change needs to be removed before the PR can be merged. It's only here to facilitate testing.

amisevsk

LGTM.

This PR is pretty far behind main right now, and does not include the PVC cleanup changes -- I rebased on main while testing to verify how this works with "cleanup PVC on deletion".

Also, don't forget to remove the temporary patch before merging :)

dkwon17 · 2022-06-02T18:44:52Z

I've tested the PR with the provided instructions, and it is working 👍
}
I see that:

{"level":"error","ts":1654116778.779755,"logger":"controllers.DevWorkspace","msg":"Failed to clean up DevWorkspace storage","Request.Namespace":"devworkspace-controller","Request.Name":"theia-next","devworkspace_id":"workspace10db2004baac400d","error":"DevWorkspace PVC cleanup job failed: see logs for job \"cleanup-workspace10db2004baac400d\" for details","stacktrace":"github.com/devfile/devworkspace-operator/controllers/workspace.(*DevWorkspaceReconciler).finalize\n\t/home/aobuchow/git/devworkspace-operator/controllers/workspace/finalize.go:64\ngithub.com/devfile/devworkspace-operator/controllers/workspace.(*DevWorkspaceReconciler).Reconcile\n\t/home/aobuchow/git/devworkspace-operator/controllers/workspace/devworkspace_controller.go:130\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:298\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:253\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:214"}

is being printed quite often, but that is expected since new cleanup pods will start and will fail continuously for our test case, is that right?

AObuchow · 2022-06-02T18:55:59Z

I've tested the PR with the provided instructions, and it is working +1 } I see that:

{"level":"error","ts":1654116778.779755,"logger":"controllers.DevWorkspace","msg":"Failed to clean up DevWorkspace storage","Request.Namespace":"devworkspace-controller","Request.Name":"theia-next","devworkspace_id":"workspace10db2004baac400d","error":"DevWorkspace PVC cleanup job failed: see logs for job \"cleanup-workspace10db2004baac400d\" for details","stacktrace":"github.com/devfile/devworkspace-operator/controllers/workspace.(*DevWorkspaceReconciler).finalize\n\t/home/aobuchow/git/devworkspace-operator/controllers/workspace/finalize.go:64\ngithub.com/devfile/devworkspace-operator/controllers/workspace.(*DevWorkspaceReconciler).Reconcile\n\t/home/aobuchow/git/devworkspace-operator/controllers/workspace/devworkspace_controller.go:130\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:298\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:253\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/home/aobuchow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.5/pkg/internal/controller/controller.go:214"}

is being printed quite often, but that is expected since new cleanup pods will start and will fail continuously for our test case, is that right?

Thanks for testing @dkwon17 :)
That error is supposed to be logged, but it should stop being logged after a while - are you seeing it continuously being logged infinitely ? Because if so, the original bug is still occuring.

ibuziuk

LGTM, please do not forget to replace exit 1 before merging

openshift-ci · 2022-06-03T07:00:47Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [amisevsk]

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

dkwon17 · 2022-06-03T14:57:31Z

@AObuchow , sorry, I meant to write my comment: #851 (comment) on your other PR: #846

I just tested this PR and it is working for me , as I only see the

Failed to clean up DevWorkspace storage

log only once 👍

Fixdevfile#845 Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

openshift-ci · 2022-06-03T18:05:55Z

New changes are detected. LGTM label has been removed.

AObuchow · 2022-06-03T18:06:32Z

@AObuchow , sorry, I meant to write my comment: #851 (comment) on your other PR: #846
I just tested this PR and it is working for me , as I only see the
Failed to clean up DevWorkspace storage
log only once +1

Awesome, thanks for confirmation @dkwon17 :)

AObuchow requested review from amisevsk and ibuziuk as code owners June 2, 2022 05:42

AObuchow commented Jun 2, 2022
View reviewed changes

amisevsk approved these changes Jun 2, 2022
View reviewed changes

openshift-ciBot assigned amisevsk Jun 2, 2022

openshift-ciBot added lgtm approved labels Jun 2, 2022

ibuziuk approved these changes Jun 3, 2022
View reviewed changes

openshift-ciBot assigned ibuziuk Jun 3, 2022

fix: stop reconciling workspace when PVC cleanup job fails
28a3599
Fixdevfile#845 Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

AObuchow force-pushed the mark_workspace_failed_when_pvc_cleanup_fails branch from 5203833 to 28a3599Compare June 3, 2022 18:05

openshift-ciBot removed the lgtm label Jun 3, 2022

AObuchow merged commit e4013f7 into devfile:mainJun 3, 2022

AObuchow deleted the mark_workspace_failed_when_pvc_cleanup_fails branch June 3, 2022 19:31

Navigation Menu

Uh oh!

stop reconciling workspace when PVC cleanup job fails - #851

stop reconciling workspace when PVC cleanup job fails #851
AObuchow merged 1 commit into
devfile:mainfrom
AObuchow:mark_workspace_failed_when_pvc_cleanup_fails

AObuchow commented Jun 2, 2022

Uh oh!

AObuchowJun 2, 2022

Uh oh!

AObuchowJun 2, 2022

Uh oh!

amisevsk left a comment

Uh oh!

dkwon17 commented Jun 2, 2022

Uh oh!

AObuchow commented Jun 2, 2022

Uh oh!

ibuziuk left a comment

Uh oh!

openshift-ciBot commented Jun 3, 2022

Uh oh!

dkwon17 commented Jun 3, 2022•
edited
Loading

Uh oh!

openshift-ciBot commented Jun 3, 2022

Uh oh!

AObuchow commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

AObuchow commented Jun 2, 2022

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

Uh oh!

AObuchowJun 2, 2022

Choose a reason for hiding this comment

Uh oh!

AObuchowJun 2, 2022

Choose a reason for hiding this comment

Uh oh!

amisevsk left a comment

Choose a reason for hiding this comment

Uh oh!

dkwon17 commented Jun 2, 2022

Uh oh!

AObuchow commented Jun 2, 2022

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ciBot commented Jun 3, 2022

Uh oh!

dkwon17 commented Jun 3, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ciBot commented Jun 3, 2022

Uh oh!

AObuchow commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkwon17 commented Jun 3, 2022•
edited
Loading