CNTRLPLANE-3919: data race in TestEnqueueNodePoolsForCloudConfig - #9095
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
openshift-ci
Bot
added
area/testing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9095 +/- ##
=======================================
Coverage 44.51% 44.51%
=======================================
Files 774 774
Lines 96997 96997
=======================================
Hits 43179 43179
Misses 50830 50830
Partials 2988 2988
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/v2/tests/nodepool_osimagestream_test.go (1)
58-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate OSStreams feature-gate CRD check across two files. Both sites independently re-implement the same "does the CRD expose
spec.osImageStream/status.osImageStream" check viae2eutil.HasFieldInCRDSchema, which will drift if the check logic changes.
test/e2e/v2/tests/nodepool_osimagestream_test.go#L58-L78: extract this dualHasFieldInCRDSchemacheck (spec + status) into a shared exported helper, e.g.e2eutil.IsOSImageStreamFeatureEnabled(ctx, client) (bool, error), and call it here.test/e2e/v2/tests/nodepool_lifecycle_test.go#L1466-L1509: replace the inline duplicate check inverifyOSImageStreamAfterUpgradewith the same shared helper.🤖 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/e2e/v2/tests/nodepool_osimagestream_test.go` around lines 58 - 78, Extract the duplicated spec.osImageStream and status.osImageStream CRD checks into an exported e2eutil.IsOSImageStreamFeatureEnabled(ctx, client) helper returning (bool, error). Update osImageStreamBeforeEach in test/e2e/v2/tests/nodepool_osimagestream_test.go#L58-L78 and verifyOSImageStreamAfterUpgrade in test/e2e/v2/tests/nodepool_lifecycle_test.go#L1466-L1509 to use the helper, preserving each caller’s existing error handling and skip 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 `@Makefile`:
- Around line 106-114: Update PULL_BASE_SHA and the api-lint/api-lint-fix
targets so the base revision is validated before invoking golangci-lint. Resolve
a verified available ref using appropriate fallbacks such as origin/main, and
fail explicitly with a diagnostic when no suitable base ref exists instead of
passing an empty --new-from-rev value.
In `@test/e2e/v2/tests/nodepool_osimagestream_test.go`:
- Around line 356-426: Update NodePoolOSImageStreamExplicitDefaultNoRolloutTest
to avoid patching the live default NodePool: clone its relevant configuration
into an isolated zero-replica NodePool using buildTestNodePool, then apply the
explicit version-derived osImageStream to that test resource and assert its
config hash remains unchanged. Preserve the existing default-stream selection
and no-rollout verification while removing cleanup that mutates the shared
default NodePool.
---
Nitpick comments:
In `@test/e2e/v2/tests/nodepool_osimagestream_test.go`:
- Around line 58-78: Extract the duplicated spec.osImageStream and
status.osImageStream CRD checks into an exported
e2eutil.IsOSImageStreamFeatureEnabled(ctx, client) helper returning (bool,
error). Update osImageStreamBeforeEach in
test/e2e/v2/tests/nodepool_osimagestream_test.go#L58-L78 and
verifyOSImageStreamAfterUpgrade in
test/e2e/v2/tests/nodepool_lifecycle_test.go#L1466-L1509 to use the helper,
preserving each caller’s existing error handling and skip 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d395c690-68f5-40bf-9baf-7e2a327e65cb
⛔ Files ignored due to path filters (12)
control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/AROSwift/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/GCP/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/ingress-operator/zz_fixture_TestControlPlaneComponents_ingress_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/GCP/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/IBMCloud/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/ModernTLS/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (12)
Makefilecontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/ingress-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-apiserver/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/ingressoperator/deployment.gohypershift-operator/controllers/nodepool/nodepool_controller_test.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.gotest/e2e/util/eventually.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/tests/nodepool_lifecycle_test.gotest/e2e/v2/tests/nodepool_osimagestream_test.go
| PULL_BASE_SHA ?= $(if $(UPSTREAM_REMOTE),$(shell git rev-parse $(UPSTREAM_REMOTE)/main), $(shell git rev-parse main)) | ||
|
|
||
| .PHONY: api-lint | ||
| api-lint: $(GOLANGCI_LINT) $(KUBEAPILINTER_PLUGIN) | ||
| cd api && $(GOLANGCI_LINT) run --config ./.golangci.yml --modules-download-mode=readonly -v $(if $(PULL_BASE_SHA),--new-from-rev=$(PULL_BASE_SHA) --whole-files) | ||
| cd api && $(GOLANGCI_LINT) run --config ./.golangci.yml --modules-download-mode=readonly -v --new-from-rev=${PULL_BASE_SHA} | ||
|
|
||
| .PHONY: api-lint-fix | ||
| api-lint-fix: $(GOLANGCI_LINT) $(KUBEAPILINTER_PLUGIN) | ||
| cd api && $(GOLANGCI_LINT) run --config ./.golangci.yml --fix -v $(if $(PULL_BASE_SHA),--new-from-rev=$(PULL_BASE_SHA) --whole-files) | ||
| cd api && $(GOLANGCI_LINT) run --config ./.golangci.yml --fix -v --new-from-rev=${PULL_BASE_SHA} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure PULL_BASE_SHA resolves before invoking the linter.
git rev-parse $(UPSTREAM_REMOTE)/main and git rev-parse main are unguarded. In a checkout with only origin/main, or without a fetched local/upstream main, the variable becomes empty and both targets pass --new-from-rev=, causing lint to fail. Use verified fallbacks such as origin/main, or fail explicitly with a diagnostic before invoking golangci-lint.
🤖 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 `@Makefile` around lines 106 - 114, Update PULL_BASE_SHA and the
api-lint/api-lint-fix targets so the base revision is validated before invoking
golangci-lint. Resolve a verified available ref using appropriate fallbacks such
as origin/main, and fail explicitly with a diagnostic when no suitable base ref
exists instead of passing an empty --new-from-rev value.
|
/retitle CNTRLPLANE-3919: data race in TestEnqueueNodePoolsForCloudConfig |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sdminonne: This pull request references CNTRLPLANE-3919 which is a valid jira issue. 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. |
…udConfig Parallel subtests share the same HostedControlPlane and NodePool pointers. fake.ClientBuilder.Build() mutates these objects via SetResourceVersion(), causing a data race detected by -race. Deep-copy each object before passing it to WithObjects() so each subtest operates on its own copy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
df76eba to
74f889d
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, sdminonne 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
|
/lgtm |
|
/verified by unit tests Being a fix for a unit test, don't need more explanation. |
|
/label acknowledge-critical-fixes-only |
openshift-ci-robot
added
the
verified
|
@jparrill: 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
Bot
added
the
acknowledge-critical-fixes-only
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-aks-4-22 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-aws-4-22 |
|
/test e2e-aks-4-22 |
|
@sdminonne: 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
Summary
TestEnqueueNodePoolsForCloudConfigparallel subtests share mutableHostedControlPlaneandNodePoolobject pointers.fake.ClientBuilder.Build()mutates these viaSetResourceVersion(), causing a data race that fails the entirenodepooltest package intermittently inci/prow/unit.WithObjects()so each parallel subtest operates on its own copy.Test plan
go test -race -run TestEnqueueNodePoolsForCloudConfig -count=5 ./hypershift-operator/controllers/nodepool/passes clean🤖 Generated with Claude Code
Summary by CodeRabbit