Skip to content

Commit

Permalink
feat: deprecated AMIs should be discoverable when specifying ami id o… (
Browse files Browse the repository at this point in the history 
#6500)

Co-authored-by: skagalwala <[email protected]>
  • Loading branch information
@shabbskagalwala
shabbskagalwala and skagalwala authored Sep 25, 2024
1 parent dde5589 commit bab6f25
Show file tree
Hide file tree
Showing 5 changed files with 321 additions and 24 deletions.
2 changes: 1 addition & 1 deletion pkg/operator/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont
)
versionProvider := version.NewDefaultProvider(operator.KubernetesInterface, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
ssmProvider := ssmp.NewDefaultProvider(ssm.New(sess), cache.New(awscache.SSMGetParametersByPathTTL, awscache.DefaultCleanupInterval))
amiProvider := amifamily.NewDefaultProvider(versionProvider, ssmProvider, ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
amiProvider := amifamily.NewDefaultProvider(operator.Clock, versionProvider, ssmProvider, ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval))
amiResolver := amifamily.NewDefaultResolver()
launchTemplateProvider := launchtemplate.NewDefaultProvider(
ctx,
Expand Down
59 changes: 45 additions & 14 deletions pkg/providers/amifamily/ami.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ import (
"context"
"fmt"
"sync"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
"github.com/mitchellh/hashstructure/v2"
"github.com/patrickmn/go-cache"
"github.com/samber/lo"
"k8s.io/utils/clock"
"sigs.k8s.io/controller-runtime/pkg/log"

v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
Expand All @@ -44,15 +44,18 @@ type Provider interface {

type DefaultProvider struct {
sync.Mutex

clk clock.Clock
cache *cache.Cache
ec2api ec2iface.EC2API
cm *pretty.ChangeMonitor
versionProvider version.Provider
ssmProvider ssm.Provider
}

func NewDefaultProvider(versionProvider version.Provider, ssmProvider ssm.Provider, ec2api ec2iface.EC2API, cache *cache.Cache) *DefaultProvider {
func NewDefaultProvider(clk clock.Clock, versionProvider version.Provider, ssmProvider ssm.Provider, ec2api ec2iface.EC2API, cache *cache.Cache) *DefaultProvider {
return &DefaultProvider{
clk: clk,
cache: cache,
ec2api: ec2api,
cm: pretty.NewChangeMonitor(),
Expand Down Expand Up @@ -166,24 +169,25 @@ func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery
// Each image may have multiple associated sets of requirements. For example, an image may be compatible with Neuron instances
// and GPU instances. In that case, we'll have a set of requirements for each, and will create one "image" for each.
for _, reqs := range query.RequirementsForImageWithArchitecture(lo.FromPtr(image.ImageId), arch) {
// If we already have an image with the same set of requirements, but this image is newer, replace the previous image.
// Checks and store for AMIs
// Following checks are needed in order to always priortize non deprecated AMIs
// If we already have an image with the same set of requirements, but this image (candidate) is newer, replace the previous (existing) image.
// If we already have an image with the same set of requirements which is deprecated, but this image (candidate) is newer or non deprecated, replace the previous (existing) image
reqsHash := lo.Must(hashstructure.Hash(reqs.NodeSelectorRequirements(), hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}))
if v, ok := images[reqsHash]; ok {
candidateCreationTime, _ := time.Parse(time.RFC3339, lo.FromPtr(image.CreationDate))
existingCreationTime, _ := time.Parse(time.RFC3339, v.CreationDate)
if existingCreationTime == candidateCreationTime && lo.FromPtr(image.Name) < v.Name {
continue
}
if candidateCreationTime.Unix() < existingCreationTime.Unix() {
continue
}
}
images[reqsHash] = AMI{
candidateDeprecated := parseTimeWithDefault(lo.FromPtr(image.DeprecationTime), maxTime).Unix() <= p.clk.Now().Unix()
ami := AMI{
Name: lo.FromPtr(image.Name),
AmiID: lo.FromPtr(image.ImageId),
CreationDate: lo.FromPtr(image.CreationDate),
Deprecated: candidateDeprecated,
Requirements: reqs,
}
if v, ok := images[reqsHash]; ok {
if cmpResult := compareAMI(v, ami); cmpResult <= 0 {
continue
}
}
images[reqsHash] = ami
}
}
return true
Expand Down Expand Up @@ -211,3 +215,30 @@ func MapToInstanceTypes(instanceTypes []*cloudprovider.InstanceType, amis []v1.A
}
return amiIDs
}

// Compare two AMI's based on their deprecation status, creation time or name
// If both AMIs are deprecated, compare creation time and return the one with the newer creation time
// If both AMIs are non-deprecated, compare creation time and return the one with the newer creation time
// If one AMI is deprecated, return the non deprecated one
// The result will be
// 0 if AMI i == AMI j, where creation date, deprecation status and name are all equal
// -1 if AMI i < AMI j, if AMI i is non-deprecated or newer than AMI j
// +1 if AMI i > AMI j, if AMI j is non-deprecated or newer than AMI i
func compareAMI(i, j AMI) int {
iCreationDate := parseTimeWithDefault(i.CreationDate, minTime)
jCreationDate := parseTimeWithDefault(j.CreationDate, minTime)
// Prioritize non-deprecated AMIs over deprecated ones
if i.Deprecated != j.Deprecated {
return lo.Ternary(i.Deprecated, 1, -1)
}
// If both are either non-deprecated or deprecated, compare by creation date
if iCreationDate.Unix() != jCreationDate.Unix() {
return lo.Ternary(iCreationDate.Unix() > jCreationDate.Unix(), -1, 1)
}
// If they have the same creation date, use the name as a tie-breaker
if i.Name != j.Name {
return lo.Ternary(i.Name > j.Name, -1, 1)
}
// If all attributes are are equal, both AMIs are exactly identical
return 0
}
242 changes: 242 additions & 0 deletions pkg/providers/amifamily/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,182 @@ var _ = Describe("AMIProvider", func() {
}))
})
})
Context("AMI List requirements", func() {
BeforeEach(func() {
// Set time using the injectable/fake clock to now
awsEnv.Clock.SetTime(time.Now())
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
{
Tags: map[string]string{"*": "*"},
},
}
})
It("should priortize the older non-deprecated ami without deprecation time", func() {
// Here we have two AMIs one which is deprecated and newer and one which is older and non-deprecated without a deprecation time
// List operation will priortize the non-deprecated AMI without the deprecation time
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
Images: []*ec2.Image{
{
Name: aws.String(amd64AMI),
ImageId: aws.String("ami-5678"),
CreationDate: aws.String("2021-08-31T00:12:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-1 * time.Hour).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
{
Name: aws.String(amd64AMI),
ImageId: aws.String("ami-1234"),
CreationDate: aws.String("2020-08-31T00:08:42.000Z"),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
},
})
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(1))
Expect(amis).To(ConsistOf(amifamily.AMI{
Name: amd64AMI,
AmiID: "ami-1234",
CreationDate: "2020-08-31T00:08:42.000Z",
Requirements: scheduling.NewRequirements(
scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
),
}))
})
It("should priortize the non-deprecated ami with deprecation time when both have same creation time", func() {
// Here we have two AMIs one which is deprecated and one which is non-deprecated both with the same creation time
// List operation will priortize the non-deprecated AMI
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
Images: []*ec2.Image{
{
Name: aws.String(amd64AMI),
ImageId: aws.String("ami-5678"),
CreationDate: aws.String("2021-08-31T00:12:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-10 * time.Minute).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
{
Name: aws.String(amd64AMI),
ImageId: aws.String("ami-1234"),
CreationDate: aws.String("2021-08-31T00:12:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(10 * time.Minute).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
},
})
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(1))
Expect(amis).To(ConsistOf(amifamily.AMI{
Name: amd64AMI,
AmiID: "ami-1234",
CreationDate: "2021-08-31T00:12:42.000Z",
Deprecated: false,
Requirements: scheduling.NewRequirements(
scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
),
}))
})
It("should priortize the non-deprecated ami with deprecation time when both have same creation time and different name", func() {
// Here we have two AMIs one which is deprecated and one which is non-deprecated both with the same creation time but with different names
// List operation will priortize the non-deprecated AMI
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
Images: []*ec2.Image{
{
Name: aws.String("test-ami-2"),
ImageId: aws.String("ami-5678"),
CreationDate: aws.String("2021-08-31T00:12:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-10 * time.Minute).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String("test-ami-2")},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
{
Name: aws.String("test-ami-1"),
ImageId: aws.String("ami-1234"),
CreationDate: aws.String("2021-08-31T00:12:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(10 * time.Minute).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String("test-ami-1")},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
},
})
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(1))
Expect(amis).To(ConsistOf(amifamily.AMI{
Name: "test-ami-1",
AmiID: "ami-1234",
CreationDate: "2021-08-31T00:12:42.000Z",
Deprecated: false,
Requirements: scheduling.NewRequirements(
scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
),
}))
})
It("should priortize the newer ami if both are deprecated", func() {
//Both amis are deprecated and have the same deprecation time
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{
Images: []*ec2.Image{
{
Name: aws.String(amd64AMI),
ImageId: aws.String("ami-5678"),
CreationDate: aws.String("2021-08-31T00:12:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-1 * time.Hour).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
{
Name: aws.String(amd64AMI),
ImageId: aws.String("ami-1234"),
CreationDate: aws.String("2020-08-31T00:08:42.000Z"),
DeprecationTime: aws.String(awsEnv.Clock.Now().Add(-1 * time.Hour).Format(time.RFC3339)),
Architecture: aws.String("x86_64"),
Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String(amd64AMI)},
{Key: aws.String("foo"), Value: aws.String("bar")},
},
},
},
})
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(1))
Expect(amis).To(ConsistOf(amifamily.AMI{
Name: amd64AMI,
AmiID: "ami-5678",
CreationDate: "2021-08-31T00:12:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(
scheduling.NewRequirement(corev1.LabelArchStable, corev1.NodeSelectorOpIn, karpv1.ArchitectureAmd64),
),
}))
})
})
Context("AMI Selectors", func() {
// When you tag public or shared resources, the tags you assign are available only to your AWS account; no other AWS account will have access to those tags
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions
Expand Down Expand Up @@ -553,6 +729,72 @@ var _ = Describe("AMIProvider", func() {
},
))
})
It("should sort deprecated amis with the same name and deprecation time consistently", func() {
amis := amifamily.AMIs{
{
Name: "test-ami-1",
AmiID: "test-ami-4-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
{
Name: "test-ami-1",
AmiID: "test-ami-3-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
{
Name: "test-ami-1",
AmiID: "test-ami-2-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
{
Name: "test-ami-1",
AmiID: "test-ami-1-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
}

amis.Sort()
Expect(amis).To(Equal(
amifamily.AMIs{
{
Name: "test-ami-1",
AmiID: "test-ami-1-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
{
Name: "test-ami-1",
AmiID: "test-ami-2-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
{
Name: "test-ami-1",
AmiID: "test-ami-3-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
{
Name: "test-ami-1",
AmiID: "test-ami-4-id",
CreationDate: "2021-08-31T00:10:42.000Z",
Deprecated: true,
Requirements: scheduling.NewRequirements(),
},
},
))
})
})
})

Expand Down
Loading

0 comments on commit bab6f25

Please sign in to comment.