OCPBUGS-60093: propagate nodeSelector removal to control plane workloads#8321
OCPBUGS-60093: propagate nodeSelector removal to control plane workloads#8321sdminonne wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughPod template nodeSelector assignment was changed so workload pod templates always receive HostedControlPlane.Spec.NodeSelector (including propagating nil to clear existing selectors). Upsert/apply logic now records original label and nodeSelector counts, clones original labels/annotations to avoid aliasing, and forces an update when the new pod-template nodeSelector has fewer entries than the original. Unit tests were added to validate nodeSelector propagation and that ApplyManifest updates Deployments/StatefulSets when nodeSelector keys are removed; tests also cover label-value updates with unchanged key counts. Sequence Diagram(s)sequenceDiagram
participant HCP as HostedControlPlane
participant Controller as ControlPlaneComponent
participant Upserter as Upsert/Apply
participant KAPI as Kubernetes API
HCP->>Controller: provide Spec.NodeSelector
Controller->>Controller: setDefaultOptions(assign NodeSelector to podTemplate)
Controller->>Upserter: ApplyManifest(desired object)
Upserter->>KAPI: Get(existing)
KAPI->>Upserter: return existing object (labels, annotations, podTemplate.nodeSelector)
Upserter->>Upserter: record originalLabelCount, originalNodeSelectorCount
Upserter->>Upserter: preserveOriginalMetadata(clone originals, apply removals/overwrites)
Upserter->>Upserter: compare desired vs existing (DeepDerivative)
alt desired.nodeSelector count < originalNodeSelectorCount
Upserter->>Upserter: treat as changed (force update)
end
Upserter->>KAPI: Update(mutated)
KAPI->>Upserter: return updated
Upserter->>Controller: return OperationResult (Updated) and updated object
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
support/upsert/apply_test.go (1)
259-315: Add a StatefulSet variant to match implementation coverage.The production path includes StatefulSet handling, but this test validates only Deployment. A second subtest for StatefulSet would better protect against regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/upsert/apply_test.go` around lines 259 - 315, Add a parallel subtest to TestApplyManifestNodeSelectorRemoval that mirrors the Deployment scenario for StatefulSet: create existingStatefulSet with a Pod template Spec.NodeSelector set (use appsv1.StatefulSet, same metadata/name/namespace pattern), create desiredStatefulSet with an empty Pod template Spec, use fake.NewClientBuilder().WithObjects(existingStatefulSet).Build(), call provider.ApplyManifest(t.Context(), c, desiredStatefulSet) and assert no error, OperationResultUpdated, then Get the updated StatefulSet and assert updated.Spec.Template.Spec.NodeSelector is empty; reuse the same provider (applyProvider) and testing helpers (NewWithT, t.Context(), types.NamespacedName) as in the Deployment subtest to keep consistency.support/controlplane-component/defaults_test.go (1)
314-367: Nice targeted coverage; optional extension for StatefulSet would strengthen confidence.Current cases validate Deployment well. Since
setDefaultOptionsis generic, a lightweight StatefulSet case would improve confidence across providers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/controlplane-component/defaults_test.go` around lines 314 - 367, Add a parity test for StatefulSet nodeSelector behavior mirroring TestSetDefaultOptionsNodeSelector: create a controlPlaneWorkload parameterized for *appsv1.StatefulSet (use the statefulset provider type analogous to deploymentProvider, e.g. statefulSetProvider), instantiate a StatefulSet with existingNodeSelector, construct an HCP with hcpNodeSelector, call cpw.setDefaultOptions with the StatefulSet object and the same fake client/scheme, and assert the StatefulSet.Spec.Template.Spec.NodeSelector is set or cleared to match expectedNodeSelector; keep test cases identical to the Deployment ones to ensure generic behavior across workload types.support/upsert/apply.go (1)
255-264: Consider future-proofing nodeSelector handling across more workload kinds.Line 255 currently handles only
DeploymentandStatefulSet. IfApplyManifestis used with other pod-template resources later, nodeSelector removals there would still be skipped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/upsert/apply.go` around lines 255 - 264, getNodeSelectorCount currently only checks *appsv1.Deployment and *appsv1.StatefulSet so other pod-template workloads (DaemonSet, ReplicaSet, Job, CronJob, etc.) will be missed by ApplyManifest; modify getNodeSelectorCount to detect nodeSelector generically by converting the crclient.Object to an unstructured.Unstructured (or use unstructured.NestedStringMap) and read "spec.template.spec.nodeSelector" safely, returning its length when present, and keep the existing type switches as fallback for typed objects (refer to getNodeSelectorCount, ApplyManifest, and the Deployment/StatefulSet cases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@support/controlplane-component/defaults_test.go`:
- Around line 314-367: Add a parity test for StatefulSet nodeSelector behavior
mirroring TestSetDefaultOptionsNodeSelector: create a controlPlaneWorkload
parameterized for *appsv1.StatefulSet (use the statefulset provider type
analogous to deploymentProvider, e.g. statefulSetProvider), instantiate a
StatefulSet with existingNodeSelector, construct an HCP with hcpNodeSelector,
call cpw.setDefaultOptions with the StatefulSet object and the same fake
client/scheme, and assert the StatefulSet.Spec.Template.Spec.NodeSelector is set
or cleared to match expectedNodeSelector; keep test cases identical to the
Deployment ones to ensure generic behavior across workload types.
In `@support/upsert/apply_test.go`:
- Around line 259-315: Add a parallel subtest to
TestApplyManifestNodeSelectorRemoval that mirrors the Deployment scenario for
StatefulSet: create existingStatefulSet with a Pod template Spec.NodeSelector
set (use appsv1.StatefulSet, same metadata/name/namespace pattern), create
desiredStatefulSet with an empty Pod template Spec, use
fake.NewClientBuilder().WithObjects(existingStatefulSet).Build(), call
provider.ApplyManifest(t.Context(), c, desiredStatefulSet) and assert no error,
OperationResultUpdated, then Get the updated StatefulSet and assert
updated.Spec.Template.Spec.NodeSelector is empty; reuse the same provider
(applyProvider) and testing helpers (NewWithT, t.Context(),
types.NamespacedName) as in the Deployment subtest to keep consistency.
In `@support/upsert/apply.go`:
- Around line 255-264: getNodeSelectorCount currently only checks
*appsv1.Deployment and *appsv1.StatefulSet so other pod-template workloads
(DaemonSet, ReplicaSet, Job, CronJob, etc.) will be missed by ApplyManifest;
modify getNodeSelectorCount to detect nodeSelector generically by converting the
crclient.Object to an unstructured.Unstructured (or use
unstructured.NestedStringMap) and read "spec.template.spec.nodeSelector" safely,
returning its length when present, and keep the existing type switches as
fallback for typed objects (refer to getNodeSelectorCount, ApplyManifest, and
the Deployment/StatefulSet cases).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8d79e44d-bdc8-4c2f-8cbf-35693f6a01b4
📒 Files selected for processing (4)
support/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.gosupport/upsert/apply.gosupport/upsert/apply_test.go
openshift-ci
Bot
added
the
area/testing
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 59076c7c-3904-41f6-a50f-11ae95f22b57
📒 Files selected for processing (3)
support/upsert/apply.gosupport/upsert/apply_test.gotest/e2e/nodepool_test.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/nodepool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- support/upsert/apply_test.go
sdminonne
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@sdminonne: This pull request references Jira Issue OCPBUGS-60093, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sdminonne: This pull request references Jira Issue OCPBUGS-60093, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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
area/platform/aws
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8321 +/- ##
==========================================
+ Coverage 37.53% 37.57% +0.03%
==========================================
Files 751 751
Lines 92026 92051 +25
==========================================
+ Hits 34544 34584 +40
+ Misses 54841 54827 -14
+ Partials 2641 2640 -1
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
mehabhalodiya
left a comment
mehabhalodiya
left a comment
There was a problem hiding this comment.
The fix is well-structured.
The main things I'd raise as comments are:
(1) the maps.Clone concern in preserveOriginalMetadata (addressing coderabbit's proposed fix),
(2) an explicit acknowledgment of the kubernetes.io/os: linux removal side effect for components that previously had it as a template default, and
(3) the Gomega assertion message is for test quality,
| name: tmp-dir | ||
| enableServiceLinks: false | ||
| nodeSelector: | ||
| kubernetes.io/os: linux |
There was a problem hiding this comment.
Previously, components like aws-node-termination-handler had a hardcoded kubernetes.io/os: linux default in their YAML templates that was preserved when no HCP nodeSelector was set. After this change, that default is lost.
The removal of kubernetes.io/os: linux from these fixtures represents a behavior change for clusters that never set a nodeSelector: previously, these pods were constrained to Linux nodes; now they are not.
Is this intentional? If these workloads can only run on Linux, consider either:
(a) not hardcoding the nodeSelector in YAML templates (since it'll be wiped anyway), or
(b) merging HCP nodeSelector on top of template defaults rather than overwriting them entirely.
If Windows nodes are not possible in the management cluster context, this is a no-op risk-wise, but it should be explicitly acknowledged.
| provider := &applyProvider{} | ||
|
|
||
| result, err := provider.ApplyManifest(t.Context(), c, desiredDeployment) | ||
| g.Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
This improves failure message output and satisfies the repo's test quality guidelines.
g.Expect(err).ToNot(HaveOccurred(), "ApplyManifest should succeed without error")
There was a problem hiding this comment.
Thanks for @mehabhalodiya but my understanding the test name should be clear enough and gomega failure matcher should report that we wanted the err not occurred. I would keep it as is
|
@sdminonne: This pull request references Jira Issue OCPBUGS-60093, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
sdminonne
force-pushed
the
OCPBUGS-60093
branch
from
c9700ac to
66bba45
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/upsert/apply_test.go (1)
265-420: Optional: consolidate duplicated setup with a small table-driven helper.The Deployment and StatefulSet scenarios repeat the same Arrange/Act/Assert shape; extracting shared setup would make future cases cheaper to add.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/upsert/apply_test.go` around lines 265 - 420, Extract the duplicated Arrange/Act/Assert into a small table-driven helper used by the tests that call applyProvider.ApplyManifest: create a helper function (e.g., runNodeSelectorTest or assertApplyClearsOrUpdatesNodeSelector) that accepts parameters for the existing object, desired object, expected OperationResult (controllerutil.OperationResultUpdated), expected final nodeSelector map, and a testing.T or Gomega instance; move the fake client creation (fake.NewClientBuilder().WithObjects(...).Build()), provider := &applyProvider{}, the call to provider.ApplyManifest, and the Get+assert checks into that helper and then replace each t.Run body with a single call to the helper passing the appropriate existingDeployment/existingStatefulSet, desiredDeployment/desiredStatefulSet, and expected nodeSelector result to eliminate repeated setup and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@support/upsert/apply_test.go`:
- Around line 265-420: Extract the duplicated Arrange/Act/Assert into a small
table-driven helper used by the tests that call applyProvider.ApplyManifest:
create a helper function (e.g., runNodeSelectorTest or
assertApplyClearsOrUpdatesNodeSelector) that accepts parameters for the existing
object, desired object, expected OperationResult
(controllerutil.OperationResultUpdated), expected final nodeSelector map, and a
testing.T or Gomega instance; move the fake client creation
(fake.NewClientBuilder().WithObjects(...).Build()), provider :=
&applyProvider{}, the call to provider.ApplyManifest, and the Get+assert checks
into that helper and then replace each t.Run body with a single call to the
helper passing the appropriate existingDeployment/existingStatefulSet,
desiredDeployment/desiredStatefulSet, and expected nodeSelector result to
eliminate repeated setup and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 78352707-86af-4102-b5c1-d7bc2ddc8b1f
⛔ Files ignored due to path filters (25)
control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/AROSwift/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/GCP/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/IBMCloud/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/certified-operators-catalog/AROSwift/zz_fixture_TestControlPlaneComponents_certified_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/certified-operators-catalog/GCP/zz_fixture_TestControlPlaneComponents_certified_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/certified-operators-catalog/IBMCloud/zz_fixture_TestControlPlaneComponents_certified_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/certified-operators-catalog/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_certified_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/certified-operators-catalog/zz_fixture_TestControlPlaneComponents_certified_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/community-operators-catalog/AROSwift/zz_fixture_TestControlPlaneComponents_community_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/community-operators-catalog/GCP/zz_fixture_TestControlPlaneComponents_community_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/community-operators-catalog/IBMCloud/zz_fixture_TestControlPlaneComponents_community_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/community-operators-catalog/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_community_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/community-operators-catalog/zz_fixture_TestControlPlaneComponents_community_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-marketplace-catalog/AROSwift/zz_fixture_TestControlPlaneComponents_redhat_marketplace_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-marketplace-catalog/GCP/zz_fixture_TestControlPlaneComponents_redhat_marketplace_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-marketplace-catalog/IBMCloud/zz_fixture_TestControlPlaneComponents_redhat_marketplace_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-marketplace-catalog/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_redhat_marketplace_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-marketplace-catalog/zz_fixture_TestControlPlaneComponents_redhat_marketplace_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-operators-catalog/AROSwift/zz_fixture_TestControlPlaneComponents_redhat_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-operators-catalog/GCP/zz_fixture_TestControlPlaneComponents_redhat_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-operators-catalog/IBMCloud/zz_fixture_TestControlPlaneComponents_redhat_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-operators-catalog/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_redhat_operators_catalog_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/redhat-operators-catalog/zz_fixture_TestControlPlaneComponents_redhat_operators_catalog_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
support/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.gosupport/upsert/apply.gosupport/upsert/apply_test.gotest/e2e/nodepool_test.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/nodepool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- support/controlplane-component/defaults.go
sdminonne
force-pushed
the
OCPBUGS-60093
branch
from
66bba45 to
6f1fe56
Compare
|
@coderabbitai PTAL |
|
✅ Actions performedReview triggered.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mehabhalodiya, sdminonne 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 Resultse2e-aws
e2e-aks
|
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
sdminonne
force-pushed
the
OCPBUGS-60093
branch
from
63f07d9 to
cfe9ee5
Compare
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-azure-self-managed |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| } | ||
| // The assignment is unconditional so that removing nodeSelector from the HCP | ||
| // also clears it from workload pod templates. | ||
| podTemplateSpec.Spec.NodeSelector = hcp.Spec.NodeSelector |
There was a problem hiding this comment.
This now unconditionally overwrites any nodeSelector set in the component YAML templates. Five components still hardcode nodeSelector: kubernetes.io/os: linux in their source templates
There was a problem hiding this comment.
Right. Changed this to a merge via maps.Copy: HCP nodeSelector is now merged on top of the template's existing nodeSelector instead of replacing it. We now preserve the kubernetes.io/os: linux. Unit test added too
|
|
||
| // getNodeSelectorCount returns the number of nodeSelector entries for | ||
| // Deployment and StatefulSet pod templates, or 0 for other types. | ||
| func getNodeSelectorCount(obj crclient.Object) int { |
There was a problem hiding this comment.
nit: A short comment noting that this intentionally only handles Deployment and StatefulSet (the only workload types in the cpov2 framework) would help future readers understand why DaemonSet, Job, etc. are not covered.
There was a problem hiding this comment.
Done. Added a comment
| // Without cloning, the comparison in update() between 'existing' (serialized | ||
| // after this function runs) and 'obj' would see identical labels/annotations, | ||
| // hiding value changes when the key count stays the same. | ||
| labels := maps.Clone(original.GetLabels()) |
There was a problem hiding this comment.
The maps.Clone fix was applied to both labels and annotations, but only label value changes are tested here. Could you add a parallel test for annotation value changes? Same structure, just swapping labels for annotations.
Also, consider adding a test for nodeSelector value change with constant key count (e.g., {node-role: infra} → {node-role: worker}). That path goes through DeepDerivative rather than getNodeSelectorCount, and an explicit test would guard against regressions.
There was a problem hiding this comment.
Tests added and codecov should be happier :)
| } | ||
| } | ||
|
|
||
| func preserveOriginalMetadata(original, mutated crclient.Object) { |
There was a problem hiding this comment.
One note: the maps.Clone fix in preserveOriginalMetadata addresses a pre-existing map-aliasing bug (label/annotation value changes with the same key count were silently dropped). Worth calling that out in the PR description and commit msg.
There was a problem hiding this comment.
Done. In PR description and commit message too.
|
@jparrill addressed all the comments TY! |
|
/pipeline required |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest-required |
|
@sdminonne: The following test 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. |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThis is a Karpenter test teardown infrastructure flake, not a product bug introduced by this PR. The failure chain:
Why this is unrelated to PR #8321:
Recommendations
Evidence
|
…orkloads When a user removes nodeSelector from the HostedControlPlane spec, the change was not propagated to control plane Deployments and StatefulSets. Two issues prevented this: 1. setDefaultOptions only assigned nodeSelector conditionally (when non-nil), so clearing it from the HCP had no effect on pod templates. 2. ApplyManifest uses DeepDerivative for change detection, which treats nil/empty maps as "don't care", silently skipping nodeSelector removal even when the desired state had no nodeSelector. This commit fixes both issues: - Merge HCP nodeSelector on top of the template's existing nodeSelector via maps.Copy in setDefaultOptions. This preserves entries from component YAML templates (e.g., kubernetes.io/os: linux used by 5 catalog and aws-node-termination-handler deployments) while applying HCP-specified entries on top. When HCP nodeSelector is nil, only the template entries remain, so removal of HCP-added entries is handled naturally because cpov2 loads templates fresh each reconciliation. - Add nodeSelector count tracking in ApplyManifest to force updates when nodeSelector entries are removed, matching the existing pattern for label removal detection. Only Deployment and StatefulSet are handled as they are the only workload kinds in the cpov2 framework. - Clone metadata maps in preserveOriginalMetadata to prevent aliasing between the existing and desired objects. This fixes a pre-existing bug where label/annotation value changes with the same key count were silently dropped because both objects shared the same underlying map. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
New changes are detected. LGTM label has been removed. |
|
/retest-required |
5 participants
Summary
nodeSelectorremoval from HostedCluster to control plane deployments managed by the CPo-v2 frameworkApplyManifestfunction insupport/upsert/apply.gonow explicitly detects whennodeSelector(or labels) have been removed from the desired state and clears them on the existing objectdefaults.goinsupport/controlplane-componentnow merges HCP nodeSelector on top of the template's existing nodeSelector viamaps.Copy, preserving entries from component YAML templates (e.g.,kubernetes.io/os: linuxused by 5 catalog and aws-node-termination-handler deployments) while still allowing HCP-specified entries to be added or removedpreserveOriginalMetadata:maps.Cloneis now used so that label/annotation value changes with the same key count are no longer silently dropped due to the existing and desired objects sharing the same underlying mapApplyManifestandsetDefaultsto cover removal, merge, annotation value-change, and nodeSelector value-change scenariosTest plan
ApplyManifestnodeSelector removal (support/upsert/apply_test.go)ApplyManifestannotation value change (TestApplyManifestAnnotationValueChange)ApplyManifestnodeSelector value change with constant key count (TestApplyManifestNodeSelectorValueChange)setDefaultsnodeSelector merge with template preservation (support/controlplane-component/defaults_test.go)control-plane-workloadslabel filter (e2e test in separate PR)🤖 Generated with Claude Code