openshift / hypershift

Hyperscale OpenShift - clusters with hosted control planes
https://hypershift-docs.netlify.app
Apache License 2.0
414 stars 308 forks source link

OCPBUGS-36897: fix(api): Nodepool CEL validation fix #4378

Closed a-dsouza closed 1 month ago

a-dsouza commented 1 month ago

What this PR does / why we need it: Fixes Nodepool CEL validation to allow neither replicas or autoScaling to be set

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story: Fixes # https://issues.redhat.com/browse/OCPBUGS-36897

Checklist

openshift-ci-robot commented 1 month ago

@a-dsouza: This pull request references Jira Issue OCPBUGS-36897, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.17.0) matches configured target version for branch (4.17.0) * bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/hypershift/pull/4378): >**What this PR does / why we need it**: Fixes Nodepool CEL validation to allow neither replicas or autoScaling to be set > >**Which issue(s) this PR fixes** *(optional, use `fixes #(, fixes #, ...)` format, where issue_number might be a GitHub issue, or a Jira story*: >Fixes # https://issues.redhat.com/browse/OCPBUGS-36897 > >**Checklist** >- [ ] Subject and description added to both, commit and PR. >- [ ] Relevant issues have been referenced. >- [ ] This change includes docs. >- [ ] This change includes unit tests. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fhypershift). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci-robot commented 1 month ago

@a-dsouza: This pull request references Jira Issue OCPBUGS-36897, which is valid.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.17.0) matches configured target version for branch (4.17.0) * bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/hypershift/pull/4378): >**What this PR does / why we need it**: Fixes Nodepool CEL validation to allow neither replicas or autoScaling to be set > >**Which issue(s) this PR fixes** *(optional, use `fixes #(, fixes #, ...)` format, where issue_number might be a GitHub issue, or a Jira story*: >Fixes # https://issues.redhat.com/browse/OCPBUGS-36897 > >**Checklist** >- [ ] Subject and description added to both, commit and PR. >- [x] Relevant issues have been referenced. >- [ ] This change includes docs. >- [ ] This change includes unit tests. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fhypershift). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
a-dsouza commented 1 month ago

/retest-required

muraee commented 1 month ago

How about setting a default value of 0 on the .replicas field to mitigate the issue of old NodePools failing the validation, and preserve the previous behavior.

I don't think it's good to allow both .replicas or .autoScaling fields to not be set and rely on implicit defaulting.

openshift-ci[bot] commented 1 month ago

@a-dsouza: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ibmcloud-iks 2b77a810a46f474257ce06290c9f9561b7bc1d4c link false /test e2e-ibmcloud-iks
ci/prow/e2e-ibmcloud-roks 2b77a810a46f474257ce06290c9f9561b7bc1d4c link false /test e2e-ibmcloud-roks

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
enxebre commented 1 month ago

How about setting a default value of 0 on the .replicas field to mitigate the issue of old NodePools failing the validation, and preserve the previous behavior. I don't think it's good to allow both .replicas or .autoScaling fields to not be set and rely on implicit defaulting.

In my mind 0 and nil here have different meanings. If I see replicas=0..N and autoscaler.spec set, I interpret a conflicting intent. So both unset make sense to me. Happy to hear other's thoughts

muraee commented 1 month ago

If I see replicas=0..N and autoscaler.spec set, I interpret a conflicting intent

agreed, this is prevented with the current CEL which I suggest we keep as is. If we add a default only for the replicas field, we satisfy the requirement for people wanting to leave the fields unset but make the behavior of that at least explicit on the API. If the user decides to set spec.autoScaling later on, the current CEL will disallow it.

muraee commented 1 month ago

/approve

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-dsouza, hasueki, muraee, rtheis

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/hypershift/blob/main/OWNERS)~~ [muraee] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci-robot commented 1 month ago

@a-dsouza: Jira Issue OCPBUGS-36897: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-36897 has been moved to the MODIFIED state.

In response to [this](https://github.com/openshift/hypershift/pull/4378): >**What this PR does / why we need it**: Fixes Nodepool CEL validation to allow neither replicas or autoScaling to be set > >**Which issue(s) this PR fixes** *(optional, use `fixes #(, fixes #, ...)` format, where issue_number might be a GitHub issue, or a Jira story*: >Fixes # https://issues.redhat.com/browse/OCPBUGS-36897 > >**Checklist** >- [x] Subject and description added to both, commit and PR. >- [x] Relevant issues have been referenced. >- [ ] This change includes docs. >- [ ] This change includes unit tests. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fhypershift). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
rtheis commented 1 month ago

/cherry-pick release-4.16

openshift-cherrypick-robot commented 1 month ago

@rtheis: new pull request created: #4393

In response to [this](https://github.com/openshift/hypershift/pull/4378#issuecomment-2238983864): >/cherry-pick release-4.16 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
openshift-bot commented 1 month ago

[ART PR BUILD NOTIFIER]

Distgit: hypershift This PR has been included in build ose-hypershift-container-v4.17.0-202407191510.p0.gc84168e.assembly.stream.el9. All builds following this will include this PR.