feat: Disruption Budgets (#849)

pkg/apis/crds/karpenter.sh_nodepools.yaml

@@ -59,11 +59,11 @@ spec:
                             type: string
                           nodes:
                             default: 10%
-                            description: 'Nodes 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. We can''t use an intstr.IntOrString since kubebuilder doesn''t support pattern checking for int nodes for IntOrString nodes. Ref: https://github.com/kubernetes-sigs/controller-tools/blob/55efe4be40394a288216dab63156b0a64fb82929/pkg/crd/markers/validation.go#L379-L388'
+                            description: 'Nodes dictates the maximum number of NodeClaims owned by this NodePool that can be terminating at once. This is calculated by counting nodes that have a deletion timestamp set, or are actively being deleted by Karpenter. This field is required when specifying a budget. This cannot be of type intstr.IntOrString since kubebuilder doesn''t support pattern checking for int nodes for IntOrString nodes. Ref: https://github.com/kubernetes-sigs/controller-tools/blob/55efe4be40394a288216dab63156b0a64fb82929/pkg/crd/markers/validation.go#L379-L388'
                             pattern: ^((100|[0-9]{1,2})%|[0-9]+)$
                             type: string
                           schedule:
-                            description: Schedule 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.
+                            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
                         required:

pkg/apis/v1beta1/nodepool.go

@@ -101,19 +101,20 @@ type Disruption struct {
 // Budget defines when Karpenter will restrict the
 // number of Node Claims that can be terminating simultaneously.
 type Budget struct {
-	// Nodes 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.
-	// We can't use an intstr.IntOrString since kubebuilder doesn't support pattern
+	// Nodes dictates the maximum number of NodeClaims owned by this NodePool
+	// that can be terminating at once. This is calculated by counting nodes that
+	// have a deletion timestamp set, or are actively being deleted by Karpenter.
+	// This field is required when specifying a budget.
+	// This cannot be of type intstr.IntOrString since kubebuilder doesn't support pattern
 	// checking for int nodes for IntOrString nodes.
 	// Ref: https://github.com/kubernetes-sigs/controller-tools/blob/55efe4be40394a288216dab63156b0a64fb82929/pkg/crd/markers/validation.go#L379-L388
 	// +kubebuilder:validation:Pattern:="^((100|[0-9]{1,2})%|[0-9]+)$"
 	// +kubebuilder:default:="10%"
 	Nodes string `json:"nodes" hash:"ignore"`
-	// Schedule 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.
+	// 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
 	Schedule *string `json:"schedule,omitempty" hash:"ignore"`
@@ -212,46 +213,55 @@ func (pl *NodePoolList) OrderByWeight() {
 	})
 }
 
+// MustGetAllowedDisruptions calls GetAllowedDisruptions and returns 0 if the error is not nil. This reduces the
+// amount of state that the disruption controller must reconcile, while allowing the GetAllowedDisruptions()
+// to bubble up any errors in validation.
+func (in *NodePool) MustGetAllowedDisruptions(ctx context.Context, c clock.Clock, numNodes int) int {
+	val, err := in.GetAllowedDisruptions(ctx, c, numNodes)
+	if err != nil {
+		return 0
+	}
+	return val
+}
+
 // GetAllowedDisruptions returns the minimum allowed disruptions across all disruption budgets for a given node pool.
-// This returns two values as the resolved value for a percent depends on the number of current node claims.
+// This will return an error if there is a configuration error with any budget's node or schedule values.
 func (in *NodePool) GetAllowedDisruptions(ctx context.Context, c clock.Clock, numNodes int) (int, error) {
-	var errs error
 	minVal := math.MaxInt32
+	var multiErr error
 	for i := range in.Spec.Disruption.Budgets {
 		val, err := in.Spec.Disruption.Budgets[i].GetAllowedDisruptions(c, numNodes)
 		if err != nil {
-			errs = multierr.Append(errs, err)
+			multiErr = multierr.Append(multiErr, err)
 		}
 		minVal = lo.Ternary(val < minVal, val, minVal)
 	}
-	if errs != nil {
-		return 0, fmt.Errorf("getting nodepool allowed disruptions, %w", errs)
-	}
-	return minVal, nil
+	return minVal, multiErr
 }
 
 // GetAllowedDisruptions returns an intstr.IntOrString that can be used a comparison
 // for calculating if a disruption action is allowed. It returns an error if the
 // schedule is invalid. This returns MAXINT if the value is unbounded.
 func (in *Budget) GetAllowedDisruptions(c clock.Clock, numNodes int) (int, error) {
 	active, err := in.IsActive(c)
+	// If the budget is misconfigured, fail closed.
 	if err != nil {
 		return 0, err
 	}
 	if !active {
 		return math.MaxInt32, nil
 	}
-	var val intstr.IntOrString
-	// If err is nil, we treat it as an int.
-	if intVal, err := strconv.Atoi(in.Nodes); err == nil {
-		val = intstr.FromInt(intVal)
-	} else {
-		val = intstr.FromString(in.Nodes)
-	}
-	res, err := intstr.GetScaledValueFromIntOrPercent(lo.ToPtr(val), numNodes, false)
+	// This will round up to the nearest whole number. Therefore, a disruption can
+	// sometimes exceed the disruption budget. This is the same as how Kubernetes
+	// handles MaxUnavailable with PDBs. Take the case with 5% disruptions, but
+	// 10 nodes. Karpenter will opt to allow 1 node to be disrupted, rather than
+	// blocking all disruptions for this nodepool.
+	res, err := intstr.GetScaledValueFromIntOrPercent(lo.ToPtr(GetIntStrFromValue(in.Nodes)), numNodes, true)
 	if err != nil {
-		// Should almost never happen since this is validated when the nodepool is applied
-		return 0, fmt.Errorf("getting intstr scaled value, %w", err)
+		// Should never happen since this is validated when the nodepool is applied
+		// If this value is incorrectly formatted, fail closed, since we don't know what
+		// they want here.
+		return 0, err
 	}
 	return res, nil
 }
@@ -268,10 +278,20 @@ func (in *Budget) IsActive(c clock.Clock) (bool, error) {
 	}
 	schedule, err := cron.ParseStandard(lo.FromPtr(in.Schedule))
 	if err != nil {
-		return false, fmt.Errorf("parsing schedule, %w", err)
+		// Should only occur if there's a discrepancy
+		// with the validation regex and the cron package.
+		return false, fmt.Errorf("invariant violated, invalid cron %s", schedule)
 	}
 	// Walk back in time for the duration associated with the schedule
 	checkPoint := c.Now().Add(-lo.FromPtr(in.Duration).Duration)
 	nextHit := schedule.Next(checkPoint)
 	return !nextHit.After(c.Now()), nil
 }
+
+func GetIntStrFromValue(str string) intstr.IntOrString {
+	// If err is nil, we treat it as an int.
+	if intVal, err := strconv.Atoi(str); err == nil {
+		return intstr.FromInt(intVal)
+	}
+	return intstr.FromString(str)
+}

pkg/apis/v1beta1/nodepool_budgets_test.go

@@ -70,43 +70,45 @@ var _ = Describe("Budgets", func() {
 			},
 		}
 	})
-	Context("NodePool/AllowedDisruptions", func() {
+	Context("MustGetAllowedDisruptions", func() {
 		It("should return the min allowedDisruptions", func() {
-			min, err := nodePool.GetAllowedDisruptions(ctx, fakeClock, 100)
-			Expect(err).To(Succeed())
+			min := nodePool.MustGetAllowedDisruptions(ctx, fakeClock, 100)
 			Expect(min).To(BeNumerically("==", 10))
 		})
 		It("should return the min allowedDisruptions, ignoring inactive crons", func() {
 			// Make the first and third budgets inactive
 			budgets[0].Schedule = lo.ToPtr("@yearly")
 			budgets[2].Schedule = lo.ToPtr("@yearly")
-			min, err := nodePool.GetAllowedDisruptions(ctx, fakeClock, 100)
-			Expect(err).To(Succeed())
+			min := nodePool.MustGetAllowedDisruptions(ctx, fakeClock, 100)
 			Expect(min).To(BeNumerically("==", 100))
 		})
 		It("should return MaxInt32 if all crons are inactive", func() {
 			budgets[0].Schedule = lo.ToPtr("@yearly")
 			budgets[1].Schedule = lo.ToPtr("@yearly")
 			budgets[2].Schedule = lo.ToPtr("@yearly")
 			budgets[3].Schedule = lo.ToPtr("@yearly")
-			min, err := nodePool.GetAllowedDisruptions(ctx, fakeClock, 100)
-			Expect(err).To(Succeed())
+			min := nodePool.MustGetAllowedDisruptions(ctx, fakeClock, 100)
 			Expect(min).To(BeNumerically("==", math.MaxInt32))
 		})
-		It("should fail and return zero values if a schedule is invalid", func() {
+		It("should return zero values if a schedule is invalid", func() {
 			budgets[0].Schedule = lo.ToPtr("@wrongly")
-			min, err := nodePool.GetAllowedDisruptions(ctx, fakeClock, 100)
-			Expect(err).ToNot(Succeed())
+			min := nodePool.MustGetAllowedDisruptions(ctx, fakeClock, 100)
 			Expect(min).To(BeNumerically("==", 0))
 		})
 	})
-	Context("Budget/AllowedDisruptions", func() {
-		It("should fail and return zero values if a schedule is invalid", func() {
+	Context("AllowedDisruptions", func() {
+		It("should return zero values if a schedule is invalid", func() {
 			budgets[0].Schedule = lo.ToPtr("@wrongly")
 			val, err := budgets[0].GetAllowedDisruptions(fakeClock, 100)
 			Expect(err).ToNot(Succeed())
 			Expect(val).To(BeNumerically("==", 0))
 		})
+		It("should return zero values if a nodes value is invalid", func() {
+			budgets[0].Nodes = "1000a%"
+			val, err := budgets[0].GetAllowedDisruptions(fakeClock, 100)
+			Expect(err).ToNot(Succeed())
+			Expect(val).To(BeNumerically("==", 0))
+		})
 		It("should return MaxInt32 when a budget is inactive", func() {
 			budgets[0].Schedule = lo.ToPtr("@yearly")
 			budgets[0].Duration = lo.ToPtr(metav1.Duration{Duration: lo.Must(time.ParseDuration("1h"))})