diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 2871b84f..b6bdf639 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -139,7 +139,7 @@ The finalizer stays while Coder cannot confirm the deletion: When the control plane is gone, nothing can delete the keys. The finalizer goes at once, and the [cleanup record](#cleanup-records) stays. The operator logs `Releasing the finalizer without deleting the keys in Coder` with reason `ControlPlaneGone` and the key IDs: delete those keys with `coder ai-gateway keys delete`. -When you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys and providers as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first. +When you delete the control plane while `CoderAIGatewayKey`, `CoderAIProvider` or `CoderMCPServer` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys, providers and MCP server configs as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first. When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator runs one pass of at most 10 seconds without the settle window, then removes the finalizer whatever the answer. A key create that was still in flight at that moment can commit later, and its key stays in Coder. When the pass fails, the log lists the IDs that it did not delete. diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 4dfeef5e..ae933d10 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -124,7 +124,7 @@ Deleting the object deletes the provider in Coder. With only an unconfirmed crea - When Coder is unreachable, or the control plane has no operator access, the finalizer `coder.com/ai-provider-cleanup` stays and the operator retries every 60 seconds. See [Stuck finalizer](#stuck-finalizer). - When the control plane is gone, the operator cannot delete the provider now. The finalizer goes at once, and the operator logs the provider ID. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the provider. -- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. +- When you delete the control plane while `CoderAIProvider`, `CoderAIGatewayKey` or `CoderMCPServer` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator sends one delete with a 10-second limit, then removes the finalizer. Without working access, the finalizer goes without a delete. Either way a namespace deletion finishes. When the delete did not succeed, the operator logs the provider ID: delete the provider in the Coder dashboard. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts these releases. Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. A sweep deletes the leftover provider only when the namespace comes back with a control plane of the same Coder deployment. Coder deletes a provider even while chat models use it. Those models stop working. diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 2888ac79..5487e3bb 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -208,7 +208,7 @@ Deleting the object deletes the config in Coder. Coder removes the server from e - When Coder is unreachable, or the control plane has no operator access, the finalizer stays and the operator retries every 60 seconds. The operator trusts only Coder's own 404 as proof that the config is gone. A 404 from a proxy keeps the finalizer. - When the control plane is gone, nothing can delete the config. The finalizer goes at once. The operator logs the slug and the config ID with the reason `ControlPlaneGone`. Delete the config in the Coder dashboard. - When you delete the control plane while `CoderMCPServer` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus a grace period for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. -- When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed), the operator sends one delete with a 10-second limit, then removes the finalizer, also when that delete fails. So a namespace deletion finishes, but it can leave the config in Coder. +- When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator sends one delete with a 10-second limit, then removes the finalizer, also when that delete fails. Without working access, the finalizer goes without a delete. So a namespace deletion finishes, but it can leave the config in Coder. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts each release without a delete. - A withdrawn opt-in never blocks a delete. Delete `CoderMCPServer` objects before their control plane and before their namespace. diff --git a/internal/controller/coderaigatewaykey_record.go b/internal/controller/coderaigatewaykey_record.go index 9a010b1d..27c5811a 100644 --- a/internal/controller/coderaigatewaykey_record.go +++ b/internal/controller/coderaigatewaykey_record.go @@ -74,12 +74,14 @@ func (r *CoderAIGatewayKeyReconciler) recordIssuedKeyID(ctx context.Context, key } } -// recordAIGatewayKeyID writes the ID of a current key that Coder lists into -// the record. It never stops the reconcile: the record already holds the -// prefix, which finds the key too, so a failure is logged and retried. +// recordAIGatewayKeyID writes the ID of a current key that Coder lists, and a +// create time that the observe step stamped later (#353), into the record. It +// never stops the reconcile: the record already holds the prefix, which +// finds the key too, so a failure is logged and retried. func (r *CoderAIGatewayKeyReconciler) recordAIGatewayKeyID(ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey, prefix string) { record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, key.UID) - if err == nil && record != nil && record.CoderID == key.Status.CurrentKey.ID { + if err == nil && record != nil && record.CoderID == key.Status.CurrentKey.ID && + (key.Status.LastCreateAttemptTime == nil || !record.CreateAttemptTime.Before(key.Status.LastCreateAttemptTime.Time)) { return } if err == nil { diff --git a/internal/controller/coderaigatewaykey_record_test.go b/internal/controller/coderaigatewaykey_record_test.go index fad0c9fc..dcdcdfc3 100644 --- a/internal/controller/coderaigatewaykey_record_test.go +++ b/internal/controller/coderaigatewaykey_record_test.go @@ -227,3 +227,21 @@ func TestCoderAIGatewayKeyRecordBackfill(t *testing.T) { requireAIGatewayKeyReady(t, got) require.Equal(t, want, env.aiGatewayKeyRecord(ctx, t, got.UID)) } + +// #353 (#362 follow-up 9): when someone clears status.lastCreateAttemptTime +// by hand, the controller counts the settle window from now (fail closed), +// and the record gets that time too, so a sweep does not count from the +// older one. +func TestCoderAIGatewayKeyRecordAfterClearedCreateTime(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + requireAIGatewayKeyReady(t, got) + env.clock.Step(time.Hour) + got.Status.LastCreateAttemptTime = nil + require.NoError(t, k8sClient.Status().Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) + require.Equal(t, env.clock.Now().UTC(), got.Status.LastCreateAttemptTime.UTC()) + require.Equal(t, env.clock.Now().UTC().Format(time.RFC3339), env.aiGatewayKeyRecord(ctx, t, got.UID)["createAttemptTime"]) +} diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 7a67c086..e7b636af 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -83,16 +83,23 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // While the control plane holds for this object, Coder keeps running: // use the normal path, which retries, instead of the bounded pass // (#353, item q). - now := time.Now() - holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, now) + holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, time.Now()) if err != nil { return false, nil, err } deleting = !holds if holds { + // A pass under the hold ends at the hold end, however slowly + // Coder answers, so the bounded pass after it still fits in the + // control plane's grace period (#384 follow-up 4). + var cancel context.CancelFunc + ctx, cancel = context.WithDeadline(ctx, controlPlaneHoldEnd(controlPlane)) + defer cancel() defer func() { if step != nil { - step.requeue = controlPlaneHoldRequeue(controlPlane, now, step.requeue) + // From the end of the pass: a pass that ran until the + // deadline retries right after the hold end. + step.requeue = controlPlaneHoldRequeue(controlPlane, time.Now(), step.requeue) } }() } diff --git a/internal/controller/coderaiprovider_delete_internal_test.go b/internal/controller/coderaiprovider_delete_internal_test.go new file mode 100644 index 00000000..9d292436 --- /dev/null +++ b/internal/controller/coderaiprovider_delete_internal_test.go @@ -0,0 +1,69 @@ +package controller + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// #384 follow-up 4 for providers: a delete pass under the control plane hold +// ends at the hold end, also when Coder does not answer, and its retry counts +// from the end of the pass. Here the hold ends in 5 s and Coder hangs on the +// DELETE, whose own request timeout is 30 s. +func TestAIProviderDeleteUnderHoldEndsAtHoldEnd(t *testing.T) { + t.Parallel() + coder := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + <-r.Context().Done() + })) + t.Cleanup(coder.Close) + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, coderv1alpha1.AddToScheme(scheme)) + deleted := metav1.NewTime(time.Now().Add(5*time.Second - controlPlaneDependentHold)) + controlPlane := &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{ + Name: "coder", Namespace: "ns", UID: "cp-uid", DeletionTimestamp: &deleted, Finalizers: []string{workspaceRBACFinalizer}, + }} + controlPlane.Status.URL = coder.URL + controlPlane.Status.OperatorAccessReady = true + controlPlane.Status.OperatorTokenSecretRef = &coderv1alpha1.SecretKeySelector{Name: "coder-operator-token"} + token := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "coder-operator-token", Namespace: "ns", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(controlPlane, coderv1alpha1.GroupVersion.WithKind("CoderControlPlane"))}, + }, + Data: map[string][]byte{coderv1alpha1.DefaultTokenSecretKey: []byte("token")}, + } + namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns"}} + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(controlPlane, token, namespace).Build() + r := &CoderAIProviderReconciler{Client: c, APIReader: c, Scheme: scheme, OperatorNamespace: "coder-system"} + held, err := controlPlaneHolds(context.Background(), r.Client, controlPlane, time.Now()) + require.NoError(t, err) + require.True(t, held, "assertion failed: the test needs a control plane that holds") + + provider := &coderv1alpha1.CoderAIProvider{ObjectMeta: metav1.ObjectMeta{Name: "openai", Namespace: "ns", UID: "provider-uid"}} + provider.Spec.ControlPlaneRef.Name = "coder" + provider.Status.ProviderID = uuid.NewString() + start := time.Now() + release, step, err := r.deleteStep(context.Background(), provider) + require.NoError(t, err) + require.Less(t, time.Since(start), 15*time.Second, "the pass ends at the hold end, not at the 30 s request timeout") + require.False(t, release, "a pass under the hold never releases without a delete") + require.NotNil(t, step) + require.Equal(t, "CoderUnavailable", step.reason) + // The requeue counts from the end of the pass: the time left to the hold + // end, which is about 0 here, plus 1 s (controlPlaneHoldRequeue). + require.LessOrEqual(t, step.requeue, 2*time.Second, "the next pass runs right after the hold end") +} diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index cf4a9603..40b0a116 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -93,7 +93,10 @@ func (r *CoderAIProviderReconciler) ensureAIProviderRecord( if err != nil { return nil, err } - if record != nil && record.CoderID == provider.Status.ProviderID { + // A record without an ID names the deployment of a pending create, so + // the write checks the deployment ID before the create is confirmed in + // another deployment (#353). + if record != nil && record.CoderID != "" && record.CoderID == provider.Status.ProviderID { return nil, nil } return r.writeAIProviderRecord(ctx, sdk, provider, nil) diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index b30c3a76..0a21b382 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -288,3 +288,35 @@ func TestCoderAIProviderDeploymentSwitchKeepsRecord(t *testing.T) { require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in the new deployment") require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record stays for a sweep") } + +// #353 (#362 follow-up 7): a create whose answer was lost leaves a record +// without a provider ID. When the control plane then reaches another Coder +// deployment, where someone else's provider has the same name, that name +// conflict proves nothing about the old deployment. The controller keeps the +// marker and the record, so a delete keeps the record for a sweep. +func TestCoderAIProviderPendingCreateAfterDeploymentSwitch(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeCreateAIProvider, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "ConfirmingCreate") + before := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Empty(t, before["coderID"], "the create answer was lost") + + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + for _, p := range env.fake.aiProviders { + p.displayName = "Someone else's provider" + } + env.fake.mu.Unlock() + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireAIProviderStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) + require.NotNil(t, got.Status.CreateAttemptTime, "the marker stays") + require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID)) + + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in the new deployment") + require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record stays for a sweep") +} diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index afee054b..df1e7ff7 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -175,13 +175,18 @@ func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !controllerutil.ContainsFinalizer(server, coderv1alpha1.CoderMCPServerCleanupFinalizer) { return ctrl.Result{}, nil } + forgetReleasedWithoutDelete(server.UID) release, step, err := r.deleteStep(ctx, server) if err != nil { return ctrl.Result{}, err } if release { r.sentCredentials.Delete(req.NamespacedName) - return ctrl.Result{}, r.patchFinalizer(ctx, server, false) + if err := r.patchFinalizer(ctx, server, false); err != nil { + return ctrl.Result{}, err + } + releasedWithoutDeleteDone(server.UID) + return ctrl.Result{}, nil } applyMCPServerStep(server, step) return ctrl.Result{RequeueAfter: step.requeue}, r.writeStatus(ctx, server, before) diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index f1a3dae7..df3cd298 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -165,6 +165,7 @@ func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *c // releaseWithoutDelete logs what can stay in Coder, because nothing can // delete it any more, and returns true (ControlPlaneGone). func (r *CoderMCPServerReconciler) releaseWithoutDelete(ctx context.Context, server *coderv1alpha1.CoderMCPServer, why string) bool { + countReleasedWithoutDelete(server.UID, "CoderMCPServer") log.FromContext(ctx).Info("Releasing the finalizer without a delete in Coder: the MCP server config can stay in the Coder database", "reason", "ControlPlaneGone", "why", why, "slug", server.Spec.Slug, "configID", server.Status.ConfigID, "createPending", server.Status.CreateAttemptTime != nil) diff --git a/internal/controller/codermcpserver_delete_test.go b/internal/controller/codermcpserver_delete_test.go index a4616d89..2366b060 100644 --- a/internal/controller/codermcpserver_delete_test.go +++ b/internal/controller/codermcpserver_delete_test.go @@ -11,8 +11,10 @@ import ( "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/controller" @@ -306,6 +308,62 @@ func TestCoderMCPServerDeleteWhileControlPlaneDeletes(t *testing.T) { } } +// #384 follow-up 2 (#353): a release without a delete in Coder counts once in +// coder_k8s_cleanup_released_without_delete_total, after the finalizer patch +// succeeds. A release that a failed patch left recorded does not count when +// the retry deletes the config. Not parallel: other tests release MCP servers +// too. +func TestCoderMCPServerReleasedWithoutDeleteMetric(t *testing.T) { + ctx := context.Background() + failFirstPatch := func(t *testing.T, env *mcpServerEnv) { + t.Helper() + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + failed := false + env.r = &controller.CoderMCPServerReconciler{Scheme: scheme, Client: interceptor.NewClient(base, interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*coderv1alpha1.CoderMCPServer); ok && !failed { + failed = true + return apierrors.NewConflict(schema.GroupResource{Resource: "codermcpservers"}, obj.GetName(), nil) + } + return c.Patch(ctx, obj, patch, opts...) + }, + })} + } + t.Run("control plane gone", func(t *testing.T) { + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + before := releasedWithoutDelete(t, "CoderMCPServer") + require.NoError(t, k8sClient.Delete(ctx, env.controlPlane)) + require.NoError(t, k8sClient.Delete(ctx, got)) + failFirstPatch(t, &env) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.Error(t, err, "the finalizer patch fails once") + require.Equal(t, before, releasedWithoutDelete(t, "CoderMCPServer"), "no count before the finalizer is gone") + gone, _ := env.reconcileDeleting(ctx, t, got) + require.Nil(t, gone) + require.Len(t, env.fake.mcpServers(), 1, "the config stays in Coder") + require.Equal(t, before+1, releasedWithoutDelete(t, "CoderMCPServer")) + }) + t.Run("recovered", func(t *testing.T) { + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + before := releasedWithoutDelete(t, "CoderMCPServer") + holdControlPlane(ctx, t, env.namespace) + require.NoError(t, k8sClient.Delete(ctx, got)) + env.fake.failNext(routeDeleteMCPServer, fakeFault{Status: http.StatusBadGateway}) + failFirstPatch(t, &env) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.Error(t, err, "the release after the failed bounded attempt fails its finalizer patch") + gone, _ := env.reconcileDeleting(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.mcpServers(), "the retry deleted the config") + require.Equal(t, before, releasedWithoutDelete(t, "CoderMCPServer"), "a release that deleted in Coder does not count") + }) +} + // #382 (#353): a control plane that is being deleted waits for a // CoderMCPServer that is being deleted, so Coder keeps running. Under that // hold the object takes the normal delete path: a failed DELETE keeps the diff --git a/internal/controller/controlplane_hold.go b/internal/controller/controlplane_hold.go index c2a93511..75f7aade 100644 --- a/internal/controller/controlplane_hold.go +++ b/internal/controller/controlplane_hold.go @@ -53,8 +53,8 @@ const controlPlaneLiveDependentWait = 30 * time.Second // its dependents. A dependent's removal also requeues it at once. const controlPlaneDependentPoll = 10 * time.Second -// releasedWithoutDelete counts finalizers that went while their keys or -// providers can stay in Coder (#353), by kind. Today the release log line is +// releasedWithoutDelete counts finalizers that went while the object's +// resources can stay in Coder (#353), by kind. Today the release log line is // the only other signal. countReleasedWithoutDelete records a release, and // releasedWithoutDeleteDone counts it once the finalizer is gone, so a // failed finalizer patch and its retry count once. Each delete reconcile diff --git a/internal/controller/controlplane_hold_internal_test.go b/internal/controller/controlplane_hold_internal_test.go index 495bf6b6..47b565b4 100644 --- a/internal/controller/controlplane_hold_internal_test.go +++ b/internal/controller/controlplane_hold_internal_test.go @@ -2,13 +2,16 @@ package controller import ( "context" + "fmt" "testing" "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" @@ -24,6 +27,45 @@ func TestControlPlaneDependentGraceFor(t *testing.T) { require.Equal(t, 5*time.Minute, controlPlaneDependentGraceFor(1000), "the grace period has a cap") } +// #384 follow-up 5: the grace period counts the dependents of the largest +// kind, whichever kind that is. A dependent of another control plane does not +// count. +func TestControlPlaneDependentsPerKind(t *testing.T) { + t.Parallel() + scheme := runtime.NewScheme() + require.NoError(t, coderv1alpha1.AddToScheme(scheme)) + deleted := metav1.NewTime(time.Now()) + controlPlane := &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "coder", Namespace: "ns", DeletionTimestamp: &deleted}} + kinds := []func(name, controlPlane string) client.Object{ + func(name, controlPlane string) client.Object { + return &coderv1alpha1.CoderAIGatewayKey{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, Spec: coderv1alpha1.CoderAIGatewayKeySpec{ControlPlaneRef: coderv1alpha1.CoderControlPlaneReference{Name: controlPlane}}} + }, + func(name, controlPlane string) client.Object { + return &coderv1alpha1.CoderAIProvider{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, Spec: coderv1alpha1.CoderAIProviderSpec{ControlPlaneRef: coderv1alpha1.CoderControlPlaneReference{Name: controlPlane}}} + }, + func(name, controlPlane string) client.Object { + return &coderv1alpha1.CoderMCPServer{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, Spec: coderv1alpha1.CoderMCPServerSpec{ControlPlaneRef: coderv1alpha1.CoderControlPlaneReference{Name: controlPlane}}} + }, + } + for name, counts := range map[string][3]int{"keys": {3, 1, 2}, "providers": {1, 3, 2}, "MCP servers": {1, 2, 3}} { + t.Run(name+" largest", func(t *testing.T) { + t.Parallel() + var objects []client.Object + for kind, count := range counts { + for i := range count { + objects = append(objects, kinds[kind](fmt.Sprintf("d%d", i), "coder")) + } + objects = append(objects, kinds[kind]("other", "another-control-plane")) + } + reader := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + dependents, perKind, err := controlPlaneDependents(context.Background(), reader, controlPlane, deleted.Time) + require.NoError(t, err) + require.Len(t, dependents, 6) + require.Equal(t, 3, perKind) + }) + } +} + // #353, #382: the removal of each dependent kind requeues the control plane // that it references, so a waiting control plane goes at once. func TestRequestForDependentControlPlane(t *testing.T) { diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index 2ac02db0..1e2ecb5c 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -2,11 +2,13 @@ package controller_test import ( "context" + "errors" "net/http" "testing" "time" "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -25,11 +27,23 @@ import ( const workspaceRBACFinalizer = "coder.com/workspace-rbac-cleanup" -// deleteFinalizedControlPlane gives the control plane its own finalizer, as -// its reconciler does, and deletes it, so it stays Terminating. +// deleteFinalizedControlPlane gives the control plane its own finalizer and +// its Coder Deployment, as its reconciler does, and deletes it, so it stays +// Terminating. func deleteFinalizedControlPlane(ctx context.Context, t *testing.T, namespace, name string) { t.Helper() key := client.ObjectKey{Namespace: namespace, Name: name} + labels := map[string]string{"app": name} + require.NoError(t, k8sClient.Create(ctx, &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "coder", Image: "coder:test"}}}, + }, + }, + })) controlPlane := &coderv1alpha1.CoderControlPlane{} require.NoError(t, k8sClient.Get(ctx, key, controlPlane)) controlPlane.Finalizers = append(controlPlane.Finalizers, workspaceRBACFinalizer) @@ -224,6 +238,143 @@ func TestControlPlaneForegroundDeletionDoesNotHold(t *testing.T) { require.NotContains(t, controlPlane.Finalizers, workspaceRBACFinalizer, "no hold under foreground deletion") } +// #353: a status without operator access does not end the hold. A provider +// cleanup that read the operator token before a Postgres failure cleared +// operator access still calls Coder, so the deleted control plane keeps +// Coder running for it. +func TestControlPlaneHoldsForCleanupInFlight(t *testing.T) { + ensureGatewaySchemeRegistered(t) + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + ref := client.ObjectKey{Namespace: env.namespace, Name: "coder"} + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, ref, controlPlane)) + controlPlane.Spec.ExtraEnv = []corev1.EnvVar{{Name: "CODER_PG_CONNECTION_URL", Value: "postgres://example/coder"}} + require.NoError(t, k8sClient.Update(ctx, controlPlane)) + + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + tokenRead := false + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := c.Get(ctx, key, obj, opts...); err != nil { + return err + } + if _, ok := obj.(*corev1.Secret); !ok || key.Name != "coder-operator-token" || tokenRead { + return nil + } + tokenRead = true + provisioner := &fakeOperatorAccessProvisioner{err: errors.New("dial tcp: connection refused")} + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, APIReader: k8sClient, Scheme: scheme, OperatorAccessProvisioner: provisioner} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: ref}) + require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, ref, controlPlane)) + require.False(t, controlPlane.Status.OperatorAccessReady, "a Postgres failure clears operator access") + + require.NoError(t, k8sClient.Delete(ctx, controlPlane)) + provisioner.err = nil + result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: ref}) + require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, ref, controlPlane), "the control plane holds for the cleanup in flight") + require.Equal(t, 10*time.Second, result.RequeueAfter) + return nil + }, + }) + + got, _ := env.deleteAIProvider(ctx, t, provider) + require.True(t, tokenRead) + require.Nil(t, got) + _, found := env.fake.aiProvider("openai") + require.False(t, found, "the cleanup deletes the provider in Coder") +} + +// orphanCoderDeployment deletes the Coder Deployment as `kubectl delete +// --cascade=orphan` does, which leaves Coder's pods running. Envtest has no +// garbage collector, so the test removes the orphan finalizer itself. +func orphanCoderDeployment(ctx context.Context, t *testing.T, namespace string) { + t.Helper() + deployment := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: "coder"}} + require.NoError(t, k8sClient.Delete(ctx, deployment, client.PropagationPolicy(metav1.DeletePropagationOrphan))) + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), deployment); err == nil { + deployment.Finalizers = nil + require.NoError(t, k8sClient.Update(ctx, deployment)) + } + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), &appsv1.Deployment{}))) +} + +// #353: a missing Coder Deployment does not end the hold. Coder's pods can +// still run (an orphan delete), so the control plane keeps its finalizer, +// also when it reconciles before its deleted dependents, and they delete +// their resources in Coder with retries. +func TestControlPlaneHoldsWithoutCoderDeployment(t *testing.T) { + t.Parallel() + controlPlaneHolds := func(ctx context.Context, t *testing.T, namespace string) { + t.Helper() + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme} + ref := client.ObjectKey{Namespace: namespace, Name: "coder"} + result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: ref}) + require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, ref, &coderv1alpha1.CoderControlPlane{}), "the control plane holds") + require.Equal(t, 10*time.Second, result.RequeueAfter) + } + t.Run("keys", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + key := rotatedKey(ctx, t, &env) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + orphanCoderDeployment(ctx, t, env.namespace) + require.NoError(t, k8sClient.Delete(ctx, key)) + controlPlaneHolds(ctx, t, env.namespace) + env.clock.Step(5 * time.Minute) + gone, _ := env.reconcile(ctx, t, key) + require.Nil(t, gone) + require.Empty(t, env.fake.aiGatewayKeys()) + }) + t.Run("provider", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + orphanCoderDeployment(ctx, t, env.namespace) + require.NoError(t, k8sClient.Delete(ctx, provider)) + controlPlaneHolds(ctx, t, env.namespace) + gone, _ := env.reconcileDeleting(ctx, t, provider) + require.Nil(t, gone) + _, found := env.fake.aiProvider("openai") + require.False(t, found) + }) + t.Run("MCP server", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server, _ := env.applied(ctx, t, nil) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + orphanCoderDeployment(ctx, t, env.namespace) + require.NoError(t, k8sClient.Delete(ctx, server)) + controlPlaneHolds(ctx, t, env.namespace) + gone, _ := env.reconcileDeleting(ctx, t, server) + require.Nil(t, gone) + require.Empty(t, env.fake.mcpServers()) + }) + t.Run("provider retry", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + orphanCoderDeployment(ctx, t, env.namespace) + env.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusBadGateway}) + got, _ := env.deleteAIProvider(ctx, t, provider) + require.NotNil(t, got, "a failed delete keeps the finalizer while the control plane holds") + got, _ = env.reconcileDeleting(ctx, t, got) + require.Nil(t, got) + _, found := env.fake.aiProvider("openai") + require.False(t, found) + }) +} + // #353: when the hold ends, the control plane stays for a short grace period // while dependents remain, and a dependent under the hold requeues before the // hold ends, so it runs its bounded pass while Coder still runs.