Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

MCO-2205: Make ImageModeStatusReporting MCP count test more resilient on SNO#6303

Open
isabella-janssen wants to merge 2 commits into
openshift:mainopenshift/machine-config-operator:mainfrom
isabella-janssen:disrutive-suite-stabilizationisabella-janssen/machine-config-operator:disrutive-suite-stabilizationCopy head branch name to clipboard
Open

MCO-2205: Make ImageModeStatusReporting MCP count test more resilient on SNO#6303
isabella-janssen wants to merge 2 commits into
openshift:mainopenshift/machine-config-operator:mainfrom
isabella-janssen:disrutive-suite-stabilizationisabella-janssen/machine-config-operator:disrutive-suite-stabilizationCopy head branch name to clipboard

Conversation

@isabella-janssen

@isabella-janssen isabella-janssen commented Jul 14, 2026

Copy link
Copy Markdown
Member

- What I did
This adds some resiliency to the ImageModeStatusReporting test to help prevent against flakes as seen in https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3/2075276858378686464.

- How to verify it
All ImageModeStatusReporting tests should continue passing in the non-SNO suite and start consistently passing in the SNO suite.

- Description for the changelog
MCO-2205: Make ImageModeStatusReporting MCP count test more resilient on SNO

Summary by CodeRabbit

  • New Features
    • Added Topology:<name> label filtering for extension test selection (in addition to existing platform include/exclude filters).
  • Tests
    • Improved machine-count transition validation using deadline-based retries (with different time budgets for layered vs non-layered).
    • Reduced flaky failures when reading MCP/node machine counts by classifying transient connection issues as retryable.
    • Hardened MCP readiness checks to ensure status has caught up after rendered configuration changes.
    • Improved test stability by skipping earlier when the cluster API is unreachable and guarding cleanup when setup is skipped.
    • Simplified and adjusted several test setups for more reliable initialization and teardown.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

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

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

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 14, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: This pull request references MCO-2205 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 spike to target the "5.0.0" version, but no target version was set.

Details

In response to this:

- What I did

- How to verify it

- Description for the changelog

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

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2026
@openshift-ci

openshift-ci Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Note

Reviews paused

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

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The extended tests now use deadline-based machine-count retries, shared transient connection handling, stricter MachineConfigPool readiness checks, kubeconfig-based cluster reachability skips, and topology-based extension test filtering.

Changes

Extended test reliability and filtering

Layer / File(s) Summary
Machine-count transition validation
test/extended/image_mode_status_reporting.go
Replaces iteration-based retries with layered and non-layered mismatch deadlines and uses shared transient connection classification for MCP and node retrieval.
Rendered configuration readiness
test/extended/machineconfigpool.go
Waits until the MCP spec and status rendered configuration names match before reporting rollout completion.
Cluster availability and topology handling
test/extended-priv/util/clusters.go, test/extended-priv/mco_bootimages_skew.go, test/extended/custom_pool_booting.go
Adds kubeconfig-based skipping for unreachable clusters, removes single-node guards from both suites, and protects cleanup when setup is skipped.
Extension test selection
cmd/machine-config-tests-ext/main.go
Adds Topology:<name> label filtering using topology equality predicates.

Estimated code review effort: 3 (Moderate) | ~25 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error Raw client-go errors are logged in SkipIfClusterUnreachable and MCP retry paths, and those errors can include API hostnames/URLs. Avoid logging full err values here; log a sanitized message or error class only, and keep the raw error out of test logs.
Test Structure And Quality ⚠️ Warning custom_pool_booting cleanup can nil-deref clonedMachineSet, and applyLabelFilters still ORs Platform/Topology includes, so test cleanup and selection are brittle. Add a clonedMachineSet nil guard before cleanup and combine Platform/Topology predicates with AND (or one Include) so specs with both labels match only when both do.
✅ Passed checks (13 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: improving ImageModeStatusReporting MCP count test resilience on SNO.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed Touched Ginkgo titles are static string literals; no dynamic formatting, concatenation, timestamps, UUIDs, or generated values appear in test names.
Microshift Test Compatibility ✅ Passed No new Ginkgo tests were added; the modified e2e cases are already tagged [apigroup:machineconfiguration.openshift.io], so MicroShift skips them.
Single Node Openshift (Sno) Test Compatibility ✅ Passed HA-leaning suites are topology-gated (Topology:ha) and/or skip on unscalable workers; ImageModeStatusReporting already falls back to master on SNO.
Topology-Aware Scheduling Compatibility ✅ Passed PASS: The patch only changes test harness/label filtering and cluster-unreachability skips; it adds no manifests, controllers, affinities, nodeSelectors, or replica logic.
Ote Binary Stdout Contract ✅ Passed No stdout writes were added in process-level code; touched files only add GinkgoWriter-backed logging and skip helpers, with no fmt.Print/os.Stdout/klog stdout use.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No added Ginkgo test code uses IPv4 literals or public internet; the new skip helper only queries the cluster API.
No-Weak-Crypto ✅ Passed No weak crypto, custom crypto, or non-constant-time secret comparisons appear in the modified files; changes are test/control-flow only.
Container-Privileges ✅ Passed No touched file introduces privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN/allowPrivilegeEscalation settings; changes are test flow and label logic only.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci

openshift-ci Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: isabella-janssen

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

The pull request process is described here

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

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2026
@isabella-janssen
isabella-janssen force-pushed the disrutive-suite-stabilization branch from 3cfb394 to da53bae Compare July 14, 2026 16:47
@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b06371a0-7fa3-11f1-8330-cd5287550209-0

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/extended/image_mode_status_reporting.go (1)

403-446: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Prefer StopTrying(...).Now() over g.Fail() inside the Eventually poll closure.

Calling g.Fail(...) (ginkgo's Fail) from inside the func() bool polled by o.Eventually(...) is non-idiomatic. Gomega ships StopTrying(message).Now() specifically for aborting Eventually/Consistently early with a failure, and its failure message correctly surfaces at the top-level assertion rather than relying on a raw Fail panic unwinding through Gomega's AsyncAssertion internals. This also lets you drop the manual reset/format boilerplate. Additionally, the message grammar is slightly off ("do not matched expected values").

♻️ Suggested refactor
 		if !countsMatch {
 			if firstMismatchTime.IsZero() {
 				firstMismatchTime = time.Now()
 			}
 			elapsed := time.Since(firstMismatchTime)
 			if elapsed > mismatchDeadline {
-				g.Fail(fmt.Sprintf("MCP '%v' machine counts do not matched expected values.",
-					mcpName))
+				o.StopTrying(fmt.Sprintf("MCP '%v' machine counts did not match expected values for over %v.",
+					mcpName, mismatchDeadline)).Now()
 			}
 			logger.Infof("Counts do not match yet. Will retry.")
 			return false
 		}

Please double check with a quick local/CI run that g.Fail() here reliably short-circuits before the full 15/45-minute Eventually timeout rather than being retried — the exact interaction depends on the Gomega/Ginkgo version pinned in go.mod.

🤖 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 `@test/extended/image_mode_status_reporting.go` around lines 403 - 446, Update
validateMCPMachineCountTransitions to replace the g.Fail call inside the
Eventually polling closure with Gomega’s StopTrying(...).Now() when elapsed
exceeds mismatchDeadline. Preserve the existing MCP name and mismatch context in
the failure message, correct the grammar to “do not match expected values,” and
remove any now-unnecessary manual failure handling while keeping retry behavior
unchanged before the deadline.
🤖 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 `@test/extended/image_mode_status_reporting.go`:
- Around line 403-446: Update validateMCPMachineCountTransitions to replace the
g.Fail call inside the Eventually polling closure with Gomega’s
StopTrying(...).Now() when elapsed exceeds mismatchDeadline. Preserve the
existing MCP name and mismatch context in the failure message, correct the
grammar to “do not match expected values,” and remove any now-unnecessary manual
failure handling while keeping retry behavior unchanged before the deadline.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 41e75ca0-1e5e-4476-ae47-3de686557219

📥 Commits

Reviewing files that changed from the base of the PR and between 63774ee and da53bae.

📒 Files selected for processing (1)
  • test/extended/image_mode_status_reporting.go

@isabella-janssen
isabella-janssen force-pushed the disrutive-suite-stabilization branch from d5c0ce6 to 8f439d0 Compare July 14, 2026 17:17
@isabella-janssen

Copy link
Copy Markdown
Member Author

/test verify

1 similar comment
@isabella-janssen

Copy link
Copy Markdown
Member Author

/test verify

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1b487920-812a-11f1-80b8-6d96ec2f2e5a-0

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/81ff7890-8156-11f1-9b24-53e708c2330a-0

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
test/extended-priv/util/clusters.go (1)

131-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Pass a cancelable context into the topology probe.

context.TODO() is not tied to the Ginkgo spec’s cancellation. Accept a context.Context in SkipIfClusterUnreachableOrSNO and pass it to Get, while retaining the 10-second client timeout as a fallback.

As per path instructions, Go code should use context.Context for cancellation and timeouts.

🤖 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 `@test/extended-priv/util/clusters.go` around lines 131 - 132, Update
SkipIfClusterUnreachableOrSNO to accept a context.Context parameter and pass it
to the Infrastructures().Get call instead of context.TODO(). Preserve the
existing 10-second client timeout as the fallback timeout.

Source: Path instructions

🤖 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 `@test/extended-priv/mco_bootimages_skew.go`:
- Around line 22-26: The early skip hooks can run teardown before setup
initializes the resources it restores or deletes. In
test/extended-priv/mco_bootimages_skew.go:22-26, guard the AfterEach SetSpec
restore so it returns when machineConfiguration was never created; in
test/extended/custom_pool_booting.go:25-29, guard machineset cleanup so it runs
only after both machineClient and clonedMachineSet are initialized.

In `@test/extended-priv/util/clusters.go`:
- Around line 120-134: Sanitize the error details passed to e2eskipper.Skipf in
the kubeconfig, client, and infrastructure lookup paths, replacing raw %v output
with safe generic skip reasons that do not expose endpoints, hostnames, paths,
or other environment data. Update the error branches around config creation,
configv1client.NewForConfig, and Infrastructures().Get while preserving their
existing return behavior.
- Around line 131-136: Update the Infrastructure/cluster GET error handling in
the surrounding cluster-topology helper to call e2eskipper.Skipf only for
connectivity or timeout errors. Return or propagate all other errors, including
forbidden, not found, authentication, and API-server failures, so they fail CI
instead of being skipped.

---

Nitpick comments:
In `@test/extended-priv/util/clusters.go`:
- Around line 131-132: Update SkipIfClusterUnreachableOrSNO to accept a
context.Context parameter and pass it to the Infrastructures().Get call instead
of context.TODO(). Preserve the existing 10-second client timeout as the
fallback timeout.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 26925b3d-c171-49a3-89c8-d82f28840745

📥 Commits

Reviewing files that changed from the base of the PR and between c3b672c and f149c10.

📒 Files selected for processing (3)
  • test/extended-priv/mco_bootimages_skew.go
  • test/extended-priv/util/clusters.go
  • test/extended/custom_pool_booting.go

Comment thread test/extended-priv/mco_bootimages_skew.go
Comment thread test/extended-priv/util/clusters.go Outdated
Comment on lines +120 to +134
e2eskipper.Skipf("Cannot determine cluster topology (config error): %v", err)
return
}
cfg.Timeout = 10 * time.Second

configClient, err := configv1client.NewForConfig(cfg)
if err != nil {
e2eskipper.Skipf("Cannot determine cluster topology (client error): %v", err)
return
}

infra, err := configClient.ConfigV1().Infrastructures().Get(
context.TODO(), "cluster", metav1.GetOptions{})
if err != nil {
e2eskipper.Skipf("Cannot determine cluster topology, cluster may be unreachable: %v", err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Sanitize raw errors in skip output.

Skipf publishes %v from kubeconfig and API errors, which may contain API endpoints, internal hostnames, paths, or other environment details. Remove or redact the raw error before exposing it in the skip reason.

As per coding guidelines, logging must not expose internal hostnames or other sensitive environment data.

🤖 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 `@test/extended-priv/util/clusters.go` around lines 120 - 134, Sanitize the
error details passed to e2eskipper.Skipf in the kubeconfig, client, and
infrastructure lookup paths, replacing raw %v output with safe generic skip
reasons that do not expose endpoints, hostnames, paths, or other environment
data. Update the error branches around config creation,
configv1client.NewForConfig, and Infrastructures().Get while preserving their
existing return behavior.

Source: Coding guidelines

Comment thread test/extended-priv/util/clusters.go Outdated
@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9326ebf0-8159-11f1-8cc3-f224bab56933-0

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 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 `@cmd/machine-config-tests-ext/main.go`:
- Around line 45-53: Update the filter-building logic around the specs.Walk
callback so platform and topology predicates are collected separately, then
combined with et.And(...) for specs containing both label types instead of
passing each predicate directly to spec.Include. Preserve existing behavior for
specs with only one filter group, and add a regression test covering a spec with
both Platform: and Topology: labels.

In `@test/extended-priv/util/clusters.go`:
- Around line 143-150: Update isConnectionError to avoid treating every
net.Error, including *url.Error setup or TLS failures, as a connectivity
failure. Inspect the underlying wrapped cause and return true only for genuine
timeout or transient connection errors; preserve false for certificate and other
request-setup failures.

In `@test/extended/custom_pool_booting.go`:
- Around line 53-55: In the cleanup flow after the existing machineClient guard
and custom-MCP cleanup, add a separate clonedMachineSet nil check before the
machineset cleanup block. Return early when clonedMachineSet is nil so the
subsequent clonedMachineSet.Name dereference is never reached, while preserving
the existing cleanup behavior when it is assigned.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 2dc103a2-0d88-4c49-bac8-78830e097c68

📥 Commits

Reviewing files that changed from the base of the PR and between f149c10 and 1be3c1a.

📒 Files selected for processing (4)
  • cmd/machine-config-tests-ext/main.go
  • test/extended-priv/mco_bootimages_skew.go
  • test/extended-priv/util/clusters.go
  • test/extended/custom_pool_booting.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/extended-priv/mco_bootimages_skew.go

Comment thread cmd/machine-config-tests-ext/main.go Outdated
Comment on lines +45 to +53
// Apply Topology label filters: tests with Topology:topologyname only run on that topology
specs.Walk(func(spec *et.ExtensionTestSpec) {
for label := range spec.Labels {
if strings.HasPrefix(label, "Topology:") {
topologyName := strings.TrimPrefix(label, "Topology:")
spec.Include(et.TopologyEquals(topologyName))
}
}
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

go doc github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests.ExtensionTestSpec.Include
go doc github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests.And
go doc github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests.Or
rg -n --glob '*.go' 'Platform:|Topology:' .

Repository: openshift/machine-config-operator

Length of output: 15217


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

cat -n cmd/machine-config-tests-ext/main.go | sed -n '1,80p'

Repository: openshift/machine-config-operator

Length of output: 3443


Combine platform and topology filters with AND. spec.Include(...) ORs successive predicates, so a test carrying both Platform: and Topology: labels matches either one instead of both. Collect each label group separately and join them with et.And(...); add a regression test for a spec that has both labels.

🤖 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 `@cmd/machine-config-tests-ext/main.go` around lines 45 - 53, Update the
filter-building logic around the specs.Walk callback so platform and topology
predicates are collected separately, then combined with et.And(...) for specs
containing both label types instead of passing each predicate directly to
spec.Include. Preserve existing behavior for specs with only one filter group,
and add a regression test covering a spec with both Platform: and Topology:
labels.

Comment thread test/extended-priv/util/clusters.go
Comment on lines +53 to +55
if machineClient == nil {
return
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard cleanup before dereferencing clonedMachineSet.

machineClient can be non-nil when setup or the test aborts before clonedMachineSet is assigned. Cleanup then reaches Line 67 and dereferences clonedMachineSet.Name, causing a nil-pointer panic and masking the original failure. Keep the existing client guard, but add a separate clonedMachineSet == nil check after custom-MCP cleanup and before the machineset cleanup block.

Proposed fix
 if machineClient == nil {
 	return
 }
 exutil.By("Performing cleanup")
 logger.Infof("Cleaning up custom MCP %s", customMCPName)
 // Cleanup: Delete custom MCP if it exists
 deleteMCPErr := extpriv.DeleteCustomMCP(oc, customMCPName)
 ...
+if clonedMachineSet == nil {
+	return
+}
 
 // Cleanup: Scale down and delete the cloned machineset if it exists
 clonedMachineSet, err := machineClient.MachineV1beta1().MachineSets(MAPINamespace).Get(ctx, clonedMachineSet.Name, metav1.GetOptions{})
📝 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.

Suggested change
if machineClient == nil {
return
}
if machineClient == nil {
return
}
exutil.By("Performing cleanup")
logger.Infof("Cleaning up custom MCP %s", customMCPName)
// Cleanup: Delete custom MCP if it exists
deleteMCPErr := extpriv.DeleteCustomMCP(oc, customMCPName)
...
if clonedMachineSet == nil {
return
}
// Cleanup: Scale down and delete the cloned machineset if it exists
clonedMachineSet, err := machineClient.MachineV1beta1().MachineSets(MAPINamespace).Get(ctx, clonedMachineSet.Name, metav1.GetOptions{})
🤖 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 `@test/extended/custom_pool_booting.go` around lines 53 - 55, In the cleanup
flow after the existing machineClient guard and custom-MCP cleanup, add a
separate clonedMachineSet nil check before the machineset cleanup block. Return
early when clonedMachineSet is nil so the subsequent clonedMachineSet.Name
dereference is never reached, while preserving the existing cleanup behavior
when it is assigned.

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ceda9e40-81df-11f1-976d-dfdf75693c42-0

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/855c82d0-81f1-11f1-91b8-134f6428da21-0

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5ad58b10-821e-11f1-8612-ecb1af57d5e6-0

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/50d295a0-8434-11f1-86a6-29d32b75f8d9-0

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4eb0ed50-8464-11f1-92f2-bed48f84dad0-0

@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 6 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/60e5d8d0-8506-11f1-9615-21d6f680990f-0

@isabella-janssen
isabella-janssen force-pushed the disrutive-suite-stabilization branch 2 times, most recently from 37290a4 to 5fbb686 Compare July 21, 2026 16:31
@isabella-janssen
isabella-janssen force-pushed the disrutive-suite-stabilization branch from 5fbb686 to 8a80be1 Compare July 21, 2026 16:32
@isabella-janssen
isabella-janssen marked this pull request as ready for review July 21, 2026 16:32
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2026
@isabella-janssen

Copy link
Copy Markdown
Member Author

/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3

@openshift-ci

openshift-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@isabella-janssen: trigger 6 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive-techpreview-3of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3
  • periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/51ff0b30-8546-11f1-9f85-3da0a2c11bea-0

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.