Skip to content

Conversation

@frobware
Copy link
Contributor

@frobware frobware commented Aug 19, 2025

Related to #461

Problem

The original Config controller implementation has a race condition during deletion:

  1. User deletes Config resource
  2. Kubernetes immediately starts garbage collection of owned resources (ConfigMap, DaemonSets, CSIDriver)
  3. Race condition: Controller reconcile loop triggers and attempts to recreate the "missing" resources
  4. Result: Resources get recreated during deletion, causing inconsistent state

Root Cause Analysis

The race occurs because Kubernetes provides the following atomicity guarantees:

  • Resource deletion is atomic - Once DeletionTimestamp is set, the resource is marked for deletion
  • Owner reference cleanup is eventually consistent - Garbage collection happens asynchronously
  • Controller reconciliation is continuous - Controller sees "missing" owned resources and attempts recreation

The controller was missing a critical check: distinguish between "resources don't exist because they're being deleted" vs "resources don't exist because they need to be created".

Solution: Finalizer-Based Deletion Coordination

This PR implements a finalizer protection pattern that provides deterministic ordering:

1. Deletion Detection (Race Prevention)

// Check if Config is being deleted first to prevent race conditions
if \!bpfmanConfig.DeletionTimestamp.IsZero() {
    return r.handleDeletion(ctx, bpfmanConfig)
}

2. Finalizer Management (Deletion Coordination)

// Ensure finalizer exists to prevent race conditions during deletion
if \!controllerutil.ContainsFinalizer(bpfmanConfig, internal.BpfmanConfigFinalizer) {
    controllerutil.AddFinalizer(bpfmanConfig, internal.BpfmanConfigFinalizer)
    // Early return - reconcile again after finalizer is added
    return ctrl.Result{}, nil
}

3. Coordinated Cleanup

func (r *BpfmanConfigReconciler) handleDeletion(ctx context.Context, config *v1alpha1.Config) (ctrl.Result, error) {
    // No custom cleanup needed - all resources are managed via owner references
    // Remove finalizer to trigger cascading deletion of owned resources
    controllerutil.RemoveFinalizer(config, internal.BpfmanConfigFinalizer)
    return ctrl.Result{}, r.Update(ctx, config)
}

How This Eliminates the Race Condition

Safe Deletion Sequence:

  1. User deletes ConfigDeletionTimestamp set, finalizer blocks actual deletion
  2. Controller detects deletion → Stops all resource creation/updates
  3. Controller removes finalizer → Signals deletion can proceed
  4. Kubernetes garbage collection → Safely deletes all owned resources via owner references

Key Properties:

  • No resource recreation during deletion - Controller exits early when DeletionTimestamp is set
  • Leverages Kubernetes owner references - No custom cleanup logic needed
  • Atomic finalizer operations - Kubernetes guarantees finalizer add/remove atomicity
  • Two-phase reconciliation - Finalizer added in first reconcile, resources created in second

Changes

  • Add BpfmanConfigFinalizer constant for deletion coordination
  • Add deletion timestamp check to prevent race conditions
  • Add handleDeletion method for coordinated cleanup
  • Add finalizer RBAC permissions (configs/finalizers)
  • Update tests for two-phase reconciliation pattern
  • Fix undeploy targets for cluster-scoped Config CRD

Testing

Added test scripts to prove out the general workflow including Config deletion, cascading resource cleanup, and recreation with modified settings. The logic within these scripts should become unit tests for proper integration into the test suite.

Related to #461

andreaskaris and others added 6 commits August 12, 2025 22:24
Transition from ConfigMap to Config CRD to enable cascading deletion
via ownerReferences for namespace and cluster-scoped resources like
CSIDriver (ConfigMaps are namespace-scoped and cannot be owners of
cluster-scoped resources). Additional benefit of Config CRD is
that configuration can be properly validated by the API server.
Refactor of reconciliation logic with improved logging and error
handling, unit tests.

Signed-off-by: Andreas Karis <[email protected]>
Add finalizer to Config CRD to prevent race condition during deletion
where operator could recreate resources while Kubernetes garbage
collection is trying to delete them via owner references.

Changes:
- Add BpfmanConfigFinalizer constant
- Add finalizer RBAC permissions via controller-gen
- Check deletion timestamp before reconciliation
- Add finalizer management to prevent race conditions
- Add handleDeletion for coordinated cleanup
- Update tests for two-phase reconciliation
- Fix undeploy targets to handle cluster-scoped Config CRD

This ensures safe deletion sequence:
1. User deletes Config
2. Deletion timestamp set, finalizer blocks deletion
3. Operator stops creating/updating resources
4. Operator removes finalizer
5. Kubernetes garbage collection proceeds via owner references

Signed-off-by: Andrew McDermott <[email protected]>
Tests verification, resource recreation, and complete lifecycle.

***These should be done as unit tests.***

Signed-off-by: Andrew McDermott <[email protected]>
@frobware frobware changed the title Fix Config deletion race condition with finalizer protection WIP: Fix Config deletion race condition with finalizer protection Aug 19, 2025
@frobware
Copy link
Contributor Author

/hold

These are changes that should be merged into, or taken into consideration for #461. This PR shouldn't be merged – it is my playground.

@frobware
Copy link
Contributor Author

Closing as I believe @andreaskaris has taken what was useful in #461.

@frobware frobware closed this Aug 26, 2025
@frobware frobware deleted the fix-config-deletion-race branch September 3, 2025 15:48
frobware pushed a commit to frobware/bpfman-operator that referenced this pull request Sep 5, 2025
…s/component-update-ocp-bpfman-agent

chore(deps): update ocp-bpfman-agent to 6463b45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants