OCPBUGS-84695: feat(tnf): TNF job controller framework and lifecycle management#1655
OCPBUGS-84695: feat(tnf): TNF job controller framework and lifecycle management#1655jaypoulz wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@jaypoulz: This pull request references Jira Issue OCPBUGS-84695, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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
added
the
jira/valid-bug
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Pacemaker lifecycle orchestration, active-node targeting, status-collector rotation, node and cluster TNF Job controllers, retry/restart handling, REST compatibility helpers, authentication updates, tests, and lifecycle documentation. ChangesTNF lifecycle and Job execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Starter
participant LifecycleManager
participant JobControllers
participant Kubernetes
Starter->>LifecycleManager: initialize after PacemakerCluster CRD establishment
LifecycleManager->>Kubernetes: observe nodes and PacemakerCluster state
LifecycleManager->>JobControllers: start bootstrap or runtime controllers
JobControllers->>Kubernetes: create, retry, delete, and pin Jobs
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/tnf/pkg/jobs/lifecycle_test.go (2)
691-691: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDon't ignore the
Createreturn values in the retry-progression setup.The fake-client
Createcalls at Lines 691, 700, 710, 714, and 731 discard the returned error. If aCreatesilently fails, the subsequentsyncMultiNodeJobStatewould find no job (NotFound → no-op), the retry state wouldn't advance, and the test could pass or fail for the wrong reason. Assertrequire.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
expectControllerRundoesn'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 totrue, 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
📒 Files selected for processing (12)
docs/tnf/job-controllers.mdpkg/tnf/operator/nodehandler.gopkg/tnf/operator/starter.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/tools/jobs.gopkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (2)
- pkg/tnf/pkg/jobs/tnf_test.go
- pkg/tnf/pkg/jobs/tnf.go
708a96d to
37a5d9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/tnf/pkg/jobs/lifecycle.go (1)
90-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated blocked-condition tracking logic.
checkNodeReadinessAndSetCondition(single-node path) and the affected-nodes block insidesyncMultiNodeJobState(multi-node path) both independently implement "track first-blocked timestamp, set/clearTNF<JobName>Blockedafter 10 minutes" against the sharedjobBlockedSincemap. The logic has already started to diverge (message format differs:[%s]for a single node vs%vfor 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 winReuse
tools.IsNodeReadyhere. This duplicates the Ready-condition check already inpkg/tnf/pkg/tools/nodes.goand 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
📒 Files selected for processing (10)
bindata/tnfdeployment/role.yamldocs/tnf/job-controllers.mdpkg/operator/ceohelpers/node_helpers.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/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
37a5d9f to
b09bb41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tnf/pkg/jobs/lifecycle.go (1)
262-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare target nodes independent of order.
handleFencingSecretChangere-lists control-plane nodes on every sync,GetNodeNamespreserves that list order, andslicesEqualtreats 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
📒 Files selected for processing (12)
docs/tnf/job-controllers.mdpkg/tnf/operator/nodehandler.gopkg/tnf/operator/starter.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/tools/jobs.gopkg/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
b09bb41 to
c3f8662
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/tnf/pkg/jobs/lifecycle.go (1)
336-391: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRetry state is still advanced before the failed job's deletion is confirmed.
state.NodeIndex/state.AttemptNumberare mutated at Lines 379-380 beforeDeleteAndWaitruns 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
📒 Files selected for processing (12)
docs/tnf/job-controllers.mdpkg/tnf/operator/nodehandler.gopkg/tnf/operator/starter.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/tools/jobs.gopkg/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
c3f8662 to
d052f7b
Compare
| 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) |
There was a problem hiding this comment.
This is necessary because we need the update-setup job to round-robin now.
There was a problem hiding this comment.
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
📒 Files selected for processing (13)
docs/tnf/job-controllers.mdpkg/tnf/operator/nodehandler.gopkg/tnf/operator/starter.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/tools/jobs.gopkg/tnf/pkg/tools/nodes.gopkg/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
d2ae9a8 to
034cd7d
Compare
cdeeae6 to
adc8adc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tnf/operator/starter.go (1)
137-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNested polling for CRD establishment works but is convoluted.
The inner
wait.PollUntilContextTimeout(2s interval, 1 minute deadline) is wrapped in an outerforloop that re-polls every 30s on timeout. This is correct but adds an extra layer of indirection versus a singlewait.PollUntilContextCancelwith a fixed interval, which would achieve the same "poll indefinitely until CRD established or ctx cancelled" behavior with less code.♻️ Simplification using a single infinite poll
- for { - err = wait.PollUntilContextTimeout(ctx, 2*time.Second, time.Minute, true, func(ctx context.Context) (bool, error) { - crd, getErr := apiextClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "pacemakerclusters.etcd.openshift.io", metav1.GetOptions{}) - if getErr != nil { - klog.V(2).Infof("waiting for PacemakerCluster CRD: %v", getErr) - return false, nil - } - for _, cond := range crd.Status.Conditions { - if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue { - return true, nil - } - } - klog.V(2).Infof("PacemakerCluster CRD not yet established") - return false, nil - }) - if err == nil { - break - } - if ctx.Err() != nil { - klog.Infof("context done while waiting for PacemakerCluster CRD: %v", err) - return - } - klog.Warningf("PacemakerCluster CRD not established yet, will retry in 30s: %v", err) - select { - case <-time.After(30 * time.Second): - continue - case <-ctx.Done(): - return - } - } + err = wait.PollUntilContextCancel(ctx, 5*time.Second, true, func(ctx context.Context) (bool, error) { + crd, getErr := apiextClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "pacemakerclusters.etcd.openshift.io", metav1.GetOptions{}) + if getErr != nil { + klog.V(2).Infof("waiting for PacemakerCluster CRD: %v", getErr) + return false, nil + } + for _, cond := range crd.Status.Conditions { + if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue { + return true, nil + } + } + klog.V(2).Infof("PacemakerCluster CRD not yet established") + return false, nil + }) + if err != nil { + klog.Infof("context done while waiting for PacemakerCluster CRD: %v", err) + return + }🤖 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/operator/starter.go` around lines 137 - 182, Replace the nested retry structure in runPacemakerControllers with a single wait.PollUntilContextCancel that polls for the PacemakerCluster CRD at a fixed interval until it is established or ctx is cancelled. Preserve the existing CRD lookup, established-condition check, waiting logs, and cancellation handling, while removing the outer for loop and timeout-based retry logic.
🤖 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 381-427: Explicitly delete the failed or stopped existingJob in
the IsFailed/IsStopped retry branch before committing the updated
state.NodeIndex and state.AttemptNumber. Use the existing Kubernetes job
deletion mechanism and handle/report deletion errors consistently, so ApplyJob
can recreate the job even when the next node index resolves to the same node.
Keep the existing retry calculation and degraded-condition behavior unchanged.
---
Nitpick comments:
In `@pkg/tnf/operator/starter.go`:
- Around line 137-182: Replace the nested retry structure in
runPacemakerControllers with a single wait.PollUntilContextCancel that polls for
the PacemakerCluster CRD at a fixed interval until it is established or ctx is
cancelled. Preserve the existing CRD lookup, established-condition check,
waiting logs, and cancellation handling, while removing the outer for loop and
timeout-based retry logic.
🪄 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: fb03a9ab-31d9-4a07-b79a-fe57ae269681
📒 Files selected for processing (29)
docs/tnf/job-controllers.mddocs/tnf/lifecycle-manager.mdpkg/tnf/auth/runner.gopkg/tnf/operator/helpers.gopkg/tnf/operator/helpers_test.gopkg/tnf/operator/job_controllers.gopkg/tnf/operator/job_controllers_test.gopkg/tnf/operator/lifecycle_manager.gopkg/tnf/operator/nodehandler.gopkg/tnf/operator/nodehandler_test.gopkg/tnf/operator/starter.gopkg/tnf/operator/starter_test.gopkg/tnf/operator/status_collector.gopkg/tnf/operator/status_collector_test.gopkg/tnf/pkg/jobs/batch.gopkg/tnf/pkg/jobs/batch_test.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/pacemaker/healthcheck.gopkg/tnf/pkg/pacemaker/helpers.gopkg/tnf/pkg/pacemaker/statuscollector.gopkg/tnf/pkg/pcs/auth.gopkg/tnf/pkg/tools/jobs.gopkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (5)
- pkg/tnf/operator/nodehandler.go
- pkg/tnf/pkg/jobs/tnf_test.go
- pkg/tnf/operator/nodehandler_test.go
- pkg/tnf/operator/starter_test.go
- pkg/tnf/pkg/jobs/tnf.go
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/tnf/pkg/pacemaker/statuscollector.go
- pkg/tnf/auth/runner.go
- pkg/tnf/pkg/tools/jobs.go
- pkg/tnf/operator/job_controllers_test.go
- pkg/tnf/operator/helpers_test.go
- pkg/tnf/operator/status_collector.go
- pkg/tnf/pkg/pacemaker/healthcheck.go
- pkg/tnf/operator/status_collector_test.go
- pkg/tnf/pkg/jobs/jobcontroller_test.go
- pkg/tnf/operator/helpers.go
- pkg/tnf/pkg/jobs/lifecycle_test.go
- pkg/tnf/pkg/pacemaker/helpers.go
- pkg/tnf/pkg/jobs/jobcontroller.go
- pkg/tnf/operator/lifecycle_manager.go
- pkg/tnf/pkg/tools/nodes.go
- docs/tnf/job-controllers.md
- pkg/tnf/operator/job_controllers.go
80043e1 to
9b3480a
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-recovery |
|
@jaypoulz: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c3682090-86e5-11f1-8665-c2551a5d1a75-0 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tnf/pkg/jobs/lifecycle.go (1)
482-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared controller-startup boilerplate.
RunNodeJobControllerandRunClusterJobControllerduplicate the duplicate-start guard,NewJobControllerconstruction shape, and goroutine wrapper with cleanup — differing only in the hook body and log wording. The restart-side equivalent (restartJobOrRunController) already extracts this kind of shared logic; doing the same here would reduce upkeep risk when either startup path needs a fix.🤖 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 482 - 613, Extract the shared startup and cleanup logic from RunNodeJobController and RunClusterJobController into a reusable helper that accepts the job name, log context, and hook functions while retaining the existing NewJobController dependencies. Move the duplicate-start guard, controller launch, and runningControllers cleanup into that helper, then have both functions supply only their job-specific hook configuration and messages.
🤖 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.
Nitpick comments:
In `@pkg/tnf/pkg/jobs/lifecycle.go`:
- Around line 482-613: Extract the shared startup and cleanup logic from
RunNodeJobController and RunClusterJobController into a reusable helper that
accepts the job name, log context, and hook functions while retaining the
existing NewJobController dependencies. Move the duplicate-start guard,
controller launch, and runningControllers cleanup into that helper, then have
both functions supply only their job-specific hook configuration and messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4bfe652-6e4c-4242-86bd-313e09e23292
📒 Files selected for processing (29)
docs/tnf/job-controllers.mddocs/tnf/lifecycle-manager.mdpkg/tnf/auth/runner.gopkg/tnf/operator/helpers.gopkg/tnf/operator/helpers_test.gopkg/tnf/operator/job_controllers.gopkg/tnf/operator/job_controllers_test.gopkg/tnf/operator/lifecycle_manager.gopkg/tnf/operator/nodehandler.gopkg/tnf/operator/nodehandler_test.gopkg/tnf/operator/starter.gopkg/tnf/operator/starter_test.gopkg/tnf/operator/status_collector.gopkg/tnf/operator/status_collector_test.gopkg/tnf/pkg/jobs/batch.gopkg/tnf/pkg/jobs/batch_test.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/pacemaker/healthcheck.gopkg/tnf/pkg/pacemaker/helpers.gopkg/tnf/pkg/pacemaker/statuscollector.gopkg/tnf/pkg/pcs/auth.gopkg/tnf/pkg/tools/jobs.gopkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (5)
- pkg/tnf/operator/nodehandler_test.go
- pkg/tnf/pkg/jobs/tnf.go
- pkg/tnf/operator/nodehandler.go
- pkg/tnf/operator/starter_test.go
- pkg/tnf/pkg/jobs/tnf_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- pkg/tnf/pkg/pacemaker/statuscollector.go
- pkg/tnf/auth/runner.go
- pkg/tnf/pkg/tools/jobs.go
- pkg/tnf/pkg/pacemaker/healthcheck.go
- pkg/tnf/operator/lifecycle_manager.go
- pkg/tnf/pkg/jobs/batch.go
- pkg/tnf/pkg/pacemaker/helpers.go
- pkg/tnf/operator/helpers_test.go
- pkg/tnf/pkg/jobs/batch_test.go
- pkg/tnf/pkg/jobs/utils.go
- pkg/tnf/operator/job_controllers_test.go
- pkg/tnf/operator/status_collector_test.go
- pkg/tnf/pkg/jobs/jobcontroller.go
- pkg/tnf/operator/helpers.go
- pkg/tnf/operator/status_collector.go
- pkg/tnf/pkg/jobs/jobcontroller_test.go
- pkg/tnf/pkg/tools/nodes.go
- pkg/tnf/pkg/jobs/lifecycle_test.go
- docs/tnf/job-controllers.md
- pkg/tnf/operator/job_controllers.go
- pkg/tnf/operator/starter.go
a3fbf23 to
fea88ae
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-recovery |
|
@jaypoulz: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/caf054d0-86fa-11f1-923d-dcd7b9b4794a-0 |
fea88ae to
c674b1d
Compare
|
@jaypoulz: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
c674b1d to
2e5d7e5
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-recovery |
|
@jaypoulz: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/49fe52f0-879f-11f1-8862-193f43b1c36e-0 |
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
- Degraded condition for both blocked jobs (>10min timeout) and failed jobs
- Reason field distinguishes: SyncError (blocked) vs MaxRetriesExceeded (failed)
- Blocked jobs visible at ClusterOperator level (propagate via Degraded)
- JobRetryState tracks AttemptNumber, NodeIndex, TargetNodes
- RunNodeJobController/RunClusterJobController entry points
- ApplyJob drift handling: ignores retry field changes for non-failed cluster jobs (prevents recreation on operator restart)
- PascalCase condition names: TNF{Job}{Condition} format (e.g., TNFSetupJobAvailable)
- Legacy condition cleanup on upgrade: removes old tnf-xxx-job{Condition} format
- Log verbosity: steady-state checks use V(4), state changes use Info
**Lifecycle Manager (pkg/tnf/operator/lifecycle_manager.go):**
- PacemakerLifecycleManager: owns all TNF job lifecycle logic
- Event handlers: node Ready transitions (triggers bootstrap)
- Bootstrap mode: exponential backoff retry, requires 2 ready nodes
- Runtime mode: idempotent restart, handles 1-2 nodes
- Node selection: K8s ∩ Pacemaker intersection with fallback
- Fixed closure capture bugs: all affected nodes functions query informers dynamically (prevents false Degraded during node replacement)
- Fixed lifecycle context: event handler goroutines now use controller context (receive cancellation on shutdown)
**Job Implementations:**
- auth: sets hacluster password (per-node, prerequisite for Pacemaker)
- setup: creates Pacemaker cluster (one-time job, controller runs in both bootstrap and runtime modes to maintain conditions)
- after-setup: disables kubelet service (per-node, Pacemaker owns it)
- fencing: configures STONITH (cluster-wide, drift detection via ResourceVersion)
- update-setup: handles node replacements (cluster-wide, removes offline node, adds new node, updates IPs)
**Drift Detection:**
- Secret changes detected via ResourceVersion tracking in jobConfigFunc
- Syncs every minute to detect secret updates, deletions, and additions
- Eliminates need for separate secret event handlers
- Triggers job restart when fencing secret ResourceVersion changes
**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
- Log verbosity: steady-state pinning uses V(4), state changes use Info
**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>
2e5d7e5 to
d1e0b65
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-recovery |
|
@jaypoulz: trigger 8 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6ffbe9d0-87a5-11f1-96d0-e02ea1efaa82-0 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/tnf/pkg/jobs/batch.go (2)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment narrower than the implemented behavior. The retry-field exemption applies to any non-failed cluster job (Line 87
!IsFailed(*existing)), including running ones, not just completed ones.✏️ Suggested wording
-// spec, the existing job will be deleted and recreated. For completed cluster jobs +// spec, the existing job will be deleted and recreated. For non-failed cluster jobs // (labeled with job-type=cluster), drift in retry-related fields (NodeName, node-index, // attempt labels) is ignored to prevent recreation on operator restart when retry state // is reinitialized.🤖 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/batch.go` around lines 52 - 56, Update the doc comment describing retry-related drift in cluster jobs to state that the exemption applies to all non-failed cluster jobs, including running jobs, rather than only completed jobs. Keep the implementation condition around !IsFailed unchanged.
86-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the TNF label keys into shared constants.
tnf.etcd.openshift.io/job-type,.../node-index, and.../attemptare hard-coded here and again at Lines 130-133, and the writer side inlifecycle.gomust use the identical strings for this drift check to work. A typo on either side silently disables retry-drift detection rather than failing to compile.Based on learnings that
configureMultiNodeJobmust writestate.AttemptNumber/state.NodeIndexintotnf.etcd.openshift.io/attemptandtnf.etcd.openshift.io/node-indexand thatApplyJobmust key off those same labels, centralizing the keys protects that contract.#!/bin/bash # Check whether constants for these label keys already exist, and where the keys are written/read. rg -n --type=go 'tnf\.etcd\.openshift\.io/(job-type|node-index|attempt)' -g '!vendor/**'🤖 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/batch.go` around lines 86 - 97, Define shared constants for the TNF job-type, node-index, and attempt label keys, then replace every hard-coded occurrence in the ApplyJob drift checks and related code with those constants. Update the writer path in configureMultiNodeJob and lifecycle.go to use the same constants while preserving the existing label values and retry-drift behavior.Source: Learnings
pkg/tnf/pkg/jobs/lifecycle_test.go (3)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoverage summary contradicts the assertion. Line 20 says "state reset" on success, but Step 5 asserts the state is preserved (Line 377). Align the summary with the actual contract.
🤖 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 20, Update the lifecycle test coverage summary entry for “Job succeeds” to state that the state is preserved, matching the assertion in Step 5 and the established contract; retain the “degraded cleared” portion unchanged.
71-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
expectJobDeletedandexpectWaitForStoppedare never asserted. Both fields are declared and populated in every case but no verification reads them, so the "deletes and runs controller" case only actually proves the controller started. Either assert the job is gone (Getreturns NotFound) and that the wait path executed, or drop the fields.🤖 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 71 - 72, Use expectJobDeleted and expectWaitForStopped in the lifecycle test assertions: verify the job Get operation returns NotFound when expectJobDeleted is true, and verify the stopped-job wait path executed when expectWaitForStopped is true. If these expectations cannot be asserted, remove both fields and their case values instead of leaving them unused.
667-676: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVacuous assertion when the condition is missing. If no condition matches
expectedConditionType, the loop body never runs and the test passes without verifying anything. Track afoundflag and require it (same pattern at Lines 792-801).💚 Proposed hardening
if !tt.expectError && tt.blockedDuration > 0 { _, status, _, _ := fakeOperatorClient.GetStaticPodOperatorState() expectedConditionType := tools.ToPascalCase(jobName) + "Degraded" + found := false for _, cond := range status.Conditions { if cond.Type == expectedConditionType { + found = true require.Equal(t, operatorv1.ConditionFalse, cond.Status, "Degraded should be cleared when unblocked") break } } + require.True(t, found, "expected condition %s to be present", expectedConditionType) }🤖 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 667 - 676, Harden the condition check in the blocked-duration validation by adding a found flag alongside the loop over status.Conditions, setting it when expectedConditionType matches, and requiring it after the loop before or alongside the existing ConditionFalse assertion. Follow the established found-condition pattern used around the referenced later test section.pkg/tnf/operator/status_collector_test.go (2)
84-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMost-recent-job selection is untested. Every case creates a single Job, so the
CreationTimestampcomparison loop incheckLastJobFailed(pkg/tnf/operator/status_collector.goLines 141-147) never gets exercised — an inverted comparison there would go unnoticed. Add a case with an older failed Job plus a newer completed Job (and the reverse).🤖 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/operator/status_collector_test.go` around lines 84 - 105, Extend the tests around checkLastJobFailed to create multiple Jobs with distinct CreationTimestamp values, covering both an older failed Job followed by a newer completed Job and an older completed Job followed by a newer failed Job. Assert that the result is determined by the most recently created Job, exercising the timestamp comparison loop rather than the single-job path.
201-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test re-implements the rotation logic instead of exercising it. Lines 201-226 are a copy of the reset/rotate block inside
runPacemakerStatusCollectorCronJob(pkg/tnf/operator/status_collector.goLines 82-119), so the test will keep passing if the production block changes or regresses. Extract the decision into a small pure helper and call it from both the CronJob config func and the test.♻️ Sketch
// status_collector.go func nextStatusCollectorNodeIndex(state *jobs.JobRetryState, currentNodeNames []string, lastJobFailed bool) int { state.Mu.Lock() defer state.Mu.Unlock() if !tools.StringSlicesEqual(state.TargetNodes, currentNodeNames) { state.NodeIndex = 0 state.TargetNodes = currentNodeNames } if lastJobFailed && len(currentNodeNames) > 0 { state.NodeIndex = (state.NodeIndex + 1) % len(currentNodeNames) } return state.NodeIndex }🤖 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/operator/status_collector_test.go` around lines 201 - 226, Extract the reset-and-rotation decision from runPacemakerStatusCollectorCronJob into a pure helper such as nextStatusCollectorNodeIndex that updates JobRetryState, handles changed or empty node lists safely, and returns the selected index. Replace the CronJob’s inline logic and the duplicated block in the test with calls to this helper, while leaving lastJobFailed retrieval and node selection flow unchanged.pkg/tnf/operator/starter.go (1)
146-188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSurface Pacemaker startup failures instead of dropping the goroutine
newPacemakerLifecycleManageronly does local setup here, so if it returns an error the Pacemaker controllers never start and this goroutine just exits after logging. Surface that failure through operator status or fail startup instead of leaving TNF partially enabled.🤖 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/operator/starter.go` around lines 146 - 188, Update the Pacemaker startup flow around newPacemakerLifecycleManager so its initialization error is propagated through operator startup/status rather than only logged before returning from the goroutine. Ensure a failed lifecycle-manager setup causes TNF startup to fail or records an appropriate degraded/error condition, while preserving normal controller startup when initialization succeeds.
🤖 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 148-150: Update the “Create job spec” documentation to describe
the actual single-node pinning mechanism as NodeName/fixed node assignment,
replacing the incorrect nodeSelector wording. Keep the backoffLimit=retries
guidance unchanged.
- Around line 180-188: The execution flow diagram should match the
implementation order in syncMultiNodeJobState: move the affectedNodesFunc
readiness check before the schedulableNodesFunc availability check, preserving
each branch’s existing error, timeout, and Degraded-condition behavior.
In `@pkg/tnf/pkg/jobs/lifecycle_test.go`:
- Around line 322-352: Check every fake-client setup error in
pkg/tnf/pkg/jobs/lifecycle_test.go#L322-L352 by wrapping each Jobs Create/Delete
call with require.NoError and using failedJob.DeepCopy() for recreated jobs. In
pkg/tnf/pkg/jobs/lifecycle_test.go#L625-L625, assert the Nodes Create result
inside the affected-nodes loop. In pkg/tnf/pkg/jobs/lifecycle_test.go#L744-L748,
assert both Nodes Create results for readyNode and the schedulable-node loop, so
syncMultiNodeJobState tests operate on the intended cluster state.
---
Nitpick comments:
In `@pkg/tnf/operator/starter.go`:
- Around line 146-188: Update the Pacemaker startup flow around
newPacemakerLifecycleManager so its initialization error is propagated through
operator startup/status rather than only logged before returning from the
goroutine. Ensure a failed lifecycle-manager setup causes TNF startup to fail or
records an appropriate degraded/error condition, while preserving normal
controller startup when initialization succeeds.
In `@pkg/tnf/operator/status_collector_test.go`:
- Around line 84-105: Extend the tests around checkLastJobFailed to create
multiple Jobs with distinct CreationTimestamp values, covering both an older
failed Job followed by a newer completed Job and an older completed Job followed
by a newer failed Job. Assert that the result is determined by the most recently
created Job, exercising the timestamp comparison loop rather than the single-job
path.
- Around line 201-226: Extract the reset-and-rotation decision from
runPacemakerStatusCollectorCronJob into a pure helper such as
nextStatusCollectorNodeIndex that updates JobRetryState, handles changed or
empty node lists safely, and returns the selected index. Replace the CronJob’s
inline logic and the duplicated block in the test with calls to this helper,
while leaving lastJobFailed retrieval and node selection flow unchanged.
In `@pkg/tnf/pkg/jobs/batch.go`:
- Around line 52-56: Update the doc comment describing retry-related drift in
cluster jobs to state that the exemption applies to all non-failed cluster jobs,
including running jobs, rather than only completed jobs. Keep the implementation
condition around !IsFailed unchanged.
- Around line 86-97: Define shared constants for the TNF job-type, node-index,
and attempt label keys, then replace every hard-coded occurrence in the ApplyJob
drift checks and related code with those constants. Update the writer path in
configureMultiNodeJob and lifecycle.go to use the same constants while
preserving the existing label values and retry-drift behavior.
In `@pkg/tnf/pkg/jobs/lifecycle_test.go`:
- Line 20: Update the lifecycle test coverage summary entry for “Job succeeds”
to state that the state is preserved, matching the assertion in Step 5 and the
established contract; retain the “degraded cleared” portion unchanged.
- Around line 71-72: Use expectJobDeleted and expectWaitForStopped in the
lifecycle test assertions: verify the job Get operation returns NotFound when
expectJobDeleted is true, and verify the stopped-job wait path executed when
expectWaitForStopped is true. If these expectations cannot be asserted, remove
both fields and their case values instead of leaving them unused.
- Around line 667-676: Harden the condition check in the blocked-duration
validation by adding a found flag alongside the loop over status.Conditions,
setting it when expectedConditionType matches, and requiring it after the loop
before or alongside the existing ConditionFalse assertion. Follow the
established found-condition pattern used around the referenced later test
section.
🪄 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: 2cfd3857-d35a-4e45-8be2-f36321446e70
📒 Files selected for processing (29)
docs/tnf/job-controllers.mddocs/tnf/lifecycle-manager.mdpkg/tnf/auth/runner.gopkg/tnf/operator/helpers.gopkg/tnf/operator/helpers_test.gopkg/tnf/operator/job_controllers.gopkg/tnf/operator/job_controllers_test.gopkg/tnf/operator/lifecycle_manager.gopkg/tnf/operator/nodehandler.gopkg/tnf/operator/nodehandler_test.gopkg/tnf/operator/starter.gopkg/tnf/operator/starter_test.gopkg/tnf/operator/status_collector.gopkg/tnf/operator/status_collector_test.gopkg/tnf/pkg/jobs/batch.gopkg/tnf/pkg/jobs/batch_test.gopkg/tnf/pkg/jobs/jobcontroller.gopkg/tnf/pkg/jobs/jobcontroller_test.gopkg/tnf/pkg/jobs/lifecycle.gopkg/tnf/pkg/jobs/lifecycle_test.gopkg/tnf/pkg/jobs/tnf.gopkg/tnf/pkg/jobs/tnf_test.gopkg/tnf/pkg/jobs/utils.gopkg/tnf/pkg/pacemaker/healthcheck.gopkg/tnf/pkg/pacemaker/helpers.gopkg/tnf/pkg/pacemaker/statuscollector.gopkg/tnf/pkg/pcs/auth.gopkg/tnf/pkg/tools/jobs.gopkg/tnf/pkg/tools/nodes.go
💤 Files with no reviewable changes (5)
- pkg/tnf/operator/nodehandler.go
- pkg/tnf/pkg/jobs/tnf_test.go
- pkg/tnf/pkg/jobs/tnf.go
- pkg/tnf/operator/nodehandler_test.go
- pkg/tnf/operator/starter_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- pkg/tnf/auth/runner.go
- pkg/tnf/pkg/pcs/auth.go
- pkg/tnf/pkg/pacemaker/healthcheck.go
- pkg/tnf/pkg/tools/jobs.go
- pkg/tnf/pkg/pacemaker/statuscollector.go
- pkg/tnf/pkg/pacemaker/helpers.go
- pkg/tnf/operator/helpers.go
- pkg/tnf/operator/lifecycle_manager.go
- pkg/tnf/pkg/jobs/jobcontroller_test.go
- pkg/tnf/operator/helpers_test.go
- pkg/tnf/operator/status_collector.go
- pkg/tnf/pkg/jobs/utils.go
- pkg/tnf/pkg/tools/nodes.go
- pkg/tnf/pkg/jobs/jobcontroller.go
- pkg/tnf/pkg/jobs/batch_test.go
- pkg/tnf/operator/job_controllers_test.go
| │ └─ Create job spec: | ||
| │ - Schedule on target node (nodeSelector) | ||
| │ - Set backoffLimit=retries (Kubernetes handles retries) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the actual node-pinning mechanism.
The single-node implementation assigns job.Spec.Template.Spec.NodeName = node.Name (pkg/tnf/pkg/jobs/lifecycle.go, Lines 470-531), not a nodeSelector. Since nodeSelector is label-based and may match another node, this wording misstates the scheduling guarantee. Replace it with NodeName/fixed node assignment.
🤖 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 `@docs/tnf/job-controllers.md` around lines 148 - 150, Update the “Create job
spec” documentation to describe the actual single-node pinning mechanism as
NodeName/fixed node assignment, replacing the incorrect nodeSelector wording.
Keep the backoffLimit=retries guidance unchanged.
| │ ├─ Call syncMultiNodeJobState (manages retry state): | ||
| │ │ │ | ||
| │ │ ├─ Check schedulableNodesFunc() → any nodes available? | ||
| │ │ │ ├─ NO → Return error after 10min (triggers Degraded via WithSyncDegradedOnError) | ||
| │ │ │ └─ YES → Clear Degraded condition (if was blocked), continue | ||
| │ │ │ | ||
| │ │ ├─ Check affectedNodesFunc() → any nodes not ready? | ||
| │ │ │ ├─ YES → Return error after 10min (triggers Degraded via WithSyncDegradedOnError) | ||
| │ │ │ └─ NO → Clear Degraded condition (if was blocked), continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the execution flow with the implementation order.
syncMultiNodeJobState checks affectedNodesFunc readiness before evaluating schedulable nodes (pkg/tnf/pkg/jobs/lifecycle.go, Lines 210-233), but this diagram documents the reverse order. Reordering these steps will keep the documented blocking and condition behavior accurate.
🤖 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 `@docs/tnf/job-controllers.md` around lines 180 - 188, The execution flow
diagram should match the implementation order in syncMultiNodeJobState: move the
affectedNodesFunc readiness check before the schedulableNodesFunc availability
check, preserving each branch’s existing error, timeout, and Degraded-condition
behavior.
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{}) | ||
|
|
||
| err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) | ||
| require.NoError(t, err) | ||
| state = getState() | ||
| require.Equal(t, 1, state.AttemptNumber, "Should still be attempt 1") | ||
| require.Equal(t, 1, state.NodeIndex, "Should advance to node index 1") | ||
|
|
||
| // Delete job to simulate ApplyJob detecting drift (NodeName changed) and recreating | ||
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) | ||
|
|
||
| // Step 3: Job fails on node 1 -> should start attempt 2, back to node 0 | ||
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{}) | ||
| err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) | ||
| require.NoError(t, err) | ||
| state = getState() | ||
| require.Equal(t, 2, state.AttemptNumber, "Should advance to attempt 2") | ||
| require.Equal(t, 0, state.NodeIndex, "Should reset to node index 0") | ||
| require.False(t, isDegraded(), "Should not be degraded yet") | ||
|
|
||
| // Delete job to simulate ApplyJob detecting drift | ||
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) | ||
|
|
||
| // Step 4: Exhaust attempt 2 (fail on both nodes) -> should set degraded and reset to attempt 1 | ||
| // Fail on node 0 | ||
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{}) | ||
| err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) | ||
| require.NoError(t, err) | ||
| // Delete and recreate on node 1 | ||
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) | ||
| fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fake-client setup calls throughout this file discard their error returns. The shared root cause is unchecked Create/Delete results during test setup: if a setup call fails (e.g., a re-Create of the same failedJob pointer conflicting on ResourceVersion, or a duplicate node create), the subsequent syncMultiNodeJobState observes different cluster state and the assertions pass for the wrong reason.
pkg/tnf/pkg/jobs/lifecycle_test.go#L322-L352: wrap eachJobs(...).Create/Deleteinrequire.NoErrorand passfailedJob.DeepCopy()on re-creation.pkg/tnf/pkg/jobs/lifecycle_test.go#L625-L625: check the error fromNodes().Createinside the affected-nodes loop withrequire.NoError.pkg/tnf/pkg/jobs/lifecycle_test.go#L744-L748: check the errors from bothNodes().Createcalls (readyNodeand the schedulable-node loop) withrequire.NoError.
As per path instructions for **/*.go: "Never ignore error returns".
📍 Affects 1 file
pkg/tnf/pkg/jobs/lifecycle_test.go#L322-L352(this comment)pkg/tnf/pkg/jobs/lifecycle_test.go#L625-L625pkg/tnf/pkg/jobs/lifecycle_test.go#L744-L748
🤖 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 322 - 352, Check every
fake-client setup error in pkg/tnf/pkg/jobs/lifecycle_test.go#L322-L352 by
wrapping each Jobs Create/Delete call with require.NoError and using
failedJob.DeepCopy() for recreated jobs. In
pkg/tnf/pkg/jobs/lifecycle_test.go#L625-L625, assert the Nodes Create result
inside the affected-nodes loop. In pkg/tnf/pkg/jobs/lifecycle_test.go#L744-L748,
assert both Nodes Create results for readyNode and the schedulable-node loop, so
syncMultiNodeJobState tests operate on the intended cluster state.
Source: Path instructions
3 participants
Implements job controller framework for Two-Node Fencing (TNF) cluster management:
Job Controller Framework (pkg/tnf/pkg/jobs/):
Lifecycle Manager (pkg/tnf/operator/lifecycle_manager.go):
Job Implementations:
Status Collector (pkg/tnf/operator/status_collector.go):
Node Management (pkg/tnf/operator/helpers.go):
Documentation:
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit