From 201c55088cf002fe8ba05fe8e0b0abfdb96b3231 Mon Sep 17 00:00:00 2001 From: flavono123 Date: Wed, 26 Feb 2025 16:12:56 +0900 Subject: [PATCH 1/2] feat(nodepool): support timezone for disruption budgets Signed-off-by: flavono123 --- kwok/charts/crds/karpenter.sh_nodepools.yaml | 9 ++++++- pkg/apis/crds/karpenter.sh_nodepools.yaml | 9 ++++++- pkg/apis/v1/nodepool.go | 23 ++++++++++++++--- pkg/apis/v1/nodepool_budgets_test.go | 27 +++++++++++++++++--- pkg/apis/v1/zz_generated.deepcopy.go | 5 ++++ 5 files changed, 64 insertions(+), 9 deletions(-) diff --git a/kwok/charts/crds/karpenter.sh_nodepools.yaml b/kwok/charts/crds/karpenter.sh_nodepools.yaml index 7a89b6c421..3000fe78be 100644 --- a/kwok/charts/crds/karpenter.sh_nodepools.yaml +++ b/kwok/charts/crds/karpenter.sh_nodepools.yaml @@ -127,10 +127,15 @@ spec: description: |- Schedule specifies when a budget begins being active, following the upstream cronjob syntax. If omitted, the budget is always active. - Timezones are not supported. This field is required if Duration is set. pattern: ^(@(annually|yearly|monthly|weekly|daily|midnight|hourly))|((.+)\s(.+)\s(.+)\s(.+)\s(.+))$ type: string + timeZone: + description: |- + TimeZone specifies the timezone the Schedule is evaluated against. + If not specified, this will default to the UTC. + See https://en.wikipedia.org/wiki/List_of_tz_database_time_zones for valid time zones. + type: string required: - nodes type: object @@ -139,6 +144,8 @@ spec: x-kubernetes-validations: - message: '''schedule'' must be set with ''duration''' rule: self.all(x, has(x.schedule) == has(x.duration)) + - message: '''timeZone'' can only be set when ''schedule'' is set' + rule: self.all(x, !has(x.timeZone) || has(x.schedule)) consolidateAfter: description: |- ConsolidateAfter is the duration the controller will wait diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml index c31c4050b7..dd22b102c8 100644 --- a/pkg/apis/crds/karpenter.sh_nodepools.yaml +++ b/pkg/apis/crds/karpenter.sh_nodepools.yaml @@ -127,10 +127,15 @@ spec: description: |- Schedule specifies when a budget begins being active, following the upstream cronjob syntax. If omitted, the budget is always active. - Timezones are not supported. This field is required if Duration is set. pattern: ^(@(annually|yearly|monthly|weekly|daily|midnight|hourly))|((.+)\s(.+)\s(.+)\s(.+)\s(.+))$ type: string + timeZone: + description: |- + TimeZone specifies the timezone the Schedule is evaluated against. + If not specified, this will default to the UTC. + See https://en.wikipedia.org/wiki/List_of_tz_database_time_zones for valid time zones. + type: string required: - nodes type: object @@ -139,6 +144,8 @@ spec: x-kubernetes-validations: - message: '''schedule'' must be set with ''duration''' rule: self.all(x, has(x.schedule) == has(x.duration)) + - message: '''timeZone'' can only be set when ''schedule'' is set' + rule: self.all(x, !has(x.timeZone) || has(x.schedule)) consolidateAfter: description: |- ConsolidateAfter is the duration the controller will wait diff --git a/pkg/apis/v1/nodepool.go b/pkg/apis/v1/nodepool.go index 9aae97b44d..295bca7d9a 100644 --- a/pkg/apis/v1/nodepool.go +++ b/pkg/apis/v1/nodepool.go @@ -20,6 +20,7 @@ import ( "fmt" "math" "strconv" + "time" "github.com/awslabs/operatorpkg/serrors" "github.com/mitchellh/hashstructure/v2" @@ -78,6 +79,7 @@ type Disruption struct { // the most restrictive value. If left undefined, // this will default to one budget with a value to 10%. // +kubebuilder:validation:XValidation:message="'schedule' must be set with 'duration'",rule="self.all(x, has(x.schedule) == has(x.duration))" + // +kubebuilder:validation:XValidation:message="'timeZone' can only be set when 'schedule' is set",rule="self.all(x, !has(x.timeZone) || has(x.schedule))" // +kubebuilder:default:={{nodes: "10%"}} // +kubebuilder:validation:MaxItems=50 // +optional @@ -104,7 +106,6 @@ type Budget struct { Nodes string `json:"nodes" hash:"ignore"` // Schedule specifies when a budget begins being active, following // the upstream cronjob syntax. If omitted, the budget is always active. - // Timezones are not supported. // This field is required if Duration is set. // +kubebuilder:validation:Pattern:=`^(@(annually|yearly|monthly|weekly|daily|midnight|hourly))|((.+)\s(.+)\s(.+)\s(.+)\s(.+))$` // +optional @@ -119,6 +120,12 @@ type Budget struct { // +kubebuilder:validation:Type="string" // +optional Duration *metav1.Duration `json:"duration,omitempty" hash:"ignore"` + // TimeZone specifies the timezone the Schedule is evaluated against. + // If not specified, this will default to the UTC. + // See https://en.wikipedia.org/wiki/List_of_tz_database_time_zones for valid time zones. + // +kubebuilder:validation:Type="string" + // +optional + TimeZone *string `json:"timeZone,omitempty" hash:"ignore"` } type ConsolidationPolicy string @@ -355,16 +362,24 @@ func (in *Budget) IsActive(c clock.Clock) (bool, error) { if in.Schedule == nil && in.Duration == nil { return true, nil } - schedule, err := cron.ParseStandard(fmt.Sprintf("TZ=UTC %s", lo.FromPtr(in.Schedule))) + tz := "UTC" + if in.TimeZone != nil && *in.TimeZone != "" { + tz = *in.TimeZone + } + loc, err := time.LoadLocation(tz) + if err != nil { + return false, fmt.Errorf("invalid time zone %q: %w", tz, err) + } + schedule, err := cron.ParseStandard(fmt.Sprintf("TZ=%s %s", tz, lo.FromPtr(in.Schedule))) if err != nil { // Should only occur if there's a discrepancy // with the validation regex and the cron package. return false, serrors.Wrap(fmt.Errorf("invariant violated, invalid cron"), "cron", schedule) } // Walk back in time for the duration associated with the schedule - checkPoint := c.Now().UTC().Add(-lo.FromPtr(in.Duration).Duration) + checkPoint := c.Now().In(loc).Add(-lo.FromPtr(in.Duration).Duration) nextHit := schedule.Next(checkPoint) - return !nextHit.After(c.Now().UTC()), nil + return !nextHit.After(c.Now().In(loc)), nil } func GetIntStrFromValue(str string) intstr.IntOrString { diff --git a/pkg/apis/v1/nodepool_budgets_test.go b/pkg/apis/v1/nodepool_budgets_test.go index ac77a4261d..91380fe7c2 100644 --- a/pkg/apis/v1/nodepool_budgets_test.go +++ b/pkg/apis/v1/nodepool_budgets_test.go @@ -149,7 +149,6 @@ var _ = Describe("Budgets", func() { }) It("should get the minimum budget for each reason", func() { - nodePool.Spec.Disruption.Budgets = append(nodePool.Spec.Disruption.Budgets, []Budget{ { @@ -173,7 +172,6 @@ var _ = Describe("Budgets", func() { Expect(err).To(BeNil()) Expect(underutilizedAllowedDisruption).To(Equal(10)) }) - }) Context("AllowedDisruptions", func() { @@ -209,7 +207,7 @@ var _ = Describe("Budgets", func() { }) Context("IsActive", func() { - It("should always consider a schedule and time in UTC", func() { + It("should consider a schedule and time in UTC when no timezone is specified", func() { // Set the time to start of June 2000 in a time zone 1 hour ahead of UTC fakeClock = clock.NewFakeClock(time.Date(2000, time.June, 0, 0, 0, 0, 0, time.FixedZone("fake-zone", 3600))) budgets[0].Schedule = lo.ToPtr("@daily") @@ -237,6 +235,29 @@ var _ = Describe("Budgets", func() { Expect(err).To(Succeed()) Expect(active).To(BeFalse()) }) + It("should reutn that a schedule is active with a timezone", func() { + // Set the time to the middle of the year of 2000 as KST, the best year ever + fakeClock = clock.NewFakeClock(time.Date(2000, time.June, 15, 12, 30, 30, 0, time.FixedZone("Asia/Seoul", 9*3600))) + budgets[0].TimeZone = lo.ToPtr("Asia/Seoul") + active, err := budgets[0].IsActive(fakeClock) + Expect(err).To(Succeed()) + Expect(active).To(BeTrue()) + }) + It("should return that a schedule is inactive with a timezone", func() { + // Set the time to the middle of the year of 2000 as KST, the best year ever + fakeClock = clock.NewFakeClock(time.Date(2000, time.June, 15, 12, 30, 30, 0, time.FixedZone("Asia/Seoul", 9*3600))) + budgets[0].Schedule = lo.ToPtr("@yearly") + budgets[0].TimeZone = lo.ToPtr("Asia/Seoul") + active, err := budgets[0].IsActive(fakeClock) + Expect(err).To(Succeed()) + Expect(active).To(BeFalse()) + }) + It("should return an error if the timezone is invalid", func() { + budgets[0].TimeZone = lo.ToPtr("Invalid/Timezone") + _, err := budgets[0].IsActive(fakeClock) + Expect(err).ToNot(Succeed()) + Expect(err.Error()).To(ContainSubstring("invalid time zone")) + }) It("should return that a schedule is active when the schedule hit is in the middle of the duration", func() { // Set the date to the start of the year 1000, the best year ever fakeClock = clock.NewFakeClock(time.Date(1000, time.January, 1, 12, 0, 0, 0, time.UTC)) diff --git a/pkg/apis/v1/zz_generated.deepcopy.go b/pkg/apis/v1/zz_generated.deepcopy.go index 4ccbab50db..a9eff2eaf9 100644 --- a/pkg/apis/v1/zz_generated.deepcopy.go +++ b/pkg/apis/v1/zz_generated.deepcopy.go @@ -46,6 +46,11 @@ func (in *Budget) DeepCopyInto(out *Budget) { *out = new(metav1.Duration) **out = **in } + if in.TimeZone != nil { + in, out := &in.TimeZone, &out.TimeZone + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Budget. From c8baa626ffc61c86321662d14bf925bda502fb72 Mon Sep 17 00:00:00 2001 From: flavono123 Date: Wed, 4 Jun 2025 14:26:18 +0900 Subject: [PATCH 2/2] fix(nodepool-budgets): fix typo in test description Signed-off-by: flavono123 --- pkg/apis/v1/nodepool_budgets_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/v1/nodepool_budgets_test.go b/pkg/apis/v1/nodepool_budgets_test.go index 91380fe7c2..8eedfb1bf9 100644 --- a/pkg/apis/v1/nodepool_budgets_test.go +++ b/pkg/apis/v1/nodepool_budgets_test.go @@ -235,7 +235,7 @@ var _ = Describe("Budgets", func() { Expect(err).To(Succeed()) Expect(active).To(BeFalse()) }) - It("should reutn that a schedule is active with a timezone", func() { + It("should return that a schedule is active with a timezone", func() { // Set the time to the middle of the year of 2000 as KST, the best year ever fakeClock = clock.NewFakeClock(time.Date(2000, time.June, 15, 12, 30, 30, 0, time.FixedZone("Asia/Seoul", 9*3600))) budgets[0].TimeZone = lo.ToPtr("Asia/Seoul")