OCPBUGS-39587: control plane machineset unit tests regularly failing - #413
OCPBUGS-39587: control plane machineset unit tests regularly failing#413rvanderp3 wants to merge 5 commits into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@rvanderp3: This pull request references Jira Issue OCPBUGS-39587, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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 change preserves static-IP nameservers during topology operations, reorders controller test lifecycle setup, adds webhook readiness verification, and updates OpenShift/Go build images and a controller-runtime-common version. ChangesPlatform updates and validation
Estimated code review effort: 3 (Moderate) | ~15 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test ? |
|
/test e2e-nutanix-ovn |
![splat-sdlc-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
The Nutanix generator tests created the reconciler in JustBeforeEach but referenced it in inner BeforeEach blocks (isSupportedControlPlaneMachinesNumber and generateControlPlaneMachineSet calls). Since Ginkgo runs all BeforeEach blocks before JustBeforeEach, the reconciler was nil when these inner blocks executed first under random test ordering (--randomize-all). This caused the test 'with an unsupported platform should have not created the ControlPlaneMachineSet' and 'with only 1 existing control plane machine' to flake with nil pointer dereferences depending on execution order. Move the manager and reconciler creation from JustBeforeEach into the outer BeforeEach, matching the pattern used by all other platform tests (AWS, Azure, GCP, OpenStack). JustBeforeEach now only starts the manager, which is the correct ordering since inner BeforeEach blocks create infrastructure before the manager starts.
1ba3e04 to
00809b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controllers/controlplanemachinesetgenerator/controller_test.go`:
- Around line 1795-1806: The manager cleanup can run after setup fails before
mgrCancel and mgrDone are initialized. Update startManager/stopManager lifecycle
handling so cleanup is registered only after startManager succeeds, or make
stopManager safely handle nil/uninitialized values while preserving normal
teardown.
🪄 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: 98bd71bd-9109-4dc7-80ea-47793fffd4ef
📒 Files selected for processing (1)
pkg/controllers/controlplanemachinesetgenerator/controller_test.go
| }) | ||
|
|
||
| AfterEach(func() { | ||
| testutils.CleanupResources(Default, ctx, cfg, k8sClient, namespaceName, | ||
| &corev1.Node{}, | ||
| &machinev1beta1.Machine{}, | ||
| &configv1.Infrastructure{}, | ||
| &machinev1.ControlPlaneMachineSet{}, | ||
| ) | ||
| }) | ||
|
|
||
| JustBeforeEach(func() { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1760,1845p' pkg/controllers/controlplanemachinesetgenerator/controller_test.goRepository: openshift/cluster-control-plane-machine-set-operator
Length of output: 3258
🏁 Script executed:
rg -n "func stopManager|stopManager\(|func startManager|mgrCancel|mgrDone" pkg/controllers/controlplanemachinesetgenerator/controller_test.goRepository: openshift/cluster-control-plane-machine-set-operator
Length of output: 2328
Guard manager teardown against failed setup
If a preceding BeforeEach fails, JustBeforeEach never sets mgrCancel/mgrDone, but JustAfterEach still calls stopManager(). That can panic or block forever on nil values. Register the cleanup only after startManager succeeds, or make stopManager() nil-safe.
🤖 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/controllers/controlplanemachinesetgenerator/controller_test.go` around
lines 1795 - 1806, The manager cleanup can run after setup fails before
mgrCancel and mgrDone are initialized. Update startManager/stopManager lifecycle
handling so cleanup is registered only after startManager succeeds, or make
stopManager safely handle nil/uninitialized values while preserving normal
teardown.
Source: MCP tools
|
/test e2e-nutanix-ovn |
|
/test all |
Restore files to match main branch that were incorrectly divergent: - .ci-operator.yaml, Dockerfile: restore 5.0 image tags - go.mod, go.sum, go.work.sum: restore controller-runtime-common v0.0.0-20260428152732 - vsphere.go, vsphere_test.go: restore nameserver preservation (OCPBUGS-86860) - vendor/controller-runtime-common: restore NeedLeaderElection option - vendor/modules.txt: restore correct module version The PR was based on an older branch and contained changes that conflicted with recent main merges (controller-runtime-common bump in PR openshift#403, nameserver fix in PR openshift#406, ART image update in PR openshift#408). CI failures: - ci/prow/unit: timing flakes in controller integration tests (2s timeouts under race+CI load) and webhook context deadline exceeded - not related to PR changes - ci/prow/e2e-aws-operator: infrastructure flake (pod evicted, low ephemeral-storage) - ci/prow/e2e-aws-ovn-upgrade: infrastructure flake (ImageInspectError) - ci/prow/e2e-aws-ovn-serial-*: infrastructure flakes - ci/prow/e2e-aws-operator-techpreview: infrastructure flake - ci/prow/e2e-aws-ovn-etcd-scaling: infrastructure flake
openshift-ci
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Dockerfile`:
- Line 3: Replace the broad COPY . . instruction in the Dockerfile with explicit
COPY statements for only the files and directories required by the build,
ensuring sensitive or unrelated build-context content cannot enter builder
layers or caches.
- Line 7: Add a HEALTHCHECK instruction to the final image in the Dockerfile,
using an appropriate command that verifies the operator process is responsive
and returns a failure status when it is unhealthy or hung.
- Line 7: Update the final Dockerfile stage after the artifact COPY instructions
to explicitly set a non-root USER, ensuring the resulting container image does
not run as root.
In
`@pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere_test.go`:
- Around line 181-182: Update the error assertion in the InjectFailureDomain
test to include a descriptive diagnostic message using the repository-required
assertion form. Keep the existing providerConfig.ExtractFailureDomain and
InjectFailureDomain flow unchanged while making injection failures actionable.
🪄 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: 7e650ecf-11a1-4c8c-8f54-6393df8fd85d
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumvendor/github.com/openshift/controller-runtime-common/pkg/tls/controller.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
.ci-operator.yamlDockerfilego.modpkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere.gopkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere_test.go
| gzip bin/cluster-control-plane-machine-set-operator-ext | ||
|
|
||
| FROM registry.ci.openshift.org/ocp/4.22:base-rhel9 | ||
| FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a HEALTHCHECK to the final image.
Define an appropriate health check for the operator so container runtimes can detect unhealthy or hung processes.
As per path instructions, container images must define HEALTHCHECK.
🤖 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 `@Dockerfile` at line 7, Add a HEALTHCHECK instruction to the final image in
the Dockerfile, using an appropriate command that verifies the operator process
is responsive and returns a failure status when it is unhealthy or hung.
Source: Path instructions
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the final image as non-root.
The final stage has no explicit USER, and Trivy reports that it runs as root. Add a non-root USER after the artifact copies.
As per path instructions, container images must use a non-root user.
🤖 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 `@Dockerfile` at line 7, Update the final Dockerfile stage after the artifact
COPY instructions to explicitly set a non-root USER, ensuring the resulting
container image does not run as root.
Sources: Path instructions, Linters/SAST tools
| injectedConfig, err := providerConfig.InjectFailureDomain(providerConfig.ExtractFailureDomain()) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a diagnostic message to the error assertion.
Use the repository-required form with a descriptive failure message so injection failures are actionable.
Proposed fix
- Expect(err).ToNot(HaveOccurred())
+ Expect(err).NotTo(HaveOccurred(), "expected failure-domain injection to succeed")📝 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.
| injectedConfig, err := providerConfig.InjectFailureDomain(providerConfig.ExtractFailureDomain()) | |
| Expect(err).ToNot(HaveOccurred()) | |
| injectedConfig, err := providerConfig.InjectFailureDomain(providerConfig.ExtractFailureDomain()) | |
| Expect(err).NotTo(HaveOccurred(), "expected failure-domain injection to succeed") |
🤖 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/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere_test.go`
around lines 181 - 182, Update the error assertion in the InjectFailureDomain
test to include a descriptive diagnostic message using the repository-required
assertion form. Keep the existing providerConfig.ExtractFailureDomain and
InjectFailureDomain flow unchanged while making injection failures actionable.
Source: Coding guidelines
|
/test unit |
1 similar comment
|
/test unit |
Fix flaky unit test failure in ci/prow/unit where the webhook test 'when providing template with invalid path in vSphere configuration' failed with 'connection refused' because the webhook server was not yet listening when the test attempted to create a CPMS resource. Add an Eventually block after starting the manager to poll the webhook server with a TLS dial until it is accepting connections, preventing the race condition between server startup and test execution. Fix failures in: - ci/prow/unit: webhook server race condition causing connection refused
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/webhooks/controlplanemachineset/webhooks_test.go`:
- Around line 141-150: Replace the webhookAddr construction in the webhook
readiness check with net.JoinHostPort, passing LocalServingHost and the string
form of LocalServingPort, so IPv4, IPv6, and hostname addresses are formatted
safely. Keep the existing TLS dialing and retry behavior unchanged.
🪄 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: 7cc99110-0604-4563-864e-ede0753c422a
📒 Files selected for processing (1)
pkg/webhooks/controlplanemachineset/webhooks_test.go
|
/test unit |
Address reviewer comments: - Use net.JoinHostPort instead of fmt.Sprintf for webhook address formatting in webhooks_test.go to ensure IPv6 compatibility The vSphere test diagnostic message suggestion was not applicable as those files are not part of this PR's changeset.
2132641 to
93328ac
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Fix lint failures in ci/prow/lint: - gofmt: remove spaces around * in duration multiplications (Go 1.25+ formatting) - nlreturn: add blank line before return statement - wrapcheck: wrap error from tls.DialWithDialer with fmt.Errorf - nolintlint: remove unused //nolint:gosec directive (gosec excluded for test files)
|
@rvanderp3: 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. |
2 participants
fix(test): resolve flaky Nutanix CPMS generator test race condition
The Nutanix controlplanemachinesetgenerator test had a race condition
where the manager and reconciler were created and started together in
JustBeforeEach. This meant the manager's cache could start syncing
before all BeforeEach blocks (including infrastructure status updates)
had completed, causing the background reconciler to see stale
NutanixPlatformType instead of NonePlatformType.
This restructures the Nutanix test to match all other platform tests
(AWS, Azure, GCP, OpenStack) by creating the manager and reconciler in
BeforeEach and only starting the manager in JustBeforeEach. This
ensures all infrastructure setup (including the NonePlatformType status
update in the 'unsupported platform' test) completes before the manager
starts and begins cache sync.
Summary by CodeRabbit