OCPEDGE-2416: Add oc adm transition topology command for SNO to HA transitions#2295
OCPEDGE-2416: Add oc adm transition topology command for SNO to HA transitions#2295jaypoulz wants to merge 3 commits into
Conversation
|
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 a feature-gated ChangesTopology transition workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TransitionCommand
participant ClientSideValidator
participant KubernetesAPIs
User->>TransitionCommand: Choose target topology
TransitionCommand->>KubernetesAPIs: Read Infrastructure
TransitionCommand->>ClientSideValidator: Validate transition
ClientSideValidator->>KubernetesAPIs: Read cluster readiness resources
KubernetesAPIs-->>ClientSideValidator: Return validation checks
ClientSideValidator-->>TransitionCommand: Return validation result
TransitionCommand->>KubernetesAPIs: Patch Infrastructure when confirmed
TransitionCommand-->>User: Print initiation or status details
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/cli/admin/admin.go`:
- Around line 79-82: Add a small table-driven unit test for NewCommandAdmin that
verifies the feature-gated topology command registration in both enabled and
disabled cases. Cover the branch that calls
kcmdutil.FeatureGate(transitionTopologyFeatureGate).IsEnabled() and appends
transition.NewCmdTransition to clusterManagement, asserting that oc adm topology
is present only when the gate is enabled and absent when disabled.
In `@pkg/cli/admin/transition/preflight/client_validator_test.go`:
- Around line 222-230: Replace the `t.Skip(...)` placeholders in
`TestValidate_SNOToHA_SomeChecksFail` and `TestValidate_SNOToHA_AllChecksFail`
with real table-driven `testing.T` coverage for `ClientSideValidator.Validate`,
exercising the remaining operator/node/etcd/error-path branches. Use co-located
cases in `client_validator_test.go`, mirror the existing validator test style,
and assert the expected failures instead of skipping so the new logic is
actually covered.
In `@pkg/cli/admin/transition/preflight/client_validator.go`:
- Around line 270-297: The infrastructure node count check in
ClientSideValidator.validateExactInfrastructureNodeCount is too strict for
general HA transitions because it always requires zero non-control-plane nodes.
Update this validation so dedicated worker nodes are not treated as invalid for
every HA transition, either by removing the exact-zero requirement or by gating
it behind a compact-topology-specific signal; keep the existing count logic and
CheckResult flow in place.
In `@pkg/cli/admin/transition/preflight/types_test.go`:
- Around line 49-201: Add table-driven tests in types_test.go for the untested
helper methods on ValidationResult and CheckResult, especially
HasErrorCheckFailures, HasWarningCheckFailures, and ValidationResult.Error().
Cover passed, failed, and unknown combinations so the severity aggregation
behavior is locked down. Use the existing ValidationResult and CheckResult
symbols to verify the returned booleans and the aggregate error text matches the
expected block/bypass output used by client_validator.go and prompt.go.
In `@pkg/cli/admin/transition/prompt/types.go`:
- Around line 15-19: Update the interface comment in PromptForTransition to
match the actual warning-path behavior: warning-severity failures should block
and return false unless --allow-transition-with-warnings is set, in which case
they may proceed. Keep the bullet list aligned with the current implementation
and tests, and adjust the wording around the warning case so it reflects the
immediate-false flow instead of prompting.
In `@pkg/cli/admin/transition/status.go`:
- Around line 153-198: The etcd status command currently prints fetch failures
from the operator and ConfigMap lookups but still returns success, so it should
instead track those section errors and fail the command after the report is
printed. Update the status flow in the command that reads from
o.operatorClient.OperatorV1().Etcds().Get and
o.kubeClient.CoreV1().ConfigMaps(...).Get to collect any errors while still
emitting partial output, then return a utilerrors.Aggregate built from the
accumulated failures instead of nil when one or more lookups fail.
- Around line 60-64: The StatusOptions.Run flow is using a non-cancellable
context and swallowing read errors, so update it to use a timeout-bound context
instead of context.TODO() for the ConfigV1/Get reads and any related API calls.
In Run, thread that cancellable context through the infrastructure and
etcd/ConfigMap fetches, and make sure failures are returned to the caller rather
than only printed. Keep the logic in StatusOptions.Run and the read helpers
consistent so any health-check read failure causes the command to exit with an
error.
In `@pkg/cli/admin/transition/transition_test.go`:
- Around line 11-235: The current test file only covers metadata, validation,
and discovery; the new mutating code in TransitionOptions.runInitiate,
TransitionOptions.runStatus, and related status helpers still lacks coverage.
Add table-driven tests in this co-located *_test.go using fake.NewClientset()
and action recording to verify the Infrastructure update, warning-bypass
behavior, --dry-run behavior, and partial etcd failure handling. Make sure the
tests exercise the initiate/status paths in
pkg/cli/admin/transition/transition.go and pkg/cli/admin/transition/status.go
rather than only the command structure and discovery output.
In `@pkg/cli/admin/transition/transition.go`:
- Around line 208-226: Run() is using an unbounded context, so the
Infrastructure GET and later cluster calls can stall indefinitely; thread the
command context from Complete() into TransitionOptions, then use a bounded
timeout context inside TransitionOptions.Run before calling
configClient.ConfigV1().Infrastructures().Get and the discovery/initiate paths.
Update the Run flow to derive from the stored cmd.Context() instead of
context.TODO(), and ensure any helper invoked from Run uses that bounded context
as well.
- Around line 118-127: The root command in transition.NewCmd/transition command
setup currently accepts unexpected positional args because it only defines Run
without args validation. Add cobra.NoArgs to the cobra.Command for the root
“topology [status]” command so typos like extra positional arguments fail fast
instead of entering the root flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| // TestNewValidationResult tests ValidationResult construction | ||
| func TestNewValidationResult(t *testing.T) { | ||
| current := configv1.SingleReplicaTopologyMode | ||
| target := configv1.HighlyAvailableTopologyMode | ||
|
|
||
| result := NewValidationResult(current, target) | ||
|
|
||
| if result.Current != current { | ||
| t.Errorf("expected current=%s, got %s", current, result.Current) | ||
| } | ||
| if result.Target != target { | ||
| t.Errorf("expected target=%s, got %s", target, result.Target) | ||
| } | ||
| if result.Status != ValidationStatusUnknown { | ||
| t.Errorf("expected status=Unknown, got %s", result.Status) | ||
| } | ||
| if len(result.Checks) != 0 { | ||
| t.Errorf("expected empty checks slice, got %d checks", len(result.Checks)) | ||
| } | ||
| } | ||
|
|
||
| // TestValidationResult_AddCheckResult tests adding check results | ||
| func TestValidationResult_AddCheckResult(t *testing.T) { | ||
| result := NewValidationResult(configv1.SingleReplicaTopologyMode, configv1.HighlyAvailableTopologyMode) | ||
|
|
||
| check1 := CheckResult{Name: "Check 1", Severity: CheckSeverityWarning, Status: CheckStatusPassed} | ||
| check2 := CheckResult{Name: "Check 2", Severity: CheckSeverityError, Status: CheckStatusFailed, Message: "failed"} | ||
|
|
||
| result.AddCheck(check1) | ||
| if len(result.Checks) != 1 { | ||
| t.Fatalf("expected 1 check, got %d", len(result.Checks)) | ||
| } | ||
| if result.Checks[0].Name != "Check 1" { | ||
| t.Errorf("expected first check name='Check 1', got %s", result.Checks[0].Name) | ||
| } | ||
|
|
||
| result.AddCheck(check2) | ||
| if len(result.Checks) != 2 { | ||
| t.Fatalf("expected 2 checks, got %d", len(result.Checks)) | ||
| } | ||
| if result.Checks[1].Name != "Check 2" { | ||
| t.Errorf("expected second check name='Check 2', got %s", result.Checks[1].Name) | ||
| } | ||
| } | ||
|
|
||
| // TestValidationResult_Status_AllPass tests status when all checks pass | ||
| func TestValidationResult_Status_AllPass(t *testing.T) { | ||
| result := NewValidationResult(configv1.SingleReplicaTopologyMode, configv1.HighlyAvailableTopologyMode) | ||
|
|
||
| result.AddCheck(CheckResult{Name: "Check 1", Severity: CheckSeverityWarning, Status: CheckStatusPassed}) | ||
| result.AddCheck(CheckResult{Name: "Check 2", Severity: CheckSeverityError, Status: CheckStatusPassed}) | ||
|
|
||
| if result.Status != ValidationStatusAvailable { | ||
| t.Errorf("expected status=Available when all checks pass, got %s", result.Status) | ||
| } | ||
| } | ||
|
|
||
| // TestValidationResult_Status_SomeFail tests status when checks fail | ||
| func TestValidationResult_Status_SomeFail(t *testing.T) { | ||
| result := NewValidationResult(configv1.SingleReplicaTopologyMode, configv1.HighlyAvailableTopologyMode) | ||
|
|
||
| result.AddCheck(CheckResult{Name: "Check 1", Severity: CheckSeverityWarning, Status: CheckStatusPassed}) | ||
| result.AddCheck(CheckResult{Name: "Check 2", Severity: CheckSeverityError, Status: CheckStatusFailed, Message: "failed"}) | ||
|
|
||
| if result.Status != ValidationStatusUnavailable { | ||
| t.Errorf("expected status=Unavailable when a check fails, got %s", result.Status) | ||
| } | ||
| } | ||
|
|
||
| // TestValidationResult_Status_Unknown tests status when a check is unknown | ||
| func TestValidationResult_Status_Unknown(t *testing.T) { | ||
| result := NewValidationResult(configv1.SingleReplicaTopologyMode, configv1.HighlyAvailableTopologyMode) | ||
|
|
||
| result.AddCheck(CheckResult{Name: "Check 1", Severity: CheckSeverityWarning, Status: CheckStatusPassed}) | ||
| result.AddCheck(CheckResult{Name: "Check 2", Severity: CheckSeverityError, Status: CheckStatusUnknown, Message: "could not check"}) | ||
|
|
||
| if result.Status != ValidationStatusUnknown { | ||
| t.Errorf("expected status=Unknown when a check is unknown, got %s", result.Status) | ||
| } | ||
| } | ||
|
|
||
| // TestCheckResult_Construction tests CheckResult construction | ||
| func TestCheckResult_Construction(t *testing.T) { | ||
| check := CheckResult{ | ||
| Name: "Test Check", | ||
| Severity: CheckSeverityError, | ||
| Status: CheckStatusFailed, | ||
| Message: "test failure", | ||
| } | ||
|
|
||
| if check.Name != "Test Check" { | ||
| t.Errorf("expected name='Test Check', got %s", check.Name) | ||
| } | ||
| if check.Severity != CheckSeverityError { | ||
| t.Errorf("expected severity=Error, got %s", check.Severity) | ||
| } | ||
| if check.Status != CheckStatusFailed { | ||
| t.Errorf("expected status=Failed, got %s", check.Status) | ||
| } | ||
| if check.Message != "test failure" { | ||
| t.Errorf("expected message='test failure', got %s", check.Message) | ||
| } | ||
| } | ||
|
|
||
| // TestCheckResult_String_Passed tests formatting of passed check | ||
| func TestCheckResult_String_Passed(t *testing.T) { | ||
| check := CheckResult{ | ||
| Name: "Passed Check", | ||
| Severity: CheckSeverityWarning, | ||
| Status: CheckStatusPassed, | ||
| } | ||
|
|
||
| expected := " Passed Check: passed" | ||
| if check.String() != expected { | ||
| t.Errorf("expected %q, got %q", expected, check.String()) | ||
| } | ||
| } | ||
|
|
||
| // TestCheckResult_String_Failed tests formatting of failed check with message | ||
| func TestCheckResult_String_Failed(t *testing.T) { | ||
| check := CheckResult{ | ||
| Name: "Failed Check", | ||
| Severity: CheckSeverityError, | ||
| Status: CheckStatusFailed, | ||
| Message: "something went wrong", | ||
| } | ||
|
|
||
| expected := " Failed Check: something went wrong" | ||
| if check.String() != expected { | ||
| t.Errorf("expected %q, got %q", expected, check.String()) | ||
| } | ||
| } | ||
|
|
||
| // TestCheckResult_String_Unknown tests formatting of unknown check | ||
| func TestCheckResult_String_Unknown(t *testing.T) { | ||
| check := CheckResult{ | ||
| Name: "Unknown Check", | ||
| Severity: CheckSeverityWarning, | ||
| Status: CheckStatusUnknown, | ||
| Message: "could not determine", | ||
| } | ||
|
|
||
| expected := " Unknown Check: unknown (could not determine)" | ||
| if check.String() != expected { | ||
| t.Errorf("expected %q, got %q", expected, check.String()) | ||
| } | ||
| } | ||
|
|
||
| // TestClientSideValidator_ImplementsInterface tests interface compliance | ||
| func TestClientSideValidator_ImplementsInterface(t *testing.T) { | ||
| // Compile-time check that ClientSideValidator implements Validator | ||
| var _ Validator = (*ClientSideValidator)(nil) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cover the untested severity helpers and aggregate error output.
HasErrorCheckFailures, HasWarningCheckFailures, and ValidationResult.Error() drive the block/bypass behavior used by pkg/cli/admin/transition/preflight/client_validator.go and pkg/cli/admin/transition/prompt/prompt.go, but this suite never exercises them. A small table-driven set for passed/failed/unknown combinations would lock down the behavior this command now depends on. As per coding guidelines, "Write unit tests for every change" and "Unit tests should use co-located *_test.go files with table-driven tests and standard testing.T".
🤖 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/cli/admin/transition/preflight/types_test.go` around lines 49 - 201, Add
table-driven tests in types_test.go for the untested helper methods on
ValidationResult and CheckResult, especially HasErrorCheckFailures,
HasWarningCheckFailures, and ValidationResult.Error(). Cover passed, failed, and
unknown combinations so the severity aggregation behavior is locked down. Use
the existing ValidationResult and CheckResult symbols to verify the returned
booleans and the aggregate error text matches the expected block/bypass output
used by client_validator.go and prompt.go.
Source: Coding guidelines
| func (o *StatusOptions) Run() error { | ||
| ctx := context.TODO() | ||
|
|
||
| // Get Infrastructure resource | ||
| infra, err := o.configClient.ConfigV1().Infrastructures().Get(ctx, InfrastructureResourceName, metav1.GetOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby context.
sed -n '1,260p' pkg/cli/admin/transition/status.go
# Find related uses of context and error handling in nearby admin transition code.
rg -n "context\\.TODO\\(|context\\.Background\\(|WithTimeout\\(|CheckErr\\(|Aggregate|fmt\\.Printf|Fprintf|klog" pkg/cli/admin/transition -g '*.go'Repository: openshift/oc
Length of output: 14259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' pkg/cli/admin/transition/status.go
rg -n "context\\.TODO\\(|context\\.Background\\(|WithTimeout\\(|CheckErr\\(|Aggregate|fmt\\.Printf|Fprintf|klog" pkg/cli/admin/transition -g '*.go'Repository: openshift/oc
Length of output: 14259
Use a cancellable timeout context and surface read failures.
status still uses context.TODO() for every API call, so a slow request can hang indefinitely. It also prints etcd / ConfigMap read failures and returns success, which makes the command report a result when health could not be determined. Thread a timeout-bound context through the reads and return those errors instead of swallowing them.
🤖 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/cli/admin/transition/status.go` around lines 60 - 64, The
StatusOptions.Run flow is using a non-cancellable context and swallowing read
errors, so update it to use a timeout-bound context instead of context.TODO()
for the ConfigV1/Get reads and any related API calls. In Run, thread that
cancellable context through the infrastructure and etcd/ConfigMap fetches, and
make sure failures are returned to the caller rather than only printed. Keep the
logic in StatusOptions.Run and the read helpers consistent so any health-check
read failure causes the command to exit with an error.
Source: Path instructions
| // Display etcd status | ||
| fmt.Fprintf(o.Out, "\netcd Status:\n\n") | ||
|
|
||
| etcd, err := o.operatorClient.OperatorV1().Etcds().Get(ctx, EtcdOperatorResourceName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| fmt.Fprintf(o.Out, " Failed to get etcd operator: %v\n", err) | ||
| return nil | ||
| } | ||
|
|
||
| // Check etcd conditions | ||
| var etcdStatus []string | ||
| for _, cond := range etcd.Status.Conditions { | ||
| switch cond.Type { | ||
| case EtcdMembersAvailableCondition: | ||
| if cond.Status == "True" { | ||
| etcdStatus = append(etcdStatus, "Members available") | ||
| } else { | ||
| etcdStatus = append(etcdStatus, fmt.Sprintf("Members not available: %s", cond.Message)) | ||
| } | ||
| case "Progressing": | ||
| if cond.Status == "True" { | ||
| etcdStatus = append(etcdStatus, fmt.Sprintf("Progressing: %s", cond.Message)) | ||
| } | ||
| case "Degraded": | ||
| if cond.Status == "True" { | ||
| etcdStatus = append(etcdStatus, fmt.Sprintf("Degraded: %s", cond.Message)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(etcdStatus) == 0 { | ||
| fmt.Fprintf(o.Out, " etcd healthy\n") | ||
| } else { | ||
| for _, s := range etcdStatus { | ||
| fmt.Fprintf(o.Out, " %s\n", s) | ||
| } | ||
| } | ||
|
|
||
| // Get etcd voting member count | ||
| cm, err := o.kubeClient.CoreV1().ConfigMaps(EtcdNamespace).Get(ctx, EtcdEndpointsConfigMapName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| fmt.Fprintf(o.Out, " Failed to get %s ConfigMap: %v\n", EtcdEndpointsConfigMapName, err) | ||
| } else { | ||
| votingMembers := len(cm.Data) | ||
| fmt.Fprintf(o.Out, " Voting members: %d\n", votingMembers) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't return success when etcd health could not be fetched.
The etcd operator and ConfigMap failures are printed and then ignored, so scripts still get exit code 0 even though the command could not determine etcd status. Please collect those section failures and return an aggregated error after printing the partial report.
As per coding guidelines, pkg/cli/**/*.go: utilerrors.Aggregate for batch errors.
🤖 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/cli/admin/transition/status.go` around lines 153 - 198, The etcd status
command currently prints fetch failures from the operator and ConfigMap lookups
but still returns success, so it should instead track those section errors and
fail the command after the report is printed. Update the status flow in the
command that reads from o.operatorClient.OperatorV1().Etcds().Get and
o.kubeClient.CoreV1().ConfigMaps(...).Get to collect any errors while still
emitting partial output, then return a utilerrors.Aggregate built from the
accumulated failures instead of nil when one or more lookups fail.
Source: Coding guidelines
40a0310 to
f1c4c7d
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cli/admin/transition/preflight/client_validator_test.go (1)
139-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCollapse the repeated validator assertions into table-driven
cmp.Diffcases. The current one-off field checks are duplicated across several scenarios; assembling expectedValidationResult/CheckResultvalues and comparing them directly will keep this suite easier to extend and align it with the test conventions here.🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 139 - 220, The SNO→HA happy-path test is manually asserting many fields one by one instead of using the table-driven cmp-based style used elsewhere in this suite. Refactor TestValidate_SNOToHA_AllChecksPass to build the expected ValidationResult and CheckResult values, then compare the full result with cmp.Diff so the assertions are centralized and easier to extend. Keep using the existing validator setup and the Validate method, but replace the repeated per-check status, severity, and name checks with direct diff-based comparisons.Source: Coding guidelines
🤖 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/cli/admin/transition/preflight/client_validator_test.go`:
- Around line 139-220: The SNO→HA happy-path test is manually asserting many
fields one by one instead of using the table-driven cmp-based style used
elsewhere in this suite. Refactor TestValidate_SNOToHA_AllChecksPass to build
the expected ValidationResult and CheckResult values, then compare the full
result with cmp.Diff so the assertions are centralized and easier to extend.
Keep using the existing validator setup and the Validate method, but replace the
repeated per-check status, severity, and name checks with direct diff-based
comparisons.
f1c4c7d to
faf2c69
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
faf2c69 to
e3eb318
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/cli/admin/transition/transition.go (1)
147-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the command context before the cluster calls.
This still threads an unbounded
cmd.Context()into the Infrastructure GET, preflight validation, and update path, so an API stall can hang the command indefinitely. Derive a timeout-scoped context inRun()and pass that bounded context through the rest of the flow. As per path instructions,**/*.go:context.Context for cancellation and timeouts.Suggested fix
import ( "context" "fmt" + "time" @@ +const defaultTransitionTimeout = 30 * time.Second + // Run executes the topology transition command func (o *TransitionOptions) Run() error { + ctx, cancel := context.WithTimeout(o.ctx, defaultTransitionTimeout) + defer cancel() + // Get current topology from Infrastructure resource - infra, err := o.configClient.ConfigV1().Infrastructures().Get(o.ctx, InfrastructureResourceName, metav1.GetOptions{}) + infra, err := o.configClient.ConfigV1().Infrastructures().Get(ctx, InfrastructureResourceName, metav1.GetOptions{}) if err != nil { return fmt.Errorf("failed to get Infrastructure resource: %w", err) } @@ - return o.runInitiateMode(o.ctx, currentTopology, targetTopology) + return o.runInitiateMode(ctx, currentTopology, targetTopology) }Also applies to: 215-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/cli/admin/transition/transition.go` around lines 147 - 149, The transition command is still using an unbounded cmd.Context() for the Infrastructure GET, preflight validation, and update flow. In Run() on the transition command, derive a timeout-scoped context before any cluster/API calls and store/use that bounded context throughout the rest of the path. Update the relevant transition flow methods (including the Infrastructure GET and validation/update steps) to accept and pass this scoped context instead of cmd.Context().Source: Path instructions
pkg/cli/admin/transition/preflight/client_validator.go (1)
270-297: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't treat dedicated workers as a readiness failure for every HA transition.
This still hard-requires exactly
0non-control-plane nodes and reports any worker-bearing cluster as needing "HighlyAvailable compact topology". A supported SNO → HA transition can legitimately keep dedicated workers, so this warning blocks the default flow on valid clusters unless the user opts into--allow-transition-with-warnings.🤖 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/cli/admin/transition/preflight/client_validator.go` around lines 270 - 297, The HA infrastructure node check in validateExactInfrastructureNodeCount is too strict because it hardcodes required as 0 and fails any cluster with non-control-plane nodes. Update the logic so the default readiness path only warns or skips this condition for supported SNO→HA transitions, and keep the failure behind the existing --allow-transition-with-warnings behavior. Use validateExactInfrastructureNodeCount, CheckNameInfrastructureNodeCount, and the message construction to adjust the status/message so worker-bearing clusters are not treated as a blocking failure by default.
🧹 Nitpick comments (1)
pkg/cli/admin/transition/prompt/prompt_test.go (1)
134-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing warning-bypass +
--yescase.The suite covers warnings with interactive confirmation and all-pass with
--yes, but it never exercises the combinedAllowTransitionWithWarnings=true+Yes=truebranch. That's the non-interactive warning path automation will use, so I'd add it—ideally as another row in a small table-driven matrix. As per coding guidelines,**/*_test.go:Write unit tests for every changeandUnit tests should use co-located *_test.go files with table-driven tests and standard testing.T.🤖 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/cli/admin/transition/prompt/prompt_test.go` around lines 134 - 220, The transition prompt tests are missing coverage for the non-interactive warning bypass path when both AllowTransitionWithWarnings and Yes are true. Add a test case in PromptForTransition/InteractivePrompter that exercises this combined branch, ideally by converting the existing PromptForTransition cases into a small table-driven set. Verify the confirmation returns true and that the output matches the expected warning-bypass/--yes messaging without prompting for input.Source: Coding guidelines
🤖 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/cli/admin/transition/preflight/types.go`:
- Around line 101-126: Preserve the aggregate Unknown state in
ValidationResult.AddCheck: right now CheckStatusPassed can reset vr.Status from
ValidationStatusUnknown back to ValidationStatusAvailable, even when the Unknown
came from a previous failed API call. Update AddCheck in ValidationResult so
once any CheckResult has Status CheckStatusUnknown, later passes never overwrite
that state, and only the initial no-checks state can transition to Available;
keep the existing precedence logic for CheckStatusFailed and CheckStatusUnknown
while using a separate “have seen any checks” condition if needed.
In `@pkg/cli/admin/transition/transition.go`:
- Around line 235-250: The user-facing output in
TransitionOptions.runDiscoveryMode is ignoring write failures from fmt.Fprintf,
so broken pipes and other output errors are lost; update this path to propagate
every write error back to the caller, ideally with a small helper around o.Out
writes. Apply the same pattern to the related output blocks in TransitionOptions
so all CLI messages fail cleanly when writing to stdout/stderr fails.
---
Duplicate comments:
In `@pkg/cli/admin/transition/preflight/client_validator.go`:
- Around line 270-297: The HA infrastructure node check in
validateExactInfrastructureNodeCount is too strict because it hardcodes required
as 0 and fails any cluster with non-control-plane nodes. Update the logic so the
default readiness path only warns or skips this condition for supported SNO→HA
transitions, and keep the failure behind the existing
--allow-transition-with-warnings behavior. Use
validateExactInfrastructureNodeCount, CheckNameInfrastructureNodeCount, and the
message construction to adjust the status/message so worker-bearing clusters are
not treated as a blocking failure by default.
In `@pkg/cli/admin/transition/transition.go`:
- Around line 147-149: The transition command is still using an unbounded
cmd.Context() for the Infrastructure GET, preflight validation, and update flow.
In Run() on the transition command, derive a timeout-scoped context before any
cluster/API calls and store/use that bounded context throughout the rest of the
path. Update the relevant transition flow methods (including the Infrastructure
GET and validation/update steps) to accept and pass this scoped context instead
of cmd.Context().
---
Nitpick comments:
In `@pkg/cli/admin/transition/prompt/prompt_test.go`:
- Around line 134-220: The transition prompt tests are missing coverage for the
non-interactive warning bypass path when both AllowTransitionWithWarnings and
Yes are true. Add a test case in PromptForTransition/InteractivePrompter that
exercises this combined branch, ideally by converting the existing
PromptForTransition cases into a small table-driven set. Verify the confirmation
returns true and that the output matches the expected warning-bypass/--yes
messaging without prompting for input.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 68174a2e-da7f-46d4-8b4b-6882db9a6fca
📒 Files selected for processing (13)
pkg/cli/admin/admin.gopkg/cli/admin/transition/constants.gopkg/cli/admin/transition/preflight/client_validator.gopkg/cli/admin/transition/preflight/client_validator_test.gopkg/cli/admin/transition/preflight/constants.gopkg/cli/admin/transition/preflight/types.gopkg/cli/admin/transition/preflight/types_test.gopkg/cli/admin/transition/prompt/prompt.gopkg/cli/admin/transition/prompt/prompt_test.gopkg/cli/admin/transition/prompt/types.gopkg/cli/admin/transition/status.gopkg/cli/admin/transition/transition.gopkg/cli/admin/transition/transition_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/transition/constants.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/cli/admin/admin.go
- pkg/cli/admin/transition/prompt/types.go
- pkg/cli/admin/transition/preflight/constants.go
- pkg/cli/admin/transition/prompt/prompt.go
- pkg/cli/admin/transition/status.go
e3eb318 to
449810d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cli/admin/transition/preflight/client_validator_test.go (1)
72-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRefactor these validator cases into table-driven tests and compare assembled results with
cmp.Diff.
The repeated fake-client setup and field-by-field assertions across these cases make the matrix hard to extend; a table-driventesting.Tshape with full-result comparisons will keep the validator coverage easier to maintain.🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 72 - 466, The validator tests are highly repetitive and should be refactored into table-driven cases with assembled expected results compared via cmp.Diff. Consolidate the repeated fake client setup and assertions in the TestValidate_* functions, especially around NewClientSideValidator and the validate* helpers, into a single table of inputs/expected outputs. Build the expected ValidationResult/Check values per case and use cmp.Diff for the full comparison instead of many field-by-field checks.Source: Coding guidelines
🤖 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/cli/admin/transition/preflight/client_validator.go`:
- Around line 130-149: The feature-gate validation in checkFeatureGateEnabled is
too broad because it returns success when MutableTopology appears in any
featureGates entry. Restrict the scan to the authoritative active version entry
from featureGate.Status.FeatureGates so preflight only passes when that specific
version has the gate enabled, and update the CheckResult path accordingly. Add a
regression test around client_validator.go that covers mixed-version
status.featureGates data to confirm an unrelated version no longer causes
CheckNameFeatureGateEnabled to pass.
In `@pkg/cli/admin/transition/transition.go`:
- Around line 331-338: The warning-check block in transition logic is printing
an error but still returns success, so update the
`result.HasWarningCheckFailures()` path in `transition.go` to return a non-nil
error instead of `nil` after the `fprintf` calls. Keep the existing user-facing
messages, but ensure the `o.AllowTransitionWithWarnings` guard in the transition
flow causes a non-zero exit by propagating an error from this branch.
---
Nitpick comments:
In `@pkg/cli/admin/transition/preflight/client_validator_test.go`:
- Around line 72-466: The validator tests are highly repetitive and should be
refactored into table-driven cases with assembled expected results compared via
cmp.Diff. Consolidate the repeated fake client setup and assertions in the
TestValidate_* functions, especially around NewClientSideValidator and the
validate* helpers, into a single table of inputs/expected outputs. Build the
expected ValidationResult/Check values per case and use cmp.Diff for the full
comparison instead of many field-by-field checks.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2450944d-45c8-4007-95b1-d443c0454032
📒 Files selected for processing (10)
pkg/cli/admin/admin.gopkg/cli/admin/transition/constants.gopkg/cli/admin/transition/preflight/client_validator.gopkg/cli/admin/transition/preflight/client_validator_test.gopkg/cli/admin/transition/preflight/constants.gopkg/cli/admin/transition/preflight/types.gopkg/cli/admin/transition/preflight/types_test.gopkg/cli/admin/transition/status.gopkg/cli/admin/transition/transition.gopkg/cli/admin/transition/transition_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/transition/preflight/types_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cli/admin/admin.go
- pkg/cli/admin/transition/constants.go
- pkg/cli/admin/transition/preflight/constants.go
- pkg/cli/admin/transition/status.go
jaypoulz
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jaypoulz: This pull request references OCPEDGE-2416 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
449810d to
a210137
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cli/admin/transition/preflight/client_validator.go (1)
171-199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelect the feature-gate entry by payload version, not by index.
Line 183 assumes
status.featureGates[0]is the active gate set, but this list is versioned and mixed-version entries can coexist. That makes preflight depend on slice order instead of the cluster's active payload version, soMutableTopologycan be reported incorrectly. Please resolve the active version first and only inspect the matchingFeatureGateDetailsentry, with a mixed-version regression test alongside it.🤖 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/cli/admin/transition/preflight/client_validator.go` around lines 171 - 199, The FeatureGate check in client_validator.go is incorrectly assuming the first Status.FeatureGates entry is the active one; update the preflight logic in the FeatureGate validation path to resolve the cluster’s active payload version first, then inspect only the matching FeatureGateDetails entry for MutableTopology. Keep the CheckNameFeatureGateEnabled flow intact, but replace the index-based lookup in the activeVersion logic with version matching, and add a regression test covering mixed-version Status.FeatureGates ordering.
🧹 Nitpick comments (1)
pkg/cli/admin/transition/preflight/validator_helpers_test.go (1)
42-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare the full
CheckResultin the table cases.These cases only assert
Status, so a regression inName,Severity, orMessagewould still pass even though those fields are part of the validator contract and affect phase gating/output. Populate an expectedCheckResultper case and compare withcmp.Diff. As per coding guidelines, "Use google/go-cmp for test assertions by assembling the expected object and comparing with actual, rather than checking individual fields with if statements."Also applies to: 210-223
🤖 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/cli/admin/transition/preflight/validator_helpers_test.go` around lines 42 - 50, The table-driven validator tests only verify CheckStatus, so regressions in CheckResult.Name, Severity, or Message can slip through. Update the validator test cases around validatorTestCase and the related table entries to include a full expected CheckResult for each scenario, then compare the actual result from ClientSideValidator with the expected object using cmp.Diff instead of field-by-field checks. Ensure the assertions cover the complete contract returned by the validator, not just Status.Source: Coding guidelines
🤖 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/cli/admin/transition/preflight/client_validator_test.go`:
- Around line 533-543: The test currently only verifies that some warning check
failed, which can miss regressions in the degraded cluster-operator path. Update
the assertions in client_validator_test.go to explicitly find and fail the
cluster-operator warning by name in result.Checks, using the existing
CheckSeverityWarning and CheckStatusFailed checks, so the test proves the
intended warning is the one failing.
---
Duplicate comments:
In `@pkg/cli/admin/transition/preflight/client_validator.go`:
- Around line 171-199: The FeatureGate check in client_validator.go is
incorrectly assuming the first Status.FeatureGates entry is the active one;
update the preflight logic in the FeatureGate validation path to resolve the
cluster’s active payload version first, then inspect only the matching
FeatureGateDetails entry for MutableTopology. Keep the
CheckNameFeatureGateEnabled flow intact, but replace the index-based lookup in
the activeVersion logic with version matching, and add a regression test
covering mixed-version Status.FeatureGates ordering.
---
Nitpick comments:
In `@pkg/cli/admin/transition/preflight/validator_helpers_test.go`:
- Around line 42-50: The table-driven validator tests only verify CheckStatus,
so regressions in CheckResult.Name, Severity, or Message can slip through.
Update the validator test cases around validatorTestCase and the related table
entries to include a full expected CheckResult for each scenario, then compare
the actual result from ClientSideValidator with the expected object using
cmp.Diff instead of field-by-field checks. Ensure the assertions cover the
complete contract returned by the validator, not just Status.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5c62fc86-97cd-4ce0-a9af-ae636f4ac8f0
📒 Files selected for processing (11)
pkg/cli/admin/admin.gopkg/cli/admin/transition/constants.gopkg/cli/admin/transition/preflight/client_validator.gopkg/cli/admin/transition/preflight/client_validator_test.gopkg/cli/admin/transition/preflight/constants.gopkg/cli/admin/transition/preflight/types.gopkg/cli/admin/transition/preflight/types_test.gopkg/cli/admin/transition/preflight/validator_helpers_test.gopkg/cli/admin/transition/status.gopkg/cli/admin/transition/transition.gopkg/cli/admin/transition/transition_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/cli/admin/admin.go
- pkg/cli/admin/transition/preflight/types_test.go
- pkg/cli/admin/transition/constants.go
- pkg/cli/admin/transition/preflight/constants.go
- pkg/cli/admin/transition/status.go
63e0d4f to
cbd36f8
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jaypoulz 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 |
cbd36f8 to
12bde7e
Compare
|
@coderabbitai review |
|
/retest-required |
846b4f7 to
58c0361
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/cli/admin/transition/transition.go (1)
492-592: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winManual poll loop ignores context cancellation during sleep.
time.Sleep(pollInterval)at Lines 500 and 590 blocks regardless ofctxbeing canceled/done, delaying responsiveness to timeouts/interruption. Considerwait.PollUntilContextCancel/PollUntilContextTimeoutfromk8s.io/apimachinery/pkg/util/wait(already vendored) instead of reimplementing the poll loop — it selects onctx.Done()between iterations.As per contributing guidelines: "Do not reinvent the wheel. Where possible, use existing standard library or vendored library functionality."
🤖 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/cli/admin/transition/transition.go` around lines 492 - 592, The manual polling loop in the transition logic ignores ctx cancellation because it uses time.Sleep directly between attempts. Refactor the loop in the transition command’s operator-health check to use wait.PollUntilContextCancel or wait.PollUntilContextTimeout from k8s.io/apimachinery/pkg/util/wait so the polling exits immediately when ctx is done. Keep the existing operator status checks and output behavior inside the poll callback, but remove the reimplemented sleep/attempt handling in favor of the vendored polling helper.Source: Coding guidelines
pkg/cli/admin/transition/preflight/client_validator_test.go (3)
227-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win"Feature gate present but disabled" branch is untested.
newFakeFeatureGate(false)populatesStatus.FeatureGates[0].Disabledbut is never called from any test in this file. TheCheckStatusFailedpath invalidateFeatureGateEnabled(client_validator.go lines 227-233, feature gate present but not inEnabled) has no coverage — only "enabled" and "resource missing" (Unknown) are tested.✅ Suggested additional test
func TestValidateFeatureGateEnabled_Disabled(t *testing.T) { validator := NewClientSideValidator( fake.NewSimpleClientset(), fakeconfigclient.NewSimpleClientset(newFakeFeatureGate(false)), fakeoperatorclient.NewSimpleClientset(), ) result := validator.validateFeatureGateEnabled(context.Background()) if result.Status != CheckStatusFailed { t.Errorf("expected status=Failed when feature gate disabled, got %s: %s", result.Status, result.Message) } }Also applies to: 381-392
🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 227 - 234, Add a test that covers the feature-gate-present-but-disabled case in validateFeatureGateEnabled so the CheckStatusFailed branch is exercised. Use newFakeFeatureGate(false) with NewClientSideValidator and assert the result from validateFeatureGateEnabled(context.Background()) is CheckStatusFailed with the expected message, alongside the existing enabled and missing-resource coverage in client_validator_test.go.
78-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
fake.NewClientset()instead of deprecatedfake.NewSimpleClientset().Repo coding guidelines require
fake.NewClientset()for test fixtures, andNewSimpleClientsetis now the deprecated form ink8s.io/client-go/kubernetes/fake(NewClientsetis the drop-in, non-deprecated replacement with the same signature). This file (andvalidator_helpers_test.go/transition_test.go) usesNewSimpleClientsetthroughout for the kube client. As per coding guidelines, "Usefake.NewClientset()with action recording in tests to verify API call options."Also applies to: 318-323, 340-344
🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 78 - 99, Replace the deprecated fake client constructors in the test setup with the non-deprecated equivalents. In the client fixture creation for the kube client, switch from fake.NewSimpleClientset to fake.NewClientset, and do the same anywhere else in this test area that still uses the simple constructor (including the related transition and helper test files mentioned in the comment). Keep the same fixture objects passed into the clientset so the behavior of client_validator_test and the helper tests remains unchanged.Source: Coding guidelines
113-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffGuideline: prefer
google/go-cmpover field-by-fieldifassertions.Coding guidelines state assertions should assemble the expected object and compare with
google/go-cmprather than checking individual fields withifstatements. This file's tests (e.g.TestValidate_SNOToHA_AllChecksPass,TestValidate_AlreadyAtTarget) exclusively use manual per-fieldif/t.Errorfchecks againstresult.Checks/result.Status. Not blocking given existing correctness, but worth converging on the standard pattern going forward. As per coding guidelines, "Usegoogle/go-cmpfor test assertions by assembling the expected object and comparing with actual, rather than checking individual fields with if statements."Also applies to: 182-208, 242-257
🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 113 - 153, Refactor the validation tests in client_validator_test.go to follow the test guideline by replacing the manual per-field assertions in TestValidate_SNOToHA_AllChecksPass, TestValidate_AlreadyAtTarget, and the other referenced cases with a single expected result object compared against result using google/go-cmp. Build the expected ValidationResult (including Status and Checks with names, severities, and statuses) and use cmp.Diff or equivalent instead of chained if/t.Errorf checks, keeping the existing coverage but making assertions object-based.Source: Coding guidelines
pkg/cli/admin/transition/preflight/client_validator.go (1)
84-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
checkUnknown/checkPassed/listNodeshelpers — validators duplicate this logic 8x instead.
checkUnknown,checkPassed, andlistNodesare purpose-built to eliminate the repeatedCheckResult{Name:..., Severity:..., Status:..., Message:...}boilerplate, but onlycheckFailed/checkPassedare used, and only invalidateSupportedTransition. Every other validator (validateFeatureGateEnabled,validateClusterOperatorsStable,validateControlPlaneNodeCount,validateExactInfrastructureNodeCount,validateControlPlaneNodesSchedulable,validateControlPlaneNodesReady,validateEtcdQuorum,validateEtcdNotProgressing,validateEtcdVotingMembers) re-implements the sameNodes().List(...)+ error-handling + literal struct pattern inline.listNodesis entirely dead code.♻️ Example consolidation for one validator
func (v *ClientSideValidator) validateControlPlaneNodeCount(ctx context.Context, required int) CheckResult { - nodes, err := v.kubeClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) - if err != nil { - return CheckResult{ - Name: CheckNameControlPlaneNodeCount, - Severity: CheckSeverityWarning, - Status: CheckStatusUnknown, - Message: fmt.Sprintf("failed to list nodes: %v", err), - } - } + nodes, errResult := v.listNodes(ctx, CheckNameControlPlaneNodeCount, CheckSeverityWarning) + if errResult != nil { + return *errResult + } count := 0 for _, node := range nodes.Items { if isControlPlaneNode(&node) { count++ } } if count < required { - return CheckResult{ - Name: CheckNameControlPlaneNodeCount, - Severity: CheckSeverityWarning, - Status: CheckStatusFailed, - Message: fmt.Sprintf("need %d, have %d", required, count), - } + return checkFailed(CheckNameControlPlaneNodeCount, CheckSeverityWarning, fmt.Sprintf("need %d, have %d", required, count)) } - return CheckResult{ - Name: CheckNameControlPlaneNodeCount, - Severity: CheckSeverityWarning, - Status: CheckStatusPassed, - } + return checkPassed(CheckNameControlPlaneNodeCount, CheckSeverityWarning) }Also applies to: 315-554
🤖 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/cli/admin/transition/preflight/client_validator.go` around lines 84 - 123, The validator methods are duplicating the same node-listing and CheckResult construction logic instead of using the shared helpers. Update the validators in ClientSideValidator to call listNodes for the Nodes().List() + error handling path, and use checkUnknown/checkPassed where appropriate so the repeated inline CheckResult literals are removed. Also ensure checkUnknown, checkPassed, and listNodes are actually referenced from the existing validators such as validateFeatureGateEnabled, validateClusterOperatorsStable, and validateSupportedTransition.
🤖 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/cli/admin/transition/transition.go`:
- Around line 248-252: The `Run` timeout-scoped context is being reused by
`applyTransition` and `monitorTransition`, causing the monitor loop to inherit
the too-short 2-minute deadline. Keep the short-lived context in `Run` only for
the initial API calls, and create a separate longer-lived context derived from
`o.ctx` for `monitorTransition` so its polling window can actually reach the
documented 10 minutes. Update the call chain in `Run`, `applyTransition`, and
`monitorTransition` so the monitor path no longer depends on the short timeout
context.
---
Nitpick comments:
In `@pkg/cli/admin/transition/preflight/client_validator_test.go`:
- Around line 227-234: Add a test that covers the
feature-gate-present-but-disabled case in validateFeatureGateEnabled so the
CheckStatusFailed branch is exercised. Use newFakeFeatureGate(false) with
NewClientSideValidator and assert the result from
validateFeatureGateEnabled(context.Background()) is CheckStatusFailed with the
expected message, alongside the existing enabled and missing-resource coverage
in client_validator_test.go.
- Around line 78-99: Replace the deprecated fake client constructors in the test
setup with the non-deprecated equivalents. In the client fixture creation for
the kube client, switch from fake.NewSimpleClientset to fake.NewClientset, and
do the same anywhere else in this test area that still uses the simple
constructor (including the related transition and helper test files mentioned in
the comment). Keep the same fixture objects passed into the clientset so the
behavior of client_validator_test and the helper tests remains unchanged.
- Around line 113-153: Refactor the validation tests in client_validator_test.go
to follow the test guideline by replacing the manual per-field assertions in
TestValidate_SNOToHA_AllChecksPass, TestValidate_AlreadyAtTarget, and the other
referenced cases with a single expected result object compared against result
using google/go-cmp. Build the expected ValidationResult (including Status and
Checks with names, severities, and statuses) and use cmp.Diff or equivalent
instead of chained if/t.Errorf checks, keeping the existing coverage but making
assertions object-based.
In `@pkg/cli/admin/transition/preflight/client_validator.go`:
- Around line 84-123: The validator methods are duplicating the same
node-listing and CheckResult construction logic instead of using the shared
helpers. Update the validators in ClientSideValidator to call listNodes for the
Nodes().List() + error handling path, and use checkUnknown/checkPassed where
appropriate so the repeated inline CheckResult literals are removed. Also ensure
checkUnknown, checkPassed, and listNodes are actually referenced from the
existing validators such as validateFeatureGateEnabled,
validateClusterOperatorsStable, and validateSupportedTransition.
In `@pkg/cli/admin/transition/transition.go`:
- Around line 492-592: The manual polling loop in the transition logic ignores
ctx cancellation because it uses time.Sleep directly between attempts. Refactor
the loop in the transition command’s operator-health check to use
wait.PollUntilContextCancel or wait.PollUntilContextTimeout from
k8s.io/apimachinery/pkg/util/wait so the polling exits immediately when ctx is
done. Keep the existing operator status checks and output behavior inside the
poll callback, but remove the reimplemented sleep/attempt handling in favor of
the vendored polling helper.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ae628f5e-e681-4aa1-8219-ac0b56b14956
📒 Files selected for processing (12)
pkg/cli/admin/admin.gopkg/cli/admin/transition/constants.gopkg/cli/admin/transition/preflight/client_validator.gopkg/cli/admin/transition/preflight/client_validator_test.gopkg/cli/admin/transition/preflight/constants.gopkg/cli/admin/transition/preflight/types.gopkg/cli/admin/transition/preflight/types_test.gopkg/cli/admin/transition/preflight/validator_helpers_test.gopkg/cli/admin/transition/status.gopkg/cli/admin/transition/transition.gopkg/cli/admin/transition/transition_test.gopkg/helpers/describe/describer_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/cli/admin/transition/constants.go
- pkg/helpers/describe/describer_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cli/admin/transition/preflight/types_test.go
- pkg/cli/admin/admin.go
- pkg/cli/admin/transition/preflight/constants.go
- pkg/cli/admin/transition/status.go
65cd2f2 to
eb917aa
Compare
6a37e44 to
8de1730
Compare
|
/test verify |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/cli/admin/transition/status.go (1)
85-156: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo shared constants for the
TopologyTransitionController*condition contract. Both the production status parser and its tests independently hardcode the same conditionType/Reasonstring literals with nothing tying them together, so a typo or drift on either side would compile cleanly but silently break status reporting.
pkg/cli/admin/transition/status.go#L85-L156: extractTopologyTransitionControllerProgressing/Upgradeableand reason strings (PreflightCheckFailed,UnsupportedTransition) plus the"cluster"operator-config resource name into package-level constants (e.g. alongsideInfrastructureResourceName).pkg/cli/admin/transition/transition_test.go#L696-L826: reference those same constants when building the fakeoperatorv1.OperatorConditionfixtures instead of re-typing the literals, so the test and production code cannot drift silently.🤖 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/cli/admin/transition/status.go` around lines 85 - 156, Extract shared package-level constants for the operator config resource name "cluster", the TopologyTransitionControllerProgressing and TopologyTransitionControllerUpgradeable condition types, and the PreflightCheckFailed and UnsupportedTransition reasons in status.go, alongside InfrastructureResourceName. Update the condition fixtures in pkg/cli/admin/transition/transition_test.go lines 696-826 to use these constants instead of duplicated literals; both sites require changes.pkg/cli/admin/transition/preflight/client_validator_test.go (2)
233-236: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the
CheckNameSupportedTransitionconstant instead of a hardcoded string.
"Supported Transition"is hardcoded here and at Line 284, while the constantCheckNameSupportedTransitionis already used elsewhere in this file (e.g. Line 122). If the constant's value ever changes, these assertions silently stop verifying anything meaningful.Also applies to: 283-286.
🔧 Suggested fix
- if supportedTransitionCheck.Name != "Supported Transition" { + if supportedTransitionCheck.Name != CheckNameSupportedTransition { t.Errorf("expected first check name='Supported Transition', got %s", supportedTransitionCheck.Name) }🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 233 - 236, Replace the hardcoded "Supported Transition" expectations in the supportedTransitionCheck assertions, including the assertion around line 284, with the existing CheckNameSupportedTransition constant. Keep the current assertion behavior and error messages otherwise unchanged.
201-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse
google/go-cmpinstead of individual field assertions.These tests build expected outcomes and verify them field-by-field with
ifstatements, rather than assembling an expected object and diffing withcmp.Diffas done inTestValidate_SNOToHA_AllChecksPass/TestValidate_AlreadyAtTargetabove. As per coding guidelines, "Assemble expected objects and compare them with actual objects usinggoogle/go-cmp, rather than checking individual fields with if statements."Also applies to: 250-303, 305-372, 541-618, 620-709.
🤖 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/cli/admin/transition/preflight/client_validator_test.go` around lines 201 - 248, Update the affected validation tests, including TestValidate_UnsupportedTransition and the other noted test cases, to assemble complete expected validation results and compare them with actual results using google/go-cmp and cmp.Diff. Replace individual field assertions for status, checks, names, statuses, severities, and lengths while preserving the existing expected outcomes and error handling.Source: Coding guidelines
pkg/cli/admin/transition/preflight/validator_helpers_test.go (1)
55-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd error-path (
CheckStatusUnknown) cases to the table.Every case here only exercises the happy path; none of the validators' API-failure branches (node list errors, etcd Get errors, ConfigMap Get errors) are covered by any test in this PR. As per coding guidelines, "Write unit tests for every change by default... Use table-driven tests," and this table is well-suited to add a
setupKube/setupOperatorvariant that injects an error via a fake-clientset reactor withexpectedStatus: CheckStatusUnknown.🤖 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/cli/admin/transition/preflight/validator_helpers_test.go` around lines 55 - 250, Extend TestIndividualValidators with table-driven API-error cases for each validator’s failure path, injecting node list, Etcd resource Get, and ConfigMap Get errors through fake-clientset reactors in the relevant setupKube or setupOperator callbacks. Invoke the corresponding validate* method and set expectedStatus to CheckStatusUnknown while preserving each check’s expected name and severity.Source: Coding guidelines
pkg/cli/admin/transition/preflight/client_validator.go (1)
116-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
listNodeshelper — node/etcd fetch logic duplicated 4x/2x instead.
listNodescentralizesNodes().List+ error handling, but no caller uses it — each node-based validator re-implements the same list-and-check-error block inline, and the two etcd validators each independentlyGetthe same Etcd CR. This is copy/paste duplication that will drift as the fetch logic evolves (e.g., adding field selectors or richer error wrapping requires touching 4-6 places).Also applies to: 300-303 (
validateControlPlaneNodeCount), 332-340 (validateExactInfrastructureNodeCount), 368-376 (validateControlPlaneNodesSchedulable), 407-415 (validateControlPlaneNodesReady), 448-456 (validateEtcdQuorum), 476-484 (validateEtcdNotProgressing).♻️ Suggested direction
Either have the four node validators call the existing
listNodeshelper, or fetch the node list / etcd CR once inValidate()and pass it down to the sibling checks that need it.🤖 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/cli/admin/transition/preflight/client_validator.go` around lines 116 - 123, Eliminate the duplicated resource-fetch logic by making validateControlPlaneNodeCount, validateExactInfrastructureNodeCount, validateControlPlaneNodesSchedulable, and validateControlPlaneNodesReady reuse listNodes, while centralizing the Etcd CR fetch for validateEtcdQuorum and validateEtcdNotProgressing through a shared helper or Validate flow. Preserve each validator’s existing check-specific behavior and error results while ensuring fetch errors are handled consistently in one place.
🤖 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/cli/admin/transition/status.go`:
- Around line 69-84: Update the Control Plane Topology output in the
status-display function to show “(not set)” when
infra.Status.ControlPlaneTopology is empty, matching the existing fallback
behavior for the desired spec and Infrastructure Topology status while
preserving the current value when present.
In `@pkg/cli/admin/transition/transition_test.go`:
- Around line 837-840: Handle the error returned by Tracker().Create in the Add
fallback within the transition test setup. If the fallback creation fails,
immediately fail the test using the existing test error-reporting mechanism and
include the returned error, while preserving the current fallback behavior when
Create succeeds.
In `@pkg/cli/admin/transition/transition.go`:
- Around line 509-589: Update the per-operator progress display after the
existing degradedOps block to also render unavailableOps, using the same maxShow
truncation, “(+N more)” count, and newline formatting as the progressing and
degraded listings. Keep the existing unavailable count and operator collection
logic unchanged.
---
Nitpick comments:
In `@pkg/cli/admin/transition/preflight/client_validator_test.go`:
- Around line 233-236: Replace the hardcoded "Supported Transition" expectations
in the supportedTransitionCheck assertions, including the assertion around line
284, with the existing CheckNameSupportedTransition constant. Keep the current
assertion behavior and error messages otherwise unchanged.
- Around line 201-248: Update the affected validation tests, including
TestValidate_UnsupportedTransition and the other noted test cases, to assemble
complete expected validation results and compare them with actual results using
google/go-cmp and cmp.Diff. Replace individual field assertions for status,
checks, names, statuses, severities, and lengths while preserving the existing
expected outcomes and error handling.
In `@pkg/cli/admin/transition/preflight/client_validator.go`:
- Around line 116-123: Eliminate the duplicated resource-fetch logic by making
validateControlPlaneNodeCount, validateExactInfrastructureNodeCount,
validateControlPlaneNodesSchedulable, and validateControlPlaneNodesReady reuse
listNodes, while centralizing the Etcd CR fetch for validateEtcdQuorum and
validateEtcdNotProgressing through a shared helper or Validate flow. Preserve
each validator’s existing check-specific behavior and error results while
ensuring fetch errors are handled consistently in one place.
In `@pkg/cli/admin/transition/preflight/validator_helpers_test.go`:
- Around line 55-250: Extend TestIndividualValidators with table-driven
API-error cases for each validator’s failure path, injecting node list, Etcd
resource Get, and ConfigMap Get errors through fake-clientset reactors in the
relevant setupKube or setupOperator callbacks. Invoke the corresponding
validate* method and set expectedStatus to CheckStatusUnknown while preserving
each check’s expected name and severity.
In `@pkg/cli/admin/transition/status.go`:
- Around line 85-156: Extract shared package-level constants for the operator
config resource name "cluster", the TopologyTransitionControllerProgressing and
TopologyTransitionControllerUpgradeable condition types, and the
PreflightCheckFailed and UnsupportedTransition reasons in status.go, alongside
InfrastructureResourceName. Update the condition fixtures in
pkg/cli/admin/transition/transition_test.go lines 696-826 to use these constants
instead of duplicated literals; both sites require changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| var progressing, degraded, unavailable int | ||
| var progressingOps, degradedOps, unavailableOps []string | ||
|
|
||
| for _, op := range operators.Items { | ||
| var available, prog, deg bool | ||
|
|
||
| for _, condition := range op.Status.Conditions { | ||
| switch condition.Type { | ||
| case configv1.OperatorAvailable: | ||
| available = condition.Status == configv1.ConditionTrue | ||
| case configv1.OperatorProgressing: | ||
| prog = condition.Status == configv1.ConditionTrue | ||
| case configv1.OperatorDegraded: | ||
| deg = condition.Status == configv1.ConditionTrue | ||
| } | ||
| } | ||
|
|
||
| if !available { | ||
| unavailable++ | ||
| unavailableOps = append(unavailableOps, op.Name) | ||
| } | ||
| if prog { | ||
| progressing++ | ||
| progressingOps = append(progressingOps, op.Name) | ||
| } | ||
| if deg { | ||
| degraded++ | ||
| degradedOps = append(degradedOps, op.Name) | ||
| } | ||
| } | ||
|
|
||
| // Check if all operators are stable | ||
| if progressing == 0 && degraded == 0 && unavailable == 0 { | ||
| w.fprintf(" [%d] All cluster operators stable ✓\n\n", attempt) | ||
| w.fprintf("Transition complete! Cluster operators are healthy.\n") | ||
| w.fprintf(" Control Plane: %s\n", targetCP) | ||
| w.fprintf(" Infrastructure: %s\n", targetInfra) | ||
| if w.error() != nil { | ||
| return false, w.error() | ||
| } | ||
| return true, nil // Done - all operators stable | ||
| } | ||
|
|
||
| // Show progress | ||
| var status []string | ||
| if progressing > 0 { | ||
| status = append(status, fmt.Sprintf("%d progressing", progressing)) | ||
| } | ||
| if degraded > 0 { | ||
| status = append(status, fmt.Sprintf("%d degraded", degraded)) | ||
| } | ||
| if unavailable > 0 { | ||
| status = append(status, fmt.Sprintf("%d unavailable", unavailable)) | ||
| } | ||
|
|
||
| w.fprintf(" [%d] %s\n", attempt, strings.Join(status, ", ")) | ||
|
|
||
| // Show which operators (first few) | ||
| const maxShow = 3 | ||
| if len(progressingOps) > 0 { | ||
| showing := progressingOps | ||
| if len(showing) > maxShow { | ||
| showing = showing[:maxShow] | ||
| } | ||
| w.fprintf(" Progressing: %s", strings.Join(showing, ", ")) | ||
| if len(progressingOps) > maxShow { | ||
| w.fprintf(" (+%d more)", len(progressingOps)-maxShow) | ||
| } | ||
| w.fprintf("\n") | ||
| } | ||
| if len(degradedOps) > 0 { | ||
| showing := degradedOps | ||
| if len(showing) > maxShow { | ||
| showing = showing[:maxShow] | ||
| } | ||
| w.fprintf(" Degraded: %s", strings.Join(showing, ", ")) | ||
| if len(degradedOps) > maxShow { | ||
| w.fprintf(" (+%d more)", len(degradedOps)-maxShow) | ||
| } | ||
| w.fprintf("\n") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
unavailableOps is collected but never displayed.
The loop tracks unavailableOps (Line 528-529) and includes its count in the summary line, but the per-operator listing block only prints progressingOps and degradedOps — the actually-unavailable operators (arguably the most actionable signal during a live transition) are never named for the user.
🔧 Suggested fix
if len(degradedOps) > 0 {
showing := degradedOps
if len(showing) > maxShow {
showing = showing[:maxShow]
}
w.fprintf(" Degraded: %s", strings.Join(showing, ", "))
if len(degradedOps) > maxShow {
w.fprintf(" (+%d more)", len(degradedOps)-maxShow)
}
w.fprintf("\n")
}
+ if len(unavailableOps) > 0 {
+ showing := unavailableOps
+ if len(showing) > maxShow {
+ showing = showing[:maxShow]
+ }
+ w.fprintf(" Unavailable: %s", strings.Join(showing, ", "))
+ if len(unavailableOps) > maxShow {
+ w.fprintf(" (+%d more)", len(unavailableOps)-maxShow)
+ }
+ w.fprintf("\n")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var progressing, degraded, unavailable int | |
| var progressingOps, degradedOps, unavailableOps []string | |
| for _, op := range operators.Items { | |
| var available, prog, deg bool | |
| for _, condition := range op.Status.Conditions { | |
| switch condition.Type { | |
| case configv1.OperatorAvailable: | |
| available = condition.Status == configv1.ConditionTrue | |
| case configv1.OperatorProgressing: | |
| prog = condition.Status == configv1.ConditionTrue | |
| case configv1.OperatorDegraded: | |
| deg = condition.Status == configv1.ConditionTrue | |
| } | |
| } | |
| if !available { | |
| unavailable++ | |
| unavailableOps = append(unavailableOps, op.Name) | |
| } | |
| if prog { | |
| progressing++ | |
| progressingOps = append(progressingOps, op.Name) | |
| } | |
| if deg { | |
| degraded++ | |
| degradedOps = append(degradedOps, op.Name) | |
| } | |
| } | |
| // Check if all operators are stable | |
| if progressing == 0 && degraded == 0 && unavailable == 0 { | |
| w.fprintf(" [%d] All cluster operators stable ✓\n\n", attempt) | |
| w.fprintf("Transition complete! Cluster operators are healthy.\n") | |
| w.fprintf(" Control Plane: %s\n", targetCP) | |
| w.fprintf(" Infrastructure: %s\n", targetInfra) | |
| if w.error() != nil { | |
| return false, w.error() | |
| } | |
| return true, nil // Done - all operators stable | |
| } | |
| // Show progress | |
| var status []string | |
| if progressing > 0 { | |
| status = append(status, fmt.Sprintf("%d progressing", progressing)) | |
| } | |
| if degraded > 0 { | |
| status = append(status, fmt.Sprintf("%d degraded", degraded)) | |
| } | |
| if unavailable > 0 { | |
| status = append(status, fmt.Sprintf("%d unavailable", unavailable)) | |
| } | |
| w.fprintf(" [%d] %s\n", attempt, strings.Join(status, ", ")) | |
| // Show which operators (first few) | |
| const maxShow = 3 | |
| if len(progressingOps) > 0 { | |
| showing := progressingOps | |
| if len(showing) > maxShow { | |
| showing = showing[:maxShow] | |
| } | |
| w.fprintf(" Progressing: %s", strings.Join(showing, ", ")) | |
| if len(progressingOps) > maxShow { | |
| w.fprintf(" (+%d more)", len(progressingOps)-maxShow) | |
| } | |
| w.fprintf("\n") | |
| } | |
| if len(degradedOps) > 0 { | |
| showing := degradedOps | |
| if len(showing) > maxShow { | |
| showing = showing[:maxShow] | |
| } | |
| w.fprintf(" Degraded: %s", strings.Join(showing, ", ")) | |
| if len(degradedOps) > maxShow { | |
| w.fprintf(" (+%d more)", len(degradedOps)-maxShow) | |
| } | |
| w.fprintf("\n") | |
| } | |
| var progressing, degraded, unavailable int | |
| var progressingOps, degradedOps, unavailableOps []string | |
| for _, op := range operators.Items { | |
| var available, prog, deg bool | |
| for _, condition := range op.Status.Conditions { | |
| switch condition.Type { | |
| case configv1.OperatorAvailable: | |
| available = condition.Status == configv1.ConditionTrue | |
| case configv1.OperatorProgressing: | |
| prog = condition.Status == configv1.ConditionTrue | |
| case configv1.OperatorDegraded: | |
| deg = condition.Status == configv1.ConditionTrue | |
| } | |
| } | |
| if !available { | |
| unavailable++ | |
| unavailableOps = append(unavailableOps, op.Name) | |
| } | |
| if prog { | |
| progressing++ | |
| progressingOps = append(progressingOps, op.Name) | |
| } | |
| if deg { | |
| degraded++ | |
| degradedOps = append(degradedOps, op.Name) | |
| } | |
| } | |
| // Check if all operators are stable | |
| if progressing == 0 && degraded == 0 && unavailable == 0 { | |
| w.fprintf(" [%d] All cluster operators stable ✓\n\n", attempt) | |
| w.fprintf("Transition complete! Cluster operators are healthy.\n") | |
| w.fprintf(" Control Plane: %s\n", targetCP) | |
| w.fprintf(" Infrastructure: %s\n", targetInfra) | |
| if w.error() != nil { | |
| return false, w.error() | |
| } | |
| return true, nil // Done - all operators stable | |
| } | |
| // Show progress | |
| var status []string | |
| if progressing > 0 { | |
| status = append(status, fmt.Sprintf("%d progressing", progressing)) | |
| } | |
| if degraded > 0 { | |
| status = append(status, fmt.Sprintf("%d degraded", degraded)) | |
| } | |
| if unavailable > 0 { | |
| status = append(status, fmt.Sprintf("%d unavailable", unavailable)) | |
| } | |
| w.fprintf(" [%d] %s\n", attempt, strings.Join(status, ", ")) | |
| // Show which operators (first few) | |
| const maxShow = 3 | |
| if len(progressingOps) > 0 { | |
| showing := progressingOps | |
| if len(showing) > maxShow { | |
| showing = showing[:maxShow] | |
| } | |
| w.fprintf(" Progressing: %s", strings.Join(showing, ", ")) | |
| if len(progressingOps) > maxShow { | |
| w.fprintf(" (+%d more)", len(progressingOps)-maxShow) | |
| } | |
| w.fprintf("\n") | |
| } | |
| if len(degradedOps) > 0 { | |
| showing := degradedOps | |
| if len(showing) > maxShow { | |
| showing = showing[:maxShow] | |
| } | |
| w.fprintf(" Degraded: %s", strings.Join(showing, ", ")) | |
| if len(degradedOps) > maxShow { | |
| w.fprintf(" (+%d more)", len(degradedOps)-maxShow) | |
| } | |
| w.fprintf("\n") | |
| } | |
| if len(unavailableOps) > 0 { | |
| showing := unavailableOps | |
| if len(showing) > maxShow { | |
| showing = showing[:maxShow] | |
| } | |
| w.fprintf(" Unavailable: %s", strings.Join(showing, ", ")) | |
| if len(unavailableOps) > maxShow { | |
| w.fprintf(" (+%d more)", len(unavailableOps)-maxShow) | |
| } | |
| w.fprintf("\n") | |
| } |
🤖 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/cli/admin/transition/transition.go` around lines 509 - 589, Update the
per-operator progress display after the existing degradedOps block to also
render unavailableOps, using the same maxShow truncation, “(+N more)” count, and
newline formatting as the progressing and degraded listings. Keep the existing
unavailable count and operator collection logic unchanged.
Update vendored dependencies to include Infrastructure.spec.controlPlaneTopology field from OCPEDGE-2408 (Mutable Topology enhancement). Key changes: - github.com/openshift/api: updated to commit 8f87a41c3 - github.com/openshift/client-go: updated to master Transitive dependency updates pulled by go mod vendor: - github.com/go-openapi/swag (refactored internal structure) - github.com/emicklei/go-restful/v3 - Various other indirect dependencies
8de1730 to
5ab54fb
Compare
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
5ab54fb to
25a6750
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
openshift-ci
Bot
removed
the
do-not-merge/invalid-owners-file
Implement SNO to HA topology transition command with comprehensive preflight validation, user confirmation prompts, and status monitoring. Features: - Three operational modes: discovery, initiate, status - Tiered preflight validation (Error and Warning severity checks) - Client-side feature gate (OC_ENABLE_CMD_TRANSITION_TOPOLOGY) - Server-side validation (MutableTopology feature gate check) - User confirmation with bypass support (--allow-transition-with-warnings) Preflight checks (10 total): - Error-severity (blocking): Supported transition, feature gate enabled - Warning-severity (bypassable): Cluster operators stable, control plane node count/schedulability/readiness, etcd quorum/stability/voting members Implementation structure: - pkg/cli/admin/transition/transition.go: Main command orchestration - pkg/cli/admin/transition/status.go: Status monitoring subcommand - pkg/cli/admin/transition/preflight/: Validation logic and types - pkg/cli/admin/transition/constants.go: Shared resource names and condition types Status command design: - Single source of truth: reads from cluster-config-operator conditions - Displays TopologyTransitionControllerProgressing and TopologyTransitionControllerUpgradeable conditions - Shows Control Plane Topology (spec + status) - Shows Infrastructure Topology (status only - spec doesn't exist) - Acts as thin display layer for CCO status Transition command delegates monitoring to status subcommand: - Initiates transition by patching Infrastructure.spec.controlPlaneTopology - Prints usage instructions for monitoring with status command - Does not poll operators (CCO handles operator orchestration) - Clean separation: transition initiates, status monitors Test coverage: - Command structure, validation, discovery mode, initiate mode - Status command output for 5 scenarios (in progress, stable, failed, etc.) - Preflight validators with fake clients and error injection - Comprehensive API error handling with reactor-based fault injection - Assertions use cmp.Diff for better failure output Command is gated behind OC_ENABLE_CMD_TRANSITION_TOPOLOGY environment variable and only appears in cluster management commands when enabled. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Define ownership for the new oc adm transition topology command by adding OWNERS files and team aliases. Changes: - OWNERS_ALIASES: Add openshift-edge-reviewers and openshift-edge-approvers with full team membership lists - pkg/cli/admin/transition/OWNERS: Reference team aliases for approvals/reviews - pkg/cli/admin/transition/preflight/OWNERS: Same ownership for preflight subpackage This ensures proper code review routing for the topology transition command. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
25a6750 to
dc9ff4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cli/admin/transition/preflight/client_validator.go (1)
169-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the string literal directly.
fmt.Sprintfis unnecessary here because there are no format directives or arguments.🤖 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/cli/admin/transition/preflight/client_validator.go` around lines 169 - 174, Update the CheckNameSupportedTransition failure message in the cpTransitioning guard to pass the static text directly, removing the unnecessary fmt.Sprintf call while preserving the existing return 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/cli/admin/transition/preflight/client_validator.go`:
- Around line 373-403: Update validateControlPlaneNodesSchedulable to exclude
control-plane nodes with either NoSchedule or NoExecute taints when counting
schedulable nodes. Reuse or extend hasNoScheduleTaint with the appropriate
taint-effect checks while preserving the existing unschedulable and
required-count behavior.
---
Nitpick comments:
In `@pkg/cli/admin/transition/preflight/client_validator.go`:
- Around line 169-174: Update the CheckNameSupportedTransition failure message
in the cpTransitioning guard to pass the static text directly, removing the
unnecessary fmt.Sprintf call while preserving the existing return 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
| func (v *ClientSideValidator) validateControlPlaneNodesSchedulable(ctx context.Context, required int) CheckResult { | ||
| nodes, checkResult := v.listNodes(ctx, CheckNameControlPlaneNodesSchedulable, CheckSeverityWarning) | ||
| if checkResult != nil { | ||
| return *checkResult | ||
| } | ||
|
|
||
| schedulable := 0 | ||
| for _, node := range nodes.Items { | ||
| if !isControlPlaneNode(&node) { | ||
| continue | ||
| } | ||
| if !node.Spec.Unschedulable && !hasNoScheduleTaint(&node) { | ||
| schedulable++ | ||
| } | ||
| } | ||
|
|
||
| if schedulable < required { | ||
| return CheckResult{ | ||
| Name: CheckNameControlPlaneNodesSchedulable, | ||
| Severity: CheckSeverityWarning, | ||
| Status: CheckStatusFailed, | ||
| Message: fmt.Sprintf("need %d, have %d", required, schedulable), | ||
| } | ||
| } | ||
|
|
||
| return CheckResult{ | ||
| Name: CheckNameControlPlaneNodesSchedulable, | ||
| Severity: CheckSeverityWarning, | ||
| Status: CheckStatusPassed, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and find the taint helper/usages.
git ls-files pkg/cli/admin/transition/preflight/client_validator.go
ast-grep outline pkg/cli/admin/transition/preflight/client_validator.go --view expanded || true
rg -n "hasNoScheduleTaint|TaintEffectNoSchedule|TaintEffectNoExecute|validateControlPlaneNodesSchedulable" pkg/cli/admin/transition/preflight/client_validator.go
sed -n '330,560p' pkg/cli/admin/transition/preflight/client_validator.goRepository: openshift/oc
Length of output: 9510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Pull Kubernetes taint effect semantics from the vendored or module source if present.
rg -n "type TaintEffect|string TaintEffect|NoSchedule|NoExecute" "$(go env GOPATH 2>/dev/null)/pkg/mod" -g '*.go' | head -n 80 || trueRepository: openshift/oc
Length of output: 15979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the Kubernetes source is available in the repo/module cache, inspect the taint handling docs or code.
python3 - <<'PY'
import os,glob
paths = glob.glob(os.path.expanduser('~/go/pkg/mod/**/taint*.go'), recursive=True)
for p in paths[:20]:
print(p)
PYRepository: openshift/oc
Length of output: 516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '360,560p' pkg/cli/admin/transition/preflight/client_validator.goRepository: openshift/oc
Length of output: 5798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Kubernetes scheduler taint handling in the module cache.
rg -n "NoExecute|NoSchedule|taint" /home/jailuser/go/pkg/mod/k8s.io/*`@v0.35.2/pkg` -g '*.go' | \
rg 'scheduler|predicates|framework|taint' | head -n 200Repository: openshift/oc
Length of output: 27091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the scheduler filter that decides whether taints block placement.
for f in $(rg -l "NoExecute|NoSchedule" /home/jailuser/go/pkg/mod/k8s.io/*`@v0.35.2/pkg` -g '*.go' | head -n 50); do
case "$f" in
*scheduler*|*framework*|*taint*|*node* )
echo "### $f"
rg -n "NoExecute|NoSchedule|Taint|tolerat" "$f" | head -n 40
;;
esac
doneRepository: openshift/oc
Length of output: 9570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the core API taint documentation directly.
sed -n '1,120p' /home/jailuser/go/pkg/mod/k8s.io/api@v0.35.2/core/v1/taint.go
sed -n '1,120p' /home/jailuser/go/pkg/mod/k8s.io/api@v0.35.2/core/v1/toleration.goRepository: openshift/oc
Length of output: 5351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the helper that decides whether a taint is tolerated.
rg -n "func .*Tolerat|Toleration.*Taint|Tolerates" /home/jailuser/go/pkg/mod/k8s.io/api@v0.35.2 /home/jailuser/go/pkg/mod/k8s.io/apimachinery@v0.35.2 /home/jailuser/go/pkg/mod/k8s.io/kubernetes@v0.35.2 -g '*.go' | head -n 120Repository: openshift/oc
Length of output: 12658
🌐 Web query:
Kubernetes NoExecute taint blocks new pod scheduling official docs
💡 Result:
Yes, the NoExecute taint effect prevents new pods from being scheduled onto a node unless they have a matching toleration [1][2][3]. In addition to blocking new pod scheduling, the NoExecute effect mandates the eviction of any existing pods already running on the node that do not tolerate the taint [4][1][2]. In summary, the three standard taint effects function as follows: - NoExecute: Blocks new pods from scheduling AND evicts existing non-tolerating pods [1][2][5]. - NoSchedule: Blocks new pods from scheduling, but allows existing pods to continue running [1][3][5]. - PreferNoSchedule: A "soft" version of NoSchedule where the scheduler attempts to avoid placing new pods on the node but does not strictly prohibit it [1][2][5]. Kubernetes also uses NoExecute taints internally to manage node conditions, such as when a node becomes unreachable or not ready, to trigger pod eviction [6][7][5]. While technical documentation confirms this behavior [1], users should be aware that NoExecute is more aggressive than NoSchedule due to its impact on already-running workloads [2][3].
Citations:
- 1: https://kubernetes.io/docs/reference/kubernetes-api/definitions/toleration-v1/
- 2: https://k8s.info/docs/intermediate/scheduling
- 3: https://notes.kodekloud.com/docs/Certified-Kubernetes-Administrator-CKA/Scheduling/Taints-and-Tolerations/page
- 4: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/
- 5: https://codingprotocols.com/tutorials/kubernetes-taints-tolerations
- 6: https://kubernetes.io/docs/concepts/architecture/nodes/
- 7: https://kubernetes.io/docs/reference/labels-annotations-taints/
Count NoExecute taints here too
NoExecute also blocks new pod scheduling unless tolerated, so this helper can overcount control-plane nodes as schedulable.
🤖 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/cli/admin/transition/preflight/client_validator.go` around lines 373 -
403, Update validateControlPlaneNodesSchedulable to exclude control-plane nodes
with either NoSchedule or NoExecute taints when counting schedulable nodes.
Reuse or extend hasNoScheduleTaint with the appropriate taint-effect checks
while preserving the existing unschedulable and required-count behavior.
|
@jaypoulz: The following tests 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. |
| @@ -0,0 +1,7 @@ | |||
| # See the OWNERS docs: https://git.k8s.io/community/contributors/guide/owners.md | |||
There was a problem hiding this comment.
We probably do not need this file. Upper one should also cover here.
| ) | ||
|
|
||
| var ( | ||
| transitionLong = templates.LongDesc(` |
There was a problem hiding this comment.
This is too verbose. Can you update it more concise?
| AllowTransitionWithWarnings bool | ||
|
|
||
| // Context for API calls | ||
| ctx context.Context |
There was a problem hiding this comment.
This is not best practice. We can create ctx whenever it is needed.
| } | ||
|
|
||
| // errorWriter accumulates write errors to reduce repetitive error handling | ||
| type errorWriter struct { |
There was a problem hiding this comment.
I think we definitely do not need this struct. Why can't we use fmt?
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVar(&o.ControlPlane, "control-plane", "", "Target control plane topology (HighlyAvailable or SingleReplica)") |
There was a problem hiding this comment.
| cmd.Flags().StringVar(&o.ControlPlane, "control-plane", "", "Target control plane topology (HighlyAvailable or SingleReplica)") | |
| cmd.Flags().StringVar(&o.ControlPlane, "control-plane", o.ControlPlane, "Target control plane topology (HighlyAvailable or SingleReplica)") | |
| // TransitionOptions holds all options for the topology transition command | ||
| type TransitionOptions struct { | ||
| // Target control plane topology (--control-plane flag) | ||
| ControlPlane string |
| cmd.Flags().BoolVar(&o.AllowTransitionWithWarnings, "allow-transition-with-warnings", false, "Bypass warning-severity preflight check failures (requires --confirm)") | ||
|
|
||
| // Add status subcommand | ||
| cmd.AddCommand(NewCmdStatus(f, o)) |
There was a problem hiding this comment.
Can we define this command under transition?. This is too nested.
There was a problem hiding this comment.
I thought about this. I'm fine with pulling it up to a higher level, but I'm worried it won't age as we look at other kinds of Infrastructure transitions (e.g. "platform" transitions).
My thinking was it would be helpful to be able to monitor separate transitions seperately. The advantage of your suggestion is that we be encourage a maximum of one active transition of any kind at a time, which is probably best practice.
There was a problem hiding this comment.
This is not a blocker for me and it is up to your team. But in my opinion, it is too nested.
| func (o *TransitionOptions) Run() error { | ||
| // Create a timeout-scoped context for initial API calls (2 minutes) | ||
| // This prevents unbounded waits on cluster API calls during validation | ||
| apiCtx, apiCancel := context.WithTimeout(o.ctx, 2*time.Minute) |
There was a problem hiding this comment.
move timeout value to constant
|
|
||
| // runDiscoveryMode displays current topology and available transitions | ||
| func (o *TransitionOptions) runDiscoveryMode(currentCP, currentInfra configv1.TopologyMode) error { | ||
| w := &errorWriter{w: o.Out} |
There was a problem hiding this comment.
Simple fmt.Fprintf should be sufficient
| ) | ||
|
|
||
| // Kubernetes and OpenShift resource names | ||
| const ( |
There was a problem hiding this comment.
We don't need to store them in a separate file. This can be moved under transition.go
3 participants
Summary by CodeRabbit
oc adm transition topologyworkflow to discover, dry-run validate, and initiate control-plane/infrastructure topology transitions, with confirmation and an option to proceed despite warnings.oc adm transition topology statusto display desired vs current topology and transition controller readiness, including etcd quorum/voting-member details.