Skip to content

OCPBUGS-84695: [TNF] lifecycle part 2 - update setup deadlock fix & conditions cleanup #1668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaypoulz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-84695: [TNF] lifecycle part 2 - update setup deadlock fix & conditions cleanup #1668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions docs/tnf/job-controllers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ RunNodeJobController(jobType, node, retries, ...)
│ ├─ Check node readiness via checkNodesReadinessAndSetCondition
│ │ ├─ Not ready < 10min → Return (skip job, retry on next sync)
│ │ ├─ Not ready ≥ 10min → Return error (triggers Degraded via WithSyncDegradedOnError)
│ │ └─ Ready → Clear Degraded condition (if was blocked), continue
│ │ └─ Ready → Continue (Degraded clears naturally when job succeeds/completes)
│ │
│ └─ Create job spec:
│ - Fetch fresh node from informer (handles node replacement)
Expand Down Expand Up @@ -208,11 +208,11 @@ RunClusterJobController(jobType, schedulableNodesFunc, affectedNodesFunc, jobCon
│ │ │
│ │ ├─ Check affectedNodesFunc() → any nodes not ready?
│ │ │ ├─ YES → Return error after 10min (triggers Degraded via WithSyncDegradedOnError)
│ │ │ └─ NO → Clear Degraded condition (if was blocked), continue
│ │ │ └─ NO → Continue (Degraded clears naturally when job succeeds/completes)
│ │ │
│ │ ├─ Check schedulableNodesFunc() → any nodes available?
│ │ │ ├─ NO → Return error after 10min (triggers Degraded via WithSyncDegradedOnError)
│ │ │ └─ YES → Clear Degraded condition (if was blocked), continue
│ │ │ └─ YES → Continue (Degraded clears naturally when job succeeds/completes)
│ │ │
│ │ ├─ Get or initialize retry state (AttemptNumber, NodeIndex, config)
│ │ │
Expand All @@ -224,12 +224,12 @@ RunClusterJobController(jobType, schedulableNodesFunc, affectedNodesFunc, jobCon
│ │ │
│ │ ├─ Get current job from cluster
│ │ │ │
│ │ │ ├─ Complete? → Clear degraded, preserve state, return
│ │ │ ├─ Complete? → Return success (Degraded clears naturally via syncManaged flow)
│ │ │ │
│ │ │ └─ Failed?
│ │ │ ├─ Move to next node (NodeIndex++)
│ │ │ ├─ All nodes tried? → Increment attempt (AttemptNumber++)
│ │ │ ├─ Max attempts exhausted? → Set Degraded, reset to attempt 1
│ │ │ ├─ Max attempts exhausted? → Return error (triggers Degraded via WithSyncDegradedOnError), reset to attempt 1
│ │ │ └─ Update retry state (ApplyJob will detect retry field drift and delete/recreate)
│ │ │
│ │ └─ ApplyJob detects drift (NodeName, node-index, or attempt labels changed) → Deletes and recreates job
Expand Down
5 changes: 4 additions & 1 deletion docs/tnf/lifecycle-manager.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,11 @@ startJobControllers()
└─ YES (Runtime Mode):
│
├─ Ensure auth/after-setup controllers running (per-node)
├─ Ensure setup job controller running (maintains conditions)
├─ Ensure update-setup controller running (if 2 nodes)
└─ Ensure fencing controller running (cluster-wide)
├─ Ensure fencing controller running (cluster-wide)
├─ Start status collector CronJob
└─ Start Pacemaker health check controller
```

See [Job Controllers](job-controllers.md) for details on job execution patterns and retry logic.
Expand Down
232 changes: 121 additions & 111 deletions pkg/tnf/operator/job_controllers.go
View file
Open in desktop

Large diffs are not rendered by default.

54 changes: 41 additions & 13 deletions pkg/tnf/operator/job_controllers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Job Controller Startup:
└── TestStartJobControllers - Entry point logic and path selection
├── Before transition (bootstrap path):
│ ├── 2 ready nodes → bootstrap with retry
│ └── 3 nodes → skip (pacemaker only supports 2)
│ └── 3 nodes → error (pacemaker only supports 2)
└── After transition (post-transition path):
├── 2 ready nodes → ensure running
└── 1 ready node → ensure running (single-node case)
Expand Down Expand Up @@ -147,10 +147,10 @@ func TestRetryInitialTransitionOrDegrade(t *testing.T) {
etcdInformer: etcdInformers.Operator().V1().Etcds(),
}

// Store original startTnfJobcontrollersFunc and replace with mock
originalStartFunc := startTnfJobcontrollersFunc
startTnfJobcontrollersFunc = tt.setupMockStartFunc()
defer func() { startTnfJobcontrollersFunc = originalStartFunc }()
// Store original startBootstrapJobControllersFunc and replace with mock
originalStartFunc := startBootstrapJobControllersFunc
startBootstrapJobControllersFunc = tt.setupMockStartFunc()
defer func() { startBootstrapJobControllersFunc = originalStartFunc }()

// Store original backoff config and use faster settings for testing
originalBackoff := retryBackoffConfig
Expand Down Expand Up @@ -353,14 +353,27 @@ func TestStartJobControllers(t *testing.T) {
etcdInformer: etcdInformers.Operator().V1().Etcds(),
}

// Track if startTnfJobcontrollersFunc was called
startCalled := false
originalStartFunc := startTnfJobcontrollersFunc
startTnfJobcontrollersFunc = func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error {
startCalled = true
// Track if job controller startup functions were called
bootstrapCalled := 0
runtimeCalled := 0
mockBootstrapFunc := func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error {
bootstrapCalled++
return nil
}
defer func() { startTnfJobcontrollersFunc = originalStartFunc }()
mockRuntimeFunc := func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error {
runtimeCalled++
return nil
}

// Mock both bootstrap and runtime paths
originalBootstrapFunc := startBootstrapJobControllersFunc
originalRuntimeFunc := startRuntimeJobControllersFunc
startBootstrapJobControllersFunc = mockBootstrapFunc
startRuntimeJobControllersFunc = mockRuntimeFunc
defer func() {
startBootstrapJobControllersFunc = originalBootstrapFunc
startRuntimeJobControllersFunc = originalRuntimeFunc
}()

// Use faster backoff for testing
originalBackoff := retryBackoffConfig
Expand All @@ -381,8 +394,23 @@ func TestStartJobControllers(t *testing.T) {
} else {
require.NoError(t, err)
}
require.Equal(t, tt.expectStartCalled, startCalled,
"Expected startTnfJobcontrollersFunc called=%v, got=%v", tt.expectStartCalled, startCalled)

// Check correct code path was taken
if tt.transitionComplete {
require.Equal(t, 0, bootstrapCalled, "Bootstrap should not be called when transition complete")
if tt.expectStartCalled {
require.Equal(t, 1, runtimeCalled, "Runtime should be called once when transition complete")
} else {
require.Equal(t, 0, runtimeCalled, "Runtime should not be called when start not expected")
}
} else {
require.Equal(t, 0, runtimeCalled, "Runtime should not be called when transition not complete")
if tt.expectStartCalled {
require.Equal(t, 1, bootstrapCalled, "Bootstrap should be called once when transition not complete")
} else {
require.Equal(t, 0, bootstrapCalled, "Bootstrap should not be called when start not expected")
}
}

// If retry path expected, verify TNFJobControllersDegraded condition was set
if tt.expectRetryPath && tt.expectStartCalled {
Expand Down
93 changes: 21 additions & 72 deletions pkg/tnf/operator/lifecycle_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ import (
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/v1helpers"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -70,6 +67,7 @@ type pacemakerLifecycleManager struct {
// Returns the controller, the PacemakerLifecycleManager instance, and the PacemakerCluster informer
// (which must be started separately - see runPacemakerControllers in pkg/tnf/operator/starter.go).
func newPacemakerLifecycleManager(
ctx context.Context,
operatorClient v1helpers.StaticPodOperatorClient,
kubeClient kubernetes.Interface,
eventRecorder events.Recorder,
Expand All @@ -79,56 +77,12 @@ func newPacemakerLifecycleManager(
kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces,
etcdInformer operatorv1informers.EtcdInformer,
) (factory.Controller, *pacemakerLifecycleManager, cache.SharedIndexInformer, error) {
// Create REST client for PacemakerStatus CRs
restClient, err := pacemaker.CreatePacemakerRESTClient(restConfig)
// Create PacemakerCluster informer
informer, err := pacemaker.NewPacemakerClusterInformer(restConfig)
if err != nil {
return nil, nil, nil, fmt.Errorf("failed to create REST client: %w", err)
return nil, nil, nil, err
}

// Create scheme for the parameter codec
scheme := runtime.NewScheme()
if err := pacmkrv1.AddToScheme(scheme); err != nil {
return nil, nil, nil, fmt.Errorf("failed to add scheme for informer: %w", err)
}

// Create informer for PacemakerCluster
klog.Infof("Creating PacemakerCluster informer for group %s, resource %s", pacmkrv1.SchemeGroupVersion.String(), pacemaker.PacemakerResourceName)
informer := cache.NewSharedIndexInformer(
&pacemaker.PacemakerListWatch{ListWatch: cache.ListWatch{
ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
klog.V(4).Infof("PacemakerCluster informer ListFunc called for resource %s", pacemaker.PacemakerResourceName)
sanitizedOptions := pacemaker.SanitizeListOptions(options)
result := &pacmkrv1.PacemakerClusterList{}
err := restClient.Get().
Resource(pacemaker.PacemakerResourceName).
VersionedParams(&sanitizedOptions, runtime.NewParameterCodec(scheme)).
Do(context.Background()).
Into(result)
if err != nil {
klog.Errorf("Failed to list PacemakerCluster resources (%s): %v", pacemaker.PacemakerResourceName, err)
} else {
klog.V(4).Infof("Successfully listed PacemakerCluster resources, found %d items", len(result.Items))
}
return result, err
},
WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
klog.V(4).Infof("PacemakerCluster informer WatchFunc called for resource %s", pacemaker.PacemakerResourceName)
sanitizedOptions := pacemaker.SanitizeListOptions(options)
watcher, err := restClient.Get().
Resource(pacemaker.PacemakerResourceName).
VersionedParams(&sanitizedOptions, runtime.NewParameterCodec(scheme)).
Watch(context.Background())
if err != nil {
klog.Errorf("Failed to watch PacemakerCluster resources (%s): %v", pacemaker.PacemakerResourceName, err)
}
return watcher, err
},
}},
&pacmkrv1.PacemakerCluster{},
pacemaker.HealthCheckResyncInterval,
cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc},
)

c := &pacemakerLifecycleManager{
operatorClient: operatorClient,
kubeClient: kubeClient,
Expand All @@ -138,20 +92,22 @@ func newPacemakerLifecycleManager(
controllerContext: controllerContext,
kubeInformersForNamespaces: kubeInformersForNamespaces,
etcdInformer: etcdInformer,
controllerCtx: ctx,
}

syncCtx := factory.NewSyncContext(controllerNamePacemakerLifecycle, eventRecorder.WithComponentSuffix("pacemaker-lifecycle-manager"))

klog.Infof("%s controller created, waiting for informers to sync before starting", controllerNamePacemakerLifecycle)
klog.Infof("PacemakerLifecycleManager will watch: operatorClient and %s/%s resource", pacmkrv1.SchemeGroupVersion.String(), pacemaker.PacemakerResourceName)

// ResyncEvery ensures the sync function is called at regular intervals (1 minute)
// even if no informer events are detected.
// ResyncEvery ensures the sync function is called every minute with fresh cache data.
// We use WithBareInformers to keep caches synced without triggering sync on every event.
// Custom event handlers (registered below) handle specific cases like node Ready transitions.

@fonta-rh fonta-rh Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit (non-blocking): The WithBareInformers + custom UpdateFunc goroutine pattern works, but it bypasses the controller work queue — the goroutine at line 151 and the periodic sync can both trigger restartUpdateSetupJob concurrently (serialized by restartJobLocks, so no data race, but non-standard for library-go controllers).

A framework-compliant alternative: WithFilteredEventsInformers with a filter that returns true only for Ready nodes on the node informer, and plain WithBareInformers for the operator/PacemakerCluster informers. This gives instant Ready-transition responsiveness through the work queue without the goroutine, while still avoiding sync noise from unrelated events. The ResyncEvery(1m) provides the baseline detection.

Not blocking — the current approach is correct and safe.

@jaypoulz jaypoulz Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My plan is to remove the event handler entirely in lifecycle refactor part 3. Would you be OK with us making a conscious decision to ignore this, knowing the plan it to remove the event handler in favor of sync-based detection of NodeController<->Pacemaker mismatches?

controller := factory.New().
WithSyncContext(syncCtx).
ResyncEvery(time.Minute).
WithSync(c.sync).
WithInformers(
WithBareInformers(
operatorClient.Informer(),
informer,
controlPlaneNodeInformer,
Expand Down Expand Up @@ -193,18 +149,12 @@ func (c *pacemakerLifecycleManager) registerNodeEventHandlers() error {
if !oldReady && newReady {
klog.Infof("node %s transitioned to ready state - restarting update-setup job", newNode.GetName())
go func() {
// Use controller context (cancelled on shutdown) instead of background context
// Use controller context (cancelled on shutdown)
// Context is initialized in constructor, so always available
c.controllerCtxMu.Lock()
ctx := c.controllerCtx
c.controllerCtxMu.Unlock()

if ctx == nil {
// Controller hasn't started yet, event fired before first sync
// This shouldn't happen (informers sync before events fire), but be defensive
klog.V(4).Infof("Skipping node ready event handler - controller context not yet available")
return
}

// Restart update-setup job when nodes become ready (e.g., after replacement)
// This ensures update-setup reruns if it completed before auth ran on new node
if err := c.restartUpdateSetupJob(ctx); err != nil {
Expand All @@ -228,13 +178,6 @@ func (c *pacemakerLifecycleManager) sync(ctx context.Context, syncCtx factory.Sy
klog.V(4).Infof("PacemakerLifecycleManager sync started")
defer klog.V(4).Infof("PacemakerLifecycleManager sync completed")

// Store controller context on first sync (for event handler goroutines)
c.controllerCtxMu.Lock()
if c.controllerCtx == nil {
c.controllerCtx = ctx
}
c.controllerCtxMu.Unlock()

// Start job controllers (runs in both bootstrap and runtime modes)
if err := c.startJobControllers(ctx); err != nil {
klog.Errorf("Failed to start job controllers: %v", err)
Expand All @@ -255,7 +198,6 @@ func (c *pacemakerLifecycleManager) runPacemakerHealthCheckController(ctx contex
klog.V(4).Infof("Health check controller already started, skipping duplicate start")
return
}
c.healthCheckStarted = true
c.healthCheckMu.Unlock()

healthCheckController, _, err := pacemaker.NewHealthCheckWithInformer(
Expand All @@ -269,6 +211,11 @@ func (c *pacemakerLifecycleManager) runPacemakerHealthCheckController(ctx contex
return
}

// Only set flag after successful creation
c.healthCheckMu.Lock()
c.healthCheckStarted = true
c.healthCheckMu.Unlock()

go healthCheckController.Run(ctx, 1)
klog.Infof("Health check controller started")
}
Expand Down Expand Up @@ -307,10 +254,12 @@ func (c *pacemakerLifecycleManager) restartUpdateSetupJob(ctx context.Context) e
return c.getActivePacemakerNodes()
}

// affectedNodesFunc: all control plane nodes (ready or not)
// Job waits for these nodes to become ready before proceeding
// affectedNodesFunc: K8s ∩ Pacemaker active nodes (ready or not)
// Only blocks on nodes that are actually in the Pacemaker cluster configuration.
// This prevents deadlock when a node is removed from Pacemaker but can't become Ready
// without update-setup re-adding it (kubelet is a Pacemaker resource).
updateSetupAffectedNodesFunc := func() ([]*corev1.Node, error) {
return tools.ListNodesFromInformer(c.controlPlaneNodeInformer)
return c.getActivePacemakerNodes()
}

klog.Infof("Restarting update-setup job controller after node ready event")
Expand Down
42 changes: 27 additions & 15 deletions pkg/tnf/operator/starter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1"
operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
"github.com/openshift/library-go/pkg/controller/controllercmd"
"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/staticresourcecontroller"
"github.com/openshift/library-go/pkg/operator/v1helpers"
Expand Down Expand Up @@ -68,10 +69,8 @@ func HandleDualReplicaClusters(
return false, err
}
// Start pacemaker controllers (lifecycle manager, status collector)
// PacemakerLifecycleManager handles ALL node lifecycle events:
// - UpdateFunc: Ready transitions for initial bootstrap
// - AddFunc/DeleteFunc: drift-driven reconciliation
// Secret handler registration happens inside runPacemakerControllers after lifecycleManager is created
// PacemakerLifecycleManager registers UpdateFunc handler for node Ready transitions
// to trigger update-setup job restart during initial bootstrap.
runPacemakerControllers(ctx, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer, controlPlaneNodeInformer, dynamicClient)

return true, nil
Expand Down Expand Up @@ -172,18 +171,31 @@ func runPacemakerControllers(ctx context.Context, controllerContext *controllerc
klog.Infof("PacemakerCluster CRD is established")

// Prerequisites met: create and start lifecycle manager controller.
lifecycleController, _, pacemakerInformer, err := newPacemakerLifecycleManager(
operatorClient,
kubeClient,
controllerContext.EventRecorder,
controllerContext.KubeConfig,
controlPlaneNodeInformer,
controllerContext,
kubeInformersForNamespaces,
etcdInformer,
)
// Retry with backoff to handle transient errors (e.g., API server unavailable).
var lifecycleController factory.Controller
var pacemakerInformer cache.SharedIndexInformer
err = wait.PollUntilContextCancel(ctx, 30*time.Second, true, func(ctx context.Context) (bool, error) {
var createErr error
lifecycleController, _, pacemakerInformer, createErr = newPacemakerLifecycleManager(
ctx,
operatorClient,
kubeClient,
controllerContext.EventRecorder,
controllerContext.KubeConfig,
controlPlaneNodeInformer,
controllerContext,
kubeInformersForNamespaces,
etcdInformer,
)
if createErr != nil {
klog.Errorf("Failed to create Pacemaker lifecycle manager, will retry: %v", createErr)
return false, nil
}
return true, nil
})
if err != nil {
klog.Fatalf("Failed to create Pacemaker lifecycle manager: %v", err)
klog.Errorf("Context cancelled while creating Pacemaker lifecycle manager: %v", err)
return
}

// Start the PacemakerCluster informer (controller waits for sync before processing events).
Expand Down
Loading