Sitelet https://github.com/kubernetes/kops/commit/3993936651025cc83eaa6cbe252890ec8d8f57f4
Skip to content

Commit 3993936

Browse files
kubernetes-prow[bot]kubernetes-prow[bot]
authored
Merge pull request #18761 from hakman/karpenter-validation
aws: validate Karpenter InstanceGroups early
2 parents e223b8c + facbebb commit 3993936

10 files changed

Lines changed: 438 additions & 160 deletions

‎docs/operations/karpenter.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ Supported image selector forms are:
8181
{{ kops_feature_table(kops_added_default='1.37') }}
8282

8383
By default, the generated `NodePool` requires one of the instance types listed in `spec.machineType` and `spec.mixedInstancesPolicy.instances`.
84-
To let Karpenter choose from any instance type within a capacity range instead, set `spec.mixedInstancesPolicy.instanceRequirements`:
84+
To let Karpenter choose instance types based on a capacity range, set `spec.mixedInstancesPolicy.instanceRequirements`:
8585

8686
```yaml
8787
spec:
@@ -103,7 +103,11 @@ spec:
103103
This generates the appropriate `karpenter.k8s.aws/instance-cpu` and `karpenter.k8s.aws/instance-memory` requirements on the `NodePool`.
104104
`excludedInstanceTypes` entries are mapped to `NotIn` requirements: a `<family>.*` wildcard excludes an instance family, and a bare instance type excludes that type.
105105

106-
When `instanceRequirements` is set, kOps omits the instance type requirement, and `spec.machineType` and `spec.mixedInstancesPolicy.instances` no longer restrict the NodePool.
106+
When `instanceRequirements` is set, neither `spec.machineType` nor `spec.mixedInstancesPolicy.instances` is required.
107+
If either field is set, its instance types further restrict the generated `NodePool`.
108+
109+
Karpenter NodePools can include both GPU and non-GPU instance types.
110+
When using kOps-managed NVIDIA support, use a dedicated GPU-only InstanceGroup because kOps applies GPU labels and taints to the entire NodePool.
107111

108112
## Karpenter-managed InstanceGroups
109113
{{ kops_feature_table(kops_added_default='1.36') }}

‎pkg/apis/kops/validation/aws.go‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -292,30 +292,36 @@ func awsValidateInstanceInterruptionBehavior(fieldPath *field.Path, ig *kops.Ins
292292
func awsValidateMixedInstancesPolicy(path *field.Path, spec *kops.MixedInstancesPolicySpec, ig *kops.InstanceGroup, cloud awsup.AWSCloud) field.ErrorList {
293293
var errs field.ErrorList
294294

295-
mainMachineTypeInfo, err := awsup.GetMachineTypeInfo(cloud, ec2types.InstanceType(ig.Spec.MachineType))
296-
if err != nil {
297-
errs = append(errs, field.Invalid(field.NewPath("spec", "machineType"), ig.Spec.MachineType, fmt.Sprintf("machine type specified is invalid: %q", ig.Spec.MachineType)))
298-
return errs
299-
}
295+
if ig.Spec.Manager == kops.InstanceManagerKarpenter {
296+
for i, instanceTypes := range spec.Instances {
297+
fld := path.Child("instances").Index(i)
298+
errs = append(errs, awsValidateInstanceTypeAndImage(fld, path.Child("image"), instanceTypes, ig.Spec.Image, cloud)...)
299+
}
300+
} else {
301+
mainMachineTypeInfo, err := awsup.GetMachineTypeInfo(cloud, ec2types.InstanceType(ig.Spec.MachineType))
302+
if err != nil {
303+
errs = append(errs, field.Invalid(field.NewPath("spec", "machineType"), ig.Spec.MachineType, fmt.Sprintf("machine type specified is invalid: %q", ig.Spec.MachineType)))
304+
return errs
305+
}
300306

301-
hasGPU := mainMachineTypeInfo.GPU
307+
hasGPU := mainMachineTypeInfo.GPU
302308

303-
// @step: check the instance types are valid
304-
for i, instanceTypes := range spec.Instances {
305-
fld := path.Child("instances").Index(i)
306-
errs = append(errs, awsValidateInstanceTypeAndImage(path.Child("instances").Index(i), path.Child("image"), instanceTypes, ig.Spec.Image, cloud)...)
309+
// @step: check the instance types are valid
310+
for i, instanceTypes := range spec.Instances {
311+
fld := path.Child("instances").Index(i)
312+
errs = append(errs, awsValidateInstanceTypeAndImage(path.Child("instances").Index(i), path.Child("image"), instanceTypes, ig.Spec.Image, cloud)...)
307313

308-
for _, instanceType := range strings.Split(instanceTypes, ",") {
309-
machineTypeInfo, err := awsup.GetMachineTypeInfo(cloud, ec2types.InstanceType(instanceType))
310-
if err != nil {
311-
errs = append(errs, field.Invalid(field.NewPath("spec", "machineType"), ig.Spec.MachineType, fmt.Sprintf("machine type specified is invalid: %q", ig.Spec.MachineType)))
312-
return errs
313-
}
314-
if machineTypeInfo.GPU != hasGPU {
315-
errs = append(errs, field.Forbidden(fld, "Cannot mix GPU and non-GPU machine types in the same Instance Group"))
314+
for _, instanceType := range strings.Split(instanceTypes, ",") {
315+
machineTypeInfo, err := awsup.GetMachineTypeInfo(cloud, ec2types.InstanceType(instanceType))
316+
if err != nil {
317+
errs = append(errs, field.Invalid(field.NewPath("spec", "machineType"), ig.Spec.MachineType, fmt.Sprintf("machine type specified is invalid: %q", ig.Spec.MachineType)))
318+
return errs
319+
}
320+
if machineTypeInfo.GPU != hasGPU {
321+
errs = append(errs, field.Forbidden(fld, "Cannot mix GPU and non-GPU machine types in the same Instance Group"))
322+
}
316323
}
317324
}
318-
319325
}
320326

321327
if spec.OnDemandBase != nil {

‎pkg/apis/kops/validation/aws_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package validation
1818

1919
import (
20+
"errors"
2021
"testing"
2122
"time"
2223

@@ -31,6 +32,17 @@ import (
3132
"k8s.io/kops/pkg/apis/kops"
3233
)
3334

35+
type emptyInstanceTypeRejectingCloud struct {
36+
awsup.AWSCloud
37+
}
38+
39+
func (c *emptyInstanceTypeRejectingCloud) DescribeInstanceType(instanceType string) (*ec2types.InstanceTypeInfo, error) {
40+
if instanceType == "" {
41+
return nil, errors.New("instance type is empty")
42+
}
43+
return c.AWSCloud.DescribeInstanceType(instanceType)
44+
}
45+
3446
func TestAWSValidateEBSCSIDriver(t *testing.T) {
3547
grid := []struct {
3648
Input kops.ClusterSpec
@@ -360,6 +372,89 @@ func TestMixedInstancePolicies(t *testing.T) {
360372
}
361373
}
362374

375+
func TestKarpenterMixedInstancesPolicyValidation(t *testing.T) {
376+
baseCloud := awsup.BuildMockAWSCloud("us-east-1", "abc")
377+
mockEC2 := &mockec2.MockEC2{}
378+
baseCloud.MockEC2 = mockEC2
379+
mockEC2.Images = append(mockEC2.Images, &ec2types.Image{
380+
ImageId: aws.String("ami-073c8c0760395aab8"),
381+
Architecture: ec2types.ArchitectureValuesX8664,
382+
})
383+
cloud := &emptyInstanceTypeRejectingCloud{AWSCloud: baseCloud}
384+
385+
grid := []struct {
386+
desc string
387+
machineType string
388+
spec *kops.MixedInstancesPolicySpec
389+
expected []string
390+
}{
391+
{
392+
desc: "instance requirements",
393+
spec: &kops.MixedInstancesPolicySpec{
394+
InstanceRequirements: &kops.InstanceRequirementsSpec{},
395+
},
396+
},
397+
{
398+
desc: "capacity types",
399+
spec: &kops.MixedInstancesPolicySpec{
400+
OnDemandAboveBase: new(int64(50)),
401+
},
402+
},
403+
{
404+
desc: "instance list",
405+
spec: &kops.MixedInstancesPolicySpec{
406+
Instances: []string{"m5.large"},
407+
},
408+
},
409+
{
410+
desc: "heterogeneous instance list with machine type",
411+
machineType: "g4dn.xlarge",
412+
spec: &kops.MixedInstancesPolicySpec{
413+
Instances: []string{"m5.large"},
414+
InstanceRequirements: &kops.InstanceRequirementsSpec{},
415+
},
416+
},
417+
{
418+
desc: "instance requirements do not suppress instance validation",
419+
spec: &kops.MixedInstancesPolicySpec{
420+
Instances: []string{"t2.invalidType"},
421+
InstanceRequirements: &kops.InstanceRequirementsSpec{},
422+
},
423+
expected: []string{"Invalid value::spec.mixedInstancesPolicy.instances[0]"},
424+
},
425+
{
426+
desc: "comma-separated machine types",
427+
machineType: "m5.large,m5.xlarge",
428+
spec: &kops.MixedInstancesPolicySpec{
429+
InstanceRequirements: &kops.InstanceRequirementsSpec{},
430+
},
431+
},
432+
{
433+
desc: "heterogeneous instance list without machine type",
434+
spec: &kops.MixedInstancesPolicySpec{
435+
Instances: []string{"g4dn.xlarge", "m5.large"},
436+
InstanceRequirements: &kops.InstanceRequirementsSpec{},
437+
},
438+
},
439+
}
440+
441+
for _, g := range grid {
442+
t.Run(g.desc, func(t *testing.T) {
443+
ig := &kops.InstanceGroup{
444+
Spec: kops.InstanceGroupSpec{
445+
Manager: kops.InstanceManagerKarpenter,
446+
Image: "ami-073c8c0760395aab8",
447+
MachineType: g.machineType,
448+
MixedInstancesPolicy: g.spec,
449+
},
450+
}
451+
452+
errs := awsValidateInstanceGroup(ig, cloud)
453+
testErrors(t, g.desc, errs, g.expected)
454+
})
455+
}
456+
}
457+
363458
func TestInstanceMetadataOptions(t *testing.T) {
364459
cloud := awsup.BuildMockAWSCloud("us-east-1", "abc")
365460

‎pkg/apis/kops/validation/instancegroup.go‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,16 @@ package validation
1818

1919
import (
2020
"fmt"
21+
"math"
22+
"regexp"
2123
"strings"
2224

2325
"k8s.io/kops/pkg/nodeidentity/aws"
2426

2527
"github.com/aws/aws-sdk-go-v2/aws/arn"
2628
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
29+
"k8s.io/apimachinery/pkg/api/resource"
30+
contentvalidation "k8s.io/apimachinery/pkg/api/validate/content"
2731
apivalidation "k8s.io/apimachinery/pkg/api/validation"
2832
"k8s.io/apimachinery/pkg/util/sets"
2933
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -330,6 +334,9 @@ func validateKarpenterInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster
330334
if cluster.GetCloudProvider() != kops.CloudProviderAWS {
331335
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "manager"), "Karpenter InstanceGroups are only supported on AWS"))
332336
}
337+
if cluster.Spec.Karpenter == nil || !cluster.Spec.Karpenter.Enabled {
338+
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "manager"), "Karpenter InstanceGroups require cluster.spec.karpenter.enabled"))
339+
}
333340
if !g.Spec.Role.HasNode() {
334341
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "role"), "Karpenter InstanceGroups must have role Node"))
335342
}
@@ -338,9 +345,119 @@ func validateKarpenterInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster
338345
}
339346
allErrs = append(allErrs, validateKarpenterAMISelectorImage(g.Spec.Image, field.NewPath("spec", "image"))...)
340347
allErrs = append(allErrs, validateKarpenterStaticCapacity(g, cluster)...)
348+
allErrs = append(allErrs, validateKarpenterInstanceRequirements(g)...)
341349
return allErrs
342350
}
343351

352+
func validateKarpenterInstanceRequirements(g *kops.InstanceGroup) field.ErrorList {
353+
if g.Spec.MixedInstancesPolicy == nil || g.Spec.MixedInstancesPolicy.InstanceRequirements == nil {
354+
return nil
355+
}
356+
357+
requirements := g.Spec.MixedInstancesPolicy.InstanceRequirements
358+
requirementsPath := field.NewPath("spec", "mixedInstancesPolicy", "instanceRequirements")
359+
var allErrs field.ErrorList
360+
361+
if requirements.CPU != nil {
362+
minValue, errs := validateKarpenterCPURequirement(requirements.CPU.Min, requirementsPath.Child("cpu", "min"))
363+
allErrs = append(allErrs, errs...)
364+
maxValue, errs := validateKarpenterCPURequirement(requirements.CPU.Max, requirementsPath.Child("cpu", "max"))
365+
allErrs = append(allErrs, errs...)
366+
allErrs = append(allErrs, validateKarpenterRequirementRange(minValue, maxValue, requirements.CPU.Max, requirementsPath.Child("cpu", "max"))...)
367+
}
368+
369+
if requirements.Memory != nil {
370+
minValue, errs := validateKarpenterMemoryRequirement(requirements.Memory.Min, requirementsPath.Child("memory", "min"), true)
371+
allErrs = append(allErrs, errs...)
372+
maxValue, errs := validateKarpenterMemoryRequirement(requirements.Memory.Max, requirementsPath.Child("memory", "max"), false)
373+
allErrs = append(allErrs, errs...)
374+
allErrs = append(allErrs, validateKarpenterRequirementRange(minValue, maxValue, requirements.Memory.Max, requirementsPath.Child("memory", "max"))...)
375+
}
376+
377+
for i, configuredEntry := range requirements.ExcludedInstanceTypes {
378+
entryPath := requirementsPath.Child("excludedInstanceTypes").Index(i)
379+
entry := strings.TrimSpace(configuredEntry)
380+
if entry == "" {
381+
allErrs = append(allErrs, field.Invalid(entryPath, configuredEntry, "must not be empty"))
382+
continue
383+
}
384+
385+
value := entry
386+
if match := karpenterExcludedInstanceFamily.FindStringSubmatch(entry); match != nil {
387+
value = match[1]
388+
} else if strings.Contains(entry, "*") {
389+
allErrs = append(allErrs, field.Invalid(
390+
entryPath,
391+
configuredEntry,
392+
"only an instance type or a \"<family>.*\" family wildcard can be expressed as a NodePool requirement",
393+
))
394+
continue
395+
}
396+
for _, msg := range contentvalidation.IsLabelValue(value) {
397+
allErrs = append(allErrs, field.Invalid(entryPath, configuredEntry, msg))
398+
}
399+
}
400+
return allErrs
401+
}
402+
403+
var karpenterExcludedInstanceFamily = regexp.MustCompile(`^([a-z0-9][a-z0-9-]*)\.\*$`)
404+
405+
func validateKarpenterCPURequirement(quantity *resource.Quantity, fldPath *field.Path) (*int64, field.ErrorList) {
406+
if quantity == nil {
407+
return nil, nil
408+
}
409+
if quantity.Sign() < 0 {
410+
return nil, field.ErrorList{field.Invalid(fldPath, quantity, "must not be negative")}
411+
}
412+
value, ok := quantity.AsInt64()
413+
if !ok {
414+
return nil, field.ErrorList{field.Invalid(fldPath, quantity, "must be a whole number")}
415+
}
416+
if value > math.MaxInt32 {
417+
return nil, field.ErrorList{field.Invalid(fldPath, quantity, "is too large")}
418+
}
419+
return &value, nil
420+
}
421+
422+
func validateKarpenterMemoryRequirement(quantity *resource.Quantity, fldPath *field.Path, roundUp bool) (*int64, field.ErrorList) {
423+
if quantity == nil {
424+
return nil, nil
425+
}
426+
if quantity.Sign() < 0 {
427+
return nil, field.ErrorList{field.Invalid(fldPath, quantity, "must not be negative")}
428+
}
429+
430+
const mib = int64(1024 * 1024)
431+
maxBytes := int64(math.MaxInt32) * mib
432+
if !roundUp {
433+
maxBytes += mib - 1
434+
}
435+
maxQuantity := resource.NewQuantity(maxBytes, resource.DecimalSI)
436+
if quantity.Cmp(*maxQuantity) > 0 {
437+
return nil, field.ErrorList{field.Invalid(fldPath, quantity, "is too large")}
438+
}
439+
440+
bytes := quantity.Value()
441+
value := bytes / mib
442+
if roundUp && bytes%mib != 0 {
443+
value++
444+
}
445+
return &value, nil
446+
}
447+
448+
func validateKarpenterRequirementRange(minValue, maxValue *int64, maxQuantity *resource.Quantity, maxPath *field.Path) field.ErrorList {
449+
if maxValue == nil {
450+
return nil
451+
}
452+
if *maxValue == 0 {
453+
return field.ErrorList{field.Invalid(maxPath, maxQuantity, "must resolve to a positive value")}
454+
}
455+
if minValue != nil && *minValue > *maxValue {
456+
return field.ErrorList{field.Invalid(maxPath, maxQuantity, "must be greater than or equal to min")}
457+
}
458+
return nil
459+
}
460+
344461
func validateKarpenterStaticCapacity(g *kops.InstanceGroup, cluster *kops.Cluster) field.ErrorList {
345462
minPath := field.NewPath("spec", "minSize")
346463

0 commit comments

Comments
 (0)