Skip to content

fix: deletion defects and resource orphans - #17

Open
Herbaert wants to merge 15 commits into
mainfrom
fix/deletion-defects
Open

fix: deletion defects and resource orphans#17
Herbaert wants to merge 15 commits into
mainfrom
fix/deletion-defects

Conversation

@Herbaert

Copy link
Copy Markdown
Collaborator

Summary

Fixes six defects in the cluster and machine reconcilers where deletion could
hang and leave STACKIT resources orphaned, and where changes that should trigger
a reconcile did not. Also stops the manager from holding Secret contents in
memory.

Changes

  • Deletion is no longer blocked by a missing owner. The owner lookups in
    both reconcilers moved behind the DeletionTimestamp branch. The cluster
    reconciler tolerates a deleted owning Cluster and takes its name from the
    ownerReference; the machine reconciler finalizes with a CleanupSkipped
    warning event when the Machine, Cluster or StackitCluster is gone, since
    without them there are neither credentials nor tags left to reach the server.
    Only a genuinely absent owner is tolerated — a transient API error still fails
    the reconcile rather than dropping a finalizer over a running server.
  • StackitCluster keeps its finalizer and requeues while Machines for the
    cluster still exist, so those machines can still resolve credentials and delete
    their servers. Cluster API orders this correctly on its own, but a namespace
    teardown or a direct delete bypasses that ordering.
  • Finalizers are persisted before the first cloud call, rather than only
    through the deferred patch at the end of Reconcile. A crash in between
    previously left a running server or load balancer behind an object carrying no
    finalizer to clean it up.
  • StackitMachine deletion resolves the server by tags when
    status.instanceID is empty, instead of treating empty status as proof that no
    VM exists.
  • The StackitClusterStackitMachine watch resolves the owning Cluster
    before matching Machine.spec.clusterName, and label-selects server-side. It
    previously matched against the StackitCluster name, which breaks whenever the
    two differ — as they always do for ClusterClass-generated infrastructure refs.
  • The credentials Secret is watched. A new mapper enqueues every
    StackitCluster whose credentialsSecretRef points at the changed Secret,
    searching across namespaces because that ref's namespace is optional.
    Previously, correcting an invalid Secret never re-reconciled the cluster,
    because the failure path deliberately returns without a requeue.
  • Secret contents are kept out of the manager's informer cache via a
    Transform that drops Data and managed fields, with DisableFor on the
    client so the reads that need the real bytes go to the API server. No label
    selector, unlike Cluster API's own equivalent: Cluster API only watches Secrets
    it stamps itself, whereas the credentials and bastion cloud-init Secrets here
    are user-created and unlabelled, so a selector would silently disable those
    watches.
  • Reconciler-internal variables renamed for readability (no behavior change);
    debug/ references dropped from test comments; the README compatibility table
    corrected from the v1beta1 to the v1beta2 contract, which is what
    metadata.yaml, the CRD labels and every import already declare.

Notes

  • Breaking changes: none. No API, CRD or RBAC changes.
  • Migration: none.
  • Behavior/infrastructure: the manager now sets Cache and Client options, so
    every Secret read is a live API call — one per cluster reconcile, two per
    machine reconcile. CAPA and CAPG do not harden the Secret cache at all and
    therefore pay no read cost; Cluster API core hardens it and compensates with a
    second, caching client for hot reads. This provider takes the hardened side
    without that compensation, which stays bounded and event-driven at the fleet
    sizes in scope. Watching Secrets as PartialObjectMetadata against a dedicated
    metadata cache would go one step further, at the cost of a second cache in
    main.go for a small remaining gain. The new credentials watch adds reconciles
    when that Secret changes.
  • Tests: an envtest regression test was added for each fix, and every one was
    verified to fail when its fix is reverted. Unit tests cover the new cache
    options. go build ./..., go test ./... and golangci-lint run ./... pass;
    make manifests produces no diff. The e2e suite was not run.

@Herbaert
Herbaert requested a review from tuunit August 20, 2026 11:05
@Herbaert Herbaert self-assigned this Aug 20, 2026
@Herbaert Herbaert added the bug Something isn't working label Aug 20, 2026
@tuunit tuunit changed the title Fix/deletion defects fix: deletion defects and resource orphans Aug 20, 2026
Comment thread controller/controller_test_helpers_test.go
Comment thread cmd/manager/main.go Outdated
Comment thread cmd/manager/main.go Outdated
Comment thread controller/constants.go Outdated
Comment thread controller/controller_test_helpers_test.go Outdated
Comment thread controller/stackitcluster_bastion.go
Comment thread controller/stackitcluster_controller.go Outdated
Comment thread controller/stackitcluster_controller_test.go Outdated
Comment thread controller/stackitcluster_infrastructure.go
Comment thread controller/stackitmachine_controller.go Outdated
Comment on lines +118 to +126
case machine == nil:
log.Info("StackitMachine has no owning Machine yet, waiting")
return ctrl.Result{}, nil
case cluster == nil:
log.Info("Machine has no owning Cluster yet, waiting")
return ctrl.Result{}, nil
case stackitCluster == nil:
log.Info("StackitCluster not found, waiting")
return ctrl.Result{}, nil

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how or when can these cases effectively happen? as far as i know provider machines are owned by machines which in turn are owned by a cluster and they are created in that order

Cluster -> MachineDeployment -> Machine -> ProviderMachine

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted

Comment thread controller/stackitmachine_infrastructure.go Outdated
Comment thread cloud/fake/client.go Outdated
Comment thread cmd/manager/main.go Outdated
Comment thread cmd/manager/main.go Outdated
Comment thread cmd/manager/main_test.go Outdated
Comment thread controller/stackitcluster_controller.go Outdated
Comment thread controller/stackitcluster_controller.go Outdated
Comment thread controller/stackitcluster_controller.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants