From c4be45e04079f0d9313150b3ae7b5313132b0e36 Mon Sep 17 00:00:00 2001 From: Nick Tran <10810510+njtran@users.noreply.github.com> Date: Sun, 14 Jul 2024 21:11:09 -0700 Subject: [PATCH] chore: capitalize disruption budgets by reason (#1417) --- designs/disruption-controls-by-reason.md | 16 ++++++++-------- kwok/charts/crds/karpenter.sh_nodepools.yaml | 16 ++++++++-------- pkg/apis/crds/karpenter.sh_nodepools.yaml | 16 ++++++++-------- pkg/apis/v1/nodepool.go | 10 +++++----- pkg/apis/v1/nodepool_validation_cel_test.go | 6 +++--- pkg/apis/v1beta1/nodepool.go | 10 +++++----- pkg/apis/v1beta1/nodepool_validation_cel_test.go | 6 +++--- 7 files changed, 40 insertions(+), 40 deletions(-) diff --git a/designs/disruption-controls-by-reason.md b/designs/disruption-controls-by-reason.md index ab6381a30f..b3825a9337 100644 --- a/designs/disruption-controls-by-reason.md +++ b/designs/disruption-controls-by-reason.md @@ -13,7 +13,7 @@ See further breakdown of some additional scenarios towards the bottom of the doc ## Clarifying the requirements and behavior **Reason and Budget Definition:** Users should be able to define an reason and a corresponding budget(s). -**Supported Reasons:** All disruption Reasons affected by the current Budgets implementation (underutilized, empty, expired, drifted) should be supported. +**Supported Reasons:** All disruption Reasons affected by the current Budgets implementation (Underutilized, Empty, Drifted) should be supported. **Default Behavior for Unspecified Reasons:** Budgets should continue to support a default behavior for all disruption reasons. # API Design @@ -29,10 +29,10 @@ type Budget struct { // If a reason is set, it will only apply to that method. If multiple reasons are specified, // this budget will apply to all of them. If a reason is unspecified we will take the min value of this budget and the rest of the active budgets. // if an unspecified reason exists we will also override all other reasons with its value if they are smaller than the unspecified reason. - // allowed reasons are "underutilized", "expired", "empty", "drifted" + // allowed reasons are "Underutilized", "Empty", "Drifted" // +kubebuilder:validation:UniqueItems // +kubebuilder:validation:MaxItems=4 - // +kubebuilder:validation:Enum:={"underutilized","expired","empty","drifted"} + // +kubebuilder:validation:Enum:={"Underutilized","Empty","Drifted"} // +optional Reasons []string `json:"reason,omitempty" hash:"ignore"` // Nodes dictates the maximum number of NodeClaims owned by this NodePool @@ -73,7 +73,7 @@ spec: # This is not a complete NodePool Spec. disruption: budgets: - schedule: "* * * * *" - reasons: [drifted, underutilized] + reasons: [Drifted, Underutilized] nodes: 10 # For all other reasons, only allow 5 nodes to be disrupted at a time - nodes: 5 @@ -99,7 +99,7 @@ In this approach, each budget entry specifies a single reason for disruption. // number of Node Claims that can be terminating simultaneously. type Budget struct { // +optional - // +kubebuilder:validation:Enum:={"underutilized","expired","empty","drifted"} + // +kubebuilder:validation:Enum:={"Underutilized","Empty","Drifted"} Reason string `json:"reason,omitempty" hash:"ignore"` // Nodes dictates the maximum number of NodeClaims owned by this NodePool // that can be terminating at once. This is calculated by counting nodes that @@ -194,7 +194,7 @@ spec: #### Considerations Some of the API choices for a given reason seem to follow a similar pattern. These include ConsolidateAfter, ExpireAfter. Moreover, when discussing disruption budgets, we talk about adding behavior for each reason. It appears there is a need for disruption controls within the budgets for each reason, not just overall. -This approach aligns well with controls that apply to all existing reasons. The proposal presented here is similar to the one mentioned above in relation to the reasons we allow to be defined (underutilized, drifted, expired, empty). +This approach aligns well with controls that apply to all existing reasons. The proposal presented here is similar to the one mentioned above in relation to the reasons we allow to be defined (Underutilized, Drifted, Empty). This proposal is currently scoped for disruptionBudgets by reason. However, we should also consider incorporating other generic disruption controls into the PerReasonControls, even if we do not implement them immediately. Moving ConsolidateAfter and ExpireAfter into the per-reason controls is a significant migration that requires careful planning and its own dedicated design. This proposal simply demonstrates a potential model that highlights the benefits of defining controls at a per-reason level of granularity. @@ -328,10 +328,10 @@ spec: # This is not a complete NodePool Spec. ```yaml budgets: - nodes: 10 - reasons: [drifted, underutilized] + reasons: [Drifted, Underutilized] schedule: "* * * * *" - nodes: 5 - reasons: [empty] + reasons: [Empty] schedule: "* * * * *" ``` In the case of a budget like above, default is undefined. Should karpenter assume the user doesn't want to disrupt any other reasons? Or should we assume that if a default is unspecified, they want us to disrupt anyway? diff --git a/kwok/charts/crds/karpenter.sh_nodepools.yaml b/kwok/charts/crds/karpenter.sh_nodepools.yaml index afaeda3d96..ed093c7d86 100644 --- a/kwok/charts/crds/karpenter.sh_nodepools.yaml +++ b/kwok/charts/crds/karpenter.sh_nodepools.yaml @@ -115,13 +115,13 @@ spec: description: |- Reasons is a list of disruption methods that this budget applies to. If Reasons is not set, this budget applies to all methods. Otherwise, this will apply to each reason defined. - allowed reasons are underutilized, empty, and drifted. + allowed reasons are Underutilized, Empty, and Drifted. items: description: DisruptionReason defines valid reasons for disruption budgets. enum: - - underutilized - - empty - - drifted + - Underutilized + - Empty + - Drifted type: string type: array schedule: @@ -591,13 +591,13 @@ spec: description: |- Reasons is a list of disruption methods that this budget applies to. If Reasons is not set, this budget applies to all methods. Otherwise, this will apply to each reason defined. - allowed reasons are underutilized, empty, and drifted. + allowed reasons are Underutilized, Empty, and Drifted. items: description: DisruptionReason defines valid reasons for disruption budgets. enum: - - underutilized - - empty - - drifted + - Underutilized + - Empty + - Drifted type: string type: array schedule: diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml index e9f14a6035..0fb3fdbae9 100644 --- a/pkg/apis/crds/karpenter.sh_nodepools.yaml +++ b/pkg/apis/crds/karpenter.sh_nodepools.yaml @@ -115,13 +115,13 @@ spec: description: |- Reasons is a list of disruption methods that this budget applies to. If Reasons is not set, this budget applies to all methods. Otherwise, this will apply to each reason defined. - allowed reasons are underutilized, empty, and drifted. + allowed reasons are Underutilized, Empty, and Drifted. items: description: DisruptionReason defines valid reasons for disruption budgets. enum: - - underutilized - - empty - - drifted + - Underutilized + - Empty + - Drifted type: string type: array schedule: @@ -589,13 +589,13 @@ spec: description: |- Reasons is a list of disruption methods that this budget applies to. If Reasons is not set, this budget applies to all methods. Otherwise, this will apply to each reason defined. - allowed reasons are underutilized, empty, and drifted. + allowed reasons are Underutilized, Empty, and Drifted. items: description: DisruptionReason defines valid reasons for disruption budgets. enum: - - underutilized - - empty - - drifted + - Underutilized + - Empty + - Drifted type: string type: array schedule: diff --git a/pkg/apis/v1/nodepool.go b/pkg/apis/v1/nodepool.go index 3b11f42e75..73edda062f 100644 --- a/pkg/apis/v1/nodepool.go +++ b/pkg/apis/v1/nodepool.go @@ -102,7 +102,7 @@ type Disruption struct { type Budget struct { // Reasons is a list of disruption methods that this budget applies to. If Reasons is not set, this budget applies to all methods. // Otherwise, this will apply to each reason defined. - // allowed reasons are underutilized, empty, and drifted. + // allowed reasons are Underutilized, Empty, and Drifted. // +optional Reasons []DisruptionReason `json:"reasons,omitempty"` // Nodes dictates the maximum number of NodeClaims owned by this NodePool @@ -142,13 +142,13 @@ const ( ) // DisruptionReason defines valid reasons for disruption budgets. -// +kubebuilder:validation:Enum={underutilized,empty,drifted} +// +kubebuilder:validation:Enum={Underutilized,Empty,Drifted} type DisruptionReason string const ( - DisruptionReasonUnderutilized DisruptionReason = "underutilized" - DisruptionReasonEmpty DisruptionReason = "empty" - DisruptionReasonDrifted DisruptionReason = "drifted" + DisruptionReasonUnderutilized DisruptionReason = "Underutilized" + DisruptionReasonEmpty DisruptionReason = "Empty" + DisruptionReasonDrifted DisruptionReason = "Drifted" ) var ( diff --git a/pkg/apis/v1/nodepool_validation_cel_test.go b/pkg/apis/v1/nodepool_validation_cel_test.go index 56d87ab42f..548794c0a5 100644 --- a/pkg/apis/v1/nodepool_validation_cel_test.go +++ b/pkg/apis/v1/nodepool_validation_cel_test.go @@ -237,9 +237,9 @@ var _ = Describe("CEL/Validation", func() { }} Expect(env.Client.Create(ctx, nodePool)).To(Succeed()) }, - Entry("should allow disruption reason drifted", DisruptionReasonDrifted), - Entry("should allow disruption reason underutilized", DisruptionReasonUnderutilized), - Entry("should allow disruption reason empty", DisruptionReasonEmpty), + Entry("should allow disruption reason Drifted", DisruptionReasonDrifted), + Entry("should allow disruption reason Underutilized", DisruptionReasonUnderutilized), + Entry("should allow disruption reason Empty", DisruptionReasonEmpty), ) DescribeTable("should fail when creating a budget with invalid reasons", func(reason string) { diff --git a/pkg/apis/v1beta1/nodepool.go b/pkg/apis/v1beta1/nodepool.go index 3078c7ab1f..21c9d9a5c6 100644 --- a/pkg/apis/v1beta1/nodepool.go +++ b/pkg/apis/v1beta1/nodepool.go @@ -102,7 +102,7 @@ type Disruption struct { type Budget struct { // Reasons is a list of disruption methods that this budget applies to. If Reasons is not set, this budget applies to all methods. // Otherwise, this will apply to each reason defined. - // allowed reasons are underutilized, empty, and drifted. + // allowed reasons are Underutilized, Empty, and Drifted. // +optional Reasons []DisruptionReason `json:"reasons,omitempty"` // Nodes dictates the maximum number of NodeClaims owned by this NodePool @@ -142,13 +142,13 @@ const ( ) // DisruptionReason defines valid reasons for disruption budgets. -// +kubebuilder:validation:Enum={underutilized,empty,drifted} +// +kubebuilder:validation:Enum={Underutilized,Empty,Drifted} type DisruptionReason string const ( - DisruptionReasonUnderutilized DisruptionReason = "underutilized" - DisruptionReasonEmpty DisruptionReason = "empty" - DisruptionReasonDrifted DisruptionReason = "drifted" + DisruptionReasonUnderutilized DisruptionReason = "Underutilized" + DisruptionReasonEmpty DisruptionReason = "Empty" + DisruptionReasonDrifted DisruptionReason = "Drifted" ) var ( diff --git a/pkg/apis/v1beta1/nodepool_validation_cel_test.go b/pkg/apis/v1beta1/nodepool_validation_cel_test.go index be3d591180..8e45e4ecd0 100644 --- a/pkg/apis/v1beta1/nodepool_validation_cel_test.go +++ b/pkg/apis/v1beta1/nodepool_validation_cel_test.go @@ -241,9 +241,9 @@ var _ = Describe("CEL/Validation", func() { }} Expect(env.Client.Create(ctx, nodePool)).To(Succeed()) }, - Entry("should allow disruption reason drifted", DisruptionReasonDrifted), - Entry("should allow disruption reason underutilized", DisruptionReasonUnderutilized), - Entry("should allow disruption reason empty", DisruptionReasonEmpty), + Entry("should allow disruption reason Drifted", DisruptionReasonDrifted), + Entry("should allow disruption reason Underutilized", DisruptionReasonUnderutilized), + Entry("should allow disruption reason Empty", DisruptionReasonEmpty), ) DescribeTable("should fail when creating a budget with invalid reasons", func(reason string) {