MCO-2205: Make ImageModeStatusReporting MCP count test more resilient on SNO - #6303
MCO-2205: Make ImageModeStatusReporting MCP count test more resilient on SNO#6303isabella-janssen wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@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. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughThe 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. ChangesExtended test reliability and filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
3cfb394 to
da53bae
Compare
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b06371a0-7fa3-11f1-8330-cd5287550209-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/image_mode_status_reporting.go (1)
403-446: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
StopTrying(...).Now()overg.Fail()inside theEventuallypoll closure.Calling
g.Fail(...)(ginkgo'sFail) from inside thefunc() boolpolled byo.Eventually(...)is non-idiomatic. Gomega shipsStopTrying(message).Now()specifically for abortingEventually/Consistentlyearly with a failure, and its failure message correctly surfaces at the top-level assertion rather than relying on a rawFailpanic unwinding through Gomega'sAsyncAssertioninternals. 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-minuteEventuallytimeout rather than being retried — the exact interaction depends on the Gomega/Ginkgo version pinned ingo.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
📒 Files selected for processing (1)
test/extended/image_mode_status_reporting.go
d5c0ce6 to
8f439d0
Compare
|
/test verify |
1 similar comment
|
/test verify |
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1b487920-812a-11f1-80b8-6d96ec2f2e5a-0 |
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/81ff7890-8156-11f1-9b24-53e708c2330a-0 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/extended-priv/util/clusters.go (1)
131-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPass a cancelable context into the topology probe.
context.TODO()is not tied to the Ginkgo spec’s cancellation. Accept acontext.ContextinSkipIfClusterUnreachableOrSNOand pass it toGet, while retaining the 10-second client timeout as a fallback.As per path instructions, Go code should use
context.Contextfor 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
📒 Files selected for processing (3)
test/extended-priv/mco_bootimages_skew.gotest/extended-priv/util/clusters.gotest/extended/custom_pool_booting.go
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9326ebf0-8159-11f1-8cc3-f224bab56933-0 |
There was a problem hiding this comment.
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
📒 Files selected for processing (4)
cmd/machine-config-tests-ext/main.gotest/extended-priv/mco_bootimages_skew.gotest/extended-priv/util/clusters.gotest/extended/custom_pool_booting.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended-priv/mco_bootimages_skew.go
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ceda9e40-81df-11f1-976d-dfdf75693c42-0 |
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/855c82d0-81f1-11f1-91b8-134f6428da21-0 |
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5ad58b10-821e-11f1-8612-ecb1af57d5e6-0 |
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/50d295a0-8434-11f1-86a6-29d32b75f8d9-0 |
|
/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 |
|
@isabella-janssen: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4eb0ed50-8464-11f1-92f2-bed48f84dad0-0 |
|
/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 |
|
@isabella-janssen: trigger 6 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/60e5d8d0-8506-11f1-9615-21d6f680990f-0 |
5fbb686 to
8a80be1
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/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 |
|
@isabella-janssen: trigger 6 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/51ff0b30-8546-11f1-9f85-3da0a2c11bea-0 |
4f367f9 to
6b01b31
Compare
|
/verified by payloads |
|
@isabella-janssen: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
verified
|
/override ci/prow/e2e-aws-ovn This only touches the disruptive tests |
|
@isabella-janssen: Overrode contexts on behalf of isabella-janssen: ci/prow/e2e-aws-ovn, ci/prow/e2e-gcp-op-ocl-part1, ci/prow/e2e-gcp-op-ocl-part2, ci/prow/e2e-gcp-op-part1, ci/prow/e2e-gcp-op-part2, ci/prow/e2e-gcp-op-single-node, ci/prow/e2e-hypershift 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 kubernetes-sigs/prow repository. |
| // not depend on the CLI wrapper. This makes it safe to call BEFORE NewCLI's | ||
| // SetupProject BeforeEach, which would otherwise fail on an unreachable API | ||
| // before the test's own skip logic gets a chance to run. | ||
| func SkipIfClusterUnreachable(kubeconfigPath string) { |
There was a problem hiding this comment.
In the extended-priv directory all communication with the cluster should be done via "oc" binary.
There was a problem hiding this comment.
I've opened a thread in Slack for us to discuss, but it is an intentional chose to not use oc since SNO clusters can become unstable to the point where we cannot make calls with oc. This gets around those cases.
| // | ||
| // Like SkipIfClusterUnreachable, this function builds a client directly from | ||
| // the kubeconfig path and is safe to call BEFORE NewCLI's SetupProject. | ||
| func SkipIfImageRegistryUnhealthy(kubeconfigPath string) { |
There was a problem hiding this comment.
Same, we should use the "oc" binary instead.
| // | ||
| // Like SkipIfClusterUnreachable, this function builds a client directly from | ||
| // the kubeconfig path and is safe to call BEFORE NewCLI's SetupProject. | ||
| func SkipIfImageRegistryUnhealthy(kubeconfigPath string) { |
There was a problem hiding this comment.
The function SkipTestIfCannotUseInternalRegistry already exists in machineosconfig.go
We can extend the conditions verified by this funciton if they are not enough.
| // not depend on the CLI wrapper. This makes it safe to call BEFORE NewCLI's | ||
| // SetupProject BeforeEach, which would otherwise fail on an unreachable API | ||
| // before the test's own skip logic gets a chance to run. | ||
| func SkipIfClusterUnreachable(kubeconfigPath string) { |
There was a problem hiding this comment.
Should this condition be checked in the preCheck function?
If the cluster's API is unreachable we don't want to execute any test, do we?
| logger.Infof("OK!\n") | ||
| if isImageUpdate { | ||
| // Wait for the OCL image build to succeed before tracking MCN transitions. | ||
| // Creating a MOSC can trigger a config reconciliation that reboots the node |
There was a problem hiding this comment.
thought (non-blocking): I'd like to figure out why that happens. I suspect it might have to do with how we bifurcated the configs into ones which trigger an image build vs. ones that do not.
6b01b31 to
f66a995
Compare
openshift-ci-robot
removed
the
verified
f66a995 to
7cdd61b
Compare
|
@isabella-janssen: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
- 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
Topology:<name>label filtering for extension test selection (in addition to existing platform include/exclude filters).