Skip to content

Commit

Permalink
Add more tests and documentation.
Browse files Browse the repository at this point in the history
  • Loading branch information
nikmohan committed Feb 15, 2024
1 parent 0ac1ae3 commit f589fb4
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 25 deletions.
6 changes: 3 additions & 3 deletions pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -489,9 +489,9 @@ spec:
description: The label key that the selector applies to.
type: string
minValues:
description: MinValues is the minimum number of unique
values required to define the flexibility of the specific
requirement.
description: |-
This is an alpha field.
MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
maximum: 100
minimum: 1
type: integer
Expand Down
4 changes: 3 additions & 1 deletion pkg/apis/crds/karpenter.sh_nodeclaims.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,9 @@ spec:
- message: label domain "karpenter.k8s.aws" is restricted
rule: self in ["karpenter.k8s.aws/instance-encryption-in-transit-supported", "karpenter.k8s.aws/instance-category", "karpenter.k8s.aws/instance-hypervisor", "karpenter.k8s.aws/instance-family", "karpenter.k8s.aws/instance-generation", "karpenter.k8s.aws/instance-local-nvme", "karpenter.k8s.aws/instance-size", "karpenter.k8s.aws/instance-cpu","karpenter.k8s.aws/instance-memory", "karpenter.k8s.aws/instance-network-bandwidth", "karpenter.k8s.aws/instance-gpu-name", "karpenter.k8s.aws/instance-gpu-manufacturer", "karpenter.k8s.aws/instance-gpu-count", "karpenter.k8s.aws/instance-gpu-memory", "karpenter.k8s.aws/instance-accelerator-name", "karpenter.k8s.aws/instance-accelerator-manufacturer", "karpenter.k8s.aws/instance-accelerator-count"] || !self.find("^([^/]+)").endsWith("karpenter.k8s.aws")
minValues:
description: MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
description: |-
This is an alpha field.
MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
maximum: 100
minimum: 1
type: integer
Expand Down
6 changes: 4 additions & 2 deletions pkg/apis/crds/karpenter.sh_nodepools.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ spec:
type: array
x-kubernetes-validations:
- message: '''schedule'' must be set with ''duration'''
rule: '!self.all(x, (has(x.schedule) && !has(x.duration)) || (!has(x.schedule) && has(x.duration)))'
rule: self.all(x, has(x.schedule) == has(x.duration))
consolidateAfter:
description: |-
ConsolidateAfter is the duration the controller will wait
Expand Down Expand Up @@ -356,7 +356,9 @@ spec:
- message: label domain "karpenter.k8s.aws" is restricted
rule: self in ["karpenter.k8s.aws/instance-encryption-in-transit-supported", "karpenter.k8s.aws/instance-category", "karpenter.k8s.aws/instance-hypervisor", "karpenter.k8s.aws/instance-family", "karpenter.k8s.aws/instance-generation", "karpenter.k8s.aws/instance-local-nvme", "karpenter.k8s.aws/instance-size", "karpenter.k8s.aws/instance-cpu","karpenter.k8s.aws/instance-memory", "karpenter.k8s.aws/instance-network-bandwidth", "karpenter.k8s.aws/instance-gpu-name", "karpenter.k8s.aws/instance-gpu-manufacturer", "karpenter.k8s.aws/instance-gpu-count", "karpenter.k8s.aws/instance-gpu-memory", "karpenter.k8s.aws/instance-accelerator-name", "karpenter.k8s.aws/instance-accelerator-manufacturer", "karpenter.k8s.aws/instance-accelerator-count"] || !self.find("^([^/]+)").endsWith("karpenter.k8s.aws")
minValues:
description: MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
description: |-
This is an alpha field.
MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
maximum: 100
minimum: 1
type: integer
Expand Down
132 changes: 113 additions & 19 deletions pkg/cloudprovider/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,15 @@ var _ = Describe("CloudProvider", func() {
createFleetInput := awsEnv.EC2API.CreateFleetBehavior.CalledWithInput.Pop()
Expect(createFleetInput.Context).To(BeNil())
})
It("should set context on the CreateFleet request and respect minValues from NodePool", func() {
It("should set context on the CreateFleet request and respect minValues for In operator requirement from NodePool", func() {
nodeClass.Spec.Context = aws.String(contextID)

// Create fake InstanceTypes where one instances can fit 2 pods and another one can fit only 1 pod.
// This specific type of inputs will help us differentiate the scenario we are trying to test where ideally
// 1 instance launch would have been sufficient to fit the pods and was cheaper but we would launch 2 separate
// instances to meet the minimum requirement.
instances := fake.MakeFakeInstances()
instances = lo.Slice(instances, 0, 2)
instances, _ = fake.MakeUniqueFakeInstancesAndFamilies(instances, 2)
instances[0].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(1)}
instances[1].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(8)}
awsEnv.EC2API.DescribeInstanceTypesOutput.Set(&ec2.DescribeInstanceTypesOutput{InstanceTypes: instances})
Expand Down Expand Up @@ -289,17 +292,14 @@ var _ = Describe("CloudProvider", func() {
// This ensures that the CreateFleet received the context.
Expect(aws.StringValue(createFleetInput.Context)).To(Equal(contextID))
})
It("should set context on the CreateFleet request and respect minValues from multiple keys in NodePool", func() {
It("should set context on the CreateFleet request and respect minValues for Exists Operator in requirement from NodePool", func() {
nodeClass.Spec.Context = aws.String(contextID)

// Create fake InstanceTypes where 2 instances can fit 2 pods individually and one can fit only 1 pod.
// Create fake InstanceTypes where one instances can fit 2 pods and another one can fit only 1 pod.
instances := fake.MakeFakeInstances()
instances = lo.Slice(instances, 0, 3)
instanceFamilies := sets.Set[string]{}
instanceFamilies = instanceFamilies.Insert(lo.Map(instances, func(info *ec2.InstanceTypeInfo, _ int) string { return strings.Split(*info.InstanceType, ".")[0] })...)
instances, _ = fake.MakeUniqueFakeInstancesAndFamilies(instances, 2)
instances[0].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(1)}
instances[1].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(4)}
instances[2].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(8)}
instances[1].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(8)}
awsEnv.EC2API.DescribeInstanceTypesOutput.Set(&ec2.DescribeInstanceTypesOutput{InstanceTypes: instances})
awsEnv.EC2API.DescribeInstanceTypeOfferingsOutput.Set(&ec2.DescribeInstanceTypeOfferingsOutput{InstanceTypeOfferings: fake.MakeFakeInstanceOfferings(instances)})
now := time.Now()
Expand All @@ -317,16 +317,114 @@ var _ = Describe("CloudProvider", func() {
SpotPrice: aws.String("0.003"),
Timestamp: &now,
},
},
})
Expect(awsEnv.PricingProvider.UpdateSpotPricing(ctx)).To(Succeed())
instanceNames := lo.Map(instances, func(info *ec2.InstanceTypeInfo, _ int) string { return *info.InstanceType })

// Define NodePool that has minValues on instance-type requirement.
nodePool = coretest.NodePool(corev1beta1.NodePool{
Spec: corev1beta1.NodePoolSpec{
Template: corev1beta1.NodeClaimTemplate{
Spec: corev1beta1.NodeClaimSpec{
NodeClassRef: &corev1beta1.NodeClassReference{
Name: nodeClass.Name,
},
Requirements: []corev1beta1.NodeSelectorRequirementWithFlexibility{
{
NodeSelectorRequirement: v1.NodeSelectorRequirement{
Key: v1.LabelInstanceTypeStable,
Operator: v1.NodeSelectorOpExists,
},
MinValues: lo.ToPtr(2),
},
{
NodeSelectorRequirement: v1.NodeSelectorRequirement{
Key: v1.LabelInstanceTypeStable,
Operator: v1.NodeSelectorOpIn,
Values: instanceNames,
},
MinValues: lo.ToPtr(1),
},
},
},
},
},
})

ExpectApplied(ctx, env.Client, nodePool, nodeClass)

// 2 pods are created with resources such that both fit together only in one of the 2 InstanceTypes created above.
pod1 := coretest.UnschedulablePod(
coretest.PodOptions{
ResourceRequirements: v1.ResourceRequirements{Requests: v1.ResourceList{
v1.ResourceCPU: resource.MustParse("0.9")},
},
})
pod2 := coretest.UnschedulablePod(
coretest.PodOptions{
ResourceRequirements: v1.ResourceRequirements{Requests: v1.ResourceList{
v1.ResourceCPU: resource.MustParse("0.9")},
},
})
ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod1, pod2)

// Under normal circumstances 1 node would have been created that fits both the pods but
// here minValue enforces to include both the instances. And since one of the instances can
// only fit 1 pod, only 1 pod is scheduled to run in the node to be launched by CreateFleet.
node1 := ExpectScheduled(ctx, env.Client, pod1)
node2 := ExpectScheduled(ctx, env.Client, pod2)

// This ensures that the pods are scheduled in 2 different nodes.
Expect(node1.Name).ToNot(Equal(node2.Name))
Expect(awsEnv.EC2API.CreateFleetBehavior.CalledWithInput.Len()).To(Equal(2))
createFleetInput := awsEnv.EC2API.CreateFleetBehavior.CalledWithInput.Pop()
uniqueInstanceTypes := sets.Set[string]{}
for _, launchTemplateConfig := range createFleetInput.LaunchTemplateConfigs {
for _, override := range launchTemplateConfig.Overrides {
uniqueInstanceTypes.Insert(*override.InstanceType)
}
}
// This ensures that we have sent the minimum number of requirements defined in the NodePool.
Expect(len(uniqueInstanceTypes)).To(BeNumerically(">=", 2))
// This ensures that the CreateFleet received the context.
Expect(aws.StringValue(createFleetInput.Context)).To(Equal(contextID))
})
It("should set context on the CreateFleet request and respect minValues from multiple keys in NodePool", func() {
nodeClass.Spec.Context = aws.String(contextID)
// Create fake InstanceTypes where 2 instances can fit 2 pods individually and one can fit only 1 pod.
instances := fake.MakeFakeInstances()
uniqInstanceTypes, instanceFamilies := fake.MakeUniqueFakeInstancesAndFamilies(instances, 3)
uniqInstanceTypes[0].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(1)}
uniqInstanceTypes[1].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(4)}
uniqInstanceTypes[2].VCpuInfo = &ec2.VCpuInfo{DefaultVCpus: aws.Int64(8)}
awsEnv.EC2API.DescribeInstanceTypesOutput.Set(&ec2.DescribeInstanceTypesOutput{InstanceTypes: uniqInstanceTypes})
awsEnv.EC2API.DescribeInstanceTypeOfferingsOutput.Set(&ec2.DescribeInstanceTypeOfferingsOutput{InstanceTypeOfferings: fake.MakeFakeInstanceOfferings(uniqInstanceTypes)})
now := time.Now()
awsEnv.EC2API.DescribeSpotPriceHistoryOutput.Set(&ec2.DescribeSpotPriceHistoryOutput{
SpotPriceHistory: []*ec2.SpotPrice{
{
AvailabilityZone: aws.String("test-zone-1a"),
InstanceType: uniqInstanceTypes[0].InstanceType,
SpotPrice: aws.String("0.002"),
Timestamp: &now,
},
{
AvailabilityZone: aws.String("test-zone-1a"),
InstanceType: uniqInstanceTypes[1].InstanceType,
SpotPrice: aws.String("0.003"),
Timestamp: &now,
},
{
AvailabilityZone: aws.String("test-zone-1a"),
InstanceType: instances[2].InstanceType,
InstanceType: uniqInstanceTypes[2].InstanceType,
SpotPrice: aws.String("0.004"),
Timestamp: &now,
},
},
})
Expect(awsEnv.PricingProvider.UpdateSpotPricing(ctx)).To(Succeed())
instanceNames := lo.Map(instances, func(info *ec2.InstanceTypeInfo, _ int) string { return *info.InstanceType })
instanceNames := lo.Map(uniqInstanceTypes, func(info *ec2.InstanceTypeInfo, _ int) string { return *info.InstanceType })

// Define NodePool that has minValues in multiple requirements.
nodePool = coretest.NodePool(corev1beta1.NodePool{
Expand All @@ -352,8 +450,8 @@ var _ = Describe("CloudProvider", func() {
Operator: v1.NodeSelectorOpIn,
Values: instanceFamilies.UnsortedList(),
},
// consider at least 3 unique instance families if provided by the fake instanceProvider
MinValues: lo.Ternary(len(instanceFamilies) >= 3, lo.ToPtr(3), nil),
// consider at least 3 unique instance families
MinValues: lo.ToPtr(3),
},
},
},
Expand Down Expand Up @@ -392,13 +490,9 @@ var _ = Describe("CloudProvider", func() {
}
}
// Ensure that there are at least minimum number of unique instance types as per the requirement in the CreateFleet request.
Expect(len(uniqueInstanceTypes)).To(BeNumerically(">=", 2))

Expect(len(uniqueInstanceTypes)).To(BeNumerically("==", 3))
// Ensure that there are at least minimum number of unique instance families as per the requirement in the CreateFleet request.
if len(instanceFamilies) > 2 {
Expect(len(uniqueInstanceTypes)).To(BeNumerically("==", 3))
Expect(len(uniqueInstanceFamilies)).To(BeNumerically("==", 3))
}
Expect(len(uniqueInstanceFamilies)).To(BeNumerically("==", 3))
Expect(aws.StringValue(createFleetInput.Context)).To(Equal(contextID))
})
})
Expand Down
19 changes: 19 additions & 0 deletions pkg/fake/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/samber/lo"
"k8s.io/apimachinery/pkg/util/sets"

"github.com/aws/karpenter-provider-aws/pkg/operator/options"
"github.com/aws/karpenter-provider-aws/pkg/providers/pricing"
Expand Down Expand Up @@ -192,6 +193,24 @@ func MakeFakeInstances() []*ec2.InstanceTypeInfo {
return instanceTypes
}

func MakeUniqueFakeInstancesAndFamilies(instances []*ec2.InstanceTypeInfo, numInstanceFamilies int) ([]*ec2.InstanceTypeInfo, sets.Set[string]) {
var instanceTypes []*ec2.InstanceTypeInfo
instanceFamilies := sets.Set[string]{}
for _, it := range instances {
for instFamily := range instanceFamilies {
if strings.Split(*it.InstanceType, ".")[0] == instFamily {
continue
}
}
instanceTypes = append(instanceTypes, it)
instanceFamilies.Insert(strings.Split(*it.InstanceType, ".")[0])
if len(instanceFamilies) == numInstanceFamilies {
break
}
}
return instanceTypes, instanceFamilies
}

func MakeFakeInstanceOfferings(instanceTypes []*ec2.InstanceTypeInfo) []*ec2.InstanceTypeOffering {
var instanceTypeOfferings []*ec2.InstanceTypeOffering

Expand Down

0 comments on commit f589fb4

Please sign in to comment.