Skip to content
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

Merged
merged 1 commit into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions pkg/apis/crds/karpenter.sh_nodepools.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,36 @@ spec:
expireAfter: 720h
description: Disruption contains the parameters that relate to Karpenter's disruption logic
properties:
budgets:
default:
- maxUnavailable: 10%
description: 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%.
items:
description: Budget defines when Karpenter will restrict the number of Node Claims that can be terminating simultaneously.
properties:
crontab:
description: 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.
pattern: ^(@(annually|yearly|monthly|weekly|daily|midnight|hourly))|((.*)\s(.*)\s(.*)\s(.*)\s(.*))$
type: string
duration:
description: 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.
pattern: ^(([0-9]+(s|m|h))+)|(Never)$
type: string
maxUnavailable:
anyOf:
- type: integer
- type: string
default: 10%
description: 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.
x-kubernetes-int-or-string: true
required:
- maxUnavailable
type: object
maxItems: 50
type: array
x-kubernetes-validations:
- message: '''crontab'' must be set with ''duration'''
rule: '!self.all(x, (has(x.crontab) && !has(x.duration)) || (!has(x.crontab) && has(x.duration)))'
consolidateAfter:
description: ConsolidateAfter is the duration the controller will wait before attempting to terminate nodes that are underutilized. Refer to ConsolidationPolicy for how underutilization is considered.
pattern: ^(([0-9]+(s|m|h))+)|(Never)$
Expand Down
35 changes: 35 additions & 0 deletions pkg/apis/v1beta1/nodepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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"`
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@ellistarn ellistarn Dec 5, 2023

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.

Copy link
Contributor

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.

// 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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 starts or beginnings but I think a field name of crontab indicates the syntax of the string more deliberately.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
66 changes: 66 additions & 0 deletions pkg/apis/v1beta1/nodepool_validation_cel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/ptr"

Expand Down Expand Up @@ -100,6 +101,71 @@ var _ = Describe("CEL/Validation", func() {
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenUnderutilized
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should fail when creating a budget with an invalid cron", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("*"),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("30s"))},
}}
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
It("should fail when creating a budget with a negative duration", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("* * * * *"),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("-30s"))},
}}
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
It("should fail when creating a budget with a cron but no duration", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("* * * * *"),
}}
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
It("should fail when creating a budget with a duration but no cron", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("-30s"))},
}}
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
It("should succeed when creating a budget with both duration and cron", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("* * * * *"),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("30s"))},
}}
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed when creating a budget with neither duration nor cron", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
}}
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed when creating a budget with special cased crons", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("@annually"),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("30s"))},
}}
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should fail when creating two budgets where one is invalid", func() {
nodePool.Spec.Disruption.Budgets = []Budget{{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("@annually"),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("30s"))},
},
{
MaxUnavailable: intstr.FromInt(10),
Crontab: ptr.String("*"),
Duration: &metav1.Duration{Duration: lo.Must(time.ParseDuration("30s"))},
}}
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
})
Context("KubeletConfiguration", func() {
It("should succeed on kubeReserved with invalid keys", func() {
Expand Down
33 changes: 33 additions & 0 deletions pkg/apis/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.