feat: Support passing node selectors and tolerations to hypershift install cmd #7404

ashishmax31 · 2025-12-17T06:52:31Z

What this PR does / why we need it:

Allow passing node selectors and tolerations to hypershift install cmd so that we can influence the placement of the HO pods.

Fixes

https://issues.redhat.com/browse/ARO-23223

Special notes for your reviewer:

Checklist:

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci · 2025-12-17T06:52:36Z

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 · 2025-12-17T06:52:37Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)

do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes add operator tolerations and node selectors support to HyperShift deployments. New public fields are introduced to the Options and HyperShiftOperatorDeployment structs, validation logic is implemented, a toleration parsing utility function is added, and test coverage is extended across multiple test files.

Changes

Cohort / File(s)	Change Summary
Deployment configuration `cmd/install/assets/hypershift_operator.go`	Added `OperatorTolerations` and `OperatorNodeSelectors` public fields to `HyperShiftOperatorDeployment`. Updated `Build()` method to parse toleration strings, populate node selectors, and apply both to deployment specifications.
Deployment tests `cmd/install/assets/hypershift_operator_test.go`	Extended test table with `expectedTolerations` and `expectedNodeSelectors` fields. Added new test case for operator tolerations and node selectors. Updated assertion logic to conditionally verify deployment tolerations and node selectors. Added Kubernetes pointer import.
Install command options and validation `cmd/install/install.go`	Added `OperatorTolerations` and `OperatorNodeSelectors` public fields to `Options` struct. Introduced five validation helper functions: `validateTolerationString`, `validateTolerationKeyValuePart`, `validateTolerationEffectPart`, `validateTolerationSecondsPart`, `validateNodeSelectorKey`, and `validateNodeSelectorValue`. Added CLI flags and default initialization. Propagated fields through operator resource setup paths. Added imports for `slices` and `k8s.io/apimachinery/pkg/util/validation`.
Install command tests `cmd/install/install_test.go`	Added multiple test cases to `TestOptions_Validate` covering valid/invalid operator tolerations (including effect variants), invalid node selector keys, and combined valid scenarios.
Utility parsing `cmd/util/util.go`	Added public `ParseTolerationString(s string) corev1.Toleration` function to parse toleration strings in two forms: `key[=value]:effect` and `key[=value]:effect:seconds`. Added imports for `strconv` and `strings`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

ParseTolerationString logic: Verify parsing of both toleration formats (with/without seconds), edge cases in string splitting, and correct assignment of TolerationOp based on value presence
Validation functions: Review correctness of key/value validation patterns and toleration string parsing in validateTolerationString and related helpers
Test coverage: Ensure all validation paths are exercised and test cases properly exercise edge cases for both tolerations and node selectors
Field propagation: Confirm that new fields flow correctly from Options → setupOperatorResources → HyperShiftOperatorDeployment.Build()

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2025-12-17T06:52:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ashishmax31
Once this PR has been reviewed and has the lgtm label, please assign muraee for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

muraee · 2025-12-17T10:14:40Z

@coderabbitai review

coderabbitai · 2025-12-17T10:14:48Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

cmd/util/util.go (1)

46-86: Consider extracting duplicate key/value parsing logic.

The key=value parsing logic is duplicated in both the 2-part and 3-part branches (lines 53-61 and 68-76). While the current implementation works correctly, extracting this into a helper function would improve maintainability.

Example refactor:

+func parseTolerationKeyValue(part string) (key, value string, operator corev1.TolerationOperator) {
+	if strings.Contains(part, "=") {
+		kv := strings.SplitN(part, "=", 2)
+		return kv[0], kv[1], corev1.TolerationOpEqual
+	}
+	return part, "", corev1.TolerationOpExists
+}
+
 func ParseTolerationString(s string) corev1.Toleration {
 	toleration := corev1.Toleration{}
 	parts := strings.Split(s, ":")
 
 	// No toleration seconds
 	if len(parts) == 2 {
-		if strings.Contains(parts[0], "=") {
-			kv := strings.SplitN(parts[0], "=", 2)
-			toleration.Key = kv[0]
-			toleration.Value = kv[1]
-			toleration.Operator = corev1.TolerationOpEqual
-		} else {
-			toleration.Key = parts[0]
-			toleration.Operator = corev1.TolerationOpExists
-		}
+		toleration.Key, toleration.Value, toleration.Operator = parseTolerationKeyValue(parts[0])
 		toleration.Effect = corev1.TaintEffect(parts[1])
 	}
 
 	// With toleration seconds
 	if len(parts) == 3 {
-		if strings.Contains(parts[0], "=") {
-			kv := strings.SplitN(parts[0], "=", 2)
-			toleration.Key = kv[0]
-			toleration.Value = kv[1]
-			toleration.Operator = corev1.TolerationOpEqual
-		} else {
-			toleration.Key = parts[0]
-			toleration.Operator = corev1.TolerationOpExists
-		}
+		toleration.Key, toleration.Value, toleration.Operator = parseTolerationKeyValue(parts[0])
 		toleration.Effect = corev1.TaintEffect(parts[1])
 		tolerationSeconds, _ := strconv.ParseInt(parts[2], 10, 64)
 		if tolerationSeconds > 0 {
 			toleration.TolerationSeconds = &tolerationSeconds
 		}
 	}
 	return toleration
 }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 88e53d2 and d926516.

📒 Files selected for processing (5)

cmd/install/assets/hypershift_operator.go (4 hunks)
cmd/install/assets/hypershift_operator_test.go (3 hunks)
cmd/install/install.go (7 hunks)
cmd/install/install_test.go (1 hunks)
cmd/util/util.go (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

cmd/util/util.go
cmd/install/install.go
cmd/install/install_test.go
cmd/install/assets/hypershift_operator_test.go
cmd/install/assets/hypershift_operator.go

🧬 Code graph analysis (3)

cmd/install/install_test.go (2)

cmd/install/install.go (1)

Options (79-139)

api/hypershift/v1beta1/hostedcluster_types.go (1)

NonePlatform (1215-1215)

cmd/install/assets/hypershift_operator_test.go (2)

cmd/install/assets/hypershift_operator.go (1)

HyperShiftOperatorDeployment (386-427)

api/hypershift/v1beta1/hostedcluster_types.go (1)

NonePlatform (1215-1215)

cmd/install/assets/hypershift_operator.go (1)

cmd/util/util.go (1)

ParseTolerationString (47-86)

🪛 golangci-lint (2.5.0)

cmd/install/install.go

[error] 228-228: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:228:23: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:234:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:247:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:257:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:270:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:283:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:296:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:309:26: undefined: MockdbusConn
sync-global-pullsecret/sync-global-pullsecret_test.go:327:12: undefined: NewMockdbusConn

(typecheck)

🔇 Additional comments (11)

cmd/install/install_test.go (1)

143-185: LGTM! Comprehensive test coverage for tolerations and node selectors.

The test cases effectively cover validation scenarios for both valid and invalid operator tolerations and node selectors, including edge cases like mixed valid/invalid tolerations and malformed keys.

cmd/install/assets/hypershift_operator_test.go (2)

437-483: LGTM! Well-structured test case for tolerations and node selectors.

The test case effectively validates both toleration formats (with and without values, with TolerationSeconds) and node selector application on the deployment.

493-498: LGTM! Proper conditional assertions.

The conditional checks ensure tolerations and node selectors are only validated when explicitly provided in test cases, preventing false positives.

cmd/install/assets/hypershift_operator.go (3)

406-407: LGTM! Appropriate field types for tolerations and node selectors.

The field types align well with the CLI input format and downstream usage.

617-628: LGTM! Proper parsing and filtering of tolerations and node selectors.

The implementation correctly:

Parses toleration strings using the utility function

Filters out empty tolerations

Preserves non-empty keys from node selectors (empty values are valid)

841-847: LGTM! Correct application of tolerations and node selectors to deployment.

The conditional application ensures these fields are only set when values are provided, maintaining clean deployment specs.

cmd/install/install.go (5)

254-287: LGTM! Comprehensive toleration string validation.

The validation correctly handles both toleration formats and provides clear error messages for each failure mode.

289-351: LGTM! Thorough validation helper functions.

The helper functions correctly validate each component of the toleration string using appropriate Kubernetes validators and provide clear error messages.

427-428: LGTM! Appropriate CLI flags for tolerations and node selectors.

The flags use correct types (StringSliceVar and StringToStringVar) and provide clear descriptions with format examples.

500-501: LGTM! Correct default initialization.

Initializing as empty slice and map ensures proper handling when flags are not provided.

974-975: LGTM! Fields correctly propagated to deployment.

The operator tolerations and node selectors are properly passed through to the HyperShiftOperatorDeployment builder.

cmd/install/assets/hypershift_operator.go

cmd/util/util.go

…stall cmd Signed-off-by: Ashish <[email protected]>

ashishmax31 · 2025-12-19T11:19:40Z

@muraee Ptal, i've addressed your review comments.

openshift-merge-robot · 2025-12-26T22:34:27Z

PR needs rebase.

Details

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.

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 Dec 17, 2025

openshift-ci bot added the do-not-merge/needs-area label Dec 17, 2025

openshift-ci bot added area/cli Indicates the PR includes changes for CLI and removed do-not-merge/needs-area labels Dec 17, 2025

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

muraee reviewed Dec 17, 2025

View reviewed changes

cmd/install/assets/hypershift_operator.go Outdated Show resolved Hide resolved

muraee reviewed Dec 17, 2025

View reviewed changes

cmd/util/util.go Outdated Show resolved Hide resolved

feat: support passing node selectors and tolerations in hypershift in…

af57705

…stall cmd Signed-off-by: Ashish <[email protected]>

ashishmax31 force-pushed the ARO-23223 branch from d926516 to af57705 Compare December 19, 2025 11:19

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Support passing node selectors and tolerations to hypershift install cmd #7404

feat: Support passing node selectors and tolerations to hypershift install cmd #7404

ashishmax31 commented Dec 17, 2025

Uh oh!

openshift-ci bot commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025 •

edited

Loading

Review skipped

Uh oh!

openshift-ci bot commented Dec 17, 2025

Uh oh!

muraee commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

ashishmax31 commented Dec 19, 2025

Uh oh!

openshift-merge-robot commented Dec 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: Support passing node selectors and tolerations to hypershift install cmd #7404

Are you sure you want to change the base?

feat: Support passing node selectors and tolerations to hypershift install cmd #7404

Conversation

ashishmax31 commented Dec 17, 2025

What this PR does / why we need it:

Special notes for your reviewer:

Checklist:

Uh oh!

openshift-ci bot commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Uh oh!

openshift-ci bot commented Dec 17, 2025

Uh oh!

muraee commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ashishmax31 commented Dec 19, 2025

Uh oh!

openshift-merge-robot commented Dec 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 17, 2025 •

edited

Loading