Revert "chore: Revert staging changes for budgets (#799)" #839
Conversation
This reverts commit 331c506.
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
@njtran: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
Pull Request Test Coverage Report for Build 7094762859
💛 - Coveralls |
| // This only considers NodeClaims with the karpenter.sh/disruption taint. | ||
| // +kubebuilder:validation:XIntOrString | ||
| // +kubebuilder:default:="10%" | ||
| MaxUnavailable intstr.IntOrString `json:"maxUnavailable" hash:"ignore"` |
There was a problem hiding this comment.
didn't we discuss "value" for this?
There was a problem hiding this comment.
The consensus was to do MaxUnavailable. It matches really nicely with the fact that the parent struct name is budgets, relying on the established parallel with PDB -> MaxUnavailable
There was a problem hiding this comment.
MaxUnavailable only make sense in comparison to MinAvailable. We've diverged so much from PDB, I'm not sure there's too much value in trying to stay aligned on this field.
There was a problem hiding this comment.
Also -- MaxUnavailable is a bit confusing to me, since it implies what we know what the maximum value is.
| // This is required if Duration is set. | ||
| // +kubebuilder:validation:Pattern:=`^(@(annually|yearly|monthly|weekly|daily|midnight|hourly))|((.*)\s(.*)\s(.*)\s(.*)\s(.*))$` | ||
| // +optional | ||
| Crontab *string `json:"crontab,omitempty" hash:"ignore"` |
There was a problem hiding this comment.
Cronjob uses schedule https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/
There was a problem hiding this comment.
Yep, I think schedule may be misinterpreted, since the actual schedule of this budget is determined by the combination of this crontab and the duration field. One could argue that it should be something like starts or beginnings but I think a field name of crontab indicates the syntax of the string more deliberately.
There was a problem hiding this comment.
Crontab seems a bit awkward to me, since a crontab is a file with a schedule + command: https://man7.org/linux/man-pages/man5/crontab.5.html
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonathan-innis, njtran 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:
Approvers can indicate their approval by writing |
This reverts commit 331c506.
Fixes #N/A
Description
This reverts the API reverts from a couple weeks back to begin staging the changes for budgets again.
How was this change tested?
make presubmit
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.