Skip to content

Commit

Permalink
chore: capitalize disruption budgets by reason (#1417)
Browse files Browse the repository at this point in the history
  • Loading branch information
njtran authored Jul 15, 2024
1 parent 5939241 commit c4be45e
Show file tree
Hide file tree
Showing 7 changed files with 40 additions and 40 deletions.
16 changes: 8 additions & 8 deletions designs/disruption-controls-by-reason.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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?
Expand Down
16 changes: 8 additions & 8 deletions kwok/charts/crds/karpenter.sh_nodepools.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
16 changes: 8 additions & 8 deletions pkg/apis/crds/karpenter.sh_nodepools.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
10 changes: 5 additions & 5 deletions pkg/apis/v1/nodepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 (
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/v1/nodepool_validation_cel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
10 changes: 5 additions & 5 deletions pkg/apis/v1beta1/nodepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 (
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/v1beta1/nodepool_validation_cel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit c4be45e

Please sign in to comment.