diff --git a/internal/controller/coderworkspacepoolmember_cleanup.go b/internal/controller/coderworkspacepoolmember_cleanup.go new file mode 100644 index 00000000..1ea06e6a --- /dev/null +++ b/internal/controller/coderworkspacepoolmember_cleanup.go @@ -0,0 +1,194 @@ +package controller + +import ( + "cmp" + "context" + "errors" + "fmt" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/log" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// poolMemberUnknownOutcomePoll is how often a deleted member reads Coder +// again when its create outcome is still unknown after the settle window. +const poolMemberUnknownOutcomePoll = 10 * time.Minute + +// cleanup runs the delete steps of a member being deleted (plan 6.5). It +// releases the finalizer once no workspace of the member can exist, the +// workspace is deleted, the workspace was used, or the control plane is +// gone. A workspace that nothing proves to be the member's keeps the +// finalizer, and the controller never touches it. +func (r *CoderWorkspacePoolMemberReconciler) cleanup(ctx context.Context, member *coderv1alpha1.CoderWorkspacePoolMember) (ctrl.Result, error) { + deleted := meta.FindStatusCondition(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted) + switch { + case !poolMemberMayHaveWorkspace(member), deleted != nil && deleted.Status == metav1.ConditionTrue: + return ctrl.Result{}, r.releaseFinalizer(ctx, member) + case deleted != nil && deleted.Reason == "OwnershipUnknown": + // Only a person who removes the finalizer releases the member. + return ctrl.Result{}, nil + } + gone, err := controlPlaneGone(ctx, r.Client, member.Namespace, member.Spec.ControlPlaneRef.Name) + if err != nil { + return ctrl.Result{}, err + } + if gone { + // Nothing can reach Coder any more. The docs tell admins to delete + // leftover kwp- workspaces. + log.FromContext(ctx).Info("The control plane is gone, so the controller releases the pool member without deleting its workspace", + "workspaceID", member.Status.WorkspaceID, "workspaceName", member.Status.WorkspaceName) + return ctrl.Result{}, r.releaseFinalizer(ctx, member) + } + + before := member.Status.DeepCopy() + step, requeue, release, err := r.cleanupStep(ctx, member) + if step, err = memberWaitOnWrongAnswer(ctx, step, err); err != nil { + return ctrl.Result{}, err + } + if release { + return ctrl.Result{}, r.releaseFinalizer(ctx, member) + } + now := r.Clock.Now() + condition := cmp.Or(step.deleted, &metav1.Condition{Status: metav1.ConditionFalse, Reason: step.reason, Message: step.message}) + member.Status.Phase, member.Status.Reason, member.Status.Message = coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, step.reason, step.message + setPoolMemberCondition(member, now, coderv1alpha1.CoderWorkspacePoolMemberConditionReady, metav1.ConditionFalse, step.reason, step.message) + setPoolMemberCondition(member, now, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted, condition.Status, condition.Reason, condition.Message) + if err := r.writeStatus(ctx, member, before); err != nil { + return ctrl.Result{}, err + } + if condition.Status == metav1.ConditionTrue { + return ctrl.Result{}, r.releaseFinalizer(ctx, member) + } + if condition.Reason == "OwnershipUnknown" { + return ctrl.Result{}, nil + } + // A wrong Coder answer is a wait without its own poll. + return ctrl.Result{RequeueAfter: cmp.Or(requeue, templateTestUnavailablePoll)}, nil +} + +// cleanupStep runs one delete step. release means that the finalizer can go +// without a status write: no workspace of the member exists, or someone +// used it. +func (r *CoderWorkspacePoolMemberReconciler) cleanupStep( + ctx context.Context, member *coderv1alpha1.CoderWorkspacePoolMember, +) (step *coderStep, requeue time.Duration, release bool, err error) { + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, member.Namespace, member.Spec.ControlPlaneRef.Name) + switch { + case wait != nil: + return coderWait(wait.reason, "%s", wait.message), templateTestUnavailablePoll, false, nil + case err != nil: + return nil, 0, false, err + } + now := r.Clock.Now() + if member.Status.WorkspaceID == "" { + // A create request may exist: the same confirming reads as before. + step, err := confirmPoolMemberCreate(ctx, sdk, member, now) + switch { + case err != nil: + return nil, 0, false, err + case !poolMemberMayHaveWorkspace(member): + return nil, 0, true, nil // The reads prove that no workspace exists. + case member.Status.WorkspaceID != "": + return coderWait("Deleting", "The controller adopted workspace %s to delete it.", member.Status.WorkspaceName), coderBuildPoll, false, nil + case step.deleted != nil: + return step, 0, false, nil + case now.Before(member.Status.CreateAttemptTime.Add(templateTestSettleWindow)): + return step, coderBuildPoll, false, nil + } + step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "CreateOutcomeUnknown", Message: step.message} + return step, poolMemberUnknownOutcomePoll, false, nil + } + + workspace, step, err := guardPoolMemberDelete(ctx, sdk, member) + if step != nil || err != nil { + return step, templateTestUnavailablePoll, false, err + } + if member.Spec.ClaimRef == nil && member.Status.DeleteBuildID == "" && member.Status.DeleteAttempts == 0 { + // The last use check, before the first delete build: a leaked + // workspace is better than lost work. + used := meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed) + if !used && member.Status.ReadyLastUsedAt != nil { + step, err := r.checkPoolMemberUse(ctx, sdk, member, workspace, now) + if err != nil || (step != nil && step.reason != "Used") { + return step, templateTestUnavailablePoll, false, err + } + used = step != nil + } + if used { + log.FromContext(ctx).Info("Someone used the workspace without a claim, so the controller releases the pool member without deleting it", + "workspaceID", member.Status.WorkspaceID, "workspaceName", member.Status.WorkspaceName) + return nil, 0, true, nil + } + } + + state := workspaceDeleteState{ + workspaceID: workspace.ID, workspaceName: member.Status.WorkspaceName, + deleteBuildID: member.Status.DeleteBuildID, deleteAttempts: member.Status.DeleteAttempts, + } + result, err := deleteCoderWorkspace(ctx, sdk, &state, now, false) + member.Status.DeleteBuildID, member.Status.DeleteAttempts = state.deleteBuildID, state.deleteAttempts + switch { + case err != nil: + return nil, 0, false, err + case result.deleted: + step := coderWait("Deleted", "The delete build of workspace %s succeeded.", member.Status.WorkspaceName) + step.deleted = &metav1.Condition{Status: metav1.ConditionTrue, Reason: "Deleted", Message: step.message} + return step, 0, false, nil + case result.wait.reason == "CoderUnavailable": + return result.wait, templateTestUnavailablePoll, false, nil // Cleanup has no deadline: poll slowly. + } + return result.wait, cmp.Or(result.requeue, coderBuildPoll), false, nil +} + +// guardPoolMemberDelete is the identity guard before every delete write +// (plan 6.2 and 6.5 step 1). It reads deleted workspaces too and accepts any +// latest build. The owner and the name must be the member's, and the build +// history must prove that the operator user created the workspace with the +// recorded start build. A status writer can change the IDs, but not that +// history. A nil step means the member can delete the workspace. +func guardPoolMemberDelete( + ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, +) (codersdk.Workspace, *coderStep, error) { + spec, name := member.Spec, member.Status.WorkspaceName + workspaceID, err := uuid.Parse(member.Status.WorkspaceID) + if err != nil || workspaceID == uuid.Nil { + return codersdk.Workspace{}, nil, fmt.Errorf("assertion failed: pool member %s/%s has workspace ID %q", member.Namespace, member.Name, member.Status.WorkspaceID) + } + operator, err := sdk.User(ctx, codersdk.Me) + if err != nil { + return codersdk.Workspace{}, coderStepUnavailable("get operator user", err), nil + } + if operator.ID == uuid.Nil { + return codersdk.Workspace{}, nil, &coderAnswerError{msg: "assertion failed: Coder answered the operator user without an ID"} + } + workspace, err := sdk.DeletedWorkspace(ctx, workspaceID) + if err != nil { + return workspace, coderStepUnavailable("get workspace", err), nil + } + if err := coderAnswerFor("workspace", workspaceID.String(), workspace.ID.String()); err != nil { + return workspace, nil, err + } + var startBuild *codersdk.WorkspaceBuild + if workspace.OwnerID.String() == spec.OwnerID && workspace.Name == name { + startBuild, err = workspaceProvenance(ctx, sdk, workspace, operator.ID, spec.OrganizationID, spec.TemplateID, spec.TemplateVersionID) + var answerErr *coderAnswerError + if errors.As(err, &answerErr) { + return workspace, nil, err + } else if err != nil { + return workspace, coderStepUnavailable("get workspace builds", err), nil + } + } + if startBuild == nil || startBuild.ID.String() != member.Status.StartBuildID { + step := coderWait("OwnershipUnknown", "Nothing proves that this member created workspace %s, so the controller never deletes it. Only removing the finalizer releases the member.", workspaceID) + step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "OwnershipUnknown", Message: step.message} + return workspace, step, nil + } + return workspace, nil, nil +} diff --git a/internal/controller/coderworkspacepoolmember_cleanup_test.go b/internal/controller/coderworkspacepoolmember_cleanup_test.go new file mode 100644 index 00000000..f3a88b96 --- /dev/null +++ b/internal/controller/coderworkspacepoolmember_cleanup_test.go @@ -0,0 +1,164 @@ +package controller_test + +import ( + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +func (e *poolMemberEnv) deleteMember(t *testing.T, key types.NamespacedName) { + t.Helper() + require.NoError(t, k8sClient.Delete(e.ctx, e.reconcileMember(t, key, 0))) +} + +func (e *poolMemberEnv) coderWrites() int { + return e.fake.requestCount(routeCreateBuild) + e.fake.requestCount(routeCancelBuild) + e.fake.requestCount(routeExtendWorkspace) +} + +// TestPoolMemberCleanupDeletesWorkspace: deleting a member deletes its +// workspace with the delete steps, then releases the finalizer. +func TestPoolMemberCleanupDeletesWorkspace(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + key, _ := e.readyMember(t) + e.deleteMember(t, key) + member := e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build") + requireDeletedCondition(t, member, metav1.ConditionFalse, "Deleting") + require.Equal(t, 1, e.fake.requestCount(routeCreateBuild)) + e.fake.setBuildJob(uuid.MustParse(member.Status.DeleteBuildID), codersdk.ProvisionerJobSucceeded) + require.Nil(t, e.reconcileMember(t, key, 1), "the finalizer goes once the delete build succeeded") + + // A bound member is deleted too, even after the claimant used it. + key, workspaceID := e.readyMember(t) + member = e.reconcileMember(t, key, 0) + member.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: "build-1", UID: uuid.NewString()} + require.NoError(t, k8sClient.Update(e.ctx, member)) + e.fake.markUsed(workspaceID, e.clock.Now().Add(time.Minute)) + e.deleteMember(t, key) + member = e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build") +} + +// TestPoolMemberCleanupGuardsDelete is mutation check 3: before any delete +// write, the member proves that the workspace and its start build are its +// own. Otherwise it keeps the finalizer and never touches the workspace. +func TestPoolMemberCleanupGuardsDelete(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + sdk := e.fake.client(t, 5*time.Second) + other, err := sdk.CreateUserWorkspace(e.ctx, e.tester.String(), codersdk.CreateWorkspaceRequest{TemplateVersionID: e.v1, Name: "someone-elses"}) + require.NoError(t, err) + e.fake.setBuildJob(other.LatestBuild.ID, codersdk.ProvisionerJobSucceeded) + for _, tc := range []struct { + name string + edit func(t *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, workspaceID uuid.UUID) + }{ + {name: "another workspace of the owner", edit: func(_ *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, _ uuid.UUID) { + member.Status.WorkspaceID, member.Status.StartBuildID = other.ID.String(), other.LatestBuild.ID.String() + }}, + {name: "a start build by someone else", edit: func(_ *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, workspaceID uuid.UUID) { + build := e.fake.addExternalBuild(workspaceID, e.tester, codersdk.WorkspaceTransitionStart) + e.fake.setBuildJob(build.ID, codersdk.ProvisionerJobSucceeded) + member.Status.StartBuildID = build.ID.String() + }}, + } { + key, workspaceID := e.readyMember(t) + member := e.reconcileMember(t, key, 0) + tc.edit(t, member, workspaceID) + require.NoError(t, k8sClient.Status().Update(e.ctx, member)) + e.deleteMember(t, key) + writes := e.coderWrites() + member = e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "OwnershipUnknown", "never deletes it") + requireDeletedCondition(t, member, metav1.ConditionUnknown, "OwnershipUnknown") + require.Zero(t, e.lastStep.RequeueAfter, tc.name) + reads := e.fake.totalRequests() + require.NotNil(t, e.reconcileMember(t, key, 2), "%s: the finalizer stays", tc.name) + require.Equal(t, reads, e.fake.totalRequests(), "%s: no Coder call after OwnershipUnknown", tc.name) + require.Equal(t, writes, e.coderWrites(), "%s: no Coder write", tc.name) + } + + // A person releases a member by removing its finalizer. While another + // finalizer keeps the object, the controller still never touches the + // workspace. + key, _ := e.readyMember(t) + member := e.reconcileMember(t, key, 0) + member.Finalizers = append(member.Finalizers, "test.coder.com/hold") + require.NoError(t, k8sClient.Update(e.ctx, member)) + e.deleteMember(t, key) + member = e.reconcileMember(t, key, 0) + member.Finalizers = []string{"test.coder.com/hold"} + require.NoError(t, k8sClient.Update(e.ctx, member)) + requests := e.fake.totalRequests() + member = e.reconcileMember(t, key, 2) + require.Equal(t, requests, e.fake.totalRequests(), "no Coder call after the finalizer is gone") + member.Finalizers = nil + require.NoError(t, k8sClient.Update(e.ctx, member)) +} + +// TestPoolMemberCleanupLastUseCheck is mutation check 11: before the first +// delete build of an unclaimed member, for example after its pool was +// deleted, the member reads last_used_at again. A used workspace stays in +// Coder, and the member releases its finalizer without a Coder write. +func TestPoolMemberCleanupLastUseCheck(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + key, workspaceID := e.readyMember(t) + e.fake.markUsed(workspaceID, e.clock.Now().Add(time.Minute)) + e.deleteMember(t, key) + require.Nil(t, e.reconcileMember(t, key, 1)) + require.Zero(t, e.coderWrites(), "a used workspace is never deleted") + + // A template-wide write is not use: the workspace is deleted. + key, _ = e.readyMember(t) + e.clock.SetTime(e.clock.Now().Add(2 * time.Minute)) + e.fake.touchTemplateWorkspaces(e.tplID, e.clock.Now()) + e.deleteMember(t, key) + requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build") +} + +// TestPoolMemberCleanupWithoutWorkspaceID: a member deleted while its create +// outcome is unknown confirms it by reads first, and a member whose control +// plane is gone is released without a Coder call. +func TestPoolMemberCleanupWithoutWorkspaceID(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + // The create took effect, but the answer was lost: the reads adopt the + // workspace, then the member deletes it. + key := e.uncertainMember(t, fakeFault{Status: 502, AfterCommit: true}, nil) + e.deleteMember(t, key) + member := e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "adopted workspace") + require.NotEmpty(t, member.Status.WorkspaceID) + e.reconcileMember(t, key, 1) + require.Equal(t, 1, e.fake.requestCount(routeCancelBuild), "the start build is canceled before the delete build") + + // No workspace appears: within the settle window the member reads again, + // after it the site owner's 404 proves that none exists. + key = e.uncertainMember(t, fakeFault{Status: 502}, nil) + marker := e.reconcileMember(t, key, 0).Status.CreateAttemptTime.Time + e.deleteMember(t, key) + requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "ConfirmingCreate", "") + require.Equal(t, 5*time.Second, e.lastStep.RequeueAfter) + e.clock.SetTime(marker.Add(15 * time.Minute)) + require.Nil(t, e.reconcileMember(t, key, 1)) + + // The control plane is gone: nothing can reach Coder. + e.clock.SetTime(time.Now().Truncate(time.Second)) + key, _ = e.readyMember(t) + e.deleteMember(t, key) + cp := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(e.ctx, types.NamespacedName{Namespace: e.ns, Name: "coder"}, cp)) + require.NoError(t, k8sClient.Delete(e.ctx, cp)) + requests := e.fake.totalRequests() + require.Nil(t, e.reconcileMember(t, key, 1)) + require.Equal(t, requests, e.fake.totalRequests()) +} diff --git a/internal/controller/coderworkspacepoolmember_controller.go b/internal/controller/coderworkspacepoolmember_controller.go index 8420148a..c43a31ac 100644 --- a/internal/controller/coderworkspacepoolmember_controller.go +++ b/internal/controller/coderworkspacepoolmember_controller.go @@ -67,12 +67,13 @@ func (r *CoderWorkspacePoolMemberReconciler) Reconcile(ctx context.Context, req return ctrl.Result{}, err } if !member.DeletionTimestamp.IsZero() { - // The delete steps (plan 6.5) come later. Until then, a member whose - // workspace can exist keeps its finalizer. - if poolMemberMayHaveWorkspace(member) { + if !controllerutil.ContainsFinalizer(member, coderv1alpha1.CoderWorkspacePoolMemberCleanupFinalizer) { + // Removing the finalizer is the way to release a member: the + // controller then never touches its workspace, even while another + // finalizer keeps the object. return ctrl.Result{}, nil } - return ctrl.Result{}, r.releaseFinalizer(ctx, member) + return r.cleanup(ctx, member) } switch member.Status.Phase { case coderv1alpha1.CoderWorkspacePoolMemberPhaseFailed: diff --git a/internal/controller/coderworkspacepoolmember_controller_test.go b/internal/controller/coderworkspacepoolmember_controller_test.go index 8834c444..59d05667 100644 --- a/internal/controller/coderworkspacepoolmember_controller_test.go +++ b/internal/controller/coderworkspacepoolmember_controller_test.go @@ -202,8 +202,8 @@ func TestPoolMemberCreatesWorkspace(t *testing.T) { requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseCreating, "WaitingForBuild", "") require.Equal(t, 1, e.fake.requestCount(routeCreateWorkspace), "never a second create request") - // A member whose workspace can exist keeps its finalizer on deletion - // until the delete steps run. + // A member whose workspace exists keeps its finalizer on deletion until + // the workspace is deleted. require.NoError(t, k8sClient.Delete(e.ctx, member)) require.NotNil(t, e.reconcileMember(t, key, 2)) }