Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Use AMI Status Controller to Asynchronously Hydrate AMI Data #6089

Merged
merged 6 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -478,19 +478,12 @@ spec:
type
items:
description: |-
A node selector requirement with min values is a selector that contains values, a key, an operator that relates the key and values
and minValues that represent the requirement to have at least that many values.
A node selector requirement is a selector that contains values, a key, and an operator
that relates the key and values.
properties:
key:
description: The label key that the selector applies to.
type: string
minValues:
description: |-
This field is ALPHA and can be dropped or replaced at any time
MinValues is the minimum number of unique values required to define the flexibility of the specific requirement.
maximum: 50
minimum: 1
type: integer
operator:
description: |-
Represents a key's relationship to a set of values.
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/v1beta1/ec2nodeclass_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ package v1beta1

import (
op "github.com/awslabs/operatorpkg/status"
corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1"
v1 "k8s.io/api/core/v1"
)

// Subnet contains resolved Subnet selector values utilized for node launch
Expand Down Expand Up @@ -49,7 +49,7 @@ type AMI struct {
Name string `json:"name,omitempty"`
// Requirements of the AMI to be utilized on an instance type
// +required
Requirements []corev1beta1.NodeSelectorRequirementWithMinValues `json:"requirements"`
Requirements []v1.NodeSelectorRequirement `json:"requirements"`
}

// EC2NodeClassStatus contains the resolved state of the EC2NodeClass
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 2 additions & 6 deletions pkg/cloudprovider/drift.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,10 @@ func (c *CloudProvider) isAMIDrifted(ctx context.Context, nodeClaim *corev1beta1
if !found {
return "", fmt.Errorf(`finding node instance type "%s"`, nodeClaim.Labels[v1.LabelInstanceTypeStable])
}
amis, err := c.amiProvider.Get(ctx, nodeClass, &amifamily.Options{})
if err != nil {
return "", fmt.Errorf("getting amis, %w", err)
}
if len(amis) == 0 {
if len(nodeClass.Status.AMIs) == 0 {
return "", fmt.Errorf("no amis exist given constraints")
}
mappedAMIs := amis.MapToInstanceTypes([]*cloudprovider.InstanceType{nodeInstanceType})
mappedAMIs := amifamily.MapToInstanceTypes([]*cloudprovider.InstanceType{nodeInstanceType}, nodeClass.Status.AMIs)
if !lo.Contains(lo.Keys(mappedAMIs), instance.ImageID) {
return AMIDrift, nil
}
Expand Down
101 changes: 55 additions & 46 deletions pkg/cloudprovider/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,41 +115,7 @@ var _ = Describe("CloudProvider", func() {
var nodePool *corev1beta1.NodePool
var nodeClaim *corev1beta1.NodeClaim
var _ = BeforeEach(func() {
nodeClass = test.EC2NodeClass(
v1beta1.EC2NodeClass{
Status: v1beta1.EC2NodeClassStatus{
InstanceProfile: "test-profile",
SecurityGroups: []v1beta1.SecurityGroup{
{
ID: "sg-test1",
Name: "securityGroup-test1",
},
{
ID: "sg-test2",
Name: "securityGroup-test2",
},
{
ID: "sg-test3",
Name: "securityGroup-test3",
},
},
Subnets: []v1beta1.Subnet{
{
ID: "subnet-test1",
Zone: "test-zone-1a",
},
{
ID: "subnet-test2",
Zone: "test-zone-1b",
},
{
ID: "subnet-test3",
Zone: "test-zone-1c",
},
},
},
},
)
nodeClass = test.EC2NodeClass()
nodeClass.StatusConditions().SetTrue(v1beta1.ConditionTypeNodeClassReady)
nodePool = coretest.NodePool(corev1beta1.NodePool{
Spec: corev1beta1.NodePoolSpec{
Expand Down Expand Up @@ -631,19 +597,36 @@ var _ = Describe("CloudProvider", func() {
},
},
})
nodeClass.Status.Subnets = []v1beta1.Subnet{
{
ID: validSubnet1,
Zone: "zone-1",
nodeClass.Status = v1beta1.EC2NodeClassStatus{
InstanceProfile: "test-profile",
Subnets: []v1beta1.Subnet{
{
ID: validSubnet1,
Zone: "zone-1",
},
{
ID: validSubnet2,
Zone: "zone-2",
},
},
{
ID: validSubnet2,
Zone: "zone-2",
SecurityGroups: []v1beta1.SecurityGroup{
{
ID: validSecurityGroup,
},
},
}
nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
{
ID: validSecurityGroup,
AMIs: []v1beta1.AMI{
{
ID: armAMIID,
Requirements: []v1.NodeSelectorRequirement{
{Key: v1.LabelArchStable, Operator: v1.NodeSelectorOpIn, Values: []string{corev1beta1.ArchitectureArm64}},
},
},
{
ID: amdAMIID,
Requirements: []v1.NodeSelectorRequirement{
{Key: v1.LabelArchStable, Operator: v1.NodeSelectorOpIn, Values: []string{corev1beta1.ArchitectureAmd64}},
},
},
},
}
ExpectApplied(ctx, env.Client, nodePool, nodeClass)
Expand Down Expand Up @@ -794,13 +777,27 @@ var _ = Describe("CloudProvider", func() {
})
It("should return drifted if the AMI no longer matches the existing NodeClaims instance type", func() {
nodeClass.Spec.AMISelectorTerms = []v1beta1.AMISelectorTerm{{ID: amdAMIID}}
nodeClass.Status.AMIs = []v1beta1.AMI{
{
ID: amdAMIID,
Requirements: []v1.NodeSelectorRequirement{
{Key: v1.LabelArchStable, Operator: v1.NodeSelectorOpIn, Values: []string{corev1beta1.ArchitectureAmd64}},
},
},
}
ExpectApplied(ctx, env.Client, nodeClass)
isDrifted, err := cloudProvider.IsDrifted(ctx, nodeClaim)
Expect(err).ToNot(HaveOccurred())
Expect(isDrifted).To(Equal(cloudprovider.AMIDrift))
})
Context("Static Drift Detection", func() {
BeforeEach(func() {
armRequirements := []v1.NodeSelectorRequirement{
{Key: v1.LabelArchStable, Operator: v1.NodeSelectorOpIn, Values: []string{corev1beta1.ArchitectureArm64}},
}
amdRequirements := []v1.NodeSelectorRequirement{
{Key: v1.LabelArchStable, Operator: v1.NodeSelectorOpIn, Values: []string{corev1beta1.ArchitectureAmd64}},
}
nodeClass = &v1beta1.EC2NodeClass{
ObjectMeta: nodeClass.ObjectMeta,
Spec: v1beta1.EC2NodeClassSpec{
Expand Down Expand Up @@ -839,6 +836,7 @@ var _ = Describe("CloudProvider", func() {
},
},
Status: v1beta1.EC2NodeClassStatus{
InstanceProfile: "test-profile",
Subnets: []v1beta1.Subnet{
{
ID: validSubnet1,
Expand All @@ -854,6 +852,16 @@ var _ = Describe("CloudProvider", func() {
ID: validSecurityGroup,
},
},
AMIs: []v1beta1.AMI{
{
ID: armAMIID,
Requirements: armRequirements,
},
{
ID: amdAMIID,
Requirements: amdRequirements,
},
},
},
}
nodeClass.Annotations = lo.Assign(nodeClass.Annotations, map[string]string{v1beta1.AnnotationEC2NodeClassHash: nodeClass.Hash()})
Expand Down Expand Up @@ -1100,6 +1108,7 @@ var _ = Describe("CloudProvider", func() {
},
},
Status: v1beta1.EC2NodeClassStatus{
AMIs: nodeClass.Status.AMIs,
SecurityGroups: []v1beta1.SecurityGroup{
{
ID: "sg-test1",
Expand Down
9 changes: 7 additions & 2 deletions pkg/controllers/nodeclass/status/ami.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,20 @@ import (
"time"

"github.com/samber/lo"
v1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1"
"github.com/aws/karpenter-provider-aws/pkg/providers/amifamily"
corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1"
)

type AMI struct {
amiProvider amifamily.Provider
}

func (a *AMI) Reconcile(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) (reconcile.Result, error) {
amis, err := a.amiProvider.Get(ctx, nodeClass, &amifamily.Options{})
amis, err := a.amiProvider.List(ctx, nodeClass)
engedaam marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return reconcile.Result{}, fmt.Errorf("getting amis, %w", err)
}
Expand All @@ -41,7 +43,10 @@ func (a *AMI) Reconcile(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) (r
return reconcile.Result{}, nil
}
nodeClass.Status.AMIs = lo.Map(amis, func(ami amifamily.AMI, _ int) v1beta1.AMI {
reqs := ami.Requirements.NodeSelectorRequirements()
reqs := lo.Map(ami.Requirements.NodeSelectorRequirements(), func(item corev1beta1.NodeSelectorRequirementWithMinValues, _ int) v1.NodeSelectorRequirement {
return item.NodeSelectorRequirement
})

sort.Slice(reqs, func(i, j int) bool {
if len(reqs[i].Key) != len(reqs[j].Key) {
return len(reqs[i].Key) < len(reqs[j].Key)
Expand Down
Loading
Loading