Skip to content

OCPBUGS-84695: feat(tnf): TNF job controller framework and lifecycle management - #1655

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
jaypoulz:jpoulin/commit-1-framework
Aug 3, 2026
Merged

OCPBUGS-84695: feat(tnf): TNF job controller framework and lifecycle management#1655
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
jaypoulz:jpoulin/commit-1-framework

Conversation

@jaypoulz

@jaypoulz jaypoulz commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Implements job controller framework for Two-Node Fencing (TNF) cluster management:

Job Controller Framework (pkg/tnf/pkg/jobs/):

  • Multi-node job pattern with round-robin retry across schedulable nodes
  • Single-node job pattern for node-specific operations
  • Drift detection: resets retry state when node list or config changes
  • Blocked condition when nodes not ready (10-minute timeout)
  • Degraded condition when retry budget exhausted (retries continue)
  • JobRetryState tracks AttemptNumber, NodeIndex, TargetNodes
  • RunNodeJobController/RunClusterJobController entry points

Lifecycle Manager (pkg/tnf/operator/lifecycle_manager.go):

  • PacemakerLifecycleManager: owns all TNF job lifecycle logic
  • Event handlers: node Ready transitions, fencing secret changes
  • Bootstrap mode: exponential backoff retry, requires 2 ready nodes
  • Runtime mode: idempotent restart, handles 1-2 nodes
  • Node selection: K8s ∩ Pacemaker intersection with fallback

Job Implementations:

  • auth: sets hacluster password (per-node, prerequisite for Pacemaker)
  • setup: creates Pacemaker cluster (cluster-wide, bootstrap only)
  • after-setup: disables kubelet service (per-node, Pacemaker owns it)
  • fencing: configures STONITH (cluster-wide, secret-driven restart)

Status Collector (pkg/tnf/operator/status_collector.go):

  • CronJob running every minute on active Pacemaker nodes
  • Sticky-on-success, rotate-on-failure strategy
  • Resets to first node when node list changes

Node Management (pkg/tnf/operator/helpers.go):

  • getActivePacemakerNodes(): returns K8s ∩ Pacemaker intersection
  • Staleness detection: 5-minute threshold for PacemakerCluster CR
  • Sorted output for deterministic round-robin behavior

Documentation:

  • docs/tnf/job-controllers.md: multi-node retry, drift detection, condition model
  • docs/tnf/lifecycle-manager.md: startup sequence, node selection, status collector

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

Summary by CodeRabbit

  • New Features

    • Added TNF Job Controllers documentation and a bootstrap/runtime lifecycle for Pacemaker-related Jobs, including multi-node round-robin retries and drift-aware progression.
    • Introduced a Pacemaker lifecycle manager to coordinate Job controller startup and start an idempotent status-collector CronJob.
  • Documentation

    • Documented the TNF Job Controllers framework and the Pacemaker lifecycle manager startup sequence.
  • Bug Fixes

    • Added automatic recovery for stalled Jobs, improved degraded-condition gating, reduced unnecessary Job recreations for retry-only drift, and strengthened update-setup completion checks (requires exactly two nodes online).
    • Prevented overlapping status-collector runs.
  • Tests

    • Expanded coverage for lifecycle state, controller startup branching, drift/degraded behavior, and status-collector node rotation.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Jul 21, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@jaypoulz: This pull request references Jira Issue OCPBUGS-84695, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Implements the foundational framework for multi-node job scheduling with round-robin retry logic and drift detection:

  • Multi-node job support with NodeTarget struct for node-specific jobs
  • Round-robin retry logic with state tracking (AttemptNumber, NodeIndex, LastJobConfig)
  • Job config drift detection (resets retry state when config changes)
  • Degradation when retry attempts exhausted
  • IsControllerRunning() helper to prevent duplicate controllers
  • Shared node utility functions (GetNodeNames, DetermineReconciliationActions, IsNodeReady)
  • Job type labels for filtering and cleanup

See docs/tnf/job-controllers.md for detailed multi-node retry behavior and config drift examples.

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jul 21, 2026
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds Pacemaker lifecycle orchestration, active-node targeting, status-collector rotation, TNF Job controllers, retry and restart handling, REST compatibility helpers, authentication and setup updates, tests, and lifecycle documentation.

Changes

TNF lifecycle and node orchestration

Layer / File(s) Summary
Lifecycle startup and node targeting
pkg/tnf/operator/lifecycle_manager.go, pkg/tnf/operator/job_controllers.go, pkg/tnf/operator/starter.go, pkg/tnf/operator/helpers.go, pkg/tnf/pkg/tools/*
Adds CRD-gated lifecycle startup, bootstrap/runtime paths, readiness gates, active Pacemaker-node selection, fencing-secret configuration, and startup retry handling.
Status collector execution and rotation
pkg/tnf/operator/status_collector.go, pkg/tnf/operator/status_collector_test.go, bindata/tnfdeployment/cronjob.yaml
Adds a non-overlapping status collector CronJob, completed-job failure detection, node pinning, and sticky-success/rotate-on-failure selection.

TNF Job execution

Layer / File(s) Summary
Job reconciliation and drift handling
pkg/tnf/pkg/jobs/jobcontroller.go, pkg/tnf/pkg/jobs/batch.go, pkg/tnf/pkg/jobs/utils.go, pkg/tnf/pkg/tools/jobs.go, pkg/tnf/pkg/jobs/*_test.go
Adds hook-controlled application, PascalCase condition types, stuck-job recovery, retry-aware drift handling, background deletion, and stopped-state detection.
Multi-node retry and restart lifecycle
pkg/tnf/pkg/jobs/lifecycle.go, pkg/tnf/pkg/jobs/lifecycle_test.go
Adds node and cluster controllers, readiness/degraded conditions, round-robin retries, configuration drift resets, serialized restarts, and lifecycle tests.

Supporting integrations

Layer / File(s) Summary
Pacemaker API compatibility
pkg/tnf/pkg/pacemaker/*
Adds list-option sanitization, an exported REST client constructor, and injectable PacemakerCluster informer support.
Authentication and setup runners
pkg/tnf/auth/runner.go, pkg/tnf/setup/runner.go, pkg/tnf/update-setup/runner.go, pkg/tnf/pkg/pcs/auth.go
Adds missing-node-tolerant configuration lookup, transition-aware setup short-circuiting, retry signaling, single-node authentication, and two-node cluster validation.
Lifecycle and Job controller documentation
docs/tnf/*
Documents bootstrap/runtime modes, controller APIs, retry and drift semantics, active-node selection, status collection, and lifecycle startup.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested reviewers: clobrano, everettraven

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci
openshift-ci Bot requested review from clobrano and everettraven July 21, 2026 17:04

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🧹 Nitpick comments (2)
pkg/tnf/pkg/jobs/lifecycle_test.go (2)

691-691: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Don't ignore the Create return values in the retry-progression setup.

The fake-client Create calls at Lines 691, 700, 710, 714, and 731 discard the returned error. If a Create silently fails, the subsequent syncMultiNodeJobState would find no job (NotFound → no-op), the retry state wouldn't advance, and the test could pass or fail for the wrong reason. Assert require.NoError(...) on each so setup failures surface immediately.

As per path instructions: "Never ignore error returns".

💡 Example fix for one call
-	fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{})
+	_, err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{})
+	require.NoError(t, err)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/tnf/pkg/jobs/lifecycle_test.go` at line 691, Update the retry-progression
test setup to check every fake-client Job Create call with require.NoError,
including the calls near lines 691, 700, 710, 714, and 731. Pass the test
context and each returned error so setup failures stop the test immediately
before syncMultiNodeJobState runs.

Source: Path instructions


211-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

expectControllerRun doesn't affect the assertion — the "skip when already running" case isn't actually verified.

Both branches assert require.True(t, isRunning), so the field never changes what is checked. For "Skip starting controller when already running", runningControllers[key] is pre-seeded to true, so the assertion passes whether or not a duplicate start was skipped. Consider distinguishing an actual skip (e.g., assert no new goroutine/controller was created, or that the controller count didn't change) so the dedup path is meaningfully exercised.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/tnf/pkg/jobs/lifecycle_test.go` around lines 211 - 223, The lifecycle
test’s expectControllerRun branch does not distinguish starting a controller
from skipping an already-running one. Update the verification around
runningControllers and the “Skip starting controller when already running” case
to assert an observable deduplication outcome, such as unchanged
controller/goroutine count, while retaining the running-state assertion for
newly started controllers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/tnf/job-controllers.md`:
- Around line 11-15: Update the Blocked Condition section in job-controllers.md
to remove the exhausted-retries bullet, keeping only the node-not-ready timeout
behavior. Add that exhausted retries set the <JobName>Degraded condition to the
Degraded description, using the existing condition naming and format.

---

Nitpick comments:
In `@pkg/tnf/pkg/jobs/lifecycle_test.go`:
- Line 691: Update the retry-progression test setup to check every fake-client
Job Create call with require.NoError, including the calls near lines 691, 700,
710, 714, and 731. Pass the test context and each returned error so setup
failures stop the test immediately before syncMultiNodeJobState runs.
- Around line 211-223: The lifecycle test’s expectControllerRun branch does not
distinguish starting a controller from skipping an already-running one. Update
the verification around runningControllers and the “Skip starting controller
when already running” case to assert an observable deduplication outcome, such
as unchanged controller/goroutine count, while retaining the running-state
assertion for newly started controllers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0c16170f-d107-4fba-bcb4-ba3d437bdd0c

📥 Commits

Reviewing files that changed from the base of the PR and between 1fe5873 and 708a96d.

📒 Files selected for processing (12)
  • docs/tnf/job-controllers.md
  • pkg/tnf/operator/nodehandler.go
  • pkg/tnf/operator/starter.go
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/jobs/jobcontroller_test.go
  • pkg/tnf/pkg/jobs/lifecycle.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/tools/jobs.go
  • pkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (2)
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/tnf.go

Comment thread docs/tnf/job-controllers.md Outdated

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 2

🧹 Nitpick comments (2)
pkg/tnf/pkg/jobs/lifecycle.go (1)

90-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated blocked-condition tracking logic.

checkNodeReadinessAndSetCondition (single-node path) and the affected-nodes block inside syncMultiNodeJobState (multi-node path) both independently implement "track first-blocked timestamp, set/clear TNF<JobName>Blocked after 10 minutes" against the shared jobBlockedSince map. The logic has already started to diverge (message format differs: [%s] for a single node vs %v for a slice). Extracting a shared helper (e.g. updateBlockedCondition(ctx, jobName, notReadyNodeNames []string, ...)) would remove this duplication and prevent future drift.

Also applies to: 164-231

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/tnf/pkg/jobs/lifecycle.go` around lines 90 - 153, The blocked-condition
tracking is duplicated between checkNodeReadinessAndSetCondition and the
affected-nodes handling in syncMultiNodeJobState. Extract the shared timestamp
tracking and TNF<JobName>Blocked condition set/clear behavior into a helper such
as updateBlockedCondition, pass the relevant not-ready node names, and update
both call sites to use it with consistent message formatting while preserving
existing readiness error behavior.
pkg/operator/ceohelpers/node_helpers.go (1)

52-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse tools.IsNodeReady here. This duplicates the Ready-condition check already in pkg/tnf/pkg/tools/nodes.go and can be replaced directly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/operator/ceohelpers/node_helpers.go` around lines 52 - 62, Update the
Ready-node filtering loop to call the existing tools.IsNodeReady helper for each
node instead of manually iterating node.Status.Conditions and checking condition
type/status. Preserve appending each ready node once and retain the existing
filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 164-231: Prevent the blocked-node path in syncMultiNodeJobState
from reaching configureMultiNodeJob with an unset retryState[jobName].
Initialize the JobRetryState before returning from a blocked sync, or have
configureMultiNodeJob detect a nil state after its syncMultiNodeJobState call
and return without locking it, while preserving the existing job-blocking
behavior.
- Around line 360-403: Update the failed-job handling flow around
IsFailed/IsStopped so the next node and attempt values are calculated without
mutating state, delete the failed job with DeleteAndWait first, and only commit
NodeIndex and AttemptNumber after deletion succeeds. Preserve degraded-condition
updates and retry logging, while ensuring a delete error returns without
advancing retry state.

---

Nitpick comments:
In `@pkg/operator/ceohelpers/node_helpers.go`:
- Around line 52-62: Update the Ready-node filtering loop to call the existing
tools.IsNodeReady helper for each node instead of manually iterating
node.Status.Conditions and checking condition type/status. Preserve appending
each ready node once and retain the existing filtering behavior.

In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 90-153: The blocked-condition tracking is duplicated between
checkNodeReadinessAndSetCondition and the affected-nodes handling in
syncMultiNodeJobState. Extract the shared timestamp tracking and
TNF<JobName>Blocked condition set/clear behavior into a helper such as
updateBlockedCondition, pass the relevant not-ready node names, and update both
call sites to use it with consistent message formatting while preserving
existing readiness error behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 484b8923-171e-4cb0-9423-d32c859e15c7

📥 Commits

Reviewing files that changed from the base of the PR and between 708a96d and 37a5d9f.

📒 Files selected for processing (10)
  • bindata/tnfdeployment/role.yaml
  • docs/tnf/job-controllers.md
  • pkg/operator/ceohelpers/node_helpers.go
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/jobs/lifecycle.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (2)
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go

Comment thread pkg/tnf/pkg/jobs/lifecycle.go
@jaypoulz
jaypoulz force-pushed the jpoulin/commit-1-framework branch from 37a5d9f to b09bb41 Compare July 21, 2026 18:44

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/tnf/pkg/jobs/lifecycle.go (1)

262-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Compare target nodes independent of order. handleFencingSecretChange re-lists control-plane nodes on every sync, GetNodeNames preserves that list order, and slicesEqual treats different permutations as drift. Sorting or set-comparing here avoids deleting/recreating the Job and resetting retries when the API returns the same nodes in a different order.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/tnf/pkg/jobs/lifecycle.go` around lines 262 - 263, Update the target-node
comparison in handleFencingSecretChange so state.TargetNodes and the current
GetNodeNames result are compared independent of ordering, using sorting or set
equality before slicesEqual. Preserve the existing nodesChanged behavior for
actual membership changes and avoid treating permutations as drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/tnf/job-controllers.md`:
- Around line 18-23: Update the Degraded Condition documentation in
docs/tnf/job-controllers.md to clarify that retry cycles continue after the
retry budget is exhausted: describe the limit as 6 tries per cycle, with cycles
continuing until recovery. Remove “permanent failure” and any wording implying
Degraded is terminal, while retaining the existing condition name, reason, and
active-until-success behavior.

---

Nitpick comments:
In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 262-263: Update the target-node comparison in
handleFencingSecretChange so state.TargetNodes and the current GetNodeNames
result are compared independent of ordering, using sorting or set equality
before slicesEqual. Preserve the existing nodesChanged behavior for actual
membership changes and avoid treating permutations as drift.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9b247758-ff1a-4c98-98c7-ab9a47cd52d2

📥 Commits

Reviewing files that changed from the base of the PR and between 37a5d9f and b09bb41.

📒 Files selected for processing (12)
  • docs/tnf/job-controllers.md
  • pkg/tnf/operator/nodehandler.go
  • pkg/tnf/operator/starter.go
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/jobs/jobcontroller_test.go
  • pkg/tnf/pkg/jobs/lifecycle.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/tools/jobs.go
  • pkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (2)
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/tnf.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go

Comment thread docs/tnf/job-controllers.md Outdated
@jaypoulz
jaypoulz force-pushed the jpoulin/commit-1-framework branch from b09bb41 to c3f8662 Compare July 21, 2026 19:14

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 2

♻️ Duplicate comments (1)
pkg/tnf/pkg/jobs/lifecycle.go (1)

336-391: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Retry state is still advanced before the failed job's deletion is confirmed.

state.NodeIndex/state.AttemptNumber are mutated at Lines 379-380 before DeleteAndWait runs at Lines 383-386. If deletion fails, the error is returned but the state mutation already happened; the next sync re-detects the same still-existing failed Job and advances the index/attempt again, skipping a node/attempt due to a transient delete error. This is the same issue flagged in a prior review on this segment and does not appear to have been addressed yet (unlike the sibling nil-pointer fix, which was).

🐛 Proposed fix: only commit state after a successful delete
-		// Apply state changes
-		state.NodeIndex = nextNodeIndex
-		state.AttemptNumber = nextAttemptNumber
-
 		// Delete the failed job so it can be recreated on next node
 		klog.Infof("Deleting failed job %s to retry on node index %d", jobName, nextNodeIndex)
 		if err := DeleteAndWait(ctx, kubeClient, jobName, operatorclient.TargetNamespace); err != nil {
 			return fmt.Errorf("failed to delete failed job: %w", err)
 		}
+
+		// Apply state changes only after successful deletion, so a delete
+		// failure doesn't cause a node/attempt to be skipped on retry.
+		state.NodeIndex = nextNodeIndex
+		state.AttemptNumber = nextAttemptNumber
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/tnf/pkg/jobs/lifecycle.go` around lines 336 - 391, Move the state
assignments in the failed-job retry flow so state.NodeIndex and
state.AttemptNumber are committed only after DeleteAndWait succeeds. Keep the
calculated nextNodeIndex and nextAttemptNumber as local values during deletion,
and return the deletion error without mutating retry state if deletion fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/tnf/operator/starter.go`:
- Around line 429-434: Update schedulableNodesFunc to exclude control-plane
nodes carrying the arbiter:NoSchedule taint before returning the node slice.
Preserve the existing control-plane label selection and ensure only nodes
eligible for scheduling are available to the lifecycle’s NodeName assignment.

In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 3-25: Add the standard library "slices" import to the import block
in lifecycle.go so the slicesEqual function can resolve its calls to slices.Sort
and compile successfully.

---

Duplicate comments:
In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 336-391: Move the state assignments in the failed-job retry flow
so state.NodeIndex and state.AttemptNumber are committed only after
DeleteAndWait succeeds. Keep the calculated nextNodeIndex and nextAttemptNumber
as local values during deletion, and return the deletion error without mutating
retry state if deletion fails.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bd8a999a-cd72-4a2e-9365-f75ad6b94e7d

📥 Commits

Reviewing files that changed from the base of the PR and between b09bb41 and c3f8662.

📒 Files selected for processing (12)
  • docs/tnf/job-controllers.md
  • pkg/tnf/operator/nodehandler.go
  • pkg/tnf/operator/starter.go
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/jobs/jobcontroller_test.go
  • pkg/tnf/pkg/jobs/lifecycle.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/tools/jobs.go
  • pkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (2)
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/tnf.go
🚧 Files skipped from review as they are similar to previous changes (7)
  • pkg/tnf/pkg/tools/nodes.go
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/tools/jobs.go
  • pkg/tnf/operator/nodehandler.go
  • pkg/tnf/pkg/jobs/jobcontroller_test.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go

Comment thread pkg/tnf/operator/starter.go Outdated
Comment thread pkg/tnf/pkg/jobs/lifecycle.go
@jaypoulz
jaypoulz force-pushed the jpoulin/commit-1-framework branch from c3f8662 to d052f7b Compare July 21, 2026 19:51
Comment thread pkg/tnf/update-setup/runner.go Outdated
if err != nil {
klog.Infof("Cluster not running (err: %v), skipping update-setup on this node", err)
return nil
return fmt.Errorf("pacemaker is not running on this node %s: %w", currentNodeName, err)

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.

This is necessary because we need the update-setup job to round-robin now.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/tnf/pkg/jobs/lifecycle_test.go`:
- Around line 72-148: Reconcile the node field naming and types in both
table-driven suites in pkg/tnf/pkg/jobs/lifecycle_test.go:72-148 and
pkg/tnf/pkg/jobs/lifecycle_test.go:237-254. In TestRunTNFJobController, make
struct literals match the declared field and existing tt.node reads. In the
restart/run test table, unify the declaration, literals, and loop reads with the
node parameter type expected by RestartNodeJobOrRunController and
RunNodeJobController, passing the correct node value or pointer.
- Around line 747-760: Remove the orphaned duplicate import block appearing
after TestSyncMultiNodeJobState_RetryProgression; retain only the valid
top-of-file imports and leave the test implementation unchanged.

In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 284-312: In the nodesChanged/configChanged branch of the lifecycle
logic, defer updating state.AttemptNumber, state.NodeIndex, state.TargetNodes,
and state.LastJobConfig until after the existing job lookup and any
DeleteAndWait call complete successfully. Preserve the current error returns for
lookup or deletion failures so retries still see the original state; commit the
new retry and configuration state immediately before returning nil on success.
- Around line 644-705: Fix the undefined nodeTarget reference in
RestartNodeJobOrRunController by passing the existing node parameter to
RunNodeJobController. Preserve the surrounding controller invocation and restart
handling unchanged.
- Around line 519-543: The job hook’s nil return for an unready node does not
prevent syncManaged from reaching ApplyJob. Update the syncManaged flow around
the hook execution to explicitly stop and skip ApplyJob when admission is
blocked, while preserving normal application for ready jobs and propagating
actual hook errors.

In `@pkg/tnf/pkg/tools/nodes.go`:
- Around line 54-66: The HasArbiterNoScheduleTaint function uses the wrong taint
key. Update its taint check to match OpenShift’s node-role.kubernetes.io/arbiter
key while preserving the NoSchedule effect and existing nil-node behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0f079ba3-fae7-48be-8b8c-0407a925d03f

📥 Commits

Reviewing files that changed from the base of the PR and between c3f8662 and d052f7b.

📒 Files selected for processing (13)
  • docs/tnf/job-controllers.md
  • pkg/tnf/operator/nodehandler.go
  • pkg/tnf/operator/starter.go
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/pkg/jobs/jobcontroller_test.go
  • pkg/tnf/pkg/jobs/lifecycle.go
  • pkg/tnf/pkg/jobs/lifecycle_test.go
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/tools/jobs.go
  • pkg/tnf/pkg/tools/nodes.go
  • pkg/tnf/update-setup/runner.go
💤 Files with no reviewable changes (2)
  • pkg/tnf/pkg/jobs/tnf.go
  • pkg/tnf/pkg/jobs/tnf_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
  • pkg/tnf/pkg/tools/jobs.go
  • docs/tnf/job-controllers.md
  • pkg/tnf/pkg/jobs/jobcontroller.go
  • pkg/tnf/operator/starter.go
  • pkg/tnf/pkg/jobs/utils.go
  • pkg/tnf/pkg/jobs/jobcontroller_test.go
  • pkg/tnf/operator/nodehandler.go

Comment thread pkg/tnf/pkg/jobs/lifecycle_test.go Outdated
Comment thread pkg/tnf/pkg/jobs/lifecycle_test.go Outdated
Comment thread pkg/tnf/pkg/jobs/lifecycle.go
Comment thread pkg/tnf/pkg/jobs/lifecycle.go
Comment thread pkg/tnf/pkg/jobs/lifecycle.go Outdated
Comment thread pkg/tnf/pkg/tools/nodes.go Outdated
@jaypoulz
jaypoulz force-pushed the jpoulin/commit-1-framework branch 10 times, most recently from d2ae9a8 to 034cd7d Compare July 22, 2026 16:53
@jaypoulz

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@jaypoulz

Copy link
Copy Markdown
Contributor Author

/unverify

@jaypoulz

Copy link
Copy Markdown
Contributor Author

I want to test upgrade before we admit this

@jaypoulz

Copy link
Copy Markdown
Contributor Author

/verified remove

@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Jul 30, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@jaypoulz: The verified label has been removed.

Details

In response to this:

/verified remove

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@jaypoulz

Copy link
Copy Markdown
Contributor Author

/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-upgrade

@openshift-ci

openshift-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

@jaypoulz: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-upgrade

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0d4d0e80-8c42-11f1-9e35-6d177b2c2379-0

@jaypoulz

Copy link
Copy Markdown
Contributor Author

/verified by #1655 (comment) (node replacement, recovery test stability), #1655 (comment) for e2e stability, and #1655 (comment) for upgrade stability

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jul 31, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@jaypoulz: This PR has been marked as verified by https://github.com/openshift/cluster-etcd-operator/pull/1655#issuecomment-5123653191 (node replacement,recovery test stability),https://github.com/openshift/cluster-etcd-operator/pull/1655#issuecomment-5123198799 for e2e stability,and https://github.com/openshift/cluster-etcd-operator/pull/1655#issuecomment-5134574809 for upgrade stability.

Details

In response to this:

/verified by #1655 (comment) (node replacement, recovery test stability), #1655 (comment) for e2e stability, and #1655 (comment) for upgrade stability

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@fonta-rh

Copy link
Copy Markdown
Contributor

/approve

@fonta-rh

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-agnostic-ovn
/test e2e-agnostic-ovn-upgrade
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-ovn-single-node
/test e2e-gcp-operator
/test e2e-gcp-operator-disruptive
/test e2e-metal-ipi-ovn-ipv6
/test e2e-operator

@dusk125

dusk125 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

/approve

@openshift-ci

openshift-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dusk125, fonta-rh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2026
@jaypoulz

Copy link
Copy Markdown
Contributor Author

/unhold

@jaypoulz

Copy link
Copy Markdown
Contributor Author

/cherrypick openshift-4.22

@openshift-cherrypick-robot

Copy link
Copy Markdown

@jaypoulz: once the present PR merges, I will cherry-pick it on top of openshift-4.22 in a new PR and assign it to you.

Details

In response to this:

/cherrypick openshift-4.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@jaypoulz

Copy link
Copy Markdown
Contributor Author

/cherry-pick release-4.22

@openshift-cherrypick-robot

Copy link
Copy Markdown

@jaypoulz: once the present PR merges, I will cherry-pick it on top of release-4.22 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

/retest-required

Remaining retests: 0 against base HEAD ccd95c6 and 2 for PR HEAD e1982aa in total

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

/retest-required

Remaining retests: 0 against base HEAD 10741fc and 1 for PR HEAD e1982aa in total

@fonta-rh

fonta-rh commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

/test e2e-aws-ovn-serial-2of2

@openshift-ci

openshift-ci Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@jaypoulz: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-ci-robot

Copy link
Copy Markdown

@jaypoulz: Jira Issue OCPBUGS-84695: Some pull requests linked via external trackers have merged:

The following pull request, linked via external tracker, has not merged:

All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-84695 has not been moved to the MODIFIED state.

This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload.

Details

In response to this:

Implements job controller framework for Two-Node Fencing (TNF) cluster management:

Job Controller Framework (pkg/tnf/pkg/jobs/):

  • Multi-node job pattern with round-robin retry across schedulable nodes
  • Single-node job pattern for node-specific operations
  • Drift detection: resets retry state when node list or config changes
  • Blocked condition when nodes not ready (10-minute timeout)
  • Degraded condition when retry budget exhausted (retries continue)
  • JobRetryState tracks AttemptNumber, NodeIndex, TargetNodes
  • RunNodeJobController/RunClusterJobController entry points

Lifecycle Manager (pkg/tnf/operator/lifecycle_manager.go):

  • PacemakerLifecycleManager: owns all TNF job lifecycle logic
  • Event handlers: node Ready transitions, fencing secret changes
  • Bootstrap mode: exponential backoff retry, requires 2 ready nodes
  • Runtime mode: idempotent restart, handles 1-2 nodes
  • Node selection: K8s ∩ Pacemaker intersection with fallback

Job Implementations:

  • auth: sets hacluster password (per-node, prerequisite for Pacemaker)
  • setup: creates Pacemaker cluster (cluster-wide, bootstrap only)
  • after-setup: disables kubelet service (per-node, Pacemaker owns it)
  • fencing: configures STONITH (cluster-wide, secret-driven restart)

Status Collector (pkg/tnf/operator/status_collector.go):

  • CronJob running every minute on active Pacemaker nodes
  • Sticky-on-success, rotate-on-failure strategy
  • Resets to first node when node list changes

Node Management (pkg/tnf/operator/helpers.go):

  • getActivePacemakerNodes(): returns K8s ∩ Pacemaker intersection
  • Staleness detection: 5-minute threshold for PacemakerCluster CR
  • Sorted output for deterministic round-robin behavior

Documentation:

  • docs/tnf/job-controllers.md: multi-node retry, drift detection, condition model
  • docs/tnf/lifecycle-manager.md: startup sequence, node selection, status collector

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

Summary by CodeRabbit

  • New Features

  • Added TNF Job Controllers documentation and a bootstrap/runtime lifecycle for Pacemaker-related Jobs, including multi-node round-robin retries and drift-aware progression.

  • Introduced a Pacemaker lifecycle manager to coordinate Job controller startup and start an idempotent status-collector CronJob.

  • Documentation

  • Documented the TNF Job Controllers framework and the Pacemaker lifecycle manager startup sequence.

  • Bug Fixes

  • Added automatic recovery for stalled Jobs, improved degraded-condition gating, reduced unnecessary Job recreations for retry-only drift, and strengthened update-setup completion checks (requires exactly two nodes online).

  • Prevented overlapping status-collector runs.

  • Tests

  • Expanded coverage for lifecycle state, controller startup branching, drift/degraded behavior, and status-collector node rotation.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-cherrypick-robot

Copy link
Copy Markdown

@jaypoulz: #1655 failed to apply on top of branch "release-4.22":

Applying: OCPBUGS-84695: TNF job controller framework and lifecycle management
.git/rebase-apply/patch:89: trailing whitespace.
- **Requirements:** 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	pkg/tnf/auth/runner.go
M	pkg/tnf/operator/starter.go
M	pkg/tnf/pkg/pacemaker/healthcheck.go
M	pkg/tnf/setup/runner.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/tnf/auth/runner.go
Auto-merging pkg/tnf/operator/starter.go
CONFLICT (content): Merge conflict in pkg/tnf/operator/starter.go
Auto-merging pkg/tnf/pkg/pacemaker/healthcheck.go
CONFLICT (content): Merge conflict in pkg/tnf/pkg/pacemaker/healthcheck.go
Auto-merging pkg/tnf/setup/runner.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 OCPBUGS-84695: TNF job controller framework and lifecycle management

Details

In response to this:

/cherry-pick release-4.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot

Copy link
Copy Markdown

@jaypoulz: cannot checkout openshift-4.22: error checking out "openshift-4.22": exit status 1 error: pathspec 'openshift-4.22' did not match any file(s) known to git

Details

In response to this:

/cherrypick openshift-4.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants