-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "chore: Revert staging changes for budgets (#799)" #839
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import ( | |
"github.com/samber/lo" | ||
v1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/util/intstr" | ||
"knative.dev/pkg/ptr" | ||
) | ||
|
||
|
@@ -78,6 +79,40 @@ type Disruption struct { | |
// +kubebuilder:validation:Schemaless | ||
// +optional | ||
ExpireAfter NillableDuration `json:"expireAfter"` | ||
// Budgets is a list of Budgets. | ||
// If there are multiple active budgets, Karpenter uses | ||
// the most restrictive maxUnavailable. If left undefined, | ||
// this will default to one budget with a maxUnavailable to 10%. | ||
// +kubebuilder:validation:XValidation:message="'crontab' must be set with 'duration'",rule="!self.all(x, (has(x.crontab) && !has(x.duration)) || (!has(x.crontab) && has(x.duration)))" | ||
// +kubebuilder:default:={{maxUnavailable: "10%"}} | ||
// +kubebuilder:validation:MaxItems=50 | ||
// +optional | ||
Budgets []Budget `json:"budgets,omitempty" hash:"ignore"` | ||
} | ||
|
||
// Budget defines when Karpenter will restrict the | ||
// number of Node Claims that can be terminating simultaneously. | ||
type Budget struct { | ||
// MaxUnavailable dictates how many NodeClaims owned by this NodePool | ||
// can be terminating at once. It must be set. | ||
// This only considers NodeClaims with the karpenter.sh/disruption taint. | ||
// +kubebuilder:validation:XIntOrString | ||
// +kubebuilder:default:="10%" | ||
MaxUnavailable intstr.IntOrString `json:"maxUnavailable" hash:"ignore"` | ||
// Crontab specifies when a budget begins being active, | ||
// using the upstream cronjob syntax. If omitted, the budget is always active. | ||
// Currently timezones are not supported. | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cronjob uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
// Duration determines how long a Budget is active since each Crontab hit. | ||
// If omitted, the budget is always active. | ||
// This is required if Crontab is set. | ||
// +kubebuilder:validation:Pattern=`^(([0-9]+(s|m|h))+)|(Never)$` | ||
// +kubebuilder:validation:Type="string" | ||
// +optional | ||
Duration *metav1.Duration `json:"duration,omitempty" hash:"ignore"` | ||
} | ||
|
||
type ConsolidationPolicy string | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we discuss "value" for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also -- MaxUnavailable is a bit confusing to me, since it implies what we know what the maximum value is.