OCPBUGS-100060: staticpod: add installer precondition hook - #2387
OCPBUGS-100060: staticpod: add installer precondition hook#2387mkowalski wants to merge 1 commit into
Conversation
The installer controller creates installer pods unconditionally once a node has a pending target revision. The installer pod replaces the operand static pod manifest, restarting the operand. For etcd this can break quorum: the cluster-etcd-operator's quorum checks gate only revision creation, so an installer pod can restart an etcd member while another control plane node is simultaneously down for a machine-config reboot (OCPBUGS-100060: two of three members down, ~2 minutes without an etcd leader, cluster-wide API outage). Add WithInstallerPrecondition to the installer controller and the static pod controllers builder. The precondition is consulted immediately before an installer pod is created for a node; when unmet the controller emits an InstallerPreconditionNotMet event and requeues (15s) instead of restarting the operand. A nil precondition preserves the existing behavior. Assisted-By: Claude Fable 5
openshift-ci-robot
added
jira/valid-reference
|
@mkowalski: This pull request references Jira Issue OCPBUGS-100060, which is invalid:
Comment 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. |
WalkthroughThe installer controller now accepts an optional node-level precondition, evaluates it before installer pod creation, requeues when unsafe, fails on callback errors, and exposes builder wiring and tests for each outcome. ChangesInstaller precondition gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Builder
participant InstallerController
participant InstallerPreconditionFunc
participant InstallerPod
Builder->>InstallerController: configure precondition
InstallerController->>InstallerPreconditionFunc: check node safety
InstallerPreconditionFunc-->>InstallerController: safe, reason, or error
alt safe
InstallerController->>InstallerPod: create installer pod
else unsafe
InstallerController-->>InstallerController: emit warning and requeue
else error
InstallerController-->>InstallerController: fail sync
end
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski 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
🤖 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/operator/staticpod/controller/installer/installer_controller_test.go`:
- Around line 2821-2831: Update the test around the InstallerController Sync
loop to use the controller’s queued/requeue behavior rather than relying only on
manual Sync calls. Assert that an unmet precondition schedules a 15-second
requeue and that the event recorder contains the expected
InstallerPreconditionNotMet warning, while preserving the existing no-pod and
checked-node assertions.
🪄 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: f3ed9673-b482-4a7f-bee5-0222f2be555d
📒 Files selected for processing (3)
pkg/operator/staticpod/controller/installer/installer_controller.gopkg/operator/staticpod/controller/installer/installer_controller_test.gopkg/operator/staticpod/controllers.go
| for i := 0; i < 3; i++ { | ||
| if err := c.Sync(context.TODO(), factory.NewSyncContext("InstallerController", *eventRecorder)); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
| if getPod() != nil { | ||
| t.Fatalf("expected no installer pod while the precondition is unmet") | ||
| } | ||
| if checkedNode != "test-node-1" { | ||
| t.Fatalf("expected precondition to be consulted for test-node-1, got %q", checkedNode) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert the delayed requeue and warning event.
Manual Sync calls bypass queue timing, so this still passes if an unmet precondition retries immediately or omits InstallerPreconditionNotMet. Assert the 15-second requeue and recorded event.
🤖 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/staticpod/controller/installer/installer_controller_test.go`
around lines 2821 - 2831, Update the test around the InstallerController Sync
loop to use the controller’s queued/requeue behavior rather than relying only on
manual Sync calls. Assert that an unmet precondition schedules a 15-second
requeue and that the event recorder contains the expected
InstallerPreconditionNotMet warning, while preserving the existing no-pod and
checked-node assertions.
|
@mkowalski: all tests passed! 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. |
2 participants
Summary
First of two PRs for OCPBUGS-100060: etcd quorum loss during upgrades when the etcd-operator's revision installer restarts an etcd member while MCO is simultaneously rebooting another master (2/3 members down, ~2min leaderless, cluster-wide API outage returning
429 storage is (re)initializing).QuorumCheckergates only revision creation (WithRevisionControllerPrecondition), so per-node installs of an existing revision proceed with no safety check. Evidence: in both incident runs the installer killed master-1's etcd 150–156s before master-0 finished its MCO reboot (run 2075907197388197888, run 2077192709163978752).WithInstallerPrecondition(func(ctx, nodeName) (safe bool, reason string, err error))toInstallerControllerand the static-pod controllersBuilder. Consulted immediately beforeensureInstallerPod; when unmet, emitsInstallerPreconditionNotMetand requeues (15s) instead of restarting the operand.nilprecondition preserves existing behavior for all other operators.IsSafeToRestartMember).Test plan
gofmt,go vet,go build ./pkg/operator/staticpod/...go test ./pkg/operator/staticpod/controller/installer/— newTestCreateInstallerPodPrecondition(unmet delays pod + consults correct node; met allows; error fails sync); existing tests pass (internal/atomicdir TestSwapfails identically on pristine master in my environment — pre-existing, unrelated)This PR was generated using AI. Please verify before acting on it.
Summary by CodeRabbit