feat: Support passing node selectors and tolerations to hypershift install cmd#7404
feat: Support passing node selectors and tolerations to hypershift install cmd#7404ashishmax31 wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdded support for operator tolerations and node selectors: new fields on install Options and HyperShiftOperatorDeployment, parsing and validation of toleration strings and node selector keys/values, propagation into deployment Build(), and corresponding tests and utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
area/cli
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 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.gocmd/install/install.gocmd/install/install_test.gocmd/install/assets/hypershift_operator_test.gocmd/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.
ashishmax31
force-pushed
the
ARO-23223
branch
from
d926516 to
af57705
Compare
|
@muraee Ptal, i've addressed your review comments. |
openshift-merge-robot
added
the
needs-rebase
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashishmax31, muraee 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
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cmd/install/install.go:
- Around line 239-256: The toleration validation is inconsistent with the
parser: validateTolerationSecondsPart currently allows 0 and
validateTolerationString permits a trailing :seconds for any effect, but the
parser only applies seconds when >0 and Kubernetes semantics only allow
tolerationSeconds for effect=NoExecute; update validateTolerationSecondsPart to
reject zero (require >0) and adjust validateTolerationString to only accept a
:seconds suffix when the parsed effect is "NoExecute" (and reject or error if
seconds are present for other effects), ensuring all error messages reference
the offending toleration string via the same fmt.Errorf wrapping used elsewhere.
- Around line 452-453: The help text for the --operator-tolerations flag is
incorrect; update the PersistentFlags().StringSliceVar call for
opts.OperatorTolerations so the description documents the actual expected format
"key[=value]:effect[:seconds]" and note that the operator (Exists vs Equal) is
inferred by presence/absence of '=' (no operator token). Keep the flag name
(--operator-tolerations) and opts.OperatorTolerations unchanged; only replace
the descriptive string to accurately describe key[=value]:effect[:seconds].
🧹 Nitpick comments (2)
cmd/install/assets/hypershift_operator_test.go (1)
493-498: Tests should assert absence of tolerations/nodeSelector in other cases (don’t skip checks)
Theif len(...) > 0gating reduces regression coverage; most cases can/should still assertTolerations/NodeSelectorare empty/nil.Proposed diff
g.Expect(deployment.Spec.Template.Spec.Volumes).To(BeEquivalentTo(test.expectedVolumes)) g.Expect(deployment.Spec.Template.Spec.Containers[0].VolumeMounts).To(BeEquivalentTo(test.expectedVolumeMounts)) g.Expect(deployment.Spec.Template.Spec.Containers[0].Env).To(ContainElements(test.expectedEnvVars)) - if len(test.expectedTolerations) > 0 { - g.Expect(deployment.Spec.Template.Spec.Tolerations).To(BeEquivalentTo(test.expectedTolerations)) - } - if len(test.expectedNodeSelectors) > 0 { - g.Expect(deployment.Spec.Template.Spec.NodeSelector).To(BeEquivalentTo(test.expectedNodeSelectors)) - } + g.Expect(deployment.Spec.Template.Spec.Tolerations).To(BeEquivalentTo(test.expectedTolerations)) + g.Expect(deployment.Spec.Template.Spec.NodeSelector).To(BeEquivalentTo(test.expectedNodeSelectors)) }) } }cmd/install/assets/hypershift_operator.go (1)
430-432: Defensive handling: trim/skip empty inputs; avoid silently ignoring unparsable tolerations
GivenBuild()can be called outside the CLI validation path, consider trimming and skipping empty strings/keys to reduce “it didn’t apply” surprises.Proposed diff
var volumes []corev1.Volume var tolerations []corev1.Toleration var nodeSelectors = make(map[string]string) envVars := []corev1.EnvVar{ @@ - for _, tolerationStr := range o.OperatorTolerations { - toleration := cmdutil.ParseTolerationString(tolerationStr) + for _, tolerationStr := range o.OperatorTolerations { + tolerationStr = strings.TrimSpace(tolerationStr) + if tolerationStr == "" { + continue + } + toleration := cmdutil.ParseTolerationString(tolerationStr) if toleration != nil { tolerations = append(tolerations, *toleration) } } for key, value := range o.OperatorNodeSelectors { - if key != "" { - nodeSelectors[key] = value + key = strings.TrimSpace(key) + value = strings.TrimSpace(value) + if key != "" { + nodeSelectors[key] = value } }Also applies to: 473-475, 662-673, 886-892
📜 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
📒 Files selected for processing (5)
cmd/install/assets/hypershift_operator.gocmd/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/install_test.gocmd/util/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/install/install_test.go
- cmd/util/util.go
🧰 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/install/assets/hypershift_operator_test.gocmd/install/install.gocmd/install/assets/hypershift_operator.go
🧬 Code graph analysis (2)
cmd/install/assets/hypershift_operator_test.go (2)
cmd/install/assets/hypershift_operator.go (1)
HyperShiftOperatorDeployment(410-453)api/hypershift/v1beta1/hostedcluster_types.go (1)
NonePlatform(1220-1220)
cmd/install/assets/hypershift_operator.go (1)
cmd/util/util.go (1)
ParseTolerationString(47-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
🔇 Additional comments (2)
cmd/install/assets/hypershift_operator_test.go (1)
19-27: Good coverage for tolerations/nodeSelector propagation
The new test case exercises bothkey=value:effectandkeyOnly:NoExecute:secondstoleration forms and validates node selector propagation; this aligns well with the intended feature.Also applies to: 437-483
cmd/install/install.go (1)
140-142: Wiring + defaults look correct
Defaults initialize the new fields, validation runs, and values are propagated intoassets.HyperShiftOperatorDeployment.Also applies to: 531-533, 1037-1038
| if len(o.OperatorTolerations) > 0 { | ||
| for _, tolerationStr := range o.OperatorTolerations { | ||
| if err := validateTolerationString(tolerationStr); err != nil { | ||
| errs = append(errs, fmt.Errorf("invalid operator toleration '%s': %w", tolerationStr, err)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(o.OperatorNodeSelectors) > 0 { | ||
| for nodeSelectorKey, nodeSelectorValue := range o.OperatorNodeSelectors { | ||
| if err := validateNodeSelectorKey(nodeSelectorKey); err != nil { | ||
| errs = append(errs, fmt.Errorf("invalid operator node selector key '%s': %w", nodeSelectorKey, err)) | ||
| } | ||
| if err := validateNodeSelectorValue(nodeSelectorValue); err != nil { | ||
| errs = append(errs, fmt.Errorf("invalid operator node selector value '%s': %w", nodeSelectorValue, err)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Align tolerationSeconds validation with parsing/semantics (effect+zero handling)
Today validateTolerationSecondsPart allows 0, and validateTolerationString allows :seconds with any effect—while parsing (per cmd/util/util.go snippet) only applies seconds when >0, which can silently change meaning.
Proposed diff (fail fast, consistent with current parser)
func validateTolerationString(s string) error {
@@
// Validate effect part
if err := validateTolerationEffectPart(parts[1]); err != nil {
return err
}
// Validate tolerationSeconds part (if present)
if len(parts) == 3 {
+ if parts[1] != string(corev1.TaintEffectNoExecute) {
+ return fmt.Errorf("tolerationSeconds is only valid with effect %s", corev1.TaintEffectNoExecute)
+ }
if err := validateTolerationSecondsPart(parts[2]); err != nil {
return err
}
}
@@
func validateTolerationSecondsPart(tolerationSecondsStr string) error {
if tolerationSecondsStr == "" {
return fmt.Errorf("tolerationSeconds cannot be empty when specified")
}
@@
- if seconds < 0 {
- return fmt.Errorf("tolerationSeconds must be non-negative, got %d", seconds)
+ if seconds <= 0 {
+ return fmt.Errorf("tolerationSeconds must be positive, got %d", seconds)
}
return nil
}Also applies to: 276-309, 341-373
🤖 Prompt for AI Agents
In @cmd/install/install.go around lines 239 - 256, The toleration validation is
inconsistent with the parser: validateTolerationSecondsPart currently allows 0
and validateTolerationString permits a trailing :seconds for any effect, but the
parser only applies seconds when >0 and Kubernetes semantics only allow
tolerationSeconds for effect=NoExecute; update validateTolerationSecondsPart to
reject zero (require >0) and adjust validateTolerationString to only accept a
:seconds suffix when the parsed effect is "NoExecute" (and reject or error if
seconds are present for other effects), ensuring all error messages reference
the offending toleration string via the same fmt.Errorf wrapping used elsewhere.
| cmd.PersistentFlags().StringSliceVar(&opts.OperatorTolerations, "operator-tolerations", opts.OperatorTolerations, "A list of tolerations to apply to the HyperShift Operator deployment in the format 'key1=value1:effect:operator,key2=value2:effect:operator'.") | ||
| cmd.PersistentFlags().StringToStringVar(&opts.OperatorNodeSelectors, "operator-node-selectors", opts.OperatorNodeSelectors, "Set of node selectors to apply to the HyperShift Operator deployment in the format 'key1=value1,key2=value2'.") |
There was a problem hiding this comment.
Fix --operator-tolerations flag help text: it documents the wrong format
The current description mentions ...:effect:operator but the code expects key[=value]:effect[:seconds] (and operator is inferred from presence of =).
Proposed diff
- cmd.PersistentFlags().StringSliceVar(&opts.OperatorTolerations, "operator-tolerations", opts.OperatorTolerations, "A list of tolerations to apply to the HyperShift Operator deployment in the format 'key1=value1:effect:operator,key2=value2:effect:operator'.")
+ cmd.PersistentFlags().StringSliceVar(&opts.OperatorTolerations, "operator-tolerations", opts.OperatorTolerations, "A list of tolerations to apply to the HyperShift Operator deployment in the format 'key[=value]:effect[:tolerationSeconds]' (e.g. 'infra=true:NoSchedule', 'keyOnly:NoExecute:3600').")
cmd.PersistentFlags().StringToStringVar(&opts.OperatorNodeSelectors, "operator-node-selectors", opts.OperatorNodeSelectors, "Set of node selectors to apply to the HyperShift Operator deployment in the format 'key1=value1,key2=value2'.")📝 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.
| cmd.PersistentFlags().StringSliceVar(&opts.OperatorTolerations, "operator-tolerations", opts.OperatorTolerations, "A list of tolerations to apply to the HyperShift Operator deployment in the format 'key1=value1:effect:operator,key2=value2:effect:operator'.") | |
| cmd.PersistentFlags().StringToStringVar(&opts.OperatorNodeSelectors, "operator-node-selectors", opts.OperatorNodeSelectors, "Set of node selectors to apply to the HyperShift Operator deployment in the format 'key1=value1,key2=value2'.") | |
| cmd.PersistentFlags().StringSliceVar(&opts.OperatorTolerations, "operator-tolerations", opts.OperatorTolerations, "A list of tolerations to apply to the HyperShift Operator deployment in the format 'key[=value]:effect[:tolerationSeconds]' (e.g. 'infra=true:NoSchedule', 'keyOnly:NoExecute:3600').") | |
| cmd.PersistentFlags().StringToStringVar(&opts.OperatorNodeSelectors, "operator-node-selectors", opts.OperatorNodeSelectors, "Set of node selectors to apply to the HyperShift Operator deployment in the format 'key1=value1,key2=value2'.") |
🤖 Prompt for AI Agents
In @cmd/install/install.go around lines 452 - 453, The help text for the
--operator-tolerations flag is incorrect; update the
PersistentFlags().StringSliceVar call for opts.OperatorTolerations so the
description documents the actual expected format "key[=value]:effect[:seconds]"
and note that the operator (Exists vs Equal) is inferred by presence/absence of
'=' (no operator token). Keep the flag name (--operator-tolerations) and
opts.OperatorTolerations unchanged; only replace the descriptive string to
accurately describe key[=value]:effect[:seconds].
|
/retest-required |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
3 similar comments
|
PR needs rebase. 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. |
|
PR needs rebase. 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. |
|
PR needs rebase. 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. |
openshift-ci
Bot
removed
the
needs-rebase
|
@ashishmax31: 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. |
openshift-ci
Bot
added
the
needs-rebase
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
openshift-ci
Bot
removed
the
needs-rebase
…stall cmd Signed-off-by: Ashish <asnaraya@redhat.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7404 +/- ##
==========================================
+ Coverage 41.54% 41.56% +0.01%
==========================================
Files 758 758
Lines 93838 93979 +141
==========================================
+ Hits 38986 39060 +74
- Misses 52107 52160 +53
- Partials 2745 2759 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Now I have all the information needed. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryFour distinct failures affect this PR. Codespell found a real typo ( Root CauseThere are three independent root causes across the four failures: 1. Codespell — Typo in new code (actionable) 2. ci/prow/security — Merge conflict due to stale branch (actionable) 3. ci/prow/images — Transient DNS failure (not actionable) 4. Tide — Missing merge prerequisites (downstream) Recommendations
Evidence
|
3 participants
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: