From 280734277c08f84f75048370737cf435d514f746 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 15:59:09 +0000 Subject: [PATCH 01/12] feat: write a cleanup record before each CoderAIProvider create (#353) Change-Id: Ice66d8e736e7372a3afd95d8c96a79955aca6b04 Signed-off-by: Thomas Kosiewski --- .github/workflows/ci.yaml | 1 + config/default/kustomization.yaml | 1 + config/rbac/cleanup-records-rolebinding.yaml | 16 ++ config/rbac/role.yaml | 18 ++ dist/install.yaml | 32 ++++ docs/how-to/manage-ai-gateway-keys.md | 2 +- docs/how-to/manage-ai-providers.md | 8 +- internal/app/controllerapp/controllerapp.go | 2 + internal/controller/cleanup_record.go | 165 ++++++++++++++++++ .../controller/coderaiprovider_controller.go | 17 +- .../coderaiprovider_controller_test.go | 4 +- internal/controller/coderaiprovider_create.go | 18 +- .../controller/coderaiprovider_create_test.go | 2 +- internal/controller/coderaiprovider_delete.go | 14 +- internal/controller/coderaiprovider_record.go | 71 ++++++++ .../controller/coderaiprovider_record_test.go | 154 ++++++++++++++++ internal/controller/controlplane_hold_test.go | 1 + internal/controller/fakecoder_test.go | 25 ++- 18 files changed, 526 insertions(+), 25 deletions(-) create mode 100644 config/rbac/cleanup-records-rolebinding.yaml create mode 100644 internal/controller/cleanup_record.go create mode 100644 internal/controller/coderaiprovider_record.go create mode 100644 internal/controller/coderaiprovider_record_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 7ae48d7c4..55c4f3069 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -94,6 +94,7 @@ jobs: # Shared Coder access that CoderTemplateTest and later pool controllers call. - 'internal/controller/controlplane_access.go' - 'internal/controller/controlplane_hold.go' + - 'internal/controller/cleanup_record*' - '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/config/default/kustomization.yaml b/config/default/kustomization.yaml index d3f9b0e3a..c4379d6d9 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -9,6 +9,7 @@ resources: - ../crd/bases/coder.com_coderprovisioners.yaml - ../crd/bases/coder.com_codertemplatetests.yaml - ../crd/bases/coder.com_coderworkspaceproxies.yaml + - ../rbac/cleanup-records-rolebinding.yaml - ../rbac/clusterrolebinding.yaml - ../rbac/role.yaml - ../rbac/serviceaccount.yaml diff --git a/config/rbac/cleanup-records-rolebinding.yaml b/config/rbac/cleanup-records-rolebinding.yaml new file mode 100644 index 000000000..5572237fb --- /dev/null +++ b/config/rbac/cleanup-records-rolebinding.yaml @@ -0,0 +1,16 @@ +# Lets the operator write its cleanup records (#353): ConfigMaps named ck8s-cleanup- in its own +# namespace. The namespaced Role manager-role comes from the kubebuilder RBAC markers in role.yaml. No other +# subject gets write access to these ConfigMaps. +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: coder-k8s-cleanup-records + namespace: coder-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: + - kind: ServiceAccount + name: coder-k8s + namespace: coder-system diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 561f059eb..0b240a3cb 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -201,3 +201,21 @@ rules: - patch - update - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: coder-system +rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - get + - list + - update + - watch diff --git a/dist/install.yaml b/dist/install.yaml index 23b1e3b72..eb15dae61 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6410,6 +6410,24 @@ metadata: namespace: coder-system --- apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: coder-system +rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - get + - list + - update + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: manager-role @@ -6613,6 +6631,20 @@ rules: - watch --- apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: coder-k8s-cleanup-records + namespace: coder-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: +- kind: ServiceAccount + name: coder-k8s + namespace: coder-system +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: name: coder-k8s diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index b9427ee63..62e98555f 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 (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 you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys and providers as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first. When 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 e1879d0f7..133479721 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,11 +122,17 @@ 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 (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 you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When 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. +### Cleanup records + +Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. + +The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. + ### Stuck finalizer A deleted `CoderAIProvider` stays `Terminating` while the operator cannot prove that Coder no longer holds its provider. Two cases never end by themselves: diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index d0ca9b118..1bbf7a88f 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -129,6 +129,8 @@ func SetupControllers(mgr manager.Manager) error { aiProviderReconciler := &controller.CoderAIProviderReconciler{ Client: client, Scheme: managerScheme, + // The operator's own namespace holds the cleanup records (#353). + OperatorNamespace: detectLeaderElectionNamespace(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create AI provider controller: %w", err) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go new file mode 100644 index 000000000..44eff4a80 --- /dev/null +++ b/internal/controller/cleanup_record.go @@ -0,0 +1,165 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + 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" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Cleanup records (#353 part 2, option B′). Before the operator creates +// something in Coder for an object, it writes a record: one ConfigMap named +// ck8s-cleanup- in the operator's own namespace. The record says +// which Coder deployment holds the resource and how to find it. It stays when +// the object goes without a confirmed delete in Coder (for example in a +// namespace deletion), so a later sweep through a control plane of the same +// deployment can delete the resource. A record holds no secrets. The +// ConfigMap creation time is the record's created-at. +const ( + cleanupRecordPrefix = "ck8s-cleanup-" + // cleanupRecordLabel marks a record. Its value is the object kind. + cleanupRecordLabel = "coder.com/cleanup-record" + + cleanupRecordKind = "kind" + cleanupRecordUID = "uid" + cleanupRecordNamespace = "namespace" + cleanupRecordName = "name" + cleanupRecordDeploymentID = "deploymentID" + cleanupRecordNonce = "nonce" + cleanupRecordCoderID = "coderID" + cleanupRecordCreateSentAt = "createAttemptTime" +) + +// cleanupRecord is the content of a record. +type cleanupRecord struct { + Kind string + UID types.UID + Namespace string + Name string + // DeploymentID is Coder's deployment ID (/api/v2/buildinfo), read from + // Coder in the reconcile that sends the create. It lives in the Coder + // database, so it names the database that holds the resource. + DeploymentID string + // Nonce finds a resource whose Coder ID is not known yet: the display + // name of a provider. + Nonce string + // CoderID is the Coder ID of the resource once the operator knows it. + CoderID string + // CreateAttemptTime is when the operator last sent a create. A sweep + // waits for the settle window after it. + CreateAttemptTime time.Time +} + +func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) } + +// coderDeploymentID reads Coder's deployment ID. Only a canonical UUID is +// accepted: the ID decides which resources a sweep may delete. +func coderDeploymentID(ctx context.Context, sdk *codersdk.Client) (string, error) { + info, err := sdk.BuildInfo(ctx) + if err != nil { + return "", err + } + if id, err := uuid.Parse(info.DeploymentID); err != nil || id == uuid.Nil || id.String() != info.DeploymentID { + return "", fmt.Errorf("coder answered a deployment ID %q that is not a canonical UUID", info.DeploymentID) + } + return info.DeploymentID, nil +} + +// readCleanupRecord returns the record of uid, or nil when there is none. A +// ConfigMap of that name that is not a record of uid is an error: the +// operator never writes over it. +func readCleanupRecord(ctx context.Context, reader client.Reader, namespace string, uid types.UID) (*cleanupRecord, *corev1.ConfigMap, error) { + if namespace == "" || uid == "" { + return nil, nil, errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") + } + configMap := &corev1.ConfigMap{} + if err := reader.Get(ctx, types.NamespacedName{Namespace: namespace, Name: cleanupRecordObjectName(uid)}, configMap); apierrors.IsNotFound(err) { + return nil, nil, nil + } else if err != nil { + return nil, nil, fmt.Errorf("get cleanup record %s/%s: %w", namespace, cleanupRecordObjectName(uid), err) + } + record, err := parseCleanupRecord(configMap) + if err != nil { + return nil, nil, err + } + if record.UID != uid { + return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s", namespace, configMap.Name, uid) + } + return record, configMap, nil +} + +// parseCleanupRecord reads a record from its ConfigMap. +func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { + d := configMap.Data + record := &cleanupRecord{ + Kind: d[cleanupRecordKind], UID: types.UID(d[cleanupRecordUID]), Namespace: d[cleanupRecordNamespace], Name: d[cleanupRecordName], + DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], + } + if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || + configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { + return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record", configMap.Namespace, configMap.Name) + } + if t := d[cleanupRecordCreateSentAt]; t != "" { + parsed, err := time.Parse(time.RFC3339, t) + if err != nil { + return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s: %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err) + } + record.CreateAttemptTime = parsed + } + return record, nil +} + +// writeCleanupRecord creates or replaces the record in namespace. existing +// is the ConfigMap that readCleanupRecord returned, or nil. +func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, existing *corev1.ConfigMap, record cleanupRecord) error { + if namespace == "" || record.Kind == "" || record.UID == "" || record.DeploymentID == "" { + return errors.New("assertion failed: a cleanup record needs the operator namespace, a kind, an object UID and a deployment ID") + } + data := map[string]string{ + cleanupRecordKind: record.Kind, cleanupRecordUID: string(record.UID), cleanupRecordNamespace: record.Namespace, cleanupRecordName: record.Name, + cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, + } + if !record.CreateAttemptTime.IsZero() { + data[cleanupRecordCreateSentAt] = record.CreateAttemptTime.UTC().Format(time.RFC3339) + } + if existing != nil { + updated := existing.DeepCopy() + updated.Data = data + if err := c.Update(ctx, updated); err != nil { + return fmt.Errorf("update cleanup record %s/%s: %w", namespace, updated.Name, err) + } + return nil + } + created := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: cleanupRecordObjectName(record.UID), Namespace: namespace, + Labels: map[string]string{cleanupRecordLabel: record.Kind, "app.kubernetes.io/managed-by": "coder-k8s"}, + }, + Data: data, + } + if err := c.Create(ctx, created); err != nil { + return fmt.Errorf("create cleanup record %s/%s: %w", namespace, created.Name, err) + } + return nil +} + +// deleteCleanupRecord removes the record of uid. Call it only after Coder +// confirmed that it holds no resource of the object. +func deleteCleanupRecord(ctx context.Context, c client.Client, namespace string, uid types.UID) error { + if namespace == "" || uid == "" { + return errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") + } + configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: cleanupRecordObjectName(uid)}} + if err := c.Delete(ctx, configMap); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("delete cleanup record %s/%s: %w", namespace, configMap.Name, err) + } + return nil +} diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index b65d38081..76bd7332b 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -42,6 +42,7 @@ const ( // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders,verbs=get;list;watch;patch // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders/status,verbs=get;update // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders/finalizers,verbs=update +// +kubebuilder:rbac:groups="",namespace=coder-system,resources=configmaps,verbs=get;list;watch;create;update;delete // CoderAIProviderReconciler reconciles CoderAIProvider objects (#154). It // checks the spec, the credential Secret and the @@ -52,6 +53,10 @@ type CoderAIProviderReconciler struct { client.Client Scheme *runtime.Scheme + // OperatorNamespace holds the cleanup records (#353): the operator's own + // namespace, which only the operator can write ConfigMaps in. + OperatorNamespace string + sentKeys sync.Map // types.UID -> SHA-256 of the key that Coder holds. Memory only. } @@ -139,8 +144,8 @@ func (r *CoderAIProviderReconciler) providersWith(ctx context.Context, namespace // Reconcile runs the checks in order and reports the first one that fails. func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if r.Client == nil || r.Scheme == nil { - return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client and a scheme") + if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" { + return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme and the operator namespace") } provider := &coderv1alpha1.CoderAIProvider{} if err := r.Get(ctx, req.NamespacedName, provider); err != nil { @@ -223,6 +228,11 @@ func (r *CoderAIProviderReconciler) check( // operator token Secret that appears later. return aiProviderWait(aiProviderRetry, wait.reason, "%s", wait.message), nil } + // The record carries the provider ID before the sync replaces the + // nonce, so a later sweep can still find the provider (#353). + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); step != nil || err != nil { + return step, err + } var step *aiProviderStep recreate := provider.Status.ProviderID == "" if !recreate { @@ -243,6 +253,9 @@ func (r *CoderAIProviderReconciler) check( if step == nil { // Store the ID before the sync replaces the nonce: after that, // only the stored ID proves that this object created the provider. + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); step != nil || err != nil { + return step, err + } if err := r.writeStatus(ctx, provider, before); err != nil { return nil, err } diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index 515046e0a..053347393 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -42,7 +42,7 @@ type aiProviderEnv struct { func newAIProviderEnv(ctx context.Context, t *testing.T, withControlPlane bool) aiProviderEnv { t.Helper() env := aiProviderEnv{namespace: createTestNamespace(ctx, t, "aip"), fake: newFakeCoder(t)} - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} if withControlPlane { createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) } @@ -384,7 +384,7 @@ func TestCoderAIProviderWatches(t *testing.T) { Controller: config.Controller{SkipNameValidation: ptrTo(true)}, }) require.NoError(t, err) - require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}).SetupWithManager(mgr)) + require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace}).SetupWithManager(mgr)) mgrCtx, cancel := context.WithCancel(ctx) mgrDone := make(chan error, 1) go func() { mgrDone <- mgr.Start(mgrCtx) }() diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 0348d1f24..c946a211a 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -134,16 +134,22 @@ func recordAIProvider(ctx context.Context, provider *coderv1alpha1.CoderAIProvid return nil, nil } -// createAIProvider writes the create marker and the change token of the key, -// then sends the only create request of this attempt. A conflict on the -// marker write ends the reconcile before any request. The provider starts -// disabled with the nonce as its display name. The apply step then sets the -// spec values. +// createAIProvider writes the cleanup record, then the create marker and the +// change token of the key, then sends the only create request of this +// attempt. A failed record write or a conflict on the marker write ends the +// reconcile before any request. The provider starts disabled with the nonce +// as its display name. The apply step then sets the spec values. func (r *CoderAIProviderReconciler) createAIProvider( ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value, before *coderv1alpha1.CoderAIProviderStatus, ) (*aiProviderStep, error) { token := key.Token - provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + now := time.Now() + // The record goes first: without it, a provider that a namespace + // deletion leaves behind cannot be found again (#353 invariant 1). + if step, err := r.writeAIProviderRecord(ctx, sdk, provider, &now); step != nil || err != nil { + return step, err + } + provider.Status.CreateAttemptTime = &metav1.Time{Time: now} provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{ SecretName: token.SecretName, SecretKey: token.SecretKey, SecretUID: string(token.SecretUID), SecretResourceVersion: token.ResourceVersion, } diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 3c68c8914..7b98baaea 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -39,7 +39,7 @@ func newAIProviderCreateEnv(ctx context.Context, t *testing.T) (aiProviderEnv, * // restart gives env a new reconciler, as a controller restart does. The // reconciler keeps no state between calls, so every step resumes from status. func (env *aiProviderEnv) restart() { - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} } func TestCoderAIProviderCreate(t *testing.T) { diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 527b0d301..b0866b7fd 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -59,8 +59,9 @@ func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error { // the finalizer stays. func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) (release bool, step *aiProviderStep, err error) { if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder. - return true, nil, nil + // Nothing of this object can exist in Coder: earlier Coder answers + // cleared the create marker, so the record goes too. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} @@ -107,7 +108,14 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } - if step, err = r.deleteInCoder(ctx, sdk, provider); err != nil && deleting { + step, err = r.deleteInCoder(ctx, sdk, provider) + if err == nil && step == nil { + // Coder confirmed that it holds no provider of this object. Only + // then does the record go (#353 invariant 7). A failed record + // delete retries, and the retry gets Coder's 404 for the provider. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + } + if err != nil && deleting { // The attempt ended with an impossible answer. Log it, and still // release: the one attempt is over. log.FromContext(ctx).Error(err, "The one delete attempt while CoderControlPlane is being deleted got an impossible answer") diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go new file mode 100644 index 000000000..fe28d0b16 --- /dev/null +++ b/internal/controller/coderaiprovider_record.go @@ -0,0 +1,71 @@ +package controller + +import ( + "context" + "time" + + "github.com/coder/coder/v2/codersdk" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecordKindAIProvider is the record kind of a CoderAIProvider. +const cleanupRecordKindAIProvider = "CoderAIProvider" + +// writeAIProviderRecord writes the cleanup record of provider with the +// deployment ID that Coder answers now, so the record names the database that +// the next create or the learned provider ID belongs to. createAttempt is the +// time of a create that the caller sends next, or nil. A failed read or write +// returns a wait or an error, and the caller then sends no create (#353 +// invariant 1). +func (r *CoderAIProviderReconciler) writeAIProviderRecord( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, createAttempt *time.Time, +) (*aiProviderStep, error) { + previous, existing, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + deploymentID, err := coderDeploymentID(ctx, sdk) + if err != nil { + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil + } + record := cleanupRecord{ + Kind: cleanupRecordKindAIProvider, UID: provider.UID, Namespace: provider.Namespace, Name: provider.Name, + DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, + } + if createAttempt != nil { + record.CreateAttemptTime = *createAttempt + } else if provider.Status.CreateAttemptTime != nil { + record.CreateAttemptTime = provider.Status.CreateAttemptTime.Time + } else if previous != nil { + // The marker went after a confirmed create. The sweep still waits + // for the settle window after that create. + record.CreateAttemptTime = previous.CreateAttemptTime + } + return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) +} + +// ensureAIProviderRecord makes sure that a provider which can exist in Coder +// has a record with its current provider ID. Objects from before records +// existed get one here too. +func (r *CoderAIProviderReconciler) ensureAIProviderRecord( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, +) (*aiProviderStep, error) { + if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { + return nil, nil // Nothing of this object can exist in Coder yet. The create writes the record. + } + if provider.Status.ProviderID != "" { + // A sweep deletes the recorded ID, so only a canonical UUID goes in. + if err := checkAIProviderID(provider); err != nil { + return nil, err + } + } + record, _, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + if record != nil && record.CoderID == provider.Status.ProviderID { + return nil, nil + } + return r.writeAIProviderRecord(ctx, sdk, provider, nil) +} diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go new file mode 100644 index 000000000..9cef7acec --- /dev/null +++ b/internal/controller/coderaiprovider_record_test.go @@ -0,0 +1,154 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + + "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/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecord returns the data of the cleanup record of uid in namespace, +// or nil when there is none. +func cleanupRecord(ctx context.Context, t *testing.T, namespace string, uid types.UID) map[string]string { + t.Helper() + configMap := &corev1.ConfigMap{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "ck8s-cleanup-" + string(uid)}, configMap) + if apierrors.IsNotFound(err) { + return nil + } + require.NoError(t, err) + require.Equal(t, "CoderAIProvider", configMap.Labels["coder.com/cleanup-record"]) + return configMap.Data +} + +// #353 part 2: before its create, the controller records which Coder +// deployment holds the provider, and the provider ID before the sync replaces +// the nonce. The record holds no key. A confirmed delete in Coder removes it. +func TestCoderAIProviderCleanupRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + + // Stop after the create, before the sync replaces the nonce. + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, ok := env.fake.aiProvider("openai") + require.True(t, ok) + record := cleanupRecord(ctx, t, env.namespace, got.UID) + require.NotEmpty(t, record["createAttemptTime"]) + delete(record, "createAttemptTime") + require.Equal(t, map[string]string{ + "kind": "CoderAIProvider", "uid": string(got.UID), "namespace": env.namespace, "name": "openai", + "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), + }, record, "the provider ID is recorded before the sync replaces the nonce") + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + for _, v := range cleanupRecord(ctx, t, env.namespace, got.UID) { + require.NotContains(t, v, testAIProviderKey, "a record holds no key") + } + + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone) + _, ok = env.fake.aiProvider("openai") + require.False(t, ok) + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID), "Coder confirmed the delete, so the record goes") +} + +// #353 invariant 1: without a record, the controller sends no create. +func TestCoderAIProviderNoCreateWithoutRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("record write fails", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*corev1.ConfigMap); ok { + return errors.New("injected record write failure") + } + return c.Create(ctx, obj, opts...) + }, + }) + _, err = env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "injected record write failure") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider), "no create without a record") + got := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + require.Nil(t, got.Status.CreateAttemptTime, "no create marker either") + }) + t.Run("deployment ID unknown", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusServiceUnavailable}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider)) + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID)) + }) + t.Run("deployment ID not a UUID", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.mu.Lock() + env.fake.deploymentID = "https://coder.example.com" + env.fake.mu.Unlock() + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider)) + }) +} + +// #353: a release without a delete in Coder keeps the record, so a later +// sweep can delete the provider. +func TestCoderAIProviderRecordKeptWithoutDelete(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "ControlPlaneGone releases the finalizer") + _, ok := env.fake.aiProvider("openai") + require.True(t, ok, "the provider stays in Coder") + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"], "the record stays for a sweep") +} + +// #353: a provider from before records existed gets one, with the deployment +// ID that Coder answers now. +func TestCoderAIProviderRecordBackfill(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}})) + builds := env.fake.requestCount(routeBuildInfo) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"]) + require.Equal(t, env.fake.deploymentID, record["deploymentID"]) + require.Equal(t, builds+1, env.fake.requestCount(routeBuildInfo)) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no new create") + + // A ConfigMap of that name that is not a record is never written over. + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}, configMap)) + configMap.Data["uid"] = strings.ToUpper(string(provider.UID)) + require.NoError(t, k8sClient.Update(ctx, configMap)) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "is not a valid cleanup record") +} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index 2fb44bcf2..2ac02db02 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -353,6 +353,7 @@ func TestReleasedWithoutDeleteMetricRecovered(t *testing.T) { // control plane revokes the managed workspace RBAC before it waits. func TestControlPlaneHoldRevokesWorkspaceRBAC(t *testing.T) { t.Parallel() + ensureGatewaySchemeRegistered(t) // The RBAC cleanup also reads HTTPRoutes. ctx := context.Background() namespace := createTestNamespace(ctx, t, "hold-rbac") workspaceNamespace := createTestNamespace(ctx, t, "hold-rbac-ws") diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index 3c53d4441..01824e4aa 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -28,6 +28,7 @@ const fakeCoderToken = "operator-session-token" // Route names for fakeCoder.failNext and fakeCoder.requestCount. const ( routeUser = "user" + routeBuildInfo = "buildInfo" routeOrganization = "organization" routeOrgMember = "organizationMember" routeTemplateByName = "templateByName" @@ -84,15 +85,17 @@ type fakeCoder struct { mu sync.Mutex now func() time.Time operatorID uuid.UUID - users map[uuid.UUID]codersdk.User - orgs map[uuid.UUID]codersdk.Organization - orgRoles map[uuid.UUID]map[uuid.UUID][]codersdk.SlimRole // Organization -> user -> roles. - templates map[uuid.UUID]codersdk.Template - versions map[uuid.UUID]codersdk.TemplateVersion - workspaces map[uuid.UUID]*fakeWorkspace - pool fakePoolState // Workspace pool routes, see fakecoder_pool_test.go. - faults map[string][]fakeFault - requests map[string]int // Route -> requests received. + // deploymentID is the deployment ID that /api/v2/buildinfo answers. + deploymentID string + users map[uuid.UUID]codersdk.User + orgs map[uuid.UUID]codersdk.Organization + orgRoles map[uuid.UUID]map[uuid.UUID][]codersdk.SlimRole // Organization -> user -> roles. + templates map[uuid.UUID]codersdk.Template + versions map[uuid.UUID]codersdk.TemplateVersion + workspaces map[uuid.UUID]*fakeWorkspace + pool fakePoolState // Workspace pool routes, see fakecoder_pool_test.go. + faults map[string][]fakeFault + requests map[string]int // Route -> requests received. aiProviders map[uuid.UUID]*fakeAIProvider // See fakecoder_ai_test.go. aiGateway fakeAIGatewayState // AI Gateway key routes, see fakecoder_aigateway_test.go. @@ -109,8 +112,12 @@ func newFakeCoder(t *testing.T) *fakeCoder { faults: map[string][]fakeFault{}, requests: map[string]int{}, aiProviders: map[uuid.UUID]*fakeAIProvider{}, } f.operatorID = f.addUser("operator", codersdk.LoginTypePassword, codersdk.RoleOwner) + f.deploymentID = uuid.NewString() mux := http.NewServeMux() + f.route(mux, "GET /api/v2/buildinfo", routeBuildInfo, func(*http.Request) (int, any) { + return http.StatusOK, codersdk.BuildInfoResponse{Version: "v2.37.3", DeploymentID: f.deploymentID} + }) f.route(mux, "GET /api/v2/users/{user}", routeUser, func(r *http.Request) (int, any) { return found(f.userByIdent(r.PathValue("user"))) }) From 76d30aebff353c1391e09edf63f050de015c642d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 16:09:06 +0000 Subject: [PATCH 02/12] feat: sweep the cleanup records of CoderAIProvider objects that went without a delete (#353) Change-Id: I81ad241e8a8032f3df9eac7d825c0c70fb7da105 Signed-off-by: Thomas Kosiewski --- config/rbac/role.yaml | 7 + dist/install.yaml | 7 + docs/how-to/manage-ai-providers.md | 15 +- internal/app/controllerapp/controllerapp.go | 11 + internal/controller/cleanup_sweep.go | 300 ++++++++++++++++++ .../controller/cleanup_sweep_internal_test.go | 26 ++ internal/controller/cleanup_sweep_test.go | 236 ++++++++++++++ 7 files changed, 599 insertions(+), 3 deletions(-) create mode 100644 internal/controller/cleanup_sweep.go create mode 100644 internal/controller/cleanup_sweep_internal_test.go create mode 100644 internal/controller/cleanup_sweep_test.go diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 0b240a3cb..d6db755e3 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -164,6 +164,13 @@ rules: - patch - update - watch +- apiGroups: + - events.k8s.io + resources: + - events + verbs: + - create + - patch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/dist/install.yaml b/dist/install.yaml index eb15dae61..55eb6d8d3 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6592,6 +6592,13 @@ rules: - patch - update - watch +- apiGroups: + - events.k8s.io + resources: + - events + verbs: + - create + - patch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 133479721..9aa2cc7db 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -121,17 +121,26 @@ These limits remain: Deleting the object deletes the provider in Coder. With only an unconfirmed create, the operator reads Coder first and deletes only a provider that this object created. A create request can still commit for a short time, so within 2 minutes of that request the operator reads again before it trusts that the provider is absent. - 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 the control plane is gone, the operator cannot delete the provider now. The finalizer goes at once, and the operator logs the provider ID. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the provider. - When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When 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. +Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. A sweep deletes the leftover provider only when a control plane of the same Coder deployment comes back. Coder deletes a provider even while chat models use it. Those models stop working. ### Cleanup records Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. -The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. +The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays, and the operator sweeps it: + +1. It waits until the object with that UID is gone and 5 minutes have passed since the last create, so that a create still in flight commits first. +2. It looks for a ready `CoderControlPlane`, in any namespace, whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. +3. Through that control plane it deletes the recorded provider ID. Without a recorded ID, it deletes only the provider of that name whose display name is still the record's nonce. +4. It removes the record when Coder answers the delete with success or with its own 404. Any other answer keeps the record, and the operator retries every minute. + +So after a namespace deletion with an external database, the leftover provider goes once a new control plane that uses the same database is ready. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records. + +When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the provider in the Coder dashboard (the Event names the object, and the record holds the provider ID), then delete the ConfigMap. Find the records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. ### Stuck finalizer diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index 1bbf7a88f..50c757118 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -136,6 +136,17 @@ func SetupControllers(mgr manager.Manager) error { return fmt.Errorf("unable to create AI provider controller: %w", err) } + cleanupRecordReconciler := &controller.CleanupRecordReconciler{ + Client: client, + APIReader: mgr.GetAPIReader(), + OperatorNamespace: aiProviderReconciler.OperatorNamespace, + Clock: clock.RealClock{}, + Recorder: mgr.GetEventRecorder("coder-k8s-cleanup"), + } + if err := cleanupRecordReconciler.SetupWithManager(mgr); err != nil { + return fmt.Errorf("unable to create cleanup record controller: %w", err) + } + aiGatewayKeyReconciler := &controller.CoderAIGatewayKeyReconciler{ Client: client, APIReader: mgr.GetAPIReader(), diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go new file mode 100644 index 000000000..aa0876402 --- /dev/null +++ b/internal/controller/cleanup_sweep.go @@ -0,0 +1,300 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/events" + "k8s.io/utils/clock" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "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" +) + +const ( + // cleanupSettleWindow is how long after the last create a sweep waits, + // so a create still in flight commits first (#353 invariant 6). + cleanupSettleWindow = 5 * time.Minute + // cleanupObjectRecheck is how often a record of a living object is + // checked again. The delete of the object also wakes the record. + cleanupObjectRecheck = 30 * time.Minute + // cleanupUnmatchedRecheck is how often a record that no control plane + // of its deployment reaches is checked again. + cleanupUnmatchedRecheck = 10 * time.Minute + // cleanupRetry is the retry after a failed Coder call. + cleanupRetry = time.Minute +) + +var ( + cleanupRecordsSwept = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "coder_k8s_cleanup_records_swept_total", + Help: "Cleanup records whose resources a sweep deleted in Coder, or found gone, after their object went without a delete.", + }, []string{"kind"}) + cleanupRecordsUnmatched = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Name: "coder_k8s_cleanup_records_unmatched", + Help: "Cleanup records of gone objects that no ready control plane of their Coder deployment reaches.", + }, []string{"kind"}) +) + +func init() { + metrics.Registry.MustRegister(cleanupRecordsSwept, cleanupRecordsUnmatched) +} + +// +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch + +// CleanupRecordReconciler sweeps the cleanup records of objects that went +// without a confirmed delete in Coder (#353). It acts only through a ready +// control plane whose current Coder deployment ID equals the record's, and +// it deletes only the recorded provider ID or the provider with the +// recorded nonce. +type CleanupRecordReconciler struct { + client.Client + // APIReader reads the object of a record live, so a stale cache never + // makes a living object look gone. + APIReader client.Reader + OperatorNamespace string + Clock clock.Clock + Recorder events.EventRecorder + + unmatched sync.Map // types.UID -> kind of a record that no deployment reaches. +} + +// SetupWithManager watches the records in the operator namespace. It wakes a +// record when its CoderAIProvider is deleted, and every record when a control +// plane gets operator access, since that control plane can match them. +func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { + if r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { + return errors.New("assertion failed: the cleanup record reconciler needs a client, an API reader, the operator namespace, a clock and an event recorder") + } + isRecord := predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetNamespace() == r.OperatorNamespace && obj.GetLabels()[cleanupRecordLabel] != "" + }) + return ctrl.NewControllerManagedBy(mgr). + For(&corev1.ConfigMap{}, builder.WithPredicates(isRecord)). + Watches(&coderv1alpha1.CoderAIProvider{}, handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: r.OperatorNamespace, Name: cleanupRecordObjectName(obj.GetUID())}}} + }), builder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderControlPlane{}, handler.EnqueueRequestsFromMapFunc(r.allRecords), builder.WithPredicates(operatorAccessGained)). + Named("cleanuprecord"). + Complete(r) +} + +// operatorAccessGained passes a control plane that has operator access, or +// whose access or URL changed. +var operatorAccessGained = predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + cp, ok := e.Object.(*coderv1alpha1.CoderControlPlane) + return ok && cp.Status.OperatorAccessReady + }, + UpdateFunc: func(e event.UpdateEvent) bool { + before, ok1 := e.ObjectOld.(*coderv1alpha1.CoderControlPlane) + after, ok2 := e.ObjectNew.(*coderv1alpha1.CoderControlPlane) + return ok1 && ok2 && after.Status.OperatorAccessReady && + (!before.Status.OperatorAccessReady || before.Status.URL != after.Status.URL) + }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + GenericFunc: func(event.GenericEvent) bool { return false }, +} + +// allRecords returns every record in the operator namespace. +func (r *CleanupRecordReconciler) allRecords(ctx context.Context, _ client.Object) []reconcile.Request { + records := &corev1.ConfigMapList{} + if err := r.List(ctx, records, client.InNamespace(r.OperatorNamespace), client.HasLabels{cleanupRecordLabel}); err != nil { + log.FromContext(ctx).Error(err, "Cannot list the cleanup records") + return nil + } + requests := make([]reconcile.Request, 0, len(records.Items)) + for _, record := range records.Items { + requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: record.Namespace, Name: record.Name}}) + } + return requests +} + +// Reconcile sweeps one record. +func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + if req.Namespace != r.OperatorNamespace { + return ctrl.Result{}, nil + } + configMap := &corev1.ConfigMap{} + if err := r.Get(ctx, req.NamespacedName, configMap); apierrors.IsNotFound(err) { + if uid, ok := strings.CutPrefix(req.Name, cleanupRecordPrefix); ok { + r.setUnmatched(types.UID(uid), "", false) + } + return ctrl.Result{}, nil + } else if err != nil { + return ctrl.Result{}, err + } + record, err := parseCleanupRecord(configMap) + if err != nil { + // Never act on a record that the operator did not write. + log.FromContext(ctx).Error(err, "Ignoring a ConfigMap that is not a valid cleanup record") + return ctrl.Result{}, nil + } + if record.Kind != cleanupRecordKindAIProvider { + return ctrl.Result{}, nil // Only provider records are swept so far. + } + + // Act only when the object with that UID no longer exists (invariant 6). + provider := &coderv1alpha1.CoderAIProvider{} + if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: record.Namespace, Name: record.Name}, provider); err == nil && provider.UID == record.UID { + r.setUnmatched(record.UID, record.Kind, false) + return ctrl.Result{RequeueAfter: cleanupObjectRecheck}, nil + } else if err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, err + } + settleFrom := record.CreateAttemptTime + if settleFrom.IsZero() { + settleFrom = configMap.CreationTimestamp.Time + } + if wait := settleFrom.Add(cleanupSettleWindow).Sub(r.Clock.Now()); wait > 0 { + return ctrl.Result{RequeueAfter: wait}, nil + } + + sdk, controlPlane, unknown, err := r.matchingControlPlane(ctx, record.DeploymentID) + if err != nil { + return ctrl.Result{}, err + } + if sdk == nil && unknown { + // A ready control plane did not answer its deployment ID, for + // example while its Coder pods start. It can still match. + return ctrl.Result{RequeueAfter: cleanupRetry}, nil + } + if sdk == nil { + r.setUnmatched(record.UID, record.Kind, true) + r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "NoMatchingDeployment", "Sweep", + "No ready CoderControlPlane reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", + record.DeploymentID, record.Kind, record.Namespace, record.Name) + return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil + } + r.setUnmatched(record.UID, record.Kind, false) + logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, + "controlPlane", types.NamespacedName{Namespace: controlPlane.Namespace, Name: controlPlane.Name}, "provider", record.Namespace+"/"+record.Name) + gone, why, err := sweepAIProvider(ctx, sdk, record) + if err != nil { + return ctrl.Result{}, err + } + if !gone { + logger.Info("The cleanup of a deleted provider in Coder failed, retrying", "why", why) + return ctrl.Result{RequeueAfter: cleanupRetry}, nil + } + // Coder confirmed the delete in the matching deployment (invariant 7). + if err := deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, record.UID); err != nil { + return ctrl.Result{}, err + } + cleanupRecordsSwept.WithLabelValues(record.Kind).Inc() + logger.Info("Swept the provider of a deleted CoderAIProvider in Coder", "result", why, "providerID", record.CoderID) + return ctrl.Result{}, nil +} + +// matchingControlPlane returns a Coder client of a ready control plane whose +// current deployment ID is deploymentID, or nil when there is none (#353 +// invariant 4). The ID comes from Coder now, never from the record or a URL. +// unknown reports a ready control plane whose deployment ID read failed. +func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, deploymentID string) (*codersdk.Client, *coderv1alpha1.CoderControlPlane, bool, error) { + controlPlanes := &coderv1alpha1.CoderControlPlaneList{} + if err := r.List(ctx, controlPlanes); err != nil { + return nil, nil, false, fmt.Errorf("list control planes: %w", err) + } + unknown := false + for i := range controlPlanes.Items { + cp := &controlPlanes.Items[i] + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, cp.Namespace, cp.Name) + if err != nil || wait != nil { + continue // Not ready: it cannot sweep now. + } + id, err := coderDeploymentID(ctx, sdk) + if err != nil { + log.FromContext(ctx).V(1).Info("Cannot read the deployment ID of a control plane", "controlPlane", cp.Namespace+"/"+cp.Name, "error", coderErrorSummary(err)) + unknown = true + continue + } + if id == deploymentID { + return sdk, cp, false, nil + } + } + return nil, nil, unknown, nil +} + +// sweepAIProvider deletes the provider of record in Coder. It returns gone +// when Coder confirms that it holds no provider of the record: a delete +// answered with success or Coder's own 404, a read with Coder's own 404 for +// a site owner, or a provider of that name without the record's nonce. It +// deletes only the recorded ID or the provider whose display name is the +// recorded nonce (invariant 5). +func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { + if record.Nonce != aiProviderNoncePrefix+string(record.UID) { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + } + if step := requireSiteOwner(ctx, sdk); step != nil { + // Only a site owner can trust Coder's 404 for a provider. + return false, step.message, nil + } + id := record.CoderID + if id != "" { + if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, id) + } + } else { + found, err := sdk.AIProvider(ctx, record.Name) + switch { + case isCoderResourceNotFound(err): + return true, "no provider of that name", nil + case err != nil: + return false, "get provider: " + coderErrorSummary(err), nil + } + if err := coderAnswerFor("AI provider", record.Name, found.Name); err != nil { + return false, err.Error(), nil //nolint:nilerr // A wrong answer retries the sweep. + } + if found.DisplayName != record.Nonce { + // Another provider holds the name, so no create of this object + // committed after the settle window. + return true, "the provider of that name is not this object's", nil + } + if found.ID == uuid.Nil { + return false, "", errors.New("assertion failed: Coder answered an AI provider without an ID") + } + id = found.ID.String() + } + if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderResourceNotFound(err) { + return false, "delete provider " + id + ": " + coderErrorSummary(err), nil + } else if err != nil { + return true, "provider " + id + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. + } + return true, "deleted provider " + id, nil +} + +// setUnmatched keeps the unmatched gauge equal to the records that no +// deployment reaches. +func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmatched bool) { + if unmatched { + r.unmatched.Store(uid, kind) + } else { + r.unmatched.Delete(uid) + } + counts := map[string]float64{cleanupRecordKindAIProvider: 0} + r.unmatched.Range(func(_, v any) bool { + counts[v.(string)]++ + return true + }) + for k, n := range counts { + cleanupRecordsUnmatched.WithLabelValues(k).Set(n) + } +} diff --git a/internal/controller/cleanup_sweep_internal_test.go b/internal/controller/cleanup_sweep_internal_test.go new file mode 100644 index 000000000..1c7cb4fb1 --- /dev/null +++ b/internal/controller/cleanup_sweep_internal_test.go @@ -0,0 +1,26 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/event" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// #353: a control plane wakes the cleanup records when it gets operator +// access or a new URL, since it can then match their deployment. +func TestOperatorAccessGained(t *testing.T) { + t.Parallel() + cp := func(ready bool, url string) *coderv1alpha1.CoderControlPlane { + return &coderv1alpha1.CoderControlPlane{Status: coderv1alpha1.CoderControlPlaneStatus{OperatorAccessReady: ready, URL: url}} + } + require.True(t, operatorAccessGained.Create(event.CreateEvent{Object: cp(true, "http://a")})) + require.False(t, operatorAccessGained.Create(event.CreateEvent{Object: cp(false, "http://a")})) + require.True(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(false, "http://a"), ObjectNew: cp(true, "http://a")})) + require.True(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(true, "http://b")})) + require.False(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(true, "http://a")}), "no wake on every status write") + require.False(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(false, "http://a")})) + require.False(t, operatorAccessGained.Delete(event.DeleteEvent{Object: cp(true, "http://a")})) +} diff --git a/internal/controller/cleanup_sweep_test.go b/internal/controller/cleanup_sweep_test.go new file mode 100644 index 000000000..e141aeb55 --- /dev/null +++ b/internal/controller/cleanup_sweep_test.go @@ -0,0 +1,236 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/events" + 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" + "github.com/coder/coder-k8s/internal/controller" +) + +// sweepEnv is a provider that went with its control plane, as in a +// namespace deletion: Coder keeps the provider and the record stays. +type sweepEnv struct { + aiProviderEnv + provider *coderv1alpha1.CoderAIProvider + clock *clocktesting.FakeClock + recorder *events.FakeRecorder + sweeper *controller.CleanupRecordReconciler +} + +func newSweepEnv(ctx context.Context, t *testing.T) sweepEnv { + t.Helper() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone) + require.NotNil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) + return newSweeper(t, env, provider) +} + +// newSweeper returns a sweeper for the record of provider in env. +func newSweeper(t *testing.T, env aiProviderEnv, provider *coderv1alpha1.CoderAIProvider) sweepEnv { + t.Helper() + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + // Other tests run their own control planes: the sweeper of this test + // lists only the control planes of its namespace. + scoped := interceptor.NewClient(base, interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + return c.List(ctx, list, append(opts, client.InNamespace(env.namespace))...) + }, + }) + s := sweepEnv{aiProviderEnv: env, provider: provider, clock: clocktesting.NewFakeClock(time.Now()), recorder: events.NewFakeRecorder(10)} + s.sweeper = &controller.CleanupRecordReconciler{Client: scoped, APIReader: k8sClient, OperatorNamespace: env.namespace, Clock: s.clock, Recorder: s.recorder} + return s +} + +// providerMetric reads a controller metric with the label kind=CoderAIProvider. +func providerMetric(t *testing.T, name string) float64 { + t.Helper() + families, err := metrics.Registry.Gather() + require.NoError(t, err) + for _, f := range families { + for _, m := range f.GetMetric() { + for _, l := range m.GetLabel() { + if f.GetName() == name && l.GetName() == "kind" && l.GetValue() == "CoderAIProvider" { + if m.GetGauge() != nil { + return m.GetGauge().GetValue() + } + return m.GetCounter().GetValue() + } + } + } + } + return 0 +} + +func (s sweepEnv) sweep(ctx context.Context, t *testing.T) ctrl.Result { + t.Helper() + result, err := s.sweeper.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}}) + require.NoError(t, err) + return result +} + +// #353: after a namespace deletion, a new control plane that reaches the +// same Coder deployment deletes the leftover provider, once the settle +// window has passed. A different deployment at the same URL never does. +// Not parallel: it reads process-wide metrics. +func TestCleanupSweepAIProvider(t *testing.T) { + ctx := context.Background() + s := newSweepEnv(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "the provider is left in Coder") + swept := providerMetric(t, "coder_k8s_cleanup_records_swept_total") + + requests := s.fake.totalRequests() + result := s.sweep(ctx, t) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call inside the settle window") + require.Greater(t, result.RequeueAfter, 4*time.Minute, "the settle window has not passed") + require.LessOrEqual(t, result.RequeueAfter, 5*time.Minute, "it waits for the end of the settle window") + deletes := s.fake.requestCount(routeDeleteAIProvider) + s.clock.Step(6 * time.Minute) + result = s.sweep(ctx, t) + require.Equal(t, 10*time.Minute, result.RequeueAfter, "no control plane reaches the deployment") + require.Equal(t, deletes, s.fake.requestCount(routeDeleteAIProvider)) + + // A new control plane at the same URL, but another deployment answers: + // for example a new Coder database behind the old address. + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + recorded := s.fake.deploymentID + s.fake.mu.Lock() + s.fake.deploymentID = uuid.NewString() + s.fake.mu.Unlock() + result = s.sweep(ctx, t) + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Equal(t, deletes, s.fake.requestCount(routeDeleteAIProvider), "a different deployment is never swept") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + require.Contains(t, <-s.recorder.Events, "NoMatchingDeployment") + require.Equal(t, 1.0, providerMetric(t, "coder_k8s_cleanup_records_unmatched")) + + // The same deployment comes back. + s.fake.mu.Lock() + s.fake.deploymentID = recorded + s.fake.mu.Unlock() + result = s.sweep(ctx, t) + require.Zero(t, result.RequeueAfter) + _, ok = s.fake.aiProvider("openai") + require.False(t, ok, "the sweep deleted the provider") + require.Equal(t, deletes+1, s.fake.requestCount(routeDeleteAIProvider)) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "Coder confirmed the delete, so the record goes") + require.Equal(t, swept+1, providerMetric(t, "coder_k8s_cleanup_records_swept_total")) + require.Zero(t, providerMetric(t, "coder_k8s_cleanup_records_unmatched")) +} + +// #353 invariants 5 and 6: a sweep never touches the provider of a living +// object, finds a provider without a recorded ID only by its nonce, and +// leaves a provider of the same name that the object did not create. +func TestCleanupSweepAIProviderCases(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("living object", func(t *testing.T) { + t.Parallel() + env, _, provider := newReadyAIProvider(ctx, t) + s := newSweeper(t, env, provider) + s.clock.Step(time.Hour) + requests := s.fake.totalRequests() + require.Equal(t, 30*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call for a living object") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, provider.UID)) + }) + t.Run("same name, another object", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.createProvider(ctx, t, nil) // A new object with the old name and a new UID. + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.False(t, ok, "only the UID decides that the object is gone") + }) + for name, nonce := range map[string]bool{"nonce matches": true, "nonce differs": false} { + t.Run(name, func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Data["coderID"] = "" // The create committed, but its ID was never recorded. + require.NoError(t, k8sClient.Update(ctx, configMap)) + if nonce { + s.fake.mu.Lock() + for _, p := range s.fake.aiProviders { + p.displayName = "coder-k8s:" + string(s.provider.UID) + } + s.fake.mu.Unlock() + } + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + _, ok := s.fake.aiProvider("openai") + require.Equal(t, !nonce, ok, "only the provider with the record's nonce is deleted") + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "Coder holds no provider of the record") + }) + } + t.Run("delete fails", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusBadGateway, HTML: "bad gateway"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "the record stays until Coder confirms the delete") + s.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusNotFound, HTML: "not found"}) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "only Coder's own 404 confirms") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("deployment ID not answered yet", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusBadGateway, HTML: "starting"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "a control plane that does not answer yet can still match") + require.Empty(t, s.recorder.Events, "no NoMatchingDeployment Event for an unknown deployment") + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("operator not a site owner", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.updateUser(s.fake.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIProvider), "only a site owner can trust Coder's 404") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("not a record", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Labels["coder.com/cleanup-record"] = "CoderAIGatewayKey" + require.NoError(t, k8sClient.Update(ctx, configMap)) + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "a ConfigMap that is not a valid record is never acted on") + }) +} From 553cad7604a9cecc8469e816dd54635cece789b8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 15:59:09 +0000 Subject: [PATCH 03/12] feat: write a cleanup record before each CoderAIProvider create (#353) Change-Id: Ice66d8e736e7372a3afd95d8c96a79955aca6b04 Signed-off-by: Thomas Kosiewski --- .github/workflows/ci.yaml | 1 + config/default/kustomization.yaml | 1 + config/rbac/cleanup-records-rolebinding.yaml | 16 ++ config/rbac/role.yaml | 18 ++ dist/install.yaml | 32 ++++ docs/explanation/architecture.md | 2 +- docs/how-to/manage-ai-gateway-keys.md | 2 +- docs/how-to/manage-ai-providers.md | 8 +- internal/app/controllerapp/controllerapp.go | 2 + internal/controller/cleanup_record.go | 165 ++++++++++++++++++ .../controller/coderaiprovider_controller.go | 17 +- .../coderaiprovider_controller_test.go | 4 +- internal/controller/coderaiprovider_create.go | 18 +- .../controller/coderaiprovider_create_test.go | 2 +- internal/controller/coderaiprovider_delete.go | 14 +- internal/controller/coderaiprovider_record.go | 71 ++++++++ .../controller/coderaiprovider_record_test.go | 154 ++++++++++++++++ internal/controller/controlplane_hold_test.go | 1 + internal/controller/fakecoder_test.go | 25 ++- 19 files changed, 527 insertions(+), 26 deletions(-) create mode 100644 config/rbac/cleanup-records-rolebinding.yaml create mode 100644 internal/controller/cleanup_record.go create mode 100644 internal/controller/coderaiprovider_record.go create mode 100644 internal/controller/coderaiprovider_record_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d568619ed..a759b8b11 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -94,6 +94,7 @@ jobs: # Shared Coder access that CoderTemplateTest and later pool controllers call. - 'internal/controller/controlplane_access.go' - 'internal/controller/controlplane_hold.go' + - 'internal/controller/cleanup_record*' - '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/config/default/kustomization.yaml b/config/default/kustomization.yaml index d3f9b0e3a..c4379d6d9 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -9,6 +9,7 @@ resources: - ../crd/bases/coder.com_coderprovisioners.yaml - ../crd/bases/coder.com_codertemplatetests.yaml - ../crd/bases/coder.com_coderworkspaceproxies.yaml + - ../rbac/cleanup-records-rolebinding.yaml - ../rbac/clusterrolebinding.yaml - ../rbac/role.yaml - ../rbac/serviceaccount.yaml diff --git a/config/rbac/cleanup-records-rolebinding.yaml b/config/rbac/cleanup-records-rolebinding.yaml new file mode 100644 index 000000000..5572237fb --- /dev/null +++ b/config/rbac/cleanup-records-rolebinding.yaml @@ -0,0 +1,16 @@ +# Lets the operator write its cleanup records (#353): ConfigMaps named ck8s-cleanup- in its own +# namespace. The namespaced Role manager-role comes from the kubebuilder RBAC markers in role.yaml. No other +# subject gets write access to these ConfigMaps. +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: coder-k8s-cleanup-records + namespace: coder-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: + - kind: ServiceAccount + name: coder-k8s + namespace: coder-system diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 561f059eb..0b240a3cb 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -201,3 +201,21 @@ rules: - patch - update - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: coder-system +rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - get + - list + - update + - watch diff --git a/dist/install.yaml b/dist/install.yaml index 23b1e3b72..eb15dae61 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6410,6 +6410,24 @@ metadata: namespace: coder-system --- apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: coder-system +rules: +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - get + - list + - update + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: manager-role @@ -6613,6 +6631,20 @@ rules: - watch --- apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: coder-k8s-cleanup-records + namespace: coder-system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: +- kind: ServiceAccount + name: coder-k8s + namespace: coder-system +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: name: coder-k8s diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index c6a02ff4f..987146852 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -76,7 +76,7 @@ graph TD | Path | Contents | | --- | --- | | `config/crd/bases/` | The generated CRDs for `CoderControlPlane`, `CoderProvisioner`, `CoderWorkspaceProxy`, `CoderTemplateTest`, `CoderAIProvider`, and `CoderAIGatewayKey` | -| `config/rbac/` | The `coder-k8s` ServiceAccount, the `manager-role` ClusterRole, and the bindings (including auth-delegator) | +| `config/rbac/` | The `coder-k8s` ServiceAccount, the `manager-role` ClusterRole, the `manager-role` Role in `coder-system` (write access to the cleanup-record ConfigMaps there), and the bindings (including auth-delegator) | | `config/apiserver-standalone/` | For standalone `--app=aggregated-apiserver`: the `coder-k8s-apiserver` ServiceAccount, its RBAC, and the placeholder Secret for the serving CA | | `deploy/deployment.yaml` | The `coder-k8s` Deployment (default `--app=all`) | | `deploy/apiserver-service.yaml`, `deploy/apiserver-apiservice.yaml` | The Service and APIService that make the aggregated API available | diff --git a/docs/how-to/manage-ai-gateway-keys.md b/docs/how-to/manage-ai-gateway-keys.md index b9427ee63..62e98555f 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 (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 you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys and providers as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first. When 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 e1879d0f7..133479721 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -122,11 +122,17 @@ 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 (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 you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When 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. +### Cleanup records + +Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. + +The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. + ### Stuck finalizer A deleted `CoderAIProvider` stays `Terminating` while the operator cannot prove that Coder no longer holds its provider. Two cases never end by themselves: diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index d0ca9b118..1bbf7a88f 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -129,6 +129,8 @@ func SetupControllers(mgr manager.Manager) error { aiProviderReconciler := &controller.CoderAIProviderReconciler{ Client: client, Scheme: managerScheme, + // The operator's own namespace holds the cleanup records (#353). + OperatorNamespace: detectLeaderElectionNamespace(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create AI provider controller: %w", err) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go new file mode 100644 index 000000000..44eff4a80 --- /dev/null +++ b/internal/controller/cleanup_record.go @@ -0,0 +1,165 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + 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" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Cleanup records (#353 part 2, option B′). Before the operator creates +// something in Coder for an object, it writes a record: one ConfigMap named +// ck8s-cleanup- in the operator's own namespace. The record says +// which Coder deployment holds the resource and how to find it. It stays when +// the object goes without a confirmed delete in Coder (for example in a +// namespace deletion), so a later sweep through a control plane of the same +// deployment can delete the resource. A record holds no secrets. The +// ConfigMap creation time is the record's created-at. +const ( + cleanupRecordPrefix = "ck8s-cleanup-" + // cleanupRecordLabel marks a record. Its value is the object kind. + cleanupRecordLabel = "coder.com/cleanup-record" + + cleanupRecordKind = "kind" + cleanupRecordUID = "uid" + cleanupRecordNamespace = "namespace" + cleanupRecordName = "name" + cleanupRecordDeploymentID = "deploymentID" + cleanupRecordNonce = "nonce" + cleanupRecordCoderID = "coderID" + cleanupRecordCreateSentAt = "createAttemptTime" +) + +// cleanupRecord is the content of a record. +type cleanupRecord struct { + Kind string + UID types.UID + Namespace string + Name string + // DeploymentID is Coder's deployment ID (/api/v2/buildinfo), read from + // Coder in the reconcile that sends the create. It lives in the Coder + // database, so it names the database that holds the resource. + DeploymentID string + // Nonce finds a resource whose Coder ID is not known yet: the display + // name of a provider. + Nonce string + // CoderID is the Coder ID of the resource once the operator knows it. + CoderID string + // CreateAttemptTime is when the operator last sent a create. A sweep + // waits for the settle window after it. + CreateAttemptTime time.Time +} + +func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) } + +// coderDeploymentID reads Coder's deployment ID. Only a canonical UUID is +// accepted: the ID decides which resources a sweep may delete. +func coderDeploymentID(ctx context.Context, sdk *codersdk.Client) (string, error) { + info, err := sdk.BuildInfo(ctx) + if err != nil { + return "", err + } + if id, err := uuid.Parse(info.DeploymentID); err != nil || id == uuid.Nil || id.String() != info.DeploymentID { + return "", fmt.Errorf("coder answered a deployment ID %q that is not a canonical UUID", info.DeploymentID) + } + return info.DeploymentID, nil +} + +// readCleanupRecord returns the record of uid, or nil when there is none. A +// ConfigMap of that name that is not a record of uid is an error: the +// operator never writes over it. +func readCleanupRecord(ctx context.Context, reader client.Reader, namespace string, uid types.UID) (*cleanupRecord, *corev1.ConfigMap, error) { + if namespace == "" || uid == "" { + return nil, nil, errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") + } + configMap := &corev1.ConfigMap{} + if err := reader.Get(ctx, types.NamespacedName{Namespace: namespace, Name: cleanupRecordObjectName(uid)}, configMap); apierrors.IsNotFound(err) { + return nil, nil, nil + } else if err != nil { + return nil, nil, fmt.Errorf("get cleanup record %s/%s: %w", namespace, cleanupRecordObjectName(uid), err) + } + record, err := parseCleanupRecord(configMap) + if err != nil { + return nil, nil, err + } + if record.UID != uid { + return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s", namespace, configMap.Name, uid) + } + return record, configMap, nil +} + +// parseCleanupRecord reads a record from its ConfigMap. +func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { + d := configMap.Data + record := &cleanupRecord{ + Kind: d[cleanupRecordKind], UID: types.UID(d[cleanupRecordUID]), Namespace: d[cleanupRecordNamespace], Name: d[cleanupRecordName], + DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], + } + if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || + configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { + return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record", configMap.Namespace, configMap.Name) + } + if t := d[cleanupRecordCreateSentAt]; t != "" { + parsed, err := time.Parse(time.RFC3339, t) + if err != nil { + return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s: %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err) + } + record.CreateAttemptTime = parsed + } + return record, nil +} + +// writeCleanupRecord creates or replaces the record in namespace. existing +// is the ConfigMap that readCleanupRecord returned, or nil. +func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, existing *corev1.ConfigMap, record cleanupRecord) error { + if namespace == "" || record.Kind == "" || record.UID == "" || record.DeploymentID == "" { + return errors.New("assertion failed: a cleanup record needs the operator namespace, a kind, an object UID and a deployment ID") + } + data := map[string]string{ + cleanupRecordKind: record.Kind, cleanupRecordUID: string(record.UID), cleanupRecordNamespace: record.Namespace, cleanupRecordName: record.Name, + cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, + } + if !record.CreateAttemptTime.IsZero() { + data[cleanupRecordCreateSentAt] = record.CreateAttemptTime.UTC().Format(time.RFC3339) + } + if existing != nil { + updated := existing.DeepCopy() + updated.Data = data + if err := c.Update(ctx, updated); err != nil { + return fmt.Errorf("update cleanup record %s/%s: %w", namespace, updated.Name, err) + } + return nil + } + created := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: cleanupRecordObjectName(record.UID), Namespace: namespace, + Labels: map[string]string{cleanupRecordLabel: record.Kind, "app.kubernetes.io/managed-by": "coder-k8s"}, + }, + Data: data, + } + if err := c.Create(ctx, created); err != nil { + return fmt.Errorf("create cleanup record %s/%s: %w", namespace, created.Name, err) + } + return nil +} + +// deleteCleanupRecord removes the record of uid. Call it only after Coder +// confirmed that it holds no resource of the object. +func deleteCleanupRecord(ctx context.Context, c client.Client, namespace string, uid types.UID) error { + if namespace == "" || uid == "" { + return errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") + } + configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: cleanupRecordObjectName(uid)}} + if err := c.Delete(ctx, configMap); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("delete cleanup record %s/%s: %w", namespace, configMap.Name, err) + } + return nil +} diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index b65d38081..76bd7332b 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -42,6 +42,7 @@ const ( // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders,verbs=get;list;watch;patch // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders/status,verbs=get;update // +kubebuilder:rbac:groups=coder.com,resources=coderaiproviders/finalizers,verbs=update +// +kubebuilder:rbac:groups="",namespace=coder-system,resources=configmaps,verbs=get;list;watch;create;update;delete // CoderAIProviderReconciler reconciles CoderAIProvider objects (#154). It // checks the spec, the credential Secret and the @@ -52,6 +53,10 @@ type CoderAIProviderReconciler struct { client.Client Scheme *runtime.Scheme + // OperatorNamespace holds the cleanup records (#353): the operator's own + // namespace, which only the operator can write ConfigMaps in. + OperatorNamespace string + sentKeys sync.Map // types.UID -> SHA-256 of the key that Coder holds. Memory only. } @@ -139,8 +144,8 @@ func (r *CoderAIProviderReconciler) providersWith(ctx context.Context, namespace // Reconcile runs the checks in order and reports the first one that fails. func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if r.Client == nil || r.Scheme == nil { - return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client and a scheme") + if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" { + return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme and the operator namespace") } provider := &coderv1alpha1.CoderAIProvider{} if err := r.Get(ctx, req.NamespacedName, provider); err != nil { @@ -223,6 +228,11 @@ func (r *CoderAIProviderReconciler) check( // operator token Secret that appears later. return aiProviderWait(aiProviderRetry, wait.reason, "%s", wait.message), nil } + // The record carries the provider ID before the sync replaces the + // nonce, so a later sweep can still find the provider (#353). + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); step != nil || err != nil { + return step, err + } var step *aiProviderStep recreate := provider.Status.ProviderID == "" if !recreate { @@ -243,6 +253,9 @@ func (r *CoderAIProviderReconciler) check( if step == nil { // Store the ID before the sync replaces the nonce: after that, // only the stored ID proves that this object created the provider. + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); step != nil || err != nil { + return step, err + } if err := r.writeStatus(ctx, provider, before); err != nil { return nil, err } diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index 515046e0a..053347393 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -42,7 +42,7 @@ type aiProviderEnv struct { func newAIProviderEnv(ctx context.Context, t *testing.T, withControlPlane bool) aiProviderEnv { t.Helper() env := aiProviderEnv{namespace: createTestNamespace(ctx, t, "aip"), fake: newFakeCoder(t)} - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} if withControlPlane { createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) } @@ -384,7 +384,7 @@ func TestCoderAIProviderWatches(t *testing.T) { Controller: config.Controller{SkipNameValidation: ptrTo(true)}, }) require.NoError(t, err) - require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}).SetupWithManager(mgr)) + require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace}).SetupWithManager(mgr)) mgrCtx, cancel := context.WithCancel(ctx) mgrDone := make(chan error, 1) go func() { mgrDone <- mgr.Start(mgrCtx) }() diff --git a/internal/controller/coderaiprovider_create.go b/internal/controller/coderaiprovider_create.go index 0348d1f24..c946a211a 100644 --- a/internal/controller/coderaiprovider_create.go +++ b/internal/controller/coderaiprovider_create.go @@ -134,16 +134,22 @@ func recordAIProvider(ctx context.Context, provider *coderv1alpha1.CoderAIProvid return nil, nil } -// createAIProvider writes the create marker and the change token of the key, -// then sends the only create request of this attempt. A conflict on the -// marker write ends the reconcile before any request. The provider starts -// disabled with the nonce as its display name. The apply step then sets the -// spec values. +// createAIProvider writes the cleanup record, then the create marker and the +// change token of the key, then sends the only create request of this +// attempt. A failed record write or a conflict on the marker write ends the +// reconcile before any request. The provider starts disabled with the nonce +// as its display name. The apply step then sets the spec values. func (r *CoderAIProviderReconciler) createAIProvider( ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, key credentialsecret.Value, before *coderv1alpha1.CoderAIProviderStatus, ) (*aiProviderStep, error) { token := key.Token - provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + now := time.Now() + // The record goes first: without it, a provider that a namespace + // deletion leaves behind cannot be found again (#353 invariant 1). + if step, err := r.writeAIProviderRecord(ctx, sdk, provider, &now); step != nil || err != nil { + return step, err + } + provider.Status.CreateAttemptTime = &metav1.Time{Time: now} provider.Status.Credential = &coderv1alpha1.CoderAIProviderCredentialStatus{ SecretName: token.SecretName, SecretKey: token.SecretKey, SecretUID: string(token.SecretUID), SecretResourceVersion: token.ResourceVersion, } diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 3c68c8914..7b98baaea 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -39,7 +39,7 @@ func newAIProviderCreateEnv(ctx context.Context, t *testing.T) (aiProviderEnv, * // restart gives env a new reconciler, as a controller restart does. The // reconciler keeps no state between calls, so every step resumes from status. func (env *aiProviderEnv) restart() { - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} } func TestCoderAIProviderCreate(t *testing.T) { diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 527b0d301..b0866b7fd 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -59,8 +59,9 @@ func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error { // the finalizer stays. func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) (release bool, step *aiProviderStep, err error) { if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder. - return true, nil, nil + // Nothing of this object can exist in Coder: earlier Coder answers + // cleared the create marker, so the record goes too. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} @@ -107,7 +108,14 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } - if step, err = r.deleteInCoder(ctx, sdk, provider); err != nil && deleting { + step, err = r.deleteInCoder(ctx, sdk, provider) + if err == nil && step == nil { + // Coder confirmed that it holds no provider of this object. Only + // then does the record go (#353 invariant 7). A failed record + // delete retries, and the retry gets Coder's 404 for the provider. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + } + if err != nil && deleting { // The attempt ended with an impossible answer. Log it, and still // release: the one attempt is over. log.FromContext(ctx).Error(err, "The one delete attempt while CoderControlPlane is being deleted got an impossible answer") diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go new file mode 100644 index 000000000..fe28d0b16 --- /dev/null +++ b/internal/controller/coderaiprovider_record.go @@ -0,0 +1,71 @@ +package controller + +import ( + "context" + "time" + + "github.com/coder/coder/v2/codersdk" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecordKindAIProvider is the record kind of a CoderAIProvider. +const cleanupRecordKindAIProvider = "CoderAIProvider" + +// writeAIProviderRecord writes the cleanup record of provider with the +// deployment ID that Coder answers now, so the record names the database that +// the next create or the learned provider ID belongs to. createAttempt is the +// time of a create that the caller sends next, or nil. A failed read or write +// returns a wait or an error, and the caller then sends no create (#353 +// invariant 1). +func (r *CoderAIProviderReconciler) writeAIProviderRecord( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, createAttempt *time.Time, +) (*aiProviderStep, error) { + previous, existing, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + deploymentID, err := coderDeploymentID(ctx, sdk) + if err != nil { + return aiProviderWait(aiProviderRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil + } + record := cleanupRecord{ + Kind: cleanupRecordKindAIProvider, UID: provider.UID, Namespace: provider.Namespace, Name: provider.Name, + DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, + } + if createAttempt != nil { + record.CreateAttemptTime = *createAttempt + } else if provider.Status.CreateAttemptTime != nil { + record.CreateAttemptTime = provider.Status.CreateAttemptTime.Time + } else if previous != nil { + // The marker went after a confirmed create. The sweep still waits + // for the settle window after that create. + record.CreateAttemptTime = previous.CreateAttemptTime + } + return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) +} + +// ensureAIProviderRecord makes sure that a provider which can exist in Coder +// has a record with its current provider ID. Objects from before records +// existed get one here too. +func (r *CoderAIProviderReconciler) ensureAIProviderRecord( + ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, +) (*aiProviderStep, error) { + if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { + return nil, nil // Nothing of this object can exist in Coder yet. The create writes the record. + } + if provider.Status.ProviderID != "" { + // A sweep deletes the recorded ID, so only a canonical UUID goes in. + if err := checkAIProviderID(provider); err != nil { + return nil, err + } + } + record, _, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + if err != nil { + return nil, err + } + if record != nil && record.CoderID == provider.Status.ProviderID { + return nil, nil + } + return r.writeAIProviderRecord(ctx, sdk, provider, nil) +} diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go new file mode 100644 index 000000000..9cef7acec --- /dev/null +++ b/internal/controller/coderaiprovider_record_test.go @@ -0,0 +1,154 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + + "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/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecord returns the data of the cleanup record of uid in namespace, +// or nil when there is none. +func cleanupRecord(ctx context.Context, t *testing.T, namespace string, uid types.UID) map[string]string { + t.Helper() + configMap := &corev1.ConfigMap{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "ck8s-cleanup-" + string(uid)}, configMap) + if apierrors.IsNotFound(err) { + return nil + } + require.NoError(t, err) + require.Equal(t, "CoderAIProvider", configMap.Labels["coder.com/cleanup-record"]) + return configMap.Data +} + +// #353 part 2: before its create, the controller records which Coder +// deployment holds the provider, and the provider ID before the sync replaces +// the nonce. The record holds no key. A confirmed delete in Coder removes it. +func TestCoderAIProviderCleanupRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + + // Stop after the create, before the sync replaces the nonce. + env.fake.failNext(routeUpdateAIProvider, fakeFault{Status: http.StatusInternalServerError}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + stored, ok := env.fake.aiProvider("openai") + require.True(t, ok) + record := cleanupRecord(ctx, t, env.namespace, got.UID) + require.NotEmpty(t, record["createAttemptTime"]) + delete(record, "createAttemptTime") + require.Equal(t, map[string]string{ + "kind": "CoderAIProvider", "uid": string(got.UID), "namespace": env.namespace, "name": "openai", + "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), + }, record, "the provider ID is recorded before the sync replaces the nonce") + + got, _ = env.reconcile(ctx, t, got) + requireAIProviderReady(t, got) + for _, v := range cleanupRecord(ctx, t, env.namespace, got.UID) { + require.NotContains(t, v, testAIProviderKey, "a record holds no key") + } + + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone) + _, ok = env.fake.aiProvider("openai") + require.False(t, ok) + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID), "Coder confirmed the delete, so the record goes") +} + +// #353 invariant 1: without a record, the controller sends no create. +func TestCoderAIProviderNoCreateWithoutRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("record write fails", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*corev1.ConfigMap); ok { + return errors.New("injected record write failure") + } + return c.Create(ctx, obj, opts...) + }, + }) + _, err = env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "injected record write failure") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider), "no create without a record") + got := &coderv1alpha1.CoderAIProvider{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got)) + require.Nil(t, got.Status.CreateAttemptTime, "no create marker either") + }) + t.Run("deployment ID unknown", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusServiceUnavailable}) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider)) + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID)) + }) + t.Run("deployment ID not a UUID", func(t *testing.T) { + t.Parallel() + env, _, provider := newAIProviderCreateEnv(ctx, t) + env.fake.mu.Lock() + env.fake.deploymentID = "https://coder.example.com" + env.fake.mu.Unlock() + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.fake.requestCount(routeCreateAIProvider)) + }) +} + +// #353: a release without a delete in Coder keeps the record, so a later +// sweep can delete the provider. +func TestCoderAIProviderRecordKeptWithoutDelete(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "ControlPlaneGone releases the finalizer") + _, ok := env.fake.aiProvider("openai") + require.True(t, ok, "the provider stays in Coder") + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"], "the record stays for a sweep") +} + +// #353: a provider from before records existed gets one, with the deployment +// ID that Coder answers now. +func TestCoderAIProviderRecordBackfill(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}})) + builds := env.fake.requestCount(routeBuildInfo) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + record := cleanupRecord(ctx, t, env.namespace, provider.UID) + require.Equal(t, provider.Status.ProviderID, record["coderID"]) + require.Equal(t, env.fake.deploymentID, record["deploymentID"]) + require.Equal(t, builds+1, env.fake.requestCount(routeBuildInfo)) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no new create") + + // A ConfigMap of that name that is not a record is never written over. + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}, configMap)) + configMap.Data["uid"] = strings.ToUpper(string(provider.UID)) + require.NoError(t, k8sClient.Update(ctx, configMap)) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) + require.ErrorContains(t, err, "is not a valid cleanup record") +} diff --git a/internal/controller/controlplane_hold_test.go b/internal/controller/controlplane_hold_test.go index 2fb44bcf2..2ac02db02 100644 --- a/internal/controller/controlplane_hold_test.go +++ b/internal/controller/controlplane_hold_test.go @@ -353,6 +353,7 @@ func TestReleasedWithoutDeleteMetricRecovered(t *testing.T) { // control plane revokes the managed workspace RBAC before it waits. func TestControlPlaneHoldRevokesWorkspaceRBAC(t *testing.T) { t.Parallel() + ensureGatewaySchemeRegistered(t) // The RBAC cleanup also reads HTTPRoutes. ctx := context.Background() namespace := createTestNamespace(ctx, t, "hold-rbac") workspaceNamespace := createTestNamespace(ctx, t, "hold-rbac-ws") diff --git a/internal/controller/fakecoder_test.go b/internal/controller/fakecoder_test.go index 3c53d4441..01824e4aa 100644 --- a/internal/controller/fakecoder_test.go +++ b/internal/controller/fakecoder_test.go @@ -28,6 +28,7 @@ const fakeCoderToken = "operator-session-token" // Route names for fakeCoder.failNext and fakeCoder.requestCount. const ( routeUser = "user" + routeBuildInfo = "buildInfo" routeOrganization = "organization" routeOrgMember = "organizationMember" routeTemplateByName = "templateByName" @@ -84,15 +85,17 @@ type fakeCoder struct { mu sync.Mutex now func() time.Time operatorID uuid.UUID - users map[uuid.UUID]codersdk.User - orgs map[uuid.UUID]codersdk.Organization - orgRoles map[uuid.UUID]map[uuid.UUID][]codersdk.SlimRole // Organization -> user -> roles. - templates map[uuid.UUID]codersdk.Template - versions map[uuid.UUID]codersdk.TemplateVersion - workspaces map[uuid.UUID]*fakeWorkspace - pool fakePoolState // Workspace pool routes, see fakecoder_pool_test.go. - faults map[string][]fakeFault - requests map[string]int // Route -> requests received. + // deploymentID is the deployment ID that /api/v2/buildinfo answers. + deploymentID string + users map[uuid.UUID]codersdk.User + orgs map[uuid.UUID]codersdk.Organization + orgRoles map[uuid.UUID]map[uuid.UUID][]codersdk.SlimRole // Organization -> user -> roles. + templates map[uuid.UUID]codersdk.Template + versions map[uuid.UUID]codersdk.TemplateVersion + workspaces map[uuid.UUID]*fakeWorkspace + pool fakePoolState // Workspace pool routes, see fakecoder_pool_test.go. + faults map[string][]fakeFault + requests map[string]int // Route -> requests received. aiProviders map[uuid.UUID]*fakeAIProvider // See fakecoder_ai_test.go. aiGateway fakeAIGatewayState // AI Gateway key routes, see fakecoder_aigateway_test.go. @@ -109,8 +112,12 @@ func newFakeCoder(t *testing.T) *fakeCoder { faults: map[string][]fakeFault{}, requests: map[string]int{}, aiProviders: map[uuid.UUID]*fakeAIProvider{}, } f.operatorID = f.addUser("operator", codersdk.LoginTypePassword, codersdk.RoleOwner) + f.deploymentID = uuid.NewString() mux := http.NewServeMux() + f.route(mux, "GET /api/v2/buildinfo", routeBuildInfo, func(*http.Request) (int, any) { + return http.StatusOK, codersdk.BuildInfoResponse{Version: "v2.37.3", DeploymentID: f.deploymentID} + }) f.route(mux, "GET /api/v2/users/{user}", routeUser, func(r *http.Request) (int, any) { return found(f.userByIdent(r.PathValue("user"))) }) From 0c7697dd03dae2ef12edf3233bccd5794699fc7f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 16:09:06 +0000 Subject: [PATCH 04/12] feat: sweep the cleanup records of CoderAIProvider objects that went without a delete (#353) Change-Id: I81ad241e8a8032f3df9eac7d825c0c70fb7da105 Signed-off-by: Thomas Kosiewski --- config/rbac/role.yaml | 7 + dist/install.yaml | 7 + docs/how-to/manage-ai-providers.md | 15 +- internal/app/controllerapp/controllerapp.go | 11 + internal/controller/cleanup_sweep.go | 300 ++++++++++++++++++ .../controller/cleanup_sweep_internal_test.go | 26 ++ internal/controller/cleanup_sweep_test.go | 236 ++++++++++++++ 7 files changed, 599 insertions(+), 3 deletions(-) create mode 100644 internal/controller/cleanup_sweep.go create mode 100644 internal/controller/cleanup_sweep_internal_test.go create mode 100644 internal/controller/cleanup_sweep_test.go diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 0b240a3cb..d6db755e3 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -164,6 +164,13 @@ rules: - patch - update - watch +- apiGroups: + - events.k8s.io + resources: + - events + verbs: + - create + - patch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/dist/install.yaml b/dist/install.yaml index eb15dae61..55eb6d8d3 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6592,6 +6592,13 @@ rules: - patch - update - watch +- apiGroups: + - events.k8s.io + resources: + - events + verbs: + - create + - patch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 133479721..9aa2cc7db 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -121,17 +121,26 @@ These limits remain: Deleting the object deletes the provider in Coder. With only an unconfirmed create, the operator reads Coder first and deletes only a provider that this object created. A create request can still commit for a short time, so within 2 minutes of that request the operator reads again before it trusts that the provider is absent. - 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 the control plane is gone, the operator cannot delete the provider now. The finalizer goes at once, and the operator logs the provider ID. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the provider. - When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When 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. +Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. A sweep deletes the leftover provider only when a control plane of the same Coder deployment comes back. Coder deletes a provider even while chat models use it. Those models stop working. ### Cleanup records Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. -The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. +The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays, and the operator sweeps it: + +1. It waits until the object with that UID is gone and 5 minutes have passed since the last create, so that a create still in flight commits first. +2. It looks for a ready `CoderControlPlane`, in any namespace, whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. +3. Through that control plane it deletes the recorded provider ID. Without a recorded ID, it deletes only the provider of that name whose display name is still the record's nonce. +4. It removes the record when Coder answers the delete with success or with its own 404. Any other answer keeps the record, and the operator retries every minute. + +So after a namespace deletion with an external database, the leftover provider goes once a new control plane that uses the same database is ready. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records. + +When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the provider in the Coder dashboard (the Event names the object, and the record holds the provider ID), then delete the ConfigMap. Find the records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. ### Stuck finalizer diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index 1bbf7a88f..50c757118 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -136,6 +136,17 @@ func SetupControllers(mgr manager.Manager) error { return fmt.Errorf("unable to create AI provider controller: %w", err) } + cleanupRecordReconciler := &controller.CleanupRecordReconciler{ + Client: client, + APIReader: mgr.GetAPIReader(), + OperatorNamespace: aiProviderReconciler.OperatorNamespace, + Clock: clock.RealClock{}, + Recorder: mgr.GetEventRecorder("coder-k8s-cleanup"), + } + if err := cleanupRecordReconciler.SetupWithManager(mgr); err != nil { + return fmt.Errorf("unable to create cleanup record controller: %w", err) + } + aiGatewayKeyReconciler := &controller.CoderAIGatewayKeyReconciler{ Client: client, APIReader: mgr.GetAPIReader(), diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go new file mode 100644 index 000000000..aa0876402 --- /dev/null +++ b/internal/controller/cleanup_sweep.go @@ -0,0 +1,300 @@ +package controller + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/events" + "k8s.io/utils/clock" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "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" +) + +const ( + // cleanupSettleWindow is how long after the last create a sweep waits, + // so a create still in flight commits first (#353 invariant 6). + cleanupSettleWindow = 5 * time.Minute + // cleanupObjectRecheck is how often a record of a living object is + // checked again. The delete of the object also wakes the record. + cleanupObjectRecheck = 30 * time.Minute + // cleanupUnmatchedRecheck is how often a record that no control plane + // of its deployment reaches is checked again. + cleanupUnmatchedRecheck = 10 * time.Minute + // cleanupRetry is the retry after a failed Coder call. + cleanupRetry = time.Minute +) + +var ( + cleanupRecordsSwept = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "coder_k8s_cleanup_records_swept_total", + Help: "Cleanup records whose resources a sweep deleted in Coder, or found gone, after their object went without a delete.", + }, []string{"kind"}) + cleanupRecordsUnmatched = prometheus.NewGaugeVec(prometheus.GaugeOpts{ + Name: "coder_k8s_cleanup_records_unmatched", + Help: "Cleanup records of gone objects that no ready control plane of their Coder deployment reaches.", + }, []string{"kind"}) +) + +func init() { + metrics.Registry.MustRegister(cleanupRecordsSwept, cleanupRecordsUnmatched) +} + +// +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch + +// CleanupRecordReconciler sweeps the cleanup records of objects that went +// without a confirmed delete in Coder (#353). It acts only through a ready +// control plane whose current Coder deployment ID equals the record's, and +// it deletes only the recorded provider ID or the provider with the +// recorded nonce. +type CleanupRecordReconciler struct { + client.Client + // APIReader reads the object of a record live, so a stale cache never + // makes a living object look gone. + APIReader client.Reader + OperatorNamespace string + Clock clock.Clock + Recorder events.EventRecorder + + unmatched sync.Map // types.UID -> kind of a record that no deployment reaches. +} + +// SetupWithManager watches the records in the operator namespace. It wakes a +// record when its CoderAIProvider is deleted, and every record when a control +// plane gets operator access, since that control plane can match them. +func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { + if r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { + return errors.New("assertion failed: the cleanup record reconciler needs a client, an API reader, the operator namespace, a clock and an event recorder") + } + isRecord := predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetNamespace() == r.OperatorNamespace && obj.GetLabels()[cleanupRecordLabel] != "" + }) + return ctrl.NewControllerManagedBy(mgr). + For(&corev1.ConfigMap{}, builder.WithPredicates(isRecord)). + Watches(&coderv1alpha1.CoderAIProvider{}, handler.EnqueueRequestsFromMapFunc(func(_ context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: r.OperatorNamespace, Name: cleanupRecordObjectName(obj.GetUID())}}} + }), builder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderControlPlane{}, handler.EnqueueRequestsFromMapFunc(r.allRecords), builder.WithPredicates(operatorAccessGained)). + Named("cleanuprecord"). + Complete(r) +} + +// operatorAccessGained passes a control plane that has operator access, or +// whose access or URL changed. +var operatorAccessGained = predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + cp, ok := e.Object.(*coderv1alpha1.CoderControlPlane) + return ok && cp.Status.OperatorAccessReady + }, + UpdateFunc: func(e event.UpdateEvent) bool { + before, ok1 := e.ObjectOld.(*coderv1alpha1.CoderControlPlane) + after, ok2 := e.ObjectNew.(*coderv1alpha1.CoderControlPlane) + return ok1 && ok2 && after.Status.OperatorAccessReady && + (!before.Status.OperatorAccessReady || before.Status.URL != after.Status.URL) + }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + GenericFunc: func(event.GenericEvent) bool { return false }, +} + +// allRecords returns every record in the operator namespace. +func (r *CleanupRecordReconciler) allRecords(ctx context.Context, _ client.Object) []reconcile.Request { + records := &corev1.ConfigMapList{} + if err := r.List(ctx, records, client.InNamespace(r.OperatorNamespace), client.HasLabels{cleanupRecordLabel}); err != nil { + log.FromContext(ctx).Error(err, "Cannot list the cleanup records") + return nil + } + requests := make([]reconcile.Request, 0, len(records.Items)) + for _, record := range records.Items { + requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: record.Namespace, Name: record.Name}}) + } + return requests +} + +// Reconcile sweeps one record. +func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + if req.Namespace != r.OperatorNamespace { + return ctrl.Result{}, nil + } + configMap := &corev1.ConfigMap{} + if err := r.Get(ctx, req.NamespacedName, configMap); apierrors.IsNotFound(err) { + if uid, ok := strings.CutPrefix(req.Name, cleanupRecordPrefix); ok { + r.setUnmatched(types.UID(uid), "", false) + } + return ctrl.Result{}, nil + } else if err != nil { + return ctrl.Result{}, err + } + record, err := parseCleanupRecord(configMap) + if err != nil { + // Never act on a record that the operator did not write. + log.FromContext(ctx).Error(err, "Ignoring a ConfigMap that is not a valid cleanup record") + return ctrl.Result{}, nil + } + if record.Kind != cleanupRecordKindAIProvider { + return ctrl.Result{}, nil // Only provider records are swept so far. + } + + // Act only when the object with that UID no longer exists (invariant 6). + provider := &coderv1alpha1.CoderAIProvider{} + if err := r.APIReader.Get(ctx, types.NamespacedName{Namespace: record.Namespace, Name: record.Name}, provider); err == nil && provider.UID == record.UID { + r.setUnmatched(record.UID, record.Kind, false) + return ctrl.Result{RequeueAfter: cleanupObjectRecheck}, nil + } else if err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, err + } + settleFrom := record.CreateAttemptTime + if settleFrom.IsZero() { + settleFrom = configMap.CreationTimestamp.Time + } + if wait := settleFrom.Add(cleanupSettleWindow).Sub(r.Clock.Now()); wait > 0 { + return ctrl.Result{RequeueAfter: wait}, nil + } + + sdk, controlPlane, unknown, err := r.matchingControlPlane(ctx, record.DeploymentID) + if err != nil { + return ctrl.Result{}, err + } + if sdk == nil && unknown { + // A ready control plane did not answer its deployment ID, for + // example while its Coder pods start. It can still match. + return ctrl.Result{RequeueAfter: cleanupRetry}, nil + } + if sdk == nil { + r.setUnmatched(record.UID, record.Kind, true) + r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "NoMatchingDeployment", "Sweep", + "No ready CoderControlPlane reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", + record.DeploymentID, record.Kind, record.Namespace, record.Name) + return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil + } + r.setUnmatched(record.UID, record.Kind, false) + logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, + "controlPlane", types.NamespacedName{Namespace: controlPlane.Namespace, Name: controlPlane.Name}, "provider", record.Namespace+"/"+record.Name) + gone, why, err := sweepAIProvider(ctx, sdk, record) + if err != nil { + return ctrl.Result{}, err + } + if !gone { + logger.Info("The cleanup of a deleted provider in Coder failed, retrying", "why", why) + return ctrl.Result{RequeueAfter: cleanupRetry}, nil + } + // Coder confirmed the delete in the matching deployment (invariant 7). + if err := deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, record.UID); err != nil { + return ctrl.Result{}, err + } + cleanupRecordsSwept.WithLabelValues(record.Kind).Inc() + logger.Info("Swept the provider of a deleted CoderAIProvider in Coder", "result", why, "providerID", record.CoderID) + return ctrl.Result{}, nil +} + +// matchingControlPlane returns a Coder client of a ready control plane whose +// current deployment ID is deploymentID, or nil when there is none (#353 +// invariant 4). The ID comes from Coder now, never from the record or a URL. +// unknown reports a ready control plane whose deployment ID read failed. +func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, deploymentID string) (*codersdk.Client, *coderv1alpha1.CoderControlPlane, bool, error) { + controlPlanes := &coderv1alpha1.CoderControlPlaneList{} + if err := r.List(ctx, controlPlanes); err != nil { + return nil, nil, false, fmt.Errorf("list control planes: %w", err) + } + unknown := false + for i := range controlPlanes.Items { + cp := &controlPlanes.Items[i] + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, cp.Namespace, cp.Name) + if err != nil || wait != nil { + continue // Not ready: it cannot sweep now. + } + id, err := coderDeploymentID(ctx, sdk) + if err != nil { + log.FromContext(ctx).V(1).Info("Cannot read the deployment ID of a control plane", "controlPlane", cp.Namespace+"/"+cp.Name, "error", coderErrorSummary(err)) + unknown = true + continue + } + if id == deploymentID { + return sdk, cp, false, nil + } + } + return nil, nil, unknown, nil +} + +// sweepAIProvider deletes the provider of record in Coder. It returns gone +// when Coder confirms that it holds no provider of the record: a delete +// answered with success or Coder's own 404, a read with Coder's own 404 for +// a site owner, or a provider of that name without the record's nonce. It +// deletes only the recorded ID or the provider whose display name is the +// recorded nonce (invariant 5). +func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { + if record.Nonce != aiProviderNoncePrefix+string(record.UID) { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + } + if step := requireSiteOwner(ctx, sdk); step != nil { + // Only a site owner can trust Coder's 404 for a provider. + return false, step.message, nil + } + id := record.CoderID + if id != "" { + if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, id) + } + } else { + found, err := sdk.AIProvider(ctx, record.Name) + switch { + case isCoderResourceNotFound(err): + return true, "no provider of that name", nil + case err != nil: + return false, "get provider: " + coderErrorSummary(err), nil + } + if err := coderAnswerFor("AI provider", record.Name, found.Name); err != nil { + return false, err.Error(), nil //nolint:nilerr // A wrong answer retries the sweep. + } + if found.DisplayName != record.Nonce { + // Another provider holds the name, so no create of this object + // committed after the settle window. + return true, "the provider of that name is not this object's", nil + } + if found.ID == uuid.Nil { + return false, "", errors.New("assertion failed: Coder answered an AI provider without an ID") + } + id = found.ID.String() + } + if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderResourceNotFound(err) { + return false, "delete provider " + id + ": " + coderErrorSummary(err), nil + } else if err != nil { + return true, "provider " + id + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. + } + return true, "deleted provider " + id, nil +} + +// setUnmatched keeps the unmatched gauge equal to the records that no +// deployment reaches. +func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmatched bool) { + if unmatched { + r.unmatched.Store(uid, kind) + } else { + r.unmatched.Delete(uid) + } + counts := map[string]float64{cleanupRecordKindAIProvider: 0} + r.unmatched.Range(func(_, v any) bool { + counts[v.(string)]++ + return true + }) + for k, n := range counts { + cleanupRecordsUnmatched.WithLabelValues(k).Set(n) + } +} diff --git a/internal/controller/cleanup_sweep_internal_test.go b/internal/controller/cleanup_sweep_internal_test.go new file mode 100644 index 000000000..1c7cb4fb1 --- /dev/null +++ b/internal/controller/cleanup_sweep_internal_test.go @@ -0,0 +1,26 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/event" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// #353: a control plane wakes the cleanup records when it gets operator +// access or a new URL, since it can then match their deployment. +func TestOperatorAccessGained(t *testing.T) { + t.Parallel() + cp := func(ready bool, url string) *coderv1alpha1.CoderControlPlane { + return &coderv1alpha1.CoderControlPlane{Status: coderv1alpha1.CoderControlPlaneStatus{OperatorAccessReady: ready, URL: url}} + } + require.True(t, operatorAccessGained.Create(event.CreateEvent{Object: cp(true, "http://a")})) + require.False(t, operatorAccessGained.Create(event.CreateEvent{Object: cp(false, "http://a")})) + require.True(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(false, "http://a"), ObjectNew: cp(true, "http://a")})) + require.True(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(true, "http://b")})) + require.False(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(true, "http://a")}), "no wake on every status write") + require.False(t, operatorAccessGained.Update(event.UpdateEvent{ObjectOld: cp(true, "http://a"), ObjectNew: cp(false, "http://a")})) + require.False(t, operatorAccessGained.Delete(event.DeleteEvent{Object: cp(true, "http://a")})) +} diff --git a/internal/controller/cleanup_sweep_test.go b/internal/controller/cleanup_sweep_test.go new file mode 100644 index 000000000..e141aeb55 --- /dev/null +++ b/internal/controller/cleanup_sweep_test.go @@ -0,0 +1,236 @@ +package controller_test + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/events" + 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" + "github.com/coder/coder-k8s/internal/controller" +) + +// sweepEnv is a provider that went with its control plane, as in a +// namespace deletion: Coder keeps the provider and the record stays. +type sweepEnv struct { + aiProviderEnv + provider *coderv1alpha1.CoderAIProvider + clock *clocktesting.FakeClock + recorder *events.FakeRecorder + sweeper *controller.CleanupRecordReconciler +} + +func newSweepEnv(ctx context.Context, t *testing.T) sweepEnv { + t.Helper() + env, _, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "coder"}})) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone) + require.NotNil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) + return newSweeper(t, env, provider) +} + +// newSweeper returns a sweeper for the record of provider in env. +func newSweeper(t *testing.T, env aiProviderEnv, provider *coderv1alpha1.CoderAIProvider) sweepEnv { + t.Helper() + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + // Other tests run their own control planes: the sweeper of this test + // lists only the control planes of its namespace. + scoped := interceptor.NewClient(base, interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + return c.List(ctx, list, append(opts, client.InNamespace(env.namespace))...) + }, + }) + s := sweepEnv{aiProviderEnv: env, provider: provider, clock: clocktesting.NewFakeClock(time.Now()), recorder: events.NewFakeRecorder(10)} + s.sweeper = &controller.CleanupRecordReconciler{Client: scoped, APIReader: k8sClient, OperatorNamespace: env.namespace, Clock: s.clock, Recorder: s.recorder} + return s +} + +// providerMetric reads a controller metric with the label kind=CoderAIProvider. +func providerMetric(t *testing.T, name string) float64 { + t.Helper() + families, err := metrics.Registry.Gather() + require.NoError(t, err) + for _, f := range families { + for _, m := range f.GetMetric() { + for _, l := range m.GetLabel() { + if f.GetName() == name && l.GetName() == "kind" && l.GetValue() == "CoderAIProvider" { + if m.GetGauge() != nil { + return m.GetGauge().GetValue() + } + return m.GetCounter().GetValue() + } + } + } + } + return 0 +} + +func (s sweepEnv) sweep(ctx context.Context, t *testing.T) ctrl.Result { + t.Helper() + result, err := s.sweeper.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}}) + require.NoError(t, err) + return result +} + +// #353: after a namespace deletion, a new control plane that reaches the +// same Coder deployment deletes the leftover provider, once the settle +// window has passed. A different deployment at the same URL never does. +// Not parallel: it reads process-wide metrics. +func TestCleanupSweepAIProvider(t *testing.T) { + ctx := context.Background() + s := newSweepEnv(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "the provider is left in Coder") + swept := providerMetric(t, "coder_k8s_cleanup_records_swept_total") + + requests := s.fake.totalRequests() + result := s.sweep(ctx, t) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call inside the settle window") + require.Greater(t, result.RequeueAfter, 4*time.Minute, "the settle window has not passed") + require.LessOrEqual(t, result.RequeueAfter, 5*time.Minute, "it waits for the end of the settle window") + deletes := s.fake.requestCount(routeDeleteAIProvider) + s.clock.Step(6 * time.Minute) + result = s.sweep(ctx, t) + require.Equal(t, 10*time.Minute, result.RequeueAfter, "no control plane reaches the deployment") + require.Equal(t, deletes, s.fake.requestCount(routeDeleteAIProvider)) + + // A new control plane at the same URL, but another deployment answers: + // for example a new Coder database behind the old address. + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + recorded := s.fake.deploymentID + s.fake.mu.Lock() + s.fake.deploymentID = uuid.NewString() + s.fake.mu.Unlock() + result = s.sweep(ctx, t) + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.Equal(t, deletes, s.fake.requestCount(routeDeleteAIProvider), "a different deployment is never swept") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + require.Contains(t, <-s.recorder.Events, "NoMatchingDeployment") + require.Equal(t, 1.0, providerMetric(t, "coder_k8s_cleanup_records_unmatched")) + + // The same deployment comes back. + s.fake.mu.Lock() + s.fake.deploymentID = recorded + s.fake.mu.Unlock() + result = s.sweep(ctx, t) + require.Zero(t, result.RequeueAfter) + _, ok = s.fake.aiProvider("openai") + require.False(t, ok, "the sweep deleted the provider") + require.Equal(t, deletes+1, s.fake.requestCount(routeDeleteAIProvider)) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "Coder confirmed the delete, so the record goes") + require.Equal(t, swept+1, providerMetric(t, "coder_k8s_cleanup_records_swept_total")) + require.Zero(t, providerMetric(t, "coder_k8s_cleanup_records_unmatched")) +} + +// #353 invariants 5 and 6: a sweep never touches the provider of a living +// object, finds a provider without a recorded ID only by its nonce, and +// leaves a provider of the same name that the object did not create. +func TestCleanupSweepAIProviderCases(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("living object", func(t *testing.T) { + t.Parallel() + env, _, provider := newReadyAIProvider(ctx, t) + s := newSweeper(t, env, provider) + s.clock.Step(time.Hour) + requests := s.fake.totalRequests() + require.Equal(t, 30*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Equal(t, requests, s.fake.totalRequests(), "no Coder call for a living object") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, provider.UID)) + }) + t.Run("same name, another object", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.createProvider(ctx, t, nil) // A new object with the old name and a new UID. + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.False(t, ok, "only the UID decides that the object is gone") + }) + for name, nonce := range map[string]bool{"nonce matches": true, "nonce differs": false} { + t.Run(name, func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Data["coderID"] = "" // The create committed, but its ID was never recorded. + require.NoError(t, k8sClient.Update(ctx, configMap)) + if nonce { + s.fake.mu.Lock() + for _, p := range s.fake.aiProviders { + p.displayName = "coder-k8s:" + string(s.provider.UID) + } + s.fake.mu.Unlock() + } + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + _, ok := s.fake.aiProvider("openai") + require.Equal(t, !nonce, ok, "only the provider with the record's nonce is deleted") + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "Coder holds no provider of the record") + }) + } + t.Run("delete fails", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusBadGateway, HTML: "bad gateway"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID), "the record stays until Coder confirms the delete") + s.fake.failNext(routeDeleteAIProvider, fakeFault{Status: http.StatusNotFound, HTML: "not found"}) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "only Coder's own 404 confirms") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("deployment ID not answered yet", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusBadGateway, HTML: "starting"}) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter, "a control plane that does not answer yet can still match") + require.Empty(t, s.recorder.Events, "no NoMatchingDeployment Event for an unknown deployment") + require.Zero(t, s.sweep(ctx, t).RequeueAfter) + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("operator not a site owner", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.fake.updateUser(s.fake.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + s.clock.Step(6 * time.Minute) + require.Equal(t, time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIProvider), "only a site owner can trust Coder's 404") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("not a record", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Labels["coder.com/cleanup-record"] = "CoderAIGatewayKey" + require.NoError(t, k8sClient.Update(ctx, configMap)) + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "a ConfigMap that is not a valid record is never acted on") + }) +} From 07a012629f70d651414b9210a9b63b3fd238397b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 16:50:57 +0000 Subject: [PATCH 05/12] fix: read cleanup records live, backfill them on the disable path, delete only a valid record, and use the recorded provider ID on delete Change-Id: I43013d332be2b96542173c7871a73bdf8efe4957 Signed-off-by: Thomas Kosiewski --- .cspell.json | 1 + .github/workflows/ci.yaml | 2 +- internal/app/controllerapp/controllerapp.go | 1 + internal/controller/cleanup_record.go | 28 +++++--- .../controller/coderaiprovider_controller.go | 7 +- .../coderaiprovider_controller_test.go | 4 +- .../controller/coderaiprovider_create_test.go | 2 +- internal/controller/coderaiprovider_delete.go | 24 +++++-- .../controller/coderaiprovider_disable.go | 6 ++ internal/controller/coderaiprovider_record.go | 4 +- .../controller/coderaiprovider_record_test.go | 72 +++++++++++++++++++ 11 files changed, 130 insertions(+), 21 deletions(-) diff --git a/.cspell.json b/.cspell.json index df441da00..60da9e667 100644 --- a/.cspell.json +++ b/.cspell.json @@ -2,6 +2,7 @@ "version": "0.2", "language": "en", "words": [ + "buildinfo", "allapp", "automount", "prebuild", diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a759b8b11..626ed3807 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -94,7 +94,7 @@ jobs: # Shared Coder access that CoderTemplateTest and later pool controllers call. - 'internal/controller/controlplane_access.go' - 'internal/controller/controlplane_hold.go' - - 'internal/controller/cleanup_record*' + - 'internal/controller/cleanup_*' - '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/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index 1bbf7a88f..ef8b63f14 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -131,6 +131,7 @@ func SetupControllers(mgr manager.Manager) error { Scheme: managerScheme, // The operator's own namespace holds the cleanup records (#353). OperatorNamespace: detectLeaderElectionNamespace(), + APIReader: mgr.GetAPIReader(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create AI provider controller: %w", err) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index 44eff4a80..b98c52d73 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -13,6 +13,7 @@ import ( 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/log" ) // Cleanup records (#353 part 2, option B′). Before the operator creates @@ -60,6 +61,10 @@ type cleanupRecord struct { func cleanupRecordObjectName(uid types.UID) string { return cleanupRecordPrefix + string(uid) } +// errNotACleanupRecord marks a ConfigMap of a record's name that is not a +// valid record of that object. The operator never writes over it or deletes it. +var errNotACleanupRecord = errors.New("not a cleanup record of this object") + // coderDeploymentID reads Coder's deployment ID. Only a canonical UUID is // accepted: the ID decides which resources a sweep may delete. func coderDeploymentID(ctx context.Context, sdk *codersdk.Client) (string, error) { @@ -91,7 +96,7 @@ func readCleanupRecord(ctx context.Context, reader client.Reader, namespace stri return nil, nil, err } if record.UID != uid { - return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s", namespace, configMap.Name, uid) + return nil, nil, fmt.Errorf("ConfigMap %s/%s is not the cleanup record of object %s: %w", namespace, configMap.Name, uid, errNotACleanupRecord) } return record, configMap, nil } @@ -105,12 +110,12 @@ func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { } if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { - return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record", configMap.Namespace, configMap.Name) + return nil, fmt.Errorf("ConfigMap %s/%s is not a valid cleanup record: %w", configMap.Namespace, configMap.Name, errNotACleanupRecord) } if t := d[cleanupRecordCreateSentAt]; t != "" { parsed, err := time.Parse(time.RFC3339, t) if err != nil { - return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s: %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err) + return nil, fmt.Errorf("cleanup record %s/%s has an invalid %s (%w): %w", configMap.Namespace, configMap.Name, cleanupRecordCreateSentAt, err, errNotACleanupRecord) } record.CreateAttemptTime = parsed } @@ -152,13 +157,18 @@ func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, } // deleteCleanupRecord removes the record of uid. Call it only after Coder -// confirmed that it holds no resource of the object. -func deleteCleanupRecord(ctx context.Context, c client.Client, namespace string, uid types.UID) error { - if namespace == "" || uid == "" { - return errors.New("assertion failed: a cleanup record needs the operator namespace and an object UID") +// confirmed that it holds no resource of the object. It reads the record +// through reader (live) and deletes only that ConfigMap (UID precondition). +// A ConfigMap of that name that is not a record of uid stays. +func deleteCleanupRecord(ctx context.Context, c client.Client, reader client.Reader, namespace string, uid types.UID) error { + record, configMap, err := readCleanupRecord(ctx, reader, namespace, uid) + if errors.Is(err, errNotACleanupRecord) { + log.FromContext(ctx).Error(err, "Leaving a ConfigMap that is not the cleanup record of this object") + return nil + } else if err != nil || record == nil { + return err } - configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: cleanupRecordObjectName(uid)}} - if err := c.Delete(ctx, configMap); err != nil && !apierrors.IsNotFound(err) { + if err := c.Delete(ctx, configMap, client.Preconditions{UID: &configMap.UID}); err != nil && !apierrors.IsNotFound(err) { return fmt.Errorf("delete cleanup record %s/%s: %w", namespace, configMap.Name, err) } return nil diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index 76bd7332b..ceb41cc24 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -56,6 +56,9 @@ type CoderAIProviderReconciler struct { // OperatorNamespace holds the cleanup records (#353): the operator's own // namespace, which only the operator can write ConfigMaps in. OperatorNamespace string + // APIReader reads the cleanup records live: a record written moments ago + // can be missing from the cache. + APIReader client.Reader sentKeys sync.Map // types.UID -> SHA-256 of the key that Coder holds. Memory only. } @@ -144,8 +147,8 @@ func (r *CoderAIProviderReconciler) providersWith(ctx context.Context, namespace // Reconcile runs the checks in order and reports the first one that fails. func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" { - return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme and the operator namespace") + if r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" || r.APIReader == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: AI provider reconciler needs a client, a scheme, the operator namespace and an API reader") } provider := &coderv1alpha1.CoderAIProvider{} if err := r.Get(ctx, req.NamespacedName, provider); err != nil { diff --git a/internal/controller/coderaiprovider_controller_test.go b/internal/controller/coderaiprovider_controller_test.go index 053347393..8ef2c0286 100644 --- a/internal/controller/coderaiprovider_controller_test.go +++ b/internal/controller/coderaiprovider_controller_test.go @@ -42,7 +42,7 @@ type aiProviderEnv struct { func newAIProviderEnv(ctx context.Context, t *testing.T, withControlPlane bool) aiProviderEnv { t.Helper() env := aiProviderEnv{namespace: createTestNamespace(ctx, t, "aip"), fake: newFakeCoder(t)} - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace, APIReader: k8sClient} if withControlPlane { createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) } @@ -384,7 +384,7 @@ func TestCoderAIProviderWatches(t *testing.T) { Controller: config.Controller{SkipNameValidation: ptrTo(true)}, }) require.NoError(t, err) - require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace}).SetupWithManager(mgr)) + require.NoError(t, (&controller.CoderAIProviderReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), OperatorNamespace: env.namespace, APIReader: mgr.GetAPIReader()}).SetupWithManager(mgr)) mgrCtx, cancel := context.WithCancel(ctx) mgrDone := make(chan error, 1) go func() { mgrDone <- mgr.Start(mgrCtx) }() diff --git a/internal/controller/coderaiprovider_create_test.go b/internal/controller/coderaiprovider_create_test.go index 7b98baaea..e7053d147 100644 --- a/internal/controller/coderaiprovider_create_test.go +++ b/internal/controller/coderaiprovider_create_test.go @@ -39,7 +39,7 @@ func newAIProviderCreateEnv(ctx context.Context, t *testing.T) (aiProviderEnv, * // restart gives env a new reconciler, as a controller restart does. The // reconciler keeps no state between calls, so every step resumes from status. func (env *aiProviderEnv) restart() { - env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace} + env.r = &controller.CoderAIProviderReconciler{Client: k8sClient, Scheme: scheme, OperatorNamespace: env.namespace, APIReader: k8sClient} } func TestCoderAIProviderCreate(t *testing.T) { diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index b0866b7fd..b8361bf58 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -61,7 +61,7 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { // Nothing of this object can exist in Coder: earlier Coder answers // cleared the create marker, so the record goes too. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} @@ -113,7 +113,7 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // Coder confirmed that it holds no provider of this object. Only // then does the record go (#353 invariant 7). A failed record // delete retries, and the retry gets Coder's 404 for the provider. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) } if err != nil && deleting { // The attempt ended with an impossible answer. Log it, and still @@ -134,10 +134,26 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co // only the nonce and the type prove that the provider is this object's. func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider) (*aiProviderStep, error) { id := provider.Status.ProviderID - if id != "" { - if err := checkAIProviderID(provider); err != nil { + if id == "" { + // A failed status write after the create can leave the provider ID + // only in the record. It proves ownership after the nonce is gone. + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) + if err != nil { return nil, err } + if record != nil && record.CoderID != "" { + id = record.CoderID + if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { + return nil, fmt.Errorf("assertion failed: the cleanup record of AI provider %s/%s has coderID %q, which is not a canonical UUID", provider.Namespace, provider.Name, id) + } + } + } + if id != "" { + if provider.Status.ProviderID != "" { + if err := checkAIProviderID(provider); err != nil { + return nil, err + } + } } else { found, err := sdk.AIProvider(ctx, provider.Name) switch { diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index acc08229d..d5b3f516d 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/coder/coder/v2/codersdk" + "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/credentialsecret" @@ -62,6 +63,11 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if step := checkAIProviderAnswer(ctx, provider, found); step != nil { return step, nil } + // A provider from before cleanup records gets its record here too. A + // failed record never delays the disable: the next reconcile retries it. + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); err != nil || step != nil { + log.FromContext(ctx).Info("Cannot write the cleanup record of a provider with a failing key Secret", "error", err, "step", step) + } if !found.Enabled { return stalled("Provider %s is disabled in Coder.", provider.Name), nil } diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index fe28d0b16..81e36e662 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -21,7 +21,7 @@ const cleanupRecordKindAIProvider = "CoderAIProvider" func (r *CoderAIProviderReconciler) writeAIProviderRecord( ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider, createAttempt *time.Time, ) (*aiProviderStep, error) { - previous, existing, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + previous, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) if err != nil { return nil, err } @@ -60,7 +60,7 @@ func (r *CoderAIProviderReconciler) ensureAIProviderRecord( return nil, err } } - record, _, err := readCleanupRecord(ctx, r.Client, r.OperatorNamespace, provider.UID) + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) if err != nil { return nil, err } diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 9cef7acec..9d62cb097 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" "testing" + "time" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -152,3 +153,74 @@ func TestCoderAIProviderRecordBackfill(t *testing.T) { _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)}) require.ErrorContains(t, err, "is not a valid cleanup record") } + +// #374 review: a provider from before records gets one also while its key +// Secret fails the credential contract and the controller disables it. +func TestCoderAIProviderRecordBackfillOnDisable(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, secret, provider := newReadyAIProvider(ctx, t) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}})) + editSecret(ctx, t, secret, func(s *corev1.Secret) { delete(s.Labels, "coder.com/credential") }) + got, _ := env.reconcile(ctx, t, provider) + stored, _ := env.fake.aiProvider("openai") + require.False(t, stored.enabled, "the disable still runs") + require.Equal(t, got.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"]) +} + +// #374 review: the controller reads records live. A cache that does not +// show the record yet must not make it create the record twice. +func TestCoderAIProviderRecordReadLive(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + env.r.Client = interceptor.NewClient(base, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.ConfigMap); ok { + return apierrors.NewNotFound(corev1.Resource("configmaps"), key.Name) // A stale cache. + } + return c.Get(ctx, key, obj, opts...) + }, + }) + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderReady(t, got) + require.Equal(t, got.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"]) +} + +// #374 review: a ConfigMap of the record's name that is not this object's +// record is never deleted. +func TestCoderAIProviderDeleteKeepsForeignConfigMap(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newAIProviderCreateEnv(ctx, t) + provider.Finalizers = []string{coderv1alpha1.CoderAIProviderCleanupFinalizer} + require.NoError(t, k8sClient.Update(ctx, provider)) + foreign := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(provider.UID)}, Data: map[string]string{"owner": "someone else"}} + require.NoError(t, k8sClient.Create(ctx, foreign)) + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone, "nothing of the object exists in Coder, so the finalizer goes") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(foreign), foreign), "the foreign ConfigMap stays") +} + +// #374 review: when the status write after a create failed, the record holds +// the only provider ID. A delete uses it even after the nonce is gone. +func TestCoderAIProviderDeleteUsesRecordedID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + provider.Status.ProviderID = "" + provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-time.Hour)} + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + env.fake.mu.Lock() + for _, p := range env.fake.aiProviders { + p.displayName = "Renamed in the dashboard" + } + env.fake.mu.Unlock() + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone) + _, ok := env.fake.aiProvider("openai") + require.False(t, ok, "the recorded ID proves ownership") + require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) +} From 7337267f0b0ead1dd6346459df50332349b3690b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 16:51:34 +0000 Subject: [PATCH 06/12] fix: pass the live reader to the record delete of the sweep Change-Id: Id47fff062cc502fe08acd8a4b7ae52512512480d Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_sweep.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index aa0876402..da616c4ce 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -196,7 +196,7 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{RequeueAfter: cleanupRetry}, nil } // Coder confirmed the delete in the matching deployment (invariant 7). - if err := deleteCleanupRecord(ctx, r.Client, r.OperatorNamespace, record.UID); err != nil { + if err := deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, record.UID); err != nil { return ctrl.Result{}, err } cleanupRecordsSwept.WithLabelValues(record.Kind).Inc() From 6e1d1bf90ff860c1c7825bbbfbbf36f875eddc2b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:13:28 +0000 Subject: [PATCH 07/12] fix: backfill records before every check, keep the record when the control plane reaches another deployment, and guard the setup Change-Id: Iee4add631b870c6070066c275f89f84ed86762fa Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_controller.go | 8 +++-- internal/controller/coderaiprovider_delete.go | 17 ++++++++++ .../controller/coderaiprovider_disable.go | 6 ---- internal/controller/coderaiprovider_record.go | 21 ++++++++++++ .../controller/coderaiprovider_record_test.go | 32 +++++++++++++++++++ 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/internal/controller/coderaiprovider_controller.go b/internal/controller/coderaiprovider_controller.go index ceb41cc24..c8f96fa1c 100644 --- a/internal/controller/coderaiprovider_controller.go +++ b/internal/controller/coderaiprovider_controller.go @@ -85,8 +85,8 @@ func aiProviderStalled(requeue time.Duration, reason, format string, args ...any // SetupWithManager wires the reconciler into controller-runtime. A change of // the referenced control plane or of the key Secret requeues the provider. func (r *CoderAIProviderReconciler) SetupWithManager(mgr ctrl.Manager) error { - if mgr == nil || r.Client == nil || r.Scheme == nil { - return fmt.Errorf("assertion failed: AI provider reconciler needs a manager, a client, and a scheme") + if mgr == nil || r.Client == nil || r.Scheme == nil || r.OperatorNamespace == "" || r.APIReader == nil { + return fmt.Errorf("assertion failed: AI provider reconciler needs a manager, a client, a scheme, the operator namespace and an API reader") } indexes := map[string]func(*coderv1alpha1.CoderAIProvider) string{ aiProviderControlPlaneRefIndex: func(p *coderv1alpha1.CoderAIProvider) string { return p.Spec.ControlPlaneRef.Name }, @@ -202,10 +202,12 @@ func (r *CoderAIProviderReconciler) Reconcile(ctx context.Context, req ctrl.Requ // check validates the base URL, reads the key through the credential Secret // contract, gets Coder access, and then creates or applies the provider. The // checks before Coder access send nothing to Coder, except the disable of a -// provider whose key Secret fails the credential contract (#267, #290). +// provider whose key Secret fails the credential contract (#267, #290), and +// the deployment ID read of a record backfill. func (r *CoderAIProviderReconciler) check( ctx context.Context, provider *coderv1alpha1.CoderAIProvider, before *coderv1alpha1.CoderAIProviderStatus, ) (*aiProviderStep, error) { + r.backfillAIProviderRecord(ctx, provider) baseURL, err := aiProviderBaseURL(provider.Spec.BaseURL) if err != nil { // A fixed message: the parse error repeats the URL, which can hold diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index b8361bf58..353f6b0a2 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -108,6 +108,23 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co ctx, cancel = context.WithTimeout(ctx, aiProviderBoundedDeleteTimeout) defer cancel() } + // The record names the deployment that holds the provider. When the + // control plane now reaches another deployment, a delete there proves + // nothing, so the record stays for a sweep (#353 invariant 7). + if record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID); err != nil && !errors.Is(err, errNotACleanupRecord) { + return false, nil, err + } else if record != nil && (provider.Status.ProviderID == "" || checkAIProviderID(provider) == nil) { + // A forged provider ID fails its assertion below, before any request. + id, err := coderDeploymentID(ctx, sdk) + switch { + case err != nil && deleting: + return r.releaseWithoutDelete(ctx, provider, "the deployment ID read while CoderControlPlane is being deleted failed: "+coderErrorSummary(err)), nil, nil + case err != nil: + return false, aiProviderWait(aiProviderDeletePoll, "CoderUnavailable", "read the Coder deployment ID: %s", coderErrorSummary(err)), nil + case id != record.DeploymentID: + return r.releaseWithoutDelete(ctx, provider, fmt.Sprintf("CoderControlPlane now reaches Coder deployment %s, but the provider is in deployment %s", id, record.DeploymentID)), nil, nil + } + } step, err = r.deleteInCoder(ctx, sdk, provider) if err == nil && step == nil { // Coder confirmed that it holds no provider of this object. Only diff --git a/internal/controller/coderaiprovider_disable.go b/internal/controller/coderaiprovider_disable.go index d5b3f516d..acc08229d 100644 --- a/internal/controller/coderaiprovider_disable.go +++ b/internal/controller/coderaiprovider_disable.go @@ -6,7 +6,6 @@ import ( "fmt" "github.com/coder/coder/v2/codersdk" - "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/credentialsecret" @@ -63,11 +62,6 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent( if step := checkAIProviderAnswer(ctx, provider, found); step != nil { return step, nil } - // A provider from before cleanup records gets its record here too. A - // failed record never delays the disable: the next reconcile retries it. - if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); err != nil || step != nil { - log.FromContext(ctx).Info("Cannot write the cleanup record of a provider with a failing key Secret", "error", err, "step", step) - } if !found.Enabled { return stalled("Provider %s is disabled in Coder.", provider.Name), nil } diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index 81e36e662..24c9f40a2 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -5,6 +5,7 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" ) @@ -45,6 +46,26 @@ func (r *CoderAIProviderReconciler) writeAIProviderRecord( return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) } +// backfillAIProviderRecord writes the missing record of a provider that +// exists in Coder from before records existed. It runs before every other +// check, so a stalled object (a bad base URL or key Secret) gets its record +// too. It never stops the reconcile: a failure is logged and retried. +func (r *CoderAIProviderReconciler) backfillAIProviderRecord(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) { + if provider.Status.ProviderID == "" || checkAIProviderID(provider) != nil { + return // The create path writes the record. A forged ID fails later checks. + } + if record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID); err == nil && record != nil && record.CoderID == provider.Status.ProviderID { + return + } + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, provider.Namespace, provider.Spec.ControlPlaneRef.Name) + if err != nil || wait != nil { + return // The control plane cannot be reached now. A later reconcile retries. + } + if step, err := r.ensureAIProviderRecord(ctx, sdk, provider); err != nil || step != nil { + log.FromContext(ctx).Info("Cannot write the cleanup record of a provider that exists in Coder", "error", err, "step", step) + } +} + // ensureAIProviderRecord makes sure that a provider which can exist in Coder // has a record with its current provider ID. Objects from before records // existed get one here too. diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 9d62cb097..50d8263ab 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -16,8 +17,10 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" ) // cleanupRecord returns the data of the cleanup record of uid in namespace, @@ -224,3 +227,32 @@ func TestCoderAIProviderDeleteUsesRecordedID(t *testing.T) { require.False(t, ok, "the recorded ID proves ownership") require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) } + +// #374 review round 2: when the control plane now reaches another Coder +// deployment (a new database), a delete there proves nothing. The finalizer +// goes without a delete, and the record stays for a sweep. +func TestCoderAIProviderDeleteAfterDeploymentSwitch(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() + env.fake.mu.Unlock() + gone, _ := env.deleteAIProvider(ctx, t, provider) + require.Nil(t, gone) + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in another deployment") + require.Equal(t, provider.Status.ProviderID, cleanupRecord(ctx, t, env.namespace, provider.UID)["coderID"], "the record stays") +} + +// The reconciler refuses to start without its cleanup record dependencies. +func TestCoderAIProviderSetupNeedsRecordDependencies(t *testing.T) { + t.Parallel() + for name, r := range map[string]*controller.CoderAIProviderReconciler{ + "no operator namespace": {Client: k8sClient, Scheme: scheme, APIReader: k8sClient}, + "no API reader": {Client: k8sClient, Scheme: scheme, OperatorNamespace: "coder-system"}, + } { + mgr, err := ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme, Metrics: metricsserver.Options{BindAddress: "0"}, HealthProbeBindAddress: "0"}) + require.NoError(t, err) + require.ErrorContains(t, r.SetupWithManager(mgr), "assertion failed: AI provider reconciler needs", name) + } +} From acf83855109b459b4108f3cca0a636aae02b1d05 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:14:26 +0000 Subject: [PATCH 08/12] fix: read the operator access live in the sweep, and treat a ready control plane without working access as unknown Change-Id: I14ec8db9add5723cde4a6f4f1b3308a1afb1f82c Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_sweep.go | 13 +++++++++---- internal/controller/cleanup_sweep_test.go | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index da616c4ce..d291bfce7 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -81,8 +81,8 @@ type CleanupRecordReconciler struct { // record when its CoderAIProvider is deleted, and every record when a control // plane gets operator access, since that control plane can match them. func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { - if r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { - return errors.New("assertion failed: the cleanup record reconciler needs a client, an API reader, the operator namespace, a clock and an event recorder") + if mgr == nil || r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { + return errors.New("assertion failed: the cleanup record reconciler needs a manager, a client, an API reader, the operator namespace, a clock and an event recorder") } isRecord := predicate.NewPredicateFuncs(func(obj client.Object) bool { return obj.GetNamespace() == r.OperatorNamespace && obj.GetLabels()[cleanupRecordLabel] != "" @@ -216,9 +216,14 @@ func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, depl unknown := false for i := range controlPlanes.Items { cp := &controlPlanes.Items[i] - sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, cp.Namespace, cp.Name) + // A live read: the cache can show the control plane ready before + // it shows its new operator token Secret. + sdk, _, wait, err := controlPlaneCoderClient(ctx, r.APIReader, cp.Namespace, cp.Name) if err != nil || wait != nil { - continue // Not ready: it cannot sweep now. + // A control plane that reports operator access can still match + // once its access works. + unknown = unknown || cp.Status.OperatorAccessReady + continue } id, err := coderDeploymentID(ctx, sdk) if err != nil { diff --git a/internal/controller/cleanup_sweep_test.go b/internal/controller/cleanup_sweep_test.go index e141aeb55..6b4366e07 100644 --- a/internal/controller/cleanup_sweep_test.go +++ b/internal/controller/cleanup_sweep_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/uuid" "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/client-go/tools/events" clocktesting "k8s.io/utils/clock/testing" @@ -210,6 +211,28 @@ func TestCleanupSweepAIProviderCases(t *testing.T) { require.Zero(t, s.sweep(ctx, t).RequeueAfter) require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) }) + t.Run("stale cache of the token Secret", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + cached := s.sweeper.Client + s.sweeper.Client = interceptor.NewClient(cached.(client.WithWatch), interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return apierrors.NewNotFound(corev1.Resource("secrets"), key.Name) // Not in the cache yet. + } + return c.Get(ctx, key, obj, opts...) + }, + }) + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter, "the sweep reads the operator access live") + require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) + t.Run("setup needs a manager", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + require.ErrorContains(t, s.sweeper.SetupWithManager(nil), "assertion failed: the cleanup record reconciler needs a manager") + }) t.Run("operator not a site owner", func(t *testing.T) { t.Parallel() s := newSweepEnv(ctx, t) From 0884db303783757ae8739bfe523e8e387bb2d2c8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:17:01 +0000 Subject: [PATCH 09/12] fix: sweep a record only through a control plane in the object's namespace Change-Id: I0ce5b4e4ac65226e910f22ff8055a9100069f69e Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-providers.md | 6 +++--- internal/controller/cleanup_sweep.go | 26 +++++++++++++++-------- internal/controller/cleanup_sweep_test.go | 20 ++++++++++------- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 9aa2cc7db..3c4495fb1 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -125,7 +125,7 @@ Deleting the object deletes the provider in Coder. With only an unconfirmed crea - When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. - When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator sends one delete with a 10-second limit, then removes the finalizer. Without working access, the finalizer goes without a delete. Either way a namespace deletion finishes. When the delete did not succeed, the operator logs the provider ID: delete the provider in the Coder dashboard. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts these releases. -Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. A sweep deletes the leftover provider only when a control plane of the same Coder deployment comes back. 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. A sweep deletes the leftover provider only when the namespace comes back with a control plane of the same Coder deployment. Coder deletes a provider even while chat models use it. Those models stop working. ### Cleanup records @@ -134,11 +134,11 @@ Before the operator creates a provider in Coder, it writes a cleanup record: a C The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays, and the operator sweeps it: 1. It waits until the object with that UID is gone and 5 minutes have passed since the last create, so that a create still in flight commits first. -2. It looks for a ready `CoderControlPlane`, in any namespace, whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. +2. It looks for a ready `CoderControlPlane` in the object's namespace whose Coder answers the record's deployment ID now. It never matches by URL or name: a new Coder database behind the same URL has another deployment ID and is never swept. A control plane in another namespace never sweeps the record, because Coder reports its deployment ID itself and a control plane that someone else runs could claim any ID. 3. Through that control plane it deletes the recorded provider ID. Without a recorded ID, it deletes only the provider of that name whose display name is still the record's nonce. 4. It removes the record when Coder answers the delete with success or with its own 404. Any other answer keeps the record, and the operator retries every minute. -So after a namespace deletion with an external database, the leftover provider goes once a new control plane that uses the same database is ready. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records. +So after a namespace deletion with an external database, the leftover provider goes once you create the namespace again with a control plane that uses the same database. The metric `coder_k8s_cleanup_records_swept_total` (label `kind`) counts the swept records. When no ready control plane reaches the record's deployment, the record stays. The gauge `coder_k8s_cleanup_records_unmatched` counts these records, and the operator writes a `NoMatchingDeployment` Warning Event on the ConfigMap every 10 minutes. When that Coder deployment never comes back, delete the provider in the Coder dashboard (the Event names the object, and the record holds the provider ID), then delete the ConfigMap. Find the records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index d291bfce7..6a84320f8 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -168,7 +168,7 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{RequeueAfter: wait}, nil } - sdk, controlPlane, unknown, err := r.matchingControlPlane(ctx, record.DeploymentID) + sdk, controlPlane, unknown, err := r.matchingControlPlane(ctx, record) if err != nil { return ctrl.Result{}, err } @@ -180,8 +180,8 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques if sdk == nil { r.setUnmatched(record.UID, record.Kind, true) r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "NoMatchingDeployment", "Sweep", - "No ready CoderControlPlane reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", - record.DeploymentID, record.Kind, record.Namespace, record.Name) + "No ready CoderControlPlane in namespace %s reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", + record.Namespace, record.DeploymentID, record.Kind, record.Namespace, record.Name) return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil } r.setUnmatched(record.UID, record.Kind, false) @@ -204,13 +204,21 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } -// matchingControlPlane returns a Coder client of a ready control plane whose -// current deployment ID is deploymentID, or nil when there is none (#353 -// invariant 4). The ID comes from Coder now, never from the record or a URL. -// unknown reports a ready control plane whose deployment ID read failed. -func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, deploymentID string) (*codersdk.Client, *coderv1alpha1.CoderControlPlane, bool, error) { +// matchingControlPlane returns a Coder client of a ready control plane in +// the record's namespace whose current deployment ID is the record's, or nil +// when there is none (#353 invariant 4). The ID comes from Coder now, never +// from the record or a URL. unknown reports a ready control plane whose +// deployment ID read failed. +// +// Only the record's own namespace counts: Coder reports its deployment ID +// itself, so a control plane that someone else runs could claim any ID. The +// records of a namespace, and the control planes that sweep them, are then +// under the control of the same people, and a sweep can only delete what +// their own operator access could delete. +func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, record *cleanupRecord) (*codersdk.Client, *coderv1alpha1.CoderControlPlane, bool, error) { + deploymentID := record.DeploymentID controlPlanes := &coderv1alpha1.CoderControlPlaneList{} - if err := r.List(ctx, controlPlanes); err != nil { + if err := r.List(ctx, controlPlanes, client.InNamespace(record.Namespace)); err != nil { return nil, nil, false, fmt.Errorf("list control planes: %w", err) } unknown := false diff --git a/internal/controller/cleanup_sweep_test.go b/internal/controller/cleanup_sweep_test.go index 6b4366e07..011b7102e 100644 --- a/internal/controller/cleanup_sweep_test.go +++ b/internal/controller/cleanup_sweep_test.go @@ -48,15 +48,8 @@ func newSweeper(t *testing.T, env aiProviderEnv, provider *coderv1alpha1.CoderAI t.Helper() base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) require.NoError(t, err) - // Other tests run their own control planes: the sweeper of this test - // lists only the control planes of its namespace. - scoped := interceptor.NewClient(base, interceptor.Funcs{ - List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { - return c.List(ctx, list, append(opts, client.InNamespace(env.namespace))...) - }, - }) s := sweepEnv{aiProviderEnv: env, provider: provider, clock: clocktesting.NewFakeClock(time.Now()), recorder: events.NewFakeRecorder(10)} - s.sweeper = &controller.CleanupRecordReconciler{Client: scoped, APIReader: k8sClient, OperatorNamespace: env.namespace, Clock: s.clock, Recorder: s.recorder} + s.sweeper = &controller.CleanupRecordReconciler{Client: base, APIReader: k8sClient, OperatorNamespace: env.namespace, Clock: s.clock, Recorder: s.recorder} return s } @@ -211,6 +204,17 @@ func TestCleanupSweepAIProviderCases(t *testing.T) { require.Zero(t, s.sweep(ctx, t).RequeueAfter) require.Nil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) }) + t.Run("control plane in another namespace", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + // Another namespace runs a control plane that answers the record's + // deployment ID, as a spoofing Coder could. + createTestControlPlane(ctx, t, createTestNamespace(ctx, t, "aip-other"), "coder", s.fake.server.URL) + s.clock.Step(6 * time.Minute) + require.Equal(t, 10*time.Minute, s.sweep(ctx, t).RequeueAfter) + require.Zero(t, s.fake.requestCount(routeDeleteAIProvider), "only a control plane in the object's namespace sweeps") + require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) + }) t.Run("stale cache of the token Secret", func(t *testing.T) { t.Parallel() s := newSweepEnv(ctx, t) From 887b0f10dbe0f62ef7dc712ebc669fb210dafa64 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:53:17 +0000 Subject: [PATCH 10/12] fix: keep the record when the deployment changes under a live object, record the provider type, and default the record namespace to coder-system Change-Id: I7b699d65437effb2e94cefb175ee758d163b2c83 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-ai-providers.md | 2 ++ internal/app/controllerapp/controllerapp.go | 22 +++++++++++++++- .../controllerapp_internal_test.go | 22 ++++++++++++++++ internal/controller/cleanup_record.go | 8 ++++-- internal/controller/coderaiprovider_record.go | 10 ++++++- .../controller/coderaiprovider_record_test.go | 26 ++++++++++++++++++- 6 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 internal/app/controllerapp/controllerapp_internal_test.go diff --git a/docs/how-to/manage-ai-providers.md b/docs/how-to/manage-ai-providers.md index 133479721..129ed21c0 100644 --- a/docs/how-to/manage-ai-providers.md +++ b/docs/how-to/manage-ai-providers.md @@ -131,6 +131,8 @@ Delete `CoderAIProvider` objects before their namespace: a namespace deletion re Before the operator creates a provider in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderAIProvider` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the provider. The record holds no key: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the provider's display-name nonce, and the provider ID once the operator knows it. The deployment ID lives in the Coder database, so it names the database that holds the provider. +When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the provider in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the provider in the old deployment, then delete the ConfigMap. + The operator removes the record when Coder confirms that the provider is deleted. When the finalizer goes without a delete, for example in a namespace deletion, the record stays. Find these records with `kubectl -n coder-system get configmaps -l coder.com/cleanup-record`. The operator gets write access to ConfigMaps only in that namespace (the Role `manager-role` and the RoleBinding `coder-k8s-cleanup-records`). Do not give other users write access to ConfigMaps there: a record names a Coder provider ID that a cleanup deletes. If you install the operator in another namespace, change the namespace of both. ### Stuck finalizer diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index ef8b63f14..7f5030f4e 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -130,7 +130,7 @@ func SetupControllers(mgr manager.Manager) error { Client: client, Scheme: managerScheme, // The operator's own namespace holds the cleanup records (#353). - OperatorNamespace: detectLeaderElectionNamespace(), + OperatorNamespace: detectOperatorNamespace(), APIReader: mgr.GetAPIReader(), } if err := aiProviderReconciler.SetupWithManager(mgr); err != nil { @@ -206,6 +206,26 @@ func Run(ctx context.Context) error { return nil } +// defaultOperatorNamespace is the namespace of the installed operator, where +// the Role for the cleanup records lives (config/rbac). +const defaultOperatorNamespace = "coder-system" + +// detectOperatorNamespace returns the namespace that holds the cleanup +// records: POD_NAMESPACE, else the in-cluster namespace file, else +// coder-system, which the shipped Role names. Only that fallback differs +// from detectLeaderElectionNamespace. +func detectOperatorNamespace() string { + if ns := strings.TrimSpace(os.Getenv("POD_NAMESPACE")); ns != "" { + return ns + } + if data, err := os.ReadFile(inClusterNamespacePath); err == nil { + if ns := strings.TrimSpace(string(data)); ns != "" { + return ns + } + } + return defaultOperatorNamespace +} + // detectLeaderElectionNamespace returns the namespace to use for leader-election // lease objects. Resolution order: // 1. POD_NAMESPACE env var (allows explicit override for any environment). diff --git a/internal/app/controllerapp/controllerapp_internal_test.go b/internal/app/controllerapp/controllerapp_internal_test.go new file mode 100644 index 000000000..9f1b1377e --- /dev/null +++ b/internal/app/controllerapp/controllerapp_internal_test.go @@ -0,0 +1,22 @@ +package controllerapp + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +// #353: the cleanup records live in the namespace that the shipped Role +// names when neither POD_NAMESPACE nor the in-cluster namespace file says +// otherwise, also for a controller that runs outside the cluster. +func TestDetectOperatorNamespace(t *testing.T) { + if _, err := os.Stat(inClusterNamespacePath); err == nil { + t.Skip("this host has an in-cluster namespace file") + } + t.Setenv("POD_NAMESPACE", "") + require.Equal(t, "coder-system", detectOperatorNamespace()) + require.Equal(t, "kube-system", detectLeaderElectionNamespace(), "the leader-election fallback stays") + t.Setenv("POD_NAMESPACE", "ops") + require.Equal(t, "ops", detectOperatorNamespace()) +} diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index b98c52d73..7465269a6 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -36,6 +36,7 @@ const ( cleanupRecordDeploymentID = "deploymentID" cleanupRecordNonce = "nonce" cleanupRecordCoderID = "coderID" + cleanupRecordType = "type" cleanupRecordCreateSentAt = "createAttemptTime" ) @@ -54,6 +55,9 @@ type cleanupRecord struct { Nonce string // CoderID is the Coder ID of the resource once the operator knows it. CoderID string + // Type is the provider type that the create sent. With the nonce it + // proves that a provider found by name is the object's. + Type string // CreateAttemptTime is when the operator last sent a create. A sweep // waits for the settle window after it. CreateAttemptTime time.Time @@ -106,7 +110,7 @@ func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { d := configMap.Data record := &cleanupRecord{ Kind: d[cleanupRecordKind], UID: types.UID(d[cleanupRecordUID]), Namespace: d[cleanupRecordNamespace], Name: d[cleanupRecordName], - DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], + DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], Type: d[cleanupRecordType], } if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { @@ -130,7 +134,7 @@ func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, } data := map[string]string{ cleanupRecordKind: record.Kind, cleanupRecordUID: string(record.UID), cleanupRecordNamespace: record.Namespace, cleanupRecordName: record.Name, - cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, + cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, cleanupRecordType: record.Type, } if !record.CreateAttemptTime.IsZero() { data[cleanupRecordCreateSentAt] = record.CreateAttemptTime.UTC().Format(time.RFC3339) diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index 24c9f40a2..cf4a96038 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -30,9 +30,17 @@ func (r *CoderAIProviderReconciler) writeAIProviderRecord( if err != nil { return aiProviderWait(aiProviderRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil } + if previous != nil && previous.DeploymentID != deploymentID { + // The control plane now reaches another Coder database. The record + // is the only locator of what this object made in the old one, so + // the controller neither overwrites it nor creates in the new one. + return aiProviderStalled(aiProviderResync, "CoderDeploymentChanged", + "Provider %s (ID %q) is in Coder deployment %s, but CoderControlPlane %s now reaches deployment %s. Delete the provider there, then delete ConfigMap %s/%s. Until then the controller creates nothing.", + provider.Name, previous.CoderID, previous.DeploymentID, provider.Spec.ControlPlaneRef.Name, deploymentID, r.OperatorNamespace, cleanupRecordObjectName(provider.UID)), nil + } record := cleanupRecord{ Kind: cleanupRecordKindAIProvider, UID: provider.UID, Namespace: provider.Namespace, Name: provider.Name, - DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, + DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, Type: provider.Spec.Type, } if createAttempt != nil { record.CreateAttemptTime = *createAttempt diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 50d8263ab..e40671e3b 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -56,7 +56,7 @@ func TestCoderAIProviderCleanupRecord(t *testing.T) { delete(record, "createAttemptTime") require.Equal(t, map[string]string{ "kind": "CoderAIProvider", "uid": string(got.UID), "namespace": env.namespace, "name": "openai", - "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), + "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": stored.id.String(), "type": "openai", }, record, "the provider ID is recorded before the sync replaces the nonce") got, _ = env.reconcile(ctx, t, got) @@ -256,3 +256,27 @@ func TestCoderAIProviderSetupNeedsRecordDependencies(t *testing.T) { require.ErrorContains(t, r.SetupWithManager(mgr), "assertion failed: AI provider reconciler needs", name) } } + +// #374 review round 3: when the control plane reaches another Coder +// deployment while the object lives, the provider looks gone, but the +// record still locates it in the old deployment. The controller keeps the +// record and creates nothing in the new deployment. +func TestCoderAIProviderDeploymentSwitchKeepsRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + before := cleanupRecord(ctx, t, env.namespace, provider.UID) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + for id := range env.fake.aiProviders { + delete(env.fake.aiProviders, id) + } + env.fake.mu.Unlock() + provider.Spec.DisplayName = "OpenAI (edited)" + require.NoError(t, k8sClient.Update(ctx, provider)) + got, _ := env.reconcile(ctx, t, provider) + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireAIProviderStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) + require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no create in the new deployment") + require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record keeps the old locator") +} From 5e9e8494e88203862072d8a7ec357dbe87e5246c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 17:55:56 +0000 Subject: [PATCH 11/12] fix: read the swept record live, try every matching control plane, and check the provider type on the nonce path Change-Id: I515ed4a2e1d079f0b6553532947672482b786ffe Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_sweep.go | 55 +++++++++++++-------- internal/controller/cleanup_sweep_test.go | 59 +++++++++++++++++++++++ 2 files changed, 94 insertions(+), 20 deletions(-) diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index 6a84320f8..aebeec4ed 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -133,8 +133,9 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques if req.Namespace != r.OperatorNamespace { return ctrl.Result{}, nil } + // A live read: the cache can still hold an older create time. configMap := &corev1.ConfigMap{} - if err := r.Get(ctx, req.NamespacedName, configMap); apierrors.IsNotFound(err) { + if err := r.APIReader.Get(ctx, req.NamespacedName, configMap); apierrors.IsNotFound(err) { if uid, ok := strings.CutPrefix(req.Name, cleanupRecordPrefix); ok { r.setUnmatched(types.UID(uid), "", false) } @@ -168,16 +169,16 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{RequeueAfter: wait}, nil } - sdk, controlPlane, unknown, err := r.matchingControlPlane(ctx, record) + matches, unknown, err := r.matchingControlPlanes(ctx, record) if err != nil { return ctrl.Result{}, err } - if sdk == nil && unknown { + if len(matches) == 0 && unknown { // A ready control plane did not answer its deployment ID, for // example while its Coder pods start. It can still match. return ctrl.Result{RequeueAfter: cleanupRetry}, nil } - if sdk == nil { + if len(matches) == 0 { r.setUnmatched(record.UID, record.Kind, true) r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "NoMatchingDeployment", "Sweep", "No ready CoderControlPlane in namespace %s reaches Coder deployment %s, which holds the provider of deleted %s %s/%s. Delete the provider in Coder by hand, then delete this ConfigMap.", @@ -185,14 +186,20 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil } r.setUnmatched(record.UID, record.Kind, false) - logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, - "controlPlane", types.NamespacedName{Namespace: controlPlane.Namespace, Name: controlPlane.Name}, "provider", record.Namespace+"/"+record.Name) - gone, why, err := sweepAIProvider(ctx, sdk, record) - if err != nil { - return ctrl.Result{}, err + logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, "provider", record.Namespace+"/"+record.Name) + // Each matching control plane has its own operator token, so a failure + // through one tries the next. + gone, why, through := false, "", logger + for _, m := range matches { + through = logger.WithValues("controlPlane", types.NamespacedName{Namespace: m.controlPlane.Namespace, Name: m.controlPlane.Name}) + if gone, why, err = sweepAIProvider(ctx, m.sdk, record); err != nil { + return ctrl.Result{}, err + } else if gone { + break + } + through.Info("The cleanup of a deleted provider in Coder failed", "why", why) } if !gone { - logger.Info("The cleanup of a deleted provider in Coder failed, retrying", "why", why) return ctrl.Result{RequeueAfter: cleanupRetry}, nil } // Coder confirmed the delete in the matching deployment (invariant 7). @@ -200,13 +207,19 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } cleanupRecordsSwept.WithLabelValues(record.Kind).Inc() - logger.Info("Swept the provider of a deleted CoderAIProvider in Coder", "result", why, "providerID", record.CoderID) + through.Info("Swept the provider of a deleted CoderAIProvider in Coder", "result", why, "providerID", record.CoderID) return ctrl.Result{}, nil } -// matchingControlPlane returns a Coder client of a ready control plane in -// the record's namespace whose current deployment ID is the record's, or nil -// when there is none (#353 invariant 4). The ID comes from Coder now, never +// controlPlaneMatch is a control plane whose Coder deployment is a record's. +type controlPlaneMatch struct { + sdk *codersdk.Client + controlPlane *coderv1alpha1.CoderControlPlane +} + +// matchingControlPlanes returns the Coder clients of the ready control planes +// in the record's namespace whose current deployment ID is the record's +// (#353 invariant 4). The ID comes from Coder now, never // from the record or a URL. unknown reports a ready control plane whose // deployment ID read failed. // @@ -215,13 +228,14 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques // records of a namespace, and the control planes that sweep them, are then // under the control of the same people, and a sweep can only delete what // their own operator access could delete. -func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, record *cleanupRecord) (*codersdk.Client, *coderv1alpha1.CoderControlPlane, bool, error) { +func (r *CleanupRecordReconciler) matchingControlPlanes(ctx context.Context, record *cleanupRecord) ([]controlPlaneMatch, bool, error) { deploymentID := record.DeploymentID controlPlanes := &coderv1alpha1.CoderControlPlaneList{} if err := r.List(ctx, controlPlanes, client.InNamespace(record.Namespace)); err != nil { - return nil, nil, false, fmt.Errorf("list control planes: %w", err) + return nil, false, fmt.Errorf("list control planes: %w", err) } unknown := false + var matches []controlPlaneMatch for i := range controlPlanes.Items { cp := &controlPlanes.Items[i] // A live read: the cache can show the control plane ready before @@ -240,10 +254,10 @@ func (r *CleanupRecordReconciler) matchingControlPlane(ctx context.Context, reco continue } if id == deploymentID { - return sdk, cp, false, nil + matches = append(matches, controlPlaneMatch{sdk: sdk, controlPlane: cp}) } } - return nil, nil, unknown, nil + return matches, unknown, nil } // sweepAIProvider deletes the provider of record in Coder. It returns gone @@ -276,9 +290,10 @@ func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupR if err := coderAnswerFor("AI provider", record.Name, found.Name); err != nil { return false, err.Error(), nil //nolint:nilerr // A wrong answer retries the sweep. } - if found.DisplayName != record.Nonce { + if found.DisplayName != record.Nonce || string(found.Type) != record.Type { // Another provider holds the name, so no create of this object - // committed after the settle window. + // committed after the settle window. The nonce and the type + // prove ownership, as on the controller's own paths. return true, "the provider of that name is not this object's", nil } if found.ID == uuid.Nil { diff --git a/internal/controller/cleanup_sweep_test.go b/internal/controller/cleanup_sweep_test.go index 011b7102e..9b5a8b8a5 100644 --- a/internal/controller/cleanup_sweep_test.go +++ b/internal/controller/cleanup_sweep_test.go @@ -215,6 +215,65 @@ func TestCleanupSweepAIProviderCases(t *testing.T) { require.Zero(t, s.fake.requestCount(routeDeleteAIProvider), "only a control plane in the object's namespace sweeps") require.NotNil(t, cleanupRecord(ctx, t, s.namespace, s.provider.UID)) }) + t.Run("first matching control plane has no site owner", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + other := newFakeCoder(t) // A second Coder on the same database: same deployment, its own users. + other.mu.Lock() + other.deploymentID = s.fake.deploymentID + other.mu.Unlock() + other.updateUser(other.operatorID, func(u *codersdk.User) { u.Roles = []codersdk.SlimRole{} }) + createTestControlPlane(ctx, t, s.namespace, "coder-a", other.server.URL) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t).RequeueAfter, "the next matching control plane sweeps") + _, ok := s.fake.aiProvider("openai") + require.False(t, ok) + }) + t.Run("same name and nonce, another type", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + configMap.Data["coderID"] = "" + require.NoError(t, k8sClient.Update(ctx, configMap)) + s.fake.mu.Lock() + for _, p := range s.fake.aiProviders { + p.displayName, p.providerType = "coder-k8s:"+string(s.provider.UID), codersdk.AIProviderTypeAnthropic + } + s.fake.mu.Unlock() + s.clock.Step(6 * time.Minute) + s.sweep(ctx, t) + _, ok := s.fake.aiProvider("openai") + require.True(t, ok, "only the nonce and the recorded type prove ownership") + }) + t.Run("stale cache of the record", func(t *testing.T) { + t.Parallel() + s := newSweepEnv(ctx, t) + createTestControlPlane(ctx, t, s.namespace, "coder-b", s.fake.server.URL) + // A newer create attempt, written right before; a stale cache would + // still show the old time. + configMap := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: s.namespace, Name: "ck8s-cleanup-" + string(s.provider.UID)}, configMap)) + old := configMap.DeepCopy() + configMap.Data["createAttemptTime"] = s.clock.Now().Add(6 * time.Minute).UTC().Format(time.RFC3339) + require.NoError(t, k8sClient.Update(ctx, configMap)) + cached := s.sweeper.Client + s.sweeper.Client = interceptor.NewClient(cached.(client.WithWatch), interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if cm, ok := obj.(*corev1.ConfigMap); ok { + old.DeepCopyInto(cm) + return nil + } + return c.Get(ctx, key, obj, opts...) + }, + }) + s.clock.Step(6 * time.Minute) + require.Greater(t, s.sweep(ctx, t).RequeueAfter, time.Duration(0), "the sweep waits for the settle window after the newest create") + _, ok := s.fake.aiProvider("openai") + require.True(t, ok) + }) t.Run("stale cache of the token Secret", func(t *testing.T) { t.Parallel() s := newSweepEnv(ctx, t) From a7cd76a006d448e1f3f055df29d588dd61b0a68e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 18:23:28 +0000 Subject: [PATCH 12/12] fix: keep a record with a provider ID when the object is deleted with an empty status Change-Id: Ib7fa8097dbaf483d8f50c72676c746ceceabc1c1 Signed-off-by: Thomas Kosiewski --- internal/controller/coderaiprovider_delete.go | 14 +++++++++++--- internal/controller/coderaiprovider_record_test.go | 8 ++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/controller/coderaiprovider_delete.go b/internal/controller/coderaiprovider_delete.go index 353f6b0a2..7a67c0864 100644 --- a/internal/controller/coderaiprovider_delete.go +++ b/internal/controller/coderaiprovider_delete.go @@ -59,9 +59,17 @@ func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error { // the finalizer stays. func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *coderv1alpha1.CoderAIProvider) (release bool, step *aiProviderStep, err error) { if provider.Status.ProviderID == "" && provider.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder: earlier Coder answers - // cleared the create marker, so the record goes too. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) + // Earlier Coder answers cleared the create marker. A record with a + // provider ID still locates a provider, for example in a deployment + // that the control plane no longer reaches (CoderDeploymentChanged), + // so only a record without one proves that nothing exists. + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, provider.UID) + if err != nil && !errors.Is(err, errNotACleanupRecord) { + return false, nil, err + } + if record == nil || record.CoderID == "" { + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, provider.UID) + } } ref := types.NamespacedName{Namespace: provider.Namespace, Name: provider.Spec.ControlPlaneRef.Name} controlPlane := &coderv1alpha1.CoderControlPlane{} diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index e40671e3b..b30c3a761 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -279,4 +279,12 @@ func TestCoderAIProviderDeploymentSwitchKeepsRecord(t *testing.T) { require.Contains(t, requireAIProviderStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) require.Equal(t, 1, env.fake.requestCount(routeCreateAIProvider), "no create in the new deployment") require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record keeps the old locator") + + // #374 review round 4: deleting the object in this state keeps the + // record too, so a sweep in the old deployment can still find it. + require.Empty(t, got.Status.ProviderID) + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in the new deployment") + require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record stays for a sweep") }