diff --git a/README.md b/README.md index c8a7dbe..df3df65 100644 --- a/README.md +++ b/README.md @@ -164,6 +164,12 @@ Cloud-init from a Secret: the Secret **must** carry the label labelled Secrets, so an unlabelled one is invisible (the Proxy reports `CloudInitError`). Rotating the Secret's content triggers VM replacement. +Stuck deletion: a Proxy stays `Terminating` while the operator keeps failing +to clean its instance up — credentials revoked, a provider or a KVM host +dropped from the config. Annotate it with +`crawl.example.com/skip-cleanup: "true"` to abandon the instance and release +the object. Whatever is still running then has to be removed by hand. + ## Providers `Managed` proxies are provisioned by a provider — a small compute backend diff --git a/api/v1alpha1/proxy_types.go b/api/v1alpha1/proxy_types.go index 865fad0..bd17201 100644 --- a/api/v1alpha1/proxy_types.go +++ b/api/v1alpha1/proxy_types.go @@ -62,6 +62,15 @@ const ( // must be replaced. AnnotationSpecHash = "crawl.example.com/spec-hash" + // AnnotationSkipCleanup abandons the provider-side instance: set it to + // "true" and deletion removes the finalizer without asking the provider + // to clean anything up. It is the escape hatch for a cleanup that cannot + // succeed — a provider dropped from the config, credentials revoked, a + // backend that answers permanently — where the object would otherwise + // stay in Terminating forever. The instance is left running and becomes + // the administrator's to remove. + AnnotationSkipCleanup = "crawl.example.com/skip-cleanup" + // LabelCloudInit must be set (to "true") on every Secret referenced by // spec.cloudInit.secretRef: the manager's cache only holds Secrets // carrying this label, so an unlabelled Secret is invisible to the diff --git a/docs/architecture.md b/docs/architecture.md index ee1ff9d..3f7daee 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -489,6 +489,15 @@ they read as choices, not accidents. Chronological by build step. cluster-wide Secret read RBAC — an unrestricted cache would hold every Secret in scope). Unlabelled referenced Secrets are invisible by construction, surfacing as `CloudInitError`. +- **Deletion never gives up on its own, but can be released by hand:** a + permanent cleanup failure keeps retrying with the finalizer on, because + dropping it automatically would leak a running instance on any provider + that answers "permission denied". Where `Get` fails permanently the + reconciler still calls `Delete`, since it is idempotent and may finish + where `Get` cannot converge. `crawl.example.com/skip-cleanup: "true"` + abandons the instance and releases the object; it is checked before the + provider lookup, so it also frees a Proxy whose provider is gone from the + config. - **Events RBAC from the plan is omitted:** nothing wires an EventRecorder in the prototype, and granting verbs nothing uses would be RBAC lint noise. Add the marker together with the recorder if events diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md index a59d1be..f262f6b 100644 --- a/docs/kvm-provider.md +++ b/docs/kvm-provider.md @@ -264,6 +264,14 @@ The provider logs through controller-runtime's logger, named `kvm`, with cleaned when the same Proxy reconciles again (Delete removes volumes by name even with no domain), but a Proxy CR that is gone for good leaves them for `virsh vol-list` to find. +- **A host removed from the config takes its VMs with it.** `Delete` for a + providerID naming an unconfigured host is a no-op success, so the Proxy + finishes deleting instead of pinning its finalizer, but the domain and its + volumes stay on the hypervisor and orphan GC cannot see them either (it + only sweeps configured hosts). Delete the Proxies before retiring a host, + or clean up afterwards with `virsh undefine --nvram` plus `virsh vol-delete`. + `Get` still fails permanently for such a providerID on purpose: a typo in + `hosts` must not look like a missing VM and trigger a rebuild elsewhere. - **The first image fetch is synchronous.** It holds a reconcile worker for the download (LAN speed, a few hundred MB, once per image per host). Other Proxies keep reconciling on the remaining workers. diff --git a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md new file mode 100644 index 0000000..ea4bc6d --- /dev/null +++ b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md @@ -0,0 +1,57 @@ +# Execution: fix the five code-review findings on `feat/kvm-provider` +Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222-kvm-review-fixes.md) + +- [x] Step 0 — Save plan into the repo +- [x] Step 1 — Unwedge deletion (finding 1) +- [ ] Step 2 — Validate names that become identifiers (finding 2) +- [ ] Step 3 — Read the SSH material lazily (finding 3) +- [ ] Step 4 — Guest agent via vendor-data script (findings 4 and 5) +- [ ] Step 5 — Validate, record, push + +## Step 0 — plan committed + +Copied out of `~/.claude/plans/` and committed alone before touching code, as +the repo's plan rules require. + +## Step 1 — Unwedge deletion + +Three changes, in the order the bug unfolds. + +`reconcileDelete` calls `Get` first and only drops the finalizer when it +answers not-found, so any permanent error there retried forever. It now +switches on the class: a permanent `Get` still calls `Delete`, which is +idempotent, and releases the object when that succeeds. Polling a `Get` +that cannot converge was the wedge; `Delete` is the only call that can +still make progress. + +`crawl.example.com/skip-cleanup: "true"` abandons the instance and removes +the finalizer. It is checked **before** the provider lookup, which also +frees a Proxy whose provider was deleted from the config — a second wedge +sitting three lines above the reported one, with the same shape. No +`EventRecorder` exists on the reconciler and none was added, so the +abandonment is an Info log; `docs/architecture.md` already records that +decision for the whole controller. + +In the KVM provider, `Delete` for a providerID naming an unconfigured host +now logs and returns success. The precedent is two functions further down: +`removeVolumes` treats a missing pool the same way, with a comment saying +outright that refusing "would pin the Proxy's finalizer". + +**`Get` deliberately still fails permanently for that case.** Making it +answer not-found would have been a one-line fix for the wedge, but the live +reconcile path reads not-found as "the VM is gone" and rebuilds it +elsewhere. A typo in `hosts` would then quietly duplicate every Proxy on +that host, with the originals still running where GC cannot see them, since +`ListByTag` only sweeps configured hosts. The asymmetry is now pinned by a +test that asserts both halves at once. + +Worth noting: the review filed this as a KVM finding, but the fix is mostly +in the controller, because KVM is the only provider that resolves a +providerID segment against operator config at all. GCP and Kubernetes hand +the lookup to the backend, which answers 404 for a retired zone or a deleted +namespace, so they reach not-found and release on their own. Anything else +that classifies permanently on the delete path — revoked RBAC on the +Kubernetes provider is the realistic one — was equally stuck before this +step and is what the annotation is really for. The KVM table case +`"host not configured"` moved out of `TestDelete_errors` into its own named +test, since it is no longer an error. diff --git a/internal/controller/proxy_controller.go b/internal/controller/proxy_controller.go index e5fbb2f..2159587 100644 --- a/internal/controller/proxy_controller.go +++ b/internal/controller/proxy_controller.go @@ -273,17 +273,38 @@ func (r *ProxyReconciler) reconcileDelete(ctx context.Context, p *crawlv1alpha1. controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName) return ctrl.Result{}, r.Update(ctx, p) } + if p.Annotations[crawlv1alpha1.AnnotationSkipCleanup] == "true" { + // Checked before the provider lookup so it also releases a Proxy whose + // provider is gone from the config. The instance is left running. + logf.FromContext(ctx).Info("skip-cleanup annotation set; abandoning the instance", + "providerID", p.Status.ProviderID, "provider", p.Spec.Provider) + controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName) + return ctrl.Result{}, r.Update(ctx, p) + } prov, ok := r.Providers[p.Spec.Provider] if !ok { return ctrl.Result{}, fmt.Errorf( "provider %q is not configured; cannot clean up instance %s", p.Spec.Provider, p.Status.ProviderID) } _, err := prov.Get(ctx, p.Status.ProviderID) - if provider.Class(err) == provider.ErrNotFound { + switch { + case err == nil: + case provider.Class(err) == provider.ErrNotFound: controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName) return ctrl.Result{}, r.Update(ctx, p) - } - if err != nil { + case provider.Class(err) == provider.ErrPermanent: + // Get will never answer for this instance — a host retired from the + // config, a providerID the provider can no longer resolve. Delete is + // idempotent and may still finish, so try it instead of polling a Get + // that cannot converge, and release the object when it does. + if delErr := prov.Delete(ctx, p.Status.ProviderID); delErr != nil { + return r.deletionFailure(errors.Join(err, delErr)) + } + logf.FromContext(ctx).Info("instance unreachable but cleanup succeeded; removing the finalizer", + "providerID", p.Status.ProviderID, "error", err.Error()) + controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName) + return ctrl.Result{}, r.Update(ctx, p) + default: return r.deletionFailure(err) } if err := prov.Delete(ctx, p.Status.ProviderID); err != nil { diff --git a/internal/controller/reconcile_test.go b/internal/controller/reconcile_test.go index 6f0150a..cd6097a 100644 --- a/internal/controller/reconcile_test.go +++ b/internal/controller/reconcile_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "slices" "testing" "time" @@ -78,6 +79,10 @@ func notFoundErr() error { return provider.Wrap(provider.ErrNotFound, "get", "stub", "some-id", nil) } +func permanentErr() error { + return provider.Wrap(provider.ErrPermanent, "get", "stub", "some-id", nil) +} + func testScheme(t *testing.T) *runtime.Scheme { t.Helper() s := runtime.NewScheme() @@ -125,6 +130,15 @@ func withSpecHashAnnotation(hash string) func(*crawlv1alpha1.Proxy) { } } +func withSkipCleanup() func(*crawlv1alpha1.Proxy) { + return func(p *crawlv1alpha1.Proxy) { + if p.Annotations == nil { + p.Annotations = map[string]string{} + } + p.Annotations[crawlv1alpha1.AnnotationSkipCleanup] = "true" + } +} + func deleting() func(*crawlv1alpha1.Proxy) { return func(p *crawlv1alpha1.Proxy) { now := metav1.Now() @@ -375,6 +389,59 @@ func TestReconcile_actionTable(t *testing.T) { assertProxyGone(t, r) }, }, + { + // A permanent Get (a retired KVM host, a providerID the provider + // cannot resolve) used to retry forever with the finalizer on. + name: "deletion with an unreachable instance still deletes and releases", + proxy: managedProxy(deleting(), withProviderID("stub-id-1")), + stub: &stubProvider{getErr: permanentErr()}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + assertProxyGone(t, r) + if stub.deleteCalls != 1 { + t.Errorf("deleteCalls = %d, want 1 — Delete must be tried when Get cannot answer", stub.deleteCalls) + } + }, + }, + { + name: "deletion keeps the finalizer when cleanup itself fails", + proxy: managedProxy(deleting(), withProviderID("stub-id-1")), + stub: &stubProvider{ + getErr: permanentErr(), + deleteErr: provider.Wrap(provider.ErrPermanent, "delete", "stub", "stub-id-1", nil), + }, + wantResult: ctrl.Result{}, + wantErr: true, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + // getProxy fails the test if the object is gone, which is what + // finalizer removal would have caused. + if p := getProxy(t, r); !slices.Contains(p.Finalizers, crawlv1alpha1.FinalizerName) { + t.Error("finalizer removed although cleanup failed; the instance would leak silently") + } + }, + }, + { + name: "skip-cleanup annotation abandons the instance", + proxy: managedProxy(deleting(), withProviderID("stub-id-1"), withSkipCleanup()), + stub: &stubProvider{getInst: &provider.Instance{ID: "stub-id-1", State: provider.StateRunning}}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) { + assertProxyGone(t, r) + if stub.getCalls != 0 || stub.deleteCalls != 0 { + t.Errorf("getCalls/deleteCalls = %d/%d, want 0/0 — the provider must not be asked", stub.getCalls, stub.deleteCalls) + } + }, + }, + { + name: "skip-cleanup annotation releases a proxy whose provider is gone", + proxy: managedProxy(deleting(), withProviderID("stub-id-1"), withSkipCleanup(), + func(p *crawlv1alpha1.Proxy) { p.Spec.Provider = "retired" }), + stub: &stubProvider{}, + wantResult: ctrl.Result{}, + verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) { + assertProxyGone(t, r) + }, + }, { name: "deletion deletes the instance and polls", proxy: managedProxy(deleting(), withProviderID("stub-id-1")), diff --git a/internal/provider/kvm/kvm.go b/internal/provider/kvm/kvm.go index 470fa79..a7fc2c4 100644 --- a/internal/provider/kvm/kvm.go +++ b/internal/provider/kvm/kvm.go @@ -451,7 +451,8 @@ func mapState(s libvirt.DomainState, ip string) provider.InstanceState { // domain is not destroyed again, a missing domain is not undefined, and // the per-VM volumes are removed by name even when the domain is already // gone, so a crash between volume creation and define is cleaned up on the -// next reconcile. +// next reconcile. A providerID naming a host that is no longer configured +// is a no-op success rather than an error. func (p *Provider) Delete(ctx context.Context, providerID string) error { hostName, name, err := parseProviderID(providerID) if err != nil { @@ -459,7 +460,14 @@ func (p *Provider) Delete(ctx context.Context, providerID string) error { } host, err := p.host(hostName) if err != nil { - return p.wrapErr("delete", providerID, err) + // The host is no longer configured, so this operator has no way to + // reach the domain and nothing left to clean. Same reasoning as + // removeVolumes over a missing pool: refusing would pin the Proxy's + // finalizer. Whatever is still running there is the admin's to remove, + // and orphan GC cannot see it either — see docs/kvm-provider.md. + p.logger(ctx).Info("host is not configured; delete has nothing to clean up", + "host", hostName, "providerID", providerID) + return nil } log := p.logger(ctx).WithValues("host", host.Name, "name", name) err = p.hosts.call(ctx, log, host, func(api libvirtAPI) error { diff --git a/internal/provider/kvm/kvm_test.go b/internal/provider/kvm/kvm_test.go index 7b0e232..4993126 100644 --- a/internal/provider/kvm/kvm_test.go +++ b/internal/provider/kvm/kvm_test.go @@ -726,6 +726,29 @@ func TestDelete_missingPoolIsNotAnError(t *testing.T) { } } +// A host dropped from the config leaves the operator no way to reach the +// domain, so Delete has nothing left to clean. Failing instead would pin +// the Proxy's finalizer forever, since the reconciler's delete path cannot +// make progress without a Delete that can succeed. +func TestDelete_unconfiguredHostIsNotAnError(t *testing.T) { + t.Parallel() + api := readyHost() + managedDomain(t, api, testName, testUID, true, nil) + p := newTestProvider(hostFakes{"nas1": api}, "nas1") + + if err := p.Delete(context.Background(), "hosts/nas9/domains/"+testName); err != nil { + t.Fatalf("Delete over an unconfigured host: %v", err) + } + if api.domain(testName) == nil { + t.Error("domain on the configured host was touched; only the named host's state may change") + } + // Get still refuses, so the live path cannot mistake a config typo for a + // missing VM and recreate it somewhere else. + if _, err := p.Get(context.Background(), "hosts/nas9/domains/"+testName); provider.Class(err) != provider.ErrPermanent { + t.Errorf("Get class = %v, want ErrPermanent", provider.Class(err)) + } +} + func TestDelete_errors(t *testing.T) { t.Parallel() cases := []struct { @@ -735,7 +758,6 @@ func TestDelete_errors(t *testing.T) { class error }{ {"malformed id", "nope", func(*testing.T) hostFakes { return hostFakes{"nas1": readyHost()} }, provider.ErrPermanent}, - {"host not configured", "hosts/nas9/domains/x", func(*testing.T) hostFakes { return hostFakes{"nas1": readyHost()} }, provider.ErrPermanent}, {"host unreachable", "hosts/nas1/domains/x", func(*testing.T) hostFakes { return hostFakes{} }, provider.ErrTransient}, { "undefine fails", "hosts/nas1/domains/" + testName,