From b457bee9ebd78d1a8019907623e34f12db2630cf Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 12:45:17 +0000 Subject: [PATCH 1/5] fix: hold a deleted CoderControlPlane while AI Gateway keys and providers still use it Change-Id: I164b54a6e97adda127ff939ebe618143a6ccfee3 Signed-off-by: Thomas Kosiewski --- .github/workflows/ci.yaml | 1 + docs/how-to/manage-ai-gateway-keys.md | 8 +- docs/how-to/manage-ai-providers.md | 5 +- go.mod | 2 +- .../controller/coderaigatewaykey_delete.go | 13 +- internal/controller/coderaiprovider_delete.go | 13 +- .../codercontrolplane_controller.go | 25 ++- internal/controller/controlplane_hold.go | 116 ++++++++++++ internal/controller/controlplane_hold_test.go | 179 ++++++++++++++++++ 9 files changed, 354 insertions(+), 8 deletions(-) create mode 100644 internal/controller/controlplane_hold.go create mode 100644 internal/controller/controlplane_hold_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 645ec1401..b100bf667 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -91,6 +91,7 @@ jobs: - 'internal/controller/reserved_secret_names.go' # Shared Coder access that CoderTemplateTest and later pool controllers call. - 'internal/controller/controlplane_access.go' + - 'internal/controller/controlplane_hold.go' - 'internal/controller/coder_access.go' - 'internal/controller/coder_workspace_steps.go' # The URL that the aggregated API server and the controllers use for Coder API calls. diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 80a03755e..bc26dbefb 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -139,7 +139,11 @@ 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. 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`. -While the control plane is being deleted 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. +When you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects use it, the control plane waits for them 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. After 10 minutes the control plane goes anyway. + +When the control plane does not wait (its namespace is being deleted, 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. + +Each release without a delete counts in the controller metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`), next to the log line. !!! warning "Delete the keys before the namespace" When you delete the namespace, Kubernetes deletes the control plane and the `CoderAIGatewayKey` objects at the same time. The control plane is usually gone before the operator handles the objects, so the finalizers go without a delete, and the keys stay valid in Coder. The namespace deletion finishes, and the operator logs the key IDs with reason `ControlPlaneGone`. @@ -157,7 +161,7 @@ If the namespace is already gone, delete the logged keys with `coder ai-gateway A deletion that keeps failing never ends by itself. Leave it in one of these ways: 1. **Fix Coder or the operator user.** Restore Coder access, the Premium license or the AI Gateway, the operator user's site-owner role, or the proxy that answers `404`. The next retry deletes the keys and releases the object. -2. **Delete the control plane.** The operator then runs its one bounded pass and releases the object. Do this only when you remove that Coder deployment anyway. +2. **Delete the control plane.** The control plane first waits up to 10 minutes for its objects, then the operator runs its one bounded pass and releases the object. Do this only when you remove that Coder deployment anyway. 3. **Remove the finalizer by hand, then delete the keys in Coder.** Note the key names from `status.currentKey` and `status.retiringKeys`, and the object's UID (`kubectl -n coder get coderaigatewaykey gateway -o jsonpath='{.metadata.uid}'`). Find the index of the finalizer: diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 0bc9c1572..a93612afe 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,9 +122,10 @@ 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, nothing can delete the provider. The finalizer goes at once, and the operator logs the provider ID. Delete the provider in the Coder dashboard. -- While the control plane is being deleted 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. +- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects use it, the control plane waits for them for up to 10 minutes. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. +- When the control plane does not wait (its namespace is being deleted, 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 control plane. Coder deletes a provider even while chat models use it. Those models stop working. +Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. Coder deletes a provider even while chat models use it. Those models stop working. ### Stuck finalizer diff --git a/go.mod b/go.mod index 0a8773536..bf98ff622 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/google/uuid v1.6.0 github.com/lib/pq v1.10.9 github.com/modelcontextprotocol/go-sdk v1.7.0 + github.com/prometheus/client_golang v1.24.1 github.com/stretchr/testify v1.12.0 github.com/yuin/gopher-lua v1.1.2 golang.org/x/xerrors v0.0.0-20240903120638-7835f813f4da @@ -293,7 +294,6 @@ require ( github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55 // indirect - github.com/prometheus/client_golang v1.24.1 // indirect github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.70.1 // indirect github.com/prometheus/procfs v0.21.1 // indirect diff --git a/internal/controller/coderaigatewaykey_delete.go b/internal/controller/coderaigatewaykey_delete.go index 9cb82b895..6fb025c8a 100644 --- a/internal/controller/coderaigatewaykey_delete.go +++ b/internal/controller/coderaigatewaykey_delete.go @@ -42,7 +42,17 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder return false, nil, err } deleting := !controlPlane.DeletionTimestamp.IsZero() - sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, deleting) + if deleting { + // 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). + holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, r.Clock.Now()) + if err != nil { + return false, nil, err + } + deleting = !holds + } + sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, !controlPlane.DeletionTimestamp.IsZero()) switch { case err != nil && deleting: return r.releaseWithoutDelete(ctx, key, "the Coder access of CoderControlPlane, which is being deleted, failed: "+err.Error()), nil, nil @@ -146,6 +156,7 @@ func (r *CoderAIGatewayKeyReconciler) releaseWithoutDelete(ctx context.Context, for _, retiring := range key.Status.RetiringKeys { ids = append(ids, retiring.ID) } + releasedWithoutDelete.WithLabelValues("CoderAIGatewayKey").Inc() log.FromContext(ctx).Info("Releasing the finalizer without deleting the keys in Coder: the keys can stay in Coder", "reason", "ControlPlaneGone", "why", why, "keyIDs", ids, "listedKeyIDs", left, "nextSerial", key.Status.NextSerial) return true diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 9fee2b641..a73ecffb5 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -70,7 +70,17 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co return false, nil, err } deleting := !controlPlane.DeletionTimestamp.IsZero() - sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, deleting) + if deleting { + // 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). + holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, time.Now()) + if err != nil { + return false, nil, err + } + deleting = !holds + } + sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, !controlPlane.DeletionTimestamp.IsZero()) switch { case err != nil && deleting: // One attempt only, also when the access setup fails (plan D12). @@ -155,6 +165,7 @@ func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *code // releaseWithoutDelete logs what can stay in Coder, because nothing can // delete it any more, and returns true (ControlPlaneGone). func (r *CoderAIProviderReconciler) releaseWithoutDelete(ctx context.Context, provider *coderv1alpha1.CoderAIProvider, why string) bool { + releasedWithoutDelete.WithLabelValues("CoderAIProvider").Inc() log.FromContext(ctx).Info("Releasing the finalizer without a delete in Coder: the provider can stay in the Coder database", "reason", "ControlPlaneGone", "why", why, "provider", provider.Name, "providerID", provider.Status.ProviderID, "createPending", provider.Status.CreateAttemptTime != nil) diff --git a/internal/controller/codercontrolplane_controller.go b/internal/controller/codercontrolplane_controller.go index 6009f0fa4..bf62c1308 100644 --- a/internal/controller/codercontrolplane_controller.go +++ b/internal/controller/codercontrolplane_controller.go @@ -29,9 +29,11 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" + ctrlbuilder "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -603,6 +605,22 @@ func (r *CoderControlPlaneReconciler) finalizeWorkspaceRBAC( if !controllerutil.ContainsFinalizer(coderControlPlane, workspaceRBACFinalizer) { return ctrl.Result{}, nil } + // The finalizer keeps the Coder Deployment (owned by this object) + // running while AI Gateway keys and AI providers that use it still + // delete their resources in Coder (#353). + if holds, err := controlPlaneHolds(ctx, r.Client, coderControlPlane, time.Now()); err != nil { + return ctrl.Result{}, err + } else if holds { + dependents, err := controlPlaneDependents(ctx, r.Client, coderControlPlane) + if err != nil { + return ctrl.Result{}, err + } + if len(dependents) > 0 { + log.FromContext(ctx).Info("Waiting for the objects that use this control plane to delete their resources in Coder", + "dependents", dependents, "until", coderControlPlane.DeletionTimestamp.Add(controlPlaneDependentHold).UTC()) + return ctrl.Result{RequeueAfter: controlPlaneDependentPoll}, nil + } + } if err := r.cleanupManagedWorkspaceRBAC(ctx, coderControlPlane, nil, nil); err != nil { return ctrl.Result{}, err @@ -3269,7 +3287,12 @@ func (r *CoderControlPlaneReconciler) SetupWithManager(mgr ctrl.Manager) error { Watches( &corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.reconcileRequestsForEnvFromConfigMap), - ) + ). + // A control plane that is being deleted waits for these (#353). + Watches(&coderv1alpha1.CoderAIGatewayKey{}, handler.EnqueueRequestsFromMapFunc(requestForDependentControlPlane), + ctrlbuilder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderAIProvider{}, handler.EnqueueRequestsFromMapFunc(requestForDependentControlPlane), + ctrlbuilder.WithPredicates(dependentRemoved)) // Gateway API is optional; only watch HTTPRoutes when the CRD is installed. httpRouteGVK := schema.GroupVersionKind{Group: gatewayv1.GroupVersion.Group, Version: gatewayv1.GroupVersion.Version, Kind: "HTTPRoute"} diff --git a/internal/controller/controlplane_hold.go b/internal/controller/controlplane_hold.go new file mode 100644 index 000000000..a475414c3 --- /dev/null +++ b/internal/controller/controlplane_hold.go @@ -0,0 +1,116 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/prometheus/client_golang/prometheus" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/metrics" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// controlPlaneDependentHold is how long a CoderControlPlane that is being +// deleted waits for the CoderAIGatewayKey and CoderAIProvider objects that +// use it (#353). Its Coder Deployment keeps running while it waits, so they +// can delete their keys and providers in Coder. It covers the 5-minute +// settle window of a key deletion plus retries. After it, the control plane +// goes, and they release with ControlPlaneGone. +const controlPlaneDependentHold = 10 * time.Minute + +// controlPlaneDependentPoll is the requeue of a control plane that waits for +// 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 +// the only other signal. +var releasedWithoutDelete = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "coder_k8s_cleanup_released_without_delete_total", + Help: "Finalizers released without deleting the object's resources in Coder, because Coder was not reachable.", +}, []string{"kind"}) + +func init() { + metrics.Registry.MustRegister(releasedWithoutDelete) +} + +// controlPlaneHolds reports whether controlPlane, which is being deleted, +// waits for its dependents at now: it still has its finalizer, the hold time +// has not passed, and its namespace is not being deleted. A namespace +// deletion removes Coder's pods at once, so no hold can keep Coder +// reachable then, and a hold would only delay the namespace. +func controlPlaneHolds(ctx context.Context, reader client.Reader, controlPlane *coderv1alpha1.CoderControlPlane, now time.Time) (bool, error) { + if reader == nil || controlPlane == nil { + return false, errors.New("assertion failed: the control plane hold check needs a reader and a control plane") + } + if controlPlane.DeletionTimestamp.IsZero() { + return false, errors.New("assertion failed: the control plane hold applies only while the control plane is being deleted") + } + if !controllerutil.ContainsFinalizer(controlPlane, workspaceRBACFinalizer) || !now.Before(controlPlane.DeletionTimestamp.Add(controlPlaneDependentHold)) { + return false, nil + } + namespace := &corev1.Namespace{} + if err := reader.Get(ctx, types.NamespacedName{Name: controlPlane.Namespace}, namespace); err != nil { + return false, fmt.Errorf("get namespace %s: %w", controlPlane.Namespace, err) + } + return namespace.DeletionTimestamp.IsZero() && namespace.Status.Phase != corev1.NamespaceTerminating, nil +} + +// controlPlaneDependents returns the CoderAIGatewayKey and CoderAIProvider +// objects in controlPlane's namespace that reference it, as kind/name. +func controlPlaneDependents(ctx context.Context, reader client.Reader, controlPlane *coderv1alpha1.CoderControlPlane) ([]string, error) { + var dependents []string + keys := &coderv1alpha1.CoderAIGatewayKeyList{} + if err := reader.List(ctx, keys, client.InNamespace(controlPlane.Namespace)); err != nil { + return nil, fmt.Errorf("list AI Gateway keys of namespace %s: %w", controlPlane.Namespace, err) + } + for _, k := range keys.Items { + if k.Spec.ControlPlaneRef.Name == controlPlane.Name { + dependents = append(dependents, "CoderAIGatewayKey/"+k.Name) + } + } + providers := &coderv1alpha1.CoderAIProviderList{} + if err := reader.List(ctx, providers, client.InNamespace(controlPlane.Namespace)); err != nil { + return nil, fmt.Errorf("list AI providers of namespace %s: %w", controlPlane.Namespace, err) + } + for _, p := range providers.Items { + if p.Spec.ControlPlaneRef.Name == controlPlane.Name { + dependents = append(dependents, "CoderAIProvider/"+p.Name) + } + } + return dependents, nil +} + +// dependentRemoved passes only the removal of a dependent object, the event +// that can end a control plane's hold. +var dependentRemoved = predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { return false }, + UpdateFunc: func(event.UpdateEvent) bool { return false }, + DeleteFunc: func(event.DeleteEvent) bool { return true }, + GenericFunc: func(event.GenericEvent) bool { return false }, +} + +// requestForDependentControlPlane maps a CoderAIGatewayKey or CoderAIProvider +// to the control plane that it references. +func requestForDependentControlPlane(_ context.Context, obj client.Object) []reconcile.Request { + var name string + switch o := obj.(type) { + case *coderv1alpha1.CoderAIGatewayKey: + name = o.Spec.ControlPlaneRef.Name + case *coderv1alpha1.CoderAIProvider: + name = o.Spec.ControlPlaneRef.Name + } + if name == "" { + return nil + } + return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: obj.GetNamespace(), Name: name}}} +} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go new file mode 100644 index 000000000..db1e85d68 --- /dev/null +++ b/internal/controller/controlplane_hold_test.go @@ -0,0 +1,179 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/metrics" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +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. +func deleteFinalizedControlPlane(ctx context.Context, t *testing.T, namespace, name string) { + t.Helper() + key := client.ObjectKey{Namespace: namespace, Name: name} + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, key, controlPlane)) + controlPlane.Finalizers = append(controlPlane.Finalizers, workspaceRBACFinalizer) + require.NoError(t, k8sClient.Update(ctx, controlPlane)) + require.NoError(t, k8sClient.Delete(ctx, controlPlane)) + t.Cleanup(func() { + if k8sClient.Get(context.Background(), key, controlPlane) == nil { + controlPlane.Finalizers = nil + _ = k8sClient.Update(context.Background(), controlPlane) + } + }) +} + +func releasedWithoutDelete(t *testing.T, kind string) float64 { + t.Helper() + families, err := metrics.Registry.Gather() + require.NoError(t, err) + for _, f := range families { + if f.GetName() != "coder_k8s_cleanup_released_without_delete_total" { + continue + } + for _, m := range f.GetMetric() { + for _, l := range m.GetLabel() { + if l.GetName() == "kind" && l.GetValue() == kind { + return m.GetCounter().GetValue() + } + } + } + } + return 0 +} + +// #353: a control plane that is being deleted keeps its finalizer, and so its +// Coder Deployment, while a CoderAIGatewayKey or CoderAIProvider uses it. A +// dependent of another control plane does not hold it. A namespace deletion +// does not wait: it removes Coder's pods anyway. +func TestControlPlaneWaitsForDependents(t *testing.T) { + t.Parallel() + ctx := context.Background() + reconcileGone := func(t *testing.T, namespace string) (ctrl.Result, bool) { + t.Helper() + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme} + result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: namespace, Name: "coder"}}) + require.NoError(t, err) + err = k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "coder"}, &coderv1alpha1.CoderControlPlane{}) + return result, apierrors.IsNotFound(err) + } + t.Run("dependents", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + key := env.createKey(ctx, t, "coder", testAIGatewaySecret) + other := env.createNamedKey(ctx, t, "other", "another-control-plane", "other-secret") + provider := &coderv1alpha1.CoderAIProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "openai", Namespace: env.namespace}, + Spec: coderv1alpha1.CoderAIProviderSpec{ + ControlPlaneRef: coderv1alpha1.CoderControlPlaneReference{Name: "coder"}, Type: "openai", BaseURL: "https://api.openai.com/v1", + APIKeySecretRef: coderv1alpha1.SecretKeySelector{Name: "openai-key", Key: "api-key"}, + }, + } + require.NoError(t, k8sClient.Create(ctx, provider)) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + + result, gone := reconcileGone(t, env.namespace) + require.False(t, gone, "the control plane waits for its key and provider") + require.Equal(t, 10*time.Second, result.RequeueAfter) + require.NoError(t, k8sClient.Delete(ctx, key)) + _, gone = reconcileGone(t, env.namespace) + require.False(t, gone, "the control plane waits for its provider") + require.NoError(t, k8sClient.Delete(ctx, provider)) + _, gone = reconcileGone(t, env.namespace) + require.True(t, gone, "a key of another control plane does not hold this one") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(other), other)) + }) + t.Run("namespace deletion", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + env.createKey(ctx, t, "coder", testAIGatewaySecret) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + require.NoError(t, k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: env.namespace}})) + _, gone := reconcileGone(t, env.namespace) + require.True(t, gone, "a namespace deletion does not wait") + }) +} + +// #353: while the control plane holds for it, a deleted key takes the normal +// path: it waits for the settle window, then deletes every key, instead of +// one bounded pass. A control plane without operator access makes it wait +// and retry (item q), not release. +func TestCoderAIGatewayKeyDeleteUnderControlPlaneHold(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got := rotatedKey(ctx, t, &env) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + require.NoError(t, k8sClient.Delete(ctx, got)) + + got, result := env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, false, "DeletingKeys") + require.Equal(t, 5*time.Minute, result.RequeueAfter, "the settle window, not the bounded pass") + require.Len(t, env.fake.aiGatewayKeys(), 2) + + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "coder"}, controlPlane)) + controlPlane.Status.OperatorAccessReady = false + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) + env.clock.Step(5 * time.Minute) + got, result = env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, false, "OperatorAccessNotReady") + require.Equal(t, time.Minute, result.RequeueAfter) + require.Len(t, env.fake.aiGatewayKeys(), 2, "no release without a delete") + + controlPlane.Status.OperatorAccessReady = true + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) + gone, _ := env.reconcile(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.aiGatewayKeys()) +} + +// #353: while the control plane holds for it, a failed provider delete +// retries instead of releasing after one attempt. +func TestCoderAIProviderDeleteUnderControlPlaneHold(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + env.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusBadGateway}) + got, result := env.deleteAIProvider(ctx, t, provider) + require.NotNil(t, got, "a failed delete keeps the finalizer while the control plane waits") + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Equal(t, 60*time.Second, result.RequeueAfter) + got, _ = env.reconcileDeleting(ctx, t, got) + require.Nil(t, got) + _, found := env.fake.aiProvider("openai") + require.False(t, found) + require.Equal(t, 2, env.fake.requestCount(routeDeleteAIProvider)) +} + +// #353: a release without a delete in Coder counts in +// coder_k8s_cleanup_released_without_delete_total. +func TestReleasedWithoutDeleteMetric(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got := rotatedKey(ctx, t, &env) + before := releasedWithoutDelete(t, "CoderAIGatewayKey") + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + require.NoError(t, k8sClient.Delete(ctx, got)) + gone, _ := env.reconcile(ctx, t, got) + require.Nil(t, gone) + require.Greater(t, releasedWithoutDelete(t, "CoderAIGatewayKey"), before) +} From 1874d474e23617acf4c7f24cb9d1e3d23ddb5aec Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 13:05:40 +0000 Subject: [PATCH 2/5] fix: count releases after the finalizer goes, skip the hold for foreground deletes, and give dependents a grace period Change-Id: I3364c141387c6cd90d64e6a0ba2e785549b46ecf Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 4 +- docs/how-to/manage-ai-providers.md | 4 +- .../coderaigatewaykey_controller.go | 6 +- .../controller/coderaigatewaykey_delete.go | 12 ++- .../controller/coderaiprovider_controller.go | 6 +- internal/controller/coderaiprovider_delete.go | 12 ++- .../codercontrolplane_controller.go | 16 +++- internal/controller/controlplane_hold.go | 46 ++++++++-- internal/controller/controlplane_hold_test.go | 85 ++++++++++++++++++- 9 files changed, 170 insertions(+), 21 deletions(-) diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index bc26dbefb..ad4020dbd 100644 --- a/docs/how-to/manage-ai-gateway-keys.md +++ b/docs/how-to/manage-ai-gateway-keys.md @@ -139,9 +139,9 @@ 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. 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 use it, the control plane waits for them 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. After 10 minutes the control plane goes anyway. +When you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects use it, the control plane waits for them 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. After 10 minutes, the objects that remain get 30 more seconds 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, 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. +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. Each release without a delete counts in the controller metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`), next to the log line. diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index a93612afe..f8ddededf 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,8 +122,8 @@ 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, nothing can delete the provider. The finalizer goes at once, and the operator logs the provider ID. Delete the provider in the Coder dashboard. -- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects use it, the control plane waits for them for up to 10 minutes. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. -- When the control plane does not wait (its namespace is being deleted, 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. +- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects use it, the control plane waits for them for up to 10 minutes, plus 30 seconds for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. 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. Coder deletes a provider even while chat models use it. Those models stop working. diff --git a/internal/controller/coderaigatewaykey_controller.go b/internal/controller/coderaigatewaykey_controller.go index 8c5b12e73..a7290fa1c 100644 --- a/internal/controller/coderaigatewaykey_controller.go +++ b/internal/controller/coderaigatewaykey_controller.go @@ -167,7 +167,11 @@ func (r *CoderAIGatewayKeyReconciler) Reconcile(ctx context.Context, req ctrl.Re if err != nil { return ctrl.Result{}, err } else if release { - return ctrl.Result{}, r.patchFinalizer(ctx, key, false) + if err := r.patchFinalizer(ctx, key, false); err != nil { + return ctrl.Result{}, err + } + releasedWithoutDeleteDone(key.UID) + return ctrl.Result{}, nil } step = deleteStep } else { diff --git a/internal/controller/coderaigatewaykey_delete.go b/internal/controller/coderaigatewaykey_delete.go index 6fb025c8a..b76597c6b 100644 --- a/internal/controller/coderaigatewaykey_delete.go +++ b/internal/controller/coderaigatewaykey_delete.go @@ -46,11 +46,19 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder // 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). - holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, r.Clock.Now()) + now := r.Clock.Now() + holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, now) if err != nil { return false, nil, err } deleting = !holds + if holds { + defer func() { + if step != nil { + step.requeue = controlPlaneHoldRequeue(controlPlane, now, step.requeue) + } + }() + } } sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, !controlPlane.DeletionTimestamp.IsZero()) switch { @@ -156,7 +164,7 @@ func (r *CoderAIGatewayKeyReconciler) releaseWithoutDelete(ctx context.Context, for _, retiring := range key.Status.RetiringKeys { ids = append(ids, retiring.ID) } - releasedWithoutDelete.WithLabelValues("CoderAIGatewayKey").Inc() + countReleasedWithoutDelete(key.UID, "CoderAIGatewayKey") log.FromContext(ctx).Info("Releasing the finalizer without deleting the keys in Coder: the keys can stay in Coder", "reason", "ControlPlaneGone", "why", why, "keyIDs", ids, "listedKeyIDs", left, "nextSerial", key.Status.NextSerial) return true diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 914fec42f..5d0095d4e 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -161,7 +161,11 @@ func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if release { r.sentKeys.Delete(provider.UID) - return ctrl.Result{}, r.patchFinalizer(ctx, provider, false) + if err := r.patchFinalizer(ctx, provider, false); err != nil { + return ctrl.Result{}, err + } + releasedWithoutDeleteDone(provider.UID) + return ctrl.Result{}, nil } applyAIProviderStep(provider, step) return ctrl.Result{RequeueAfter: step.requeue}, r.writeStatus(ctx, provider, before) diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index a73ecffb5..527b0d301 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -74,11 +74,19 @@ 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). - holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, time.Now()) + now := time.Now() + holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, now) if err != nil { return false, nil, err } deleting = !holds + if holds { + defer func() { + if step != nil { + step.requeue = controlPlaneHoldRequeue(controlPlane, now, step.requeue) + } + }() + } } sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, !controlPlane.DeletionTimestamp.IsZero()) switch { @@ -165,7 +173,7 @@ func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *code // releaseWithoutDelete logs what can stay in Coder, because nothing can // delete it any more, and returns true (ControlPlaneGone). func (r *CoderAIProviderReconciler) releaseWithoutDelete(ctx context.Context, provider *coderv1alpha1.CoderAIProvider, why string) bool { - releasedWithoutDelete.WithLabelValues("CoderAIProvider").Inc() + countReleasedWithoutDelete(provider.UID, "CoderAIProvider") log.FromContext(ctx).Info("Releasing the finalizer without a delete in Coder: the provider can stay in the Coder database", "reason", "ControlPlaneGone", "why", why, "provider", provider.Name, "providerID", provider.Status.ProviderID, "createPending", provider.Status.CreateAttemptTime != nil) diff --git a/internal/controller/codercontrolplane_controller.go b/internal/controller/codercontrolplane_controller.go index bf62c1308..c735187b0 100644 --- a/internal/controller/codercontrolplane_controller.go +++ b/internal/controller/codercontrolplane_controller.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/util/retry" + "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" ctrlbuilder "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -219,6 +220,16 @@ type CoderControlPlaneReconciler struct { OperatorAccessProvisioner coderbootstrap.OperatorAccessProvisioner LicenseUploader LicenseUploader EntitlementsInspector EntitlementsInspector + // Clock is optional (nil: the real clock). Tests set it to move the + // dependent hold of a deleted control plane. + Clock clock.PassiveClock +} + +func (r *CoderControlPlaneReconciler) now() time.Time { + if r.Clock == nil { + return time.Now() + } + return r.Clock.Now() } // +kubebuilder:rbac:groups=coder.com,resources=codercontrolplanes,verbs=get;list;watch;create;update;patch;delete @@ -607,8 +618,9 @@ func (r *CoderControlPlaneReconciler) finalizeWorkspaceRBAC( } // The finalizer keeps the Coder Deployment (owned by this object) // running while AI Gateway keys and AI providers that use it still - // delete their resources in Coder (#353). - if holds, err := controlPlaneHolds(ctx, r.Client, coderControlPlane, time.Now()); err != nil { + // delete their resources in Coder (#353). The grace period after the hold + // lets each of them run its bounded pass before Coder goes. + if holds, err := controlPlaneHolds(ctx, r.Client, coderControlPlane, r.now().Add(-controlPlaneDependentGrace)); err != nil { return ctrl.Result{}, err } else if holds { dependents, err := controlPlaneDependents(ctx, r.Client, coderControlPlane) diff --git a/internal/controller/controlplane_hold.go b/internal/controller/controlplane_hold.go index a475414c3..9cf00b706 100644 --- a/internal/controller/controlplane_hold.go +++ b/internal/controller/controlplane_hold.go @@ -4,10 +4,12 @@ import ( "context" "errors" "fmt" + "sync" "time" "github.com/prometheus/client_golang/prometheus" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -27,27 +29,60 @@ import ( // goes, and they release with ControlPlaneGone. const controlPlaneDependentHold = 10 * time.Minute +// controlPlaneDependentGrace is how long a control plane stays after its +// hold ends while dependents remain, so each of them sees the hold end and +// runs its bounded pass while Coder still runs. Dependents requeue before the +// hold ends (controlPlaneHoldRequeue), so they need only a short time. +const controlPlaneDependentGrace = 30 * time.Second + // controlPlaneDependentPoll is the requeue of a control plane that waits for // 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 -// the only other signal. +// 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. var releasedWithoutDelete = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "coder_k8s_cleanup_released_without_delete_total", Help: "Finalizers released without deleting the object's resources in Coder, because Coder was not reachable.", }, []string{"kind"}) +var pendingReleases sync.Map // types.UID -> kind label + func init() { metrics.Registry.MustRegister(releasedWithoutDelete) } +func countReleasedWithoutDelete(uid types.UID, kind string) { pendingReleases.Store(uid, kind) } + +// releasedWithoutDeleteDone counts the release of uid, if one was recorded, +// after its finalizer patch succeeded. +func releasedWithoutDeleteDone(uid types.UID) { + if kind, ok := pendingReleases.LoadAndDelete(uid); ok { + releasedWithoutDelete.WithLabelValues(kind.(string)).Inc() + } +} + +// controlPlaneHoldEnd is when the hold of a control plane that is being +// deleted ends. +func controlPlaneHoldEnd(controlPlane *coderv1alpha1.CoderControlPlane) time.Time { + return controlPlane.DeletionTimestamp.Add(controlPlaneDependentHold) +} + +// controlPlaneHoldRequeue caps a dependent's requeue under the hold, so it +// looks again right after the hold ends, inside the grace period. +func controlPlaneHoldRequeue(controlPlane *coderv1alpha1.CoderControlPlane, now time.Time, requeue time.Duration) time.Duration { + return min(requeue, max(controlPlaneHoldEnd(controlPlane).Sub(now)+time.Second, time.Second)) +} + // controlPlaneHolds reports whether controlPlane, which is being deleted, // waits for its dependents at now: it still has its finalizer, the hold time -// has not passed, and its namespace is not being deleted. A namespace -// deletion removes Coder's pods at once, so no hold can keep Coder -// reachable then, and a hold would only delay the namespace. +// has not passed, it is not deleted in the foreground, and its namespace is +// not being deleted. A namespace deletion removes Coder's pods at once, and a +// foreground deletion removes the Coder Deployment (blockOwnerDeletion) +// before the control plane, so no hold can keep Coder reachable then. func controlPlaneHolds(ctx context.Context, reader client.Reader, controlPlane *coderv1alpha1.CoderControlPlane, now time.Time) (bool, error) { if reader == nil || controlPlane == nil { return false, errors.New("assertion failed: the control plane hold check needs a reader and a control plane") @@ -55,7 +90,8 @@ func controlPlaneHolds(ctx context.Context, reader client.Reader, controlPlane * if controlPlane.DeletionTimestamp.IsZero() { return false, errors.New("assertion failed: the control plane hold applies only while the control plane is being deleted") } - if !controllerutil.ContainsFinalizer(controlPlane, workspaceRBACFinalizer) || !now.Before(controlPlane.DeletionTimestamp.Add(controlPlaneDependentHold)) { + if !controllerutil.ContainsFinalizer(controlPlane, workspaceRBACFinalizer) || controllerutil.ContainsFinalizer(controlPlane, metav1.FinalizerDeleteDependents) || + !now.Before(controlPlaneHoldEnd(controlPlane)) { return false, nil } namespace := &corev1.Namespace{} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index db1e85d68..201f0455b 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -10,9 +10,12 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + clocktesting "k8s.io/utils/clock/testing" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/metrics" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" @@ -163,17 +166,91 @@ func TestCoderAIProviderDeleteUnderControlPlaneHold(t *testing.T) { require.Equal(t, 2, env.fake.requestCount(routeDeleteAIProvider)) } -// #353: a release without a delete in Coder counts in -// coder_k8s_cleanup_released_without_delete_total. +// #353: a release without a delete in Coder counts once in +// coder_k8s_cleanup_released_without_delete_total, also when the finalizer +// patch fails and the release runs again. Not parallel: other tests release +// keys too. func TestReleasedWithoutDeleteMetric(t *testing.T) { - t.Parallel() ctx := context.Background() env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) got := rotatedKey(ctx, t, &env) before := releasedWithoutDelete(t, "CoderAIGatewayKey") require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) require.NoError(t, k8sClient.Delete(ctx, got)) + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + failed := false + env.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.CoderAIGatewayKey); ok && !failed { + failed = true + return apierrors.NewConflict(schema.GroupResource{Resource: "coderaigatewaykeys"}, obj.GetName(), nil) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }) + _, _, err = env.reconcileErr(ctx, t, got) + require.Error(t, err, "the finalizer patch fails once") + require.Equal(t, before, releasedWithoutDelete(t, "CoderAIGatewayKey"), "no count before the finalizer is gone") gone, _ := env.reconcile(ctx, t, got) require.Nil(t, gone) - require.Greater(t, releasedWithoutDelete(t, "CoderAIGatewayKey"), before) + require.Equal(t, before+1, releasedWithoutDelete(t, "CoderAIGatewayKey")) +} + +// #353: a foreground deletion removes the Coder Deployment before the +// control plane, so the control plane does not hold for its dependents. +func TestControlPlaneForegroundDeletionDoesNotHold(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + env.createKey(ctx, t, "coder", testAIGatewaySecret) + key := client.ObjectKey{Namespace: env.namespace, Name: "coder"} + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, key, controlPlane)) + controlPlane.Finalizers = append(controlPlane.Finalizers, workspaceRBACFinalizer) + require.NoError(t, k8sClient.Update(ctx, controlPlane)) + require.NoError(t, k8sClient.Delete(ctx, controlPlane, client.PropagationPolicy(metav1.DeletePropagationForeground))) + t.Cleanup(func() { + if k8sClient.Get(context.Background(), key, controlPlane) == nil { + controlPlane.Finalizers = nil + _ = k8sClient.Update(context.Background(), controlPlane) + } + }) + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, key, controlPlane)) + require.NotContains(t, controlPlane.Finalizers, workspaceRBACFinalizer, "no hold under foreground deletion") +} + +// #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. +func TestControlPlaneHoldEnd(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got := rotatedKey(ctx, t, &env) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "coder"}, controlPlane)) + end := controlPlane.DeletionTimestamp.Add(10 * time.Minute) + + controlPlane.Status.OperatorAccessReady = false + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) + require.NoError(t, k8sClient.Delete(ctx, got)) + env.clock.SetTime(end.Add(-30 * time.Second)) + got, result := env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, false, "OperatorAccessNotReady") + require.Equal(t, 31*time.Second, result.RequeueAfter, "the dependent looks again right after the hold ends") + + ccpClock := clocktesting.NewFakeClock(end.Add(10 * time.Second)) + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme, Clock: ccpClock} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(controlPlane)}) + require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(controlPlane), controlPlane), "the control plane stays in the grace period") + ccpClock.SetTime(end.Add(31 * time.Second)) + _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(controlPlane)}) + require.NoError(t, err) + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(controlPlane), controlPlane)), "after the grace period it goes") } From ad8cc53b4b65c020123916c92799a166e274438b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 13:28:30 +0000 Subject: [PATCH 3/5] fix: hold only for dependents that are being deleted, scale the grace period, and drop stale release records Change-Id: Ic848eea8fb199e9f2f082dc1c48502a10c574861 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 2 +- docs/how-to/manage-ai-providers.md | 2 +- .../coderaigatewaykey_controller.go | 1 + .../controller/coderaiprovider_controller.go | 1 + .../codercontrolplane_controller.go | 11 ++- internal/controller/controlplane_hold.go | 58 +++++++++---- internal/controller/controlplane_hold_test.go | 81 +++++++++++++++++++ 7 files changed, 136 insertions(+), 20 deletions(-) diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index ad4020dbd..1998666ca 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. 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 use it, the control plane waits for them 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. After 10 minutes, the objects that remain get 30 more seconds 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` or `CoderAIProvider` objects that use it are being deleted, the control plane waits for them 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. 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 30 more 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 f8ddededf..7f637ce71 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,7 +122,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, nothing can delete the provider. The finalizer goes at once, and the operator logs the provider ID. Delete the provider in the Coder dashboard. -- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects use it, the control plane waits for them for up to 10 minutes, plus 30 seconds for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. +- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus 30 seconds and 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. 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. Coder deletes a provider even while chat models use it. Those models stop working. diff --git a/internal/controller/coderaigatewaykey_controller.go b/internal/controller/coderaigatewaykey_controller.go index a7290fa1c..0413f569b 100644 --- a/internal/controller/coderaigatewaykey_controller.go +++ b/internal/controller/coderaigatewaykey_controller.go @@ -163,6 +163,7 @@ func (r *CoderAIGatewayKeyReconciler) Reconcile(ctx context.Context, req ctrl.Re if !controllerutil.ContainsFinalizer(key, coderv1alpha1.CoderAIGatewayKeyCleanupFinalizer) { return ctrl.Result{}, nil } + forgetReleasedWithoutDelete(key.UID) release, deleteStep, err := r.deleteStep(ctx, key) if err != nil { return ctrl.Result{}, err diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 5d0095d4e..b65d38081 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -155,6 +155,7 @@ func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Requ if !controllerutil.ContainsFinalizer(provider, coderv1alpha1.CoderAIProviderCleanupFinalizer) { return ctrl.Result{}, nil } + forgetReleasedWithoutDelete(provider.UID) release, step, err := r.deleteStep(ctx, provider) if err != nil { return ctrl.Result{}, err diff --git a/internal/controller/codercontrolplane_controller.go b/internal/controller/codercontrolplane_controller.go index c735187b0..cd87f9f91 100644 --- a/internal/controller/codercontrolplane_controller.go +++ b/internal/controller/codercontrolplane_controller.go @@ -620,14 +620,17 @@ func (r *CoderControlPlaneReconciler) finalizeWorkspaceRBAC( // running while AI Gateway keys and AI providers that use it still // delete their resources in Coder (#353). The grace period after the hold // lets each of them run its bounded pass before Coder goes. - if holds, err := controlPlaneHolds(ctx, r.Client, coderControlPlane, r.now().Add(-controlPlaneDependentGrace)); err != nil { + now := r.now() + dependents, perKind, err := controlPlaneDependents(ctx, r.Client, coderControlPlane, now) + if err != nil { return ctrl.Result{}, err - } else if holds { - dependents, err := controlPlaneDependents(ctx, r.Client, coderControlPlane) + } + if len(dependents) > 0 { + holds, err := controlPlaneHolds(ctx, r.Client, coderControlPlane, now.Add(-controlPlaneDependentGraceFor(perKind))) if err != nil { return ctrl.Result{}, err } - if len(dependents) > 0 { + if holds { log.FromContext(ctx).Info("Waiting for the objects that use this control plane to delete their resources in Coder", "dependents", dependents, "until", coderControlPlane.DeletionTimestamp.Add(controlPlaneDependentHold).UTC()) return ctrl.Result{RequeueAfter: controlPlaneDependentPoll}, nil diff --git a/internal/controller/controlplane_hold.go b/internal/controller/controlplane_hold.go index 9cf00b706..324426564 100644 --- a/internal/controller/controlplane_hold.go +++ b/internal/controller/controlplane_hold.go @@ -29,12 +29,21 @@ import ( // goes, and they release with ControlPlaneGone. const controlPlaneDependentHold = 10 * time.Minute -// controlPlaneDependentGrace is how long a control plane stays after its -// hold ends while dependents remain, so each of them sees the hold end and -// runs its bounded pass while Coder still runs. Dependents requeue before the -// hold ends (controlPlaneHoldRequeue), so they need only a short time. +// controlPlaneDependentGrace is the base of how long a control plane stays +// after its hold ends while dependents remain, so each of them sees the hold +// end and runs its bounded pass while Coder still runs. Dependents requeue +// before the hold ends (controlPlaneHoldRequeue). One worker runs the passes +// of one kind one after the other, so controlPlaneDependentGraceFor adds one +// bounded pass per dependent of the larger kind. const controlPlaneDependentGrace = 30 * time.Second +// controlPlaneLiveDependentWait is how long a deleted control plane waits for +// a dependent that is not being deleted itself. It covers one command that +// deletes the control plane before its dependents (kubectl delete -f). After +// it, only dependents that are being deleted hold the control plane: a live +// dependent does no cleanup, so a longer wait for it does nothing. +const controlPlaneLiveDependentWait = 30 * time.Second + // controlPlaneDependentPoll is the requeue of a control plane that waits for // its dependents. A dependent's removal also requeues it at once. const controlPlaneDependentPoll = 10 * time.Second @@ -43,7 +52,9 @@ const controlPlaneDependentPoll = 10 * time.Second // providers 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. +// failed finalizer patch and its retry count once. Each delete reconcile +// first drops an earlier record (forgetReleasedWithoutDelete), so a retry +// that then deletes in Coder does not count. var releasedWithoutDelete = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "coder_k8s_cleanup_released_without_delete_total", Help: "Finalizers released without deleting the object's resources in Coder, because Coder was not reachable.", @@ -57,6 +68,10 @@ func init() { func countReleasedWithoutDelete(uid types.UID, kind string) { pendingReleases.Store(uid, kind) } +// forgetReleasedWithoutDelete drops a release of uid that an earlier +// reconcile recorded but whose finalizer patch failed. +func forgetReleasedWithoutDelete(uid types.UID) { pendingReleases.Delete(uid) } + // releasedWithoutDeleteDone counts the release of uid, if one was recorded, // after its finalizer patch succeeded. func releasedWithoutDeleteDone(uid types.UID) { @@ -71,6 +86,12 @@ func controlPlaneHoldEnd(controlPlane *coderv1alpha1.CoderControlPlane) time.Tim return controlPlane.DeletionTimestamp.Add(controlPlaneDependentHold) } +// controlPlaneDependentGraceFor is the grace period after the hold when +// perKind dependents of one kind remain (controlPlaneDependentGrace). +func controlPlaneDependentGraceFor(perKind int) time.Duration { + return controlPlaneDependentGrace + time.Duration(perKind)*max(aiGatewayKeyBoundedDeleteTimeout, aiProviderBoundedDeleteTimeout) +} + // controlPlaneHoldRequeue caps a dependent's requeue under the hold, so it // looks again right after the hold ends, inside the grace period. func controlPlaneHoldRequeue(controlPlane *coderv1alpha1.CoderControlPlane, now time.Time, requeue time.Duration) time.Duration { @@ -102,28 +123,37 @@ func controlPlaneHolds(ctx context.Context, reader client.Reader, controlPlane * } // controlPlaneDependents returns the CoderAIGatewayKey and CoderAIProvider -// objects in controlPlane's namespace that reference it, as kind/name. -func controlPlaneDependents(ctx context.Context, reader client.Reader, controlPlane *coderv1alpha1.CoderControlPlane) ([]string, error) { +// objects in controlPlane's namespace that reference it and hold it at now, +// as kind/name, and the larger count of one kind. A dependent holds it while +// it is being deleted, and a live one only for +// controlPlaneLiveDependentWait after the control plane deletion. +func controlPlaneDependents(ctx context.Context, reader client.Reader, controlPlane *coderv1alpha1.CoderControlPlane, now time.Time) ([]string, int, error) { + if controlPlane.DeletionTimestamp.IsZero() { + return nil, 0, errors.New("assertion failed: dependents hold only a control plane that is being deleted") + } + liveHolds := now.Before(controlPlane.DeletionTimestamp.Add(controlPlaneLiveDependentWait)) + holds := func(o client.Object) bool { return liveHolds || !o.GetDeletionTimestamp().IsZero() } var dependents []string keys := &coderv1alpha1.CoderAIGatewayKeyList{} if err := reader.List(ctx, keys, client.InNamespace(controlPlane.Namespace)); err != nil { - return nil, fmt.Errorf("list AI Gateway keys of namespace %s: %w", controlPlane.Namespace, err) + return nil, 0, fmt.Errorf("list AI Gateway keys of namespace %s: %w", controlPlane.Namespace, err) } - for _, k := range keys.Items { - if k.Spec.ControlPlaneRef.Name == controlPlane.Name { + for i := range keys.Items { + if k := &keys.Items[i]; k.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(k) { dependents = append(dependents, "CoderAIGatewayKey/"+k.Name) } } + keyCount := len(dependents) providers := &coderv1alpha1.CoderAIProviderList{} if err := reader.List(ctx, providers, client.InNamespace(controlPlane.Namespace)); err != nil { - return nil, fmt.Errorf("list AI providers of namespace %s: %w", controlPlane.Namespace, err) + return nil, 0, fmt.Errorf("list AI providers of namespace %s: %w", controlPlane.Namespace, err) } - for _, p := range providers.Items { - if p.Spec.ControlPlaneRef.Name == controlPlane.Name { + for i := range providers.Items { + if p := &providers.Items[i]; p.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(p) { dependents = append(dependents, "CoderAIProvider/"+p.Name) } } - return dependents, nil + return dependents, max(keyCount, len(dependents)-keyCount), nil } // dependentRemoved passes only the removal of a dependent object, the event diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index 201f0455b..bca0993c0 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -252,5 +252,86 @@ func TestControlPlaneHoldEnd(t *testing.T) { ccpClock.SetTime(end.Add(31 * time.Second)) _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(controlPlane)}) require.NoError(t, err) + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(controlPlane), controlPlane), "each dependent adds one bounded pass to the grace period") + ccpClock.SetTime(end.Add(41 * time.Second)) + _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(controlPlane)}) + require.NoError(t, err) require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(controlPlane), controlPlane)), "after the grace period it goes") } + +// #353: a dependent that is not being deleted does no cleanup, so it holds a +// deleted control plane only for 30 s, which covers one command that deletes +// both. A dependent that is being deleted holds it for the whole hold. +func TestControlPlaneLiveDependentWait(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + env.createKey(ctx, t, "coder", testAIGatewaySecret) + deleting := env.createNamedKey(ctx, t, "deleting", "coder", "deleting-secret") + deleting.Finalizers = []string{coderv1alpha1.CoderAIGatewayKeyCleanupFinalizer} + require.NoError(t, k8sClient.Update(ctx, deleting)) + removeFinalizer := func() { + if k8sClient.Get(context.Background(), client.ObjectKeyFromObject(deleting), deleting) == nil { + deleting.Finalizers = nil + require.NoError(t, k8sClient.Update(context.Background(), deleting)) + } + } + t.Cleanup(removeFinalizer) + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + ref := client.ObjectKey{Namespace: env.namespace, Name: "coder"} + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, ref, controlPlane)) + ccpClock := clocktesting.NewFakeClock(controlPlane.DeletionTimestamp.Add(29 * time.Second)) + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme, Clock: ccpClock} + gone := func() bool { + t.Helper() + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: ref}) + require.NoError(t, err) + return apierrors.IsNotFound(k8sClient.Get(ctx, ref, &coderv1alpha1.CoderControlPlane{})) + } + require.False(t, gone(), "live keys hold the control plane for 30 s") + require.NoError(t, k8sClient.Delete(ctx, deleting)) + ccpClock.SetTime(controlPlane.DeletionTimestamp.Add(31 * time.Second)) + require.False(t, gone(), "a key that is being deleted still holds it") + removeFinalizer() + require.True(t, gone(), "after 30 s a live key does not hold it") +} + +// #353: a release that a failed finalizer patch left recorded does not count +// when the next reconcile deletes the keys in Coder. Not parallel: other +// tests release keys too. +func TestReleasedWithoutDeleteMetricRecovered(t *testing.T) { + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got := rotatedKey(ctx, t, &env) + before := releasedWithoutDelete(t, "CoderAIGatewayKey") + deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "coder"}, controlPlane)) + controlPlane.Status.OperatorAccessReady = false + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) + require.NoError(t, k8sClient.Delete(ctx, got)) + env.clock.SetTime(controlPlane.DeletionTimestamp.Add(10*time.Minute + time.Second)) // The hold has ended. + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + failed := false + env.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.CoderAIGatewayKey); ok && !failed { + failed = true + return apierrors.NewConflict(schema.GroupResource{Resource: "coderaigatewaykeys"}, obj.GetName(), nil) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }) + _, _, err = env.reconcileErr(ctx, t, got) + require.Error(t, err, "the release without a delete fails its finalizer patch") + require.Len(t, env.fake.aiGatewayKeys(), 2) + + controlPlane.Status.OperatorAccessReady = true + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) + gone, _ := env.reconcile(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.aiGatewayKeys(), "the retry deletes the keys in Coder") + require.Equal(t, before, releasedWithoutDelete(t, "CoderAIGatewayKey"), "a release that deleted in Coder does not count") +} From f667afb00e28990a16b7fa8b2baa6420fcdff44e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 14:04:12 +0000 Subject: [PATCH 4/5] fix: revoke workspace RBAC before the control plane holds for its dependents Change-Id: I7dfa9189c34f89eec3b91fb09fb442d7e7ed6534 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 2 +- docs/how-to/manage-ai-providers.md | 2 +- .../codercontrolplane_controller.go | 10 +++-- internal/controller/controlplane_hold_test.go | 39 +++++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 1998666ca..1880fd94f 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. 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 waits for them 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. 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 30 more 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` or `CoderAIProvider` objects that use it are being deleted, the control plane waits for them 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 30 more 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 7f637ce71..4445129c0 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,7 +122,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, nothing can delete the provider. The finalizer goes at once, and the operator logs the provider ID. Delete the provider in the Coder dashboard. -- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus 30 seconds and 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. 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` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus 30 seconds and 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. Coder deletes a provider even while chat models use it. Those models stop working. diff --git a/internal/controller/codercontrolplane_controller.go b/internal/controller/codercontrolplane_controller.go index cd87f9f91..804557c08 100644 --- a/internal/controller/codercontrolplane_controller.go +++ b/internal/controller/codercontrolplane_controller.go @@ -616,6 +616,12 @@ func (r *CoderControlPlaneReconciler) finalizeWorkspaceRBAC( if !controllerutil.ContainsFinalizer(coderControlPlane, workspaceRBACFinalizer) { return ctrl.Result{}, nil } + // Revoke Coder's workspace RBAC first: the hold below keeps the Coder + // Deployment and its service account, but the AI cleanup calls Coder with + // the operator's token and does not need that Kubernetes access. + if err := r.cleanupManagedWorkspaceRBAC(ctx, coderControlPlane, nil, nil); err != nil { + return ctrl.Result{}, err + } // The finalizer keeps the Coder Deployment (owned by this object) // running while AI Gateway keys and AI providers that use it still // delete their resources in Coder (#353). The grace period after the hold @@ -637,10 +643,6 @@ func (r *CoderControlPlaneReconciler) finalizeWorkspaceRBAC( } } - if err := r.cleanupManagedWorkspaceRBAC(ctx, coderControlPlane, nil, nil); err != nil { - return ctrl.Result{}, err - } - original := coderControlPlane.DeepCopy() controllerutil.RemoveFinalizer(coderControlPlane, workspaceRBACFinalizer) if err := r.Patch(ctx, coderControlPlane, client.MergeFrom(original)); err != nil { diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index bca0993c0..e0dac2b41 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -335,3 +336,41 @@ func TestReleasedWithoutDeleteMetricRecovered(t *testing.T) { require.Empty(t, env.fake.aiGatewayKeys(), "the retry deletes the keys in Coder") require.Equal(t, before, releasedWithoutDelete(t, "CoderAIGatewayKey"), "a release that deleted in Coder does not count") } + +// #353: the hold keeps the Coder Deployment and its service account, so the +// control plane revokes the managed workspace RBAC before it waits. +func TestControlPlaneHoldRevokesWorkspaceRBAC(t *testing.T) { + t.Parallel() + ctx := context.Background() + namespace := createTestNamespace(ctx, t, "hold-rbac") + workspaceNamespace := createTestNamespace(ctx, t, "hold-rbac-ws") + cp := createCoderControlPlaneUnstructured(ctx, t, "coder", namespace, map[string]any{ + "image": "test-workspace-rbac:latest", + "serviceAccount": map[string]any{"name": "coder-hold-sa"}, + "rbac": map[string]any{"workspacePerms": true, "workspaceNamespaces": []any{workspaceNamespace}}, + }) + r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme} + ref := types.NamespacedName{Namespace: namespace, Name: cp.Name} + _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: ref}) + require.NoError(t, err) + bindingRef := types.NamespacedName{Namespace: workspaceNamespace, Name: expectedWorkspaceRoleBindingName(t, cp, "coder-hold-sa")} + require.NoError(t, k8sClient.Get(ctx, bindingRef, &rbacv1.RoleBinding{}), "the reconcile grants workspace RBAC") + + key := &coderv1alpha1.CoderAIGatewayKey{ + ObjectMeta: metav1.ObjectMeta{Name: "gateway", Namespace: namespace, Finalizers: []string{coderv1alpha1.CoderAIGatewayKeyCleanupFinalizer}}, + Spec: coderv1alpha1.CoderAIGatewayKeySpec{ControlPlaneRef: coderv1alpha1.CoderControlPlaneReference{Name: cp.Name}, SecretName: "gateway-key"}, + } + require.NoError(t, k8sClient.Create(ctx, key)) + t.Cleanup(func() { + if k8sClient.Get(context.Background(), client.ObjectKeyFromObject(key), key) == nil { + key.Finalizers = nil + _ = k8sClient.Update(context.Background(), key) + } + }) + require.NoError(t, k8sClient.Delete(ctx, key)) + require.NoError(t, k8sClient.Delete(ctx, cp)) + result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: ref}) + require.NoError(t, err) + require.Equal(t, 10*time.Second, result.RequeueAfter, "the control plane holds for the key") + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, bindingRef, &rbacv1.RoleBinding{})), "the hold does not keep workspace RBAC") +} From 49e6fdc0dcc0fdf28ac64be7ca15167bd0684365 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 14:32:42 +0000 Subject: [PATCH 5/5] fix: count only dependents that still have their cleanup finalizer, and cap the grace period Change-Id: I51e0c91bb48d94962e4807298ecdf1cad6ffc68d Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-gateway-keys.md | 2 +- docs/how-to/manage-ai-providers.md | 2 +- internal/controller/controlplane_hold.go | 22 ++++++++++++++----- .../controlplane_hold_internal_test.go | 18 +++++++++++++++ internal/controller/controlplane_hold_test.go | 14 +++++++++++- 5 files changed, 49 insertions(+), 9 deletions(-) create mode 100644 internal/controller/controlplane_hold_internal_test.go diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index 1880fd94f..b9427ee63 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. 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 waits for them 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 30 more 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` or `CoderAIProvider` objects that use it are being deleted, the control plane waits for them 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 30 more seconds plus 10 seconds per object of one kind (at most 5 minutes) 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 4445129c0..e1879d0f7 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,7 +122,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, nothing can delete the provider. The finalizer goes at once, and the operator logs the provider ID. Delete the provider in the Coder dashboard. -- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus 30 seconds and 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` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus 30 seconds and 10 seconds per object of one kind (at most 5 minutes) 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. Coder deletes a provider even while chat models use it. Those models stop working. diff --git a/internal/controller/controlplane_hold.go b/internal/controller/controlplane_hold.go index 324426564..fce505be8 100644 --- a/internal/controller/controlplane_hold.go +++ b/internal/controller/controlplane_hold.go @@ -37,6 +37,10 @@ const controlPlaneDependentHold = 10 * time.Minute // bounded pass per dependent of the larger kind. const controlPlaneDependentGrace = 30 * time.Second +// controlPlaneDependentGraceMax caps the grace period, so dependents that +// someone keeps creating cannot keep the control plane forever. +const controlPlaneDependentGraceMax = 5 * time.Minute + // controlPlaneLiveDependentWait is how long a deleted control plane waits for // a dependent that is not being deleted itself. It covers one command that // deletes the control plane before its dependents (kubectl delete -f). After @@ -89,7 +93,7 @@ func controlPlaneHoldEnd(controlPlane *coderv1alpha1.CoderControlPlane) time.Tim // controlPlaneDependentGraceFor is the grace period after the hold when // perKind dependents of one kind remain (controlPlaneDependentGrace). func controlPlaneDependentGraceFor(perKind int) time.Duration { - return controlPlaneDependentGrace + time.Duration(perKind)*max(aiGatewayKeyBoundedDeleteTimeout, aiProviderBoundedDeleteTimeout) + return min(controlPlaneDependentGrace+time.Duration(perKind)*max(aiGatewayKeyBoundedDeleteTimeout, aiProviderBoundedDeleteTimeout), controlPlaneDependentGraceMax) } // controlPlaneHoldRequeue caps a dependent's requeue under the hold, so it @@ -125,21 +129,27 @@ func controlPlaneHolds(ctx context.Context, reader client.Reader, controlPlane * // controlPlaneDependents returns the CoderAIGatewayKey and CoderAIProvider // objects in controlPlane's namespace that reference it and hold it at now, // as kind/name, and the larger count of one kind. A dependent holds it while -// it is being deleted, and a live one only for -// controlPlaneLiveDependentWait after the control plane deletion. +// it is being deleted and still has its cleanup finalizer (another finalizer +// does not hold it), and a live one only for controlPlaneLiveDependentWait +// after the control plane deletion. func controlPlaneDependents(ctx context.Context, reader client.Reader, controlPlane *coderv1alpha1.CoderControlPlane, now time.Time) ([]string, int, error) { if controlPlane.DeletionTimestamp.IsZero() { return nil, 0, errors.New("assertion failed: dependents hold only a control plane that is being deleted") } liveHolds := now.Before(controlPlane.DeletionTimestamp.Add(controlPlaneLiveDependentWait)) - holds := func(o client.Object) bool { return liveHolds || !o.GetDeletionTimestamp().IsZero() } + holds := func(o client.Object, finalizer string) bool { + if o.GetDeletionTimestamp().IsZero() { + return liveHolds + } + return controllerutil.ContainsFinalizer(o, finalizer) + } var dependents []string keys := &coderv1alpha1.CoderAIGatewayKeyList{} if err := reader.List(ctx, keys, client.InNamespace(controlPlane.Namespace)); err != nil { return nil, 0, fmt.Errorf("list AI Gateway keys of namespace %s: %w", controlPlane.Namespace, err) } for i := range keys.Items { - if k := &keys.Items[i]; k.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(k) { + if k := &keys.Items[i]; k.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(k, coderv1alpha1.CoderAIGatewayKeyCleanupFinalizer) { dependents = append(dependents, "CoderAIGatewayKey/"+k.Name) } } @@ -149,7 +159,7 @@ func controlPlaneDependents(ctx context.Context, reader client.Reader, controlPl return nil, 0, fmt.Errorf("list AI providers of namespace %s: %w", controlPlane.Namespace, err) } for i := range providers.Items { - if p := &providers.Items[i]; p.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(p) { + if p := &providers.Items[i]; p.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(p, coderv1alpha1.CoderAIProviderCleanupFinalizer) { dependents = append(dependents, "CoderAIProvider/"+p.Name) } } diff --git a/internal/controller/controlplane_hold_internal_test.go b/internal/controller/controlplane_hold_internal_test.go new file mode 100644 index 000000000..9b1bd8f74 --- /dev/null +++ b/internal/controller/controlplane_hold_internal_test.go @@ -0,0 +1,18 @@ +package controller + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// #353: the grace period after the hold grows by one bounded pass per +// dependent of one kind, up to a cap, so dependents that someone keeps +// creating cannot keep a deleted control plane forever. +func TestControlPlaneDependentGraceFor(t *testing.T) { + t.Parallel() + require.Equal(t, 30*time.Second, controlPlaneDependentGraceFor(0)) + require.Equal(t, time.Minute, controlPlaneDependentGraceFor(3)) + require.Equal(t, 5*time.Minute, controlPlaneDependentGraceFor(1000), "the grace period has a cap") +} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index e0dac2b41..2fb44bcf2 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -278,6 +278,18 @@ func TestControlPlaneLiveDependentWait(t *testing.T) { } } t.Cleanup(removeFinalizer) + // A deleted key whose cleanup finalizer is gone, held only by another + // finalizer, does not hold the control plane. + foreign := env.createNamedKey(ctx, t, "foreign", "coder", "foreign-secret") + foreign.Finalizers = []string{"example.com/keep"} + require.NoError(t, k8sClient.Update(ctx, foreign)) + t.Cleanup(func() { + if k8sClient.Get(context.Background(), client.ObjectKeyFromObject(foreign), foreign) == nil { + foreign.Finalizers = nil + _ = k8sClient.Update(context.Background(), foreign) + } + }) + require.NoError(t, k8sClient.Delete(ctx, foreign)) deleteFinalizedControlPlane(ctx, t, env.namespace, "coder") ref := client.ObjectKey{Namespace: env.namespace, Name: "coder"} controlPlane := &coderv1alpha1.CoderControlPlane{} @@ -295,7 +307,7 @@ func TestControlPlaneLiveDependentWait(t *testing.T) { ccpClock.SetTime(controlPlane.DeletionTimestamp.Add(31 * time.Second)) require.False(t, gone(), "a key that is being deleted still holds it") removeFinalizer() - require.True(t, gone(), "after 30 s a live key does not hold it") + require.True(t, gone(), "after 30 s a live key, or a deleted key without its cleanup finalizer, does not hold it") } // #353: a release that a failed finalizer patch left recorded does not count