diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 525430a8bd8c..05898b4f1b68 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -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, diff --git a/pkg/providers/amifamily/ami.go b/pkg/providers/amifamily/ami.go index 7fae92e8b086..fd09b3684039 100644 --- a/pkg/providers/amifamily/ami.go +++ b/pkg/providers/amifamily/ami.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "sync" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -26,6 +25,7 @@ import ( "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" @@ -44,6 +44,8 @@ type Provider interface { type DefaultProvider struct { sync.Mutex + + clk clock.Clock cache *cache.Cache ec2api ec2iface.EC2API cm *pretty.ChangeMonitor @@ -51,8 +53,9 @@ type DefaultProvider struct { 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(), @@ -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 @@ -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 +} diff --git a/pkg/providers/amifamily/suite_test.go b/pkg/providers/amifamily/suite_test.go index 8e9110322e0d..1fa7a270af07 100644 --- a/pkg/providers/amifamily/suite_test.go +++ b/pkg/providers/amifamily/suite_test.go @@ -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 @@ -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(), + }, + }, + )) + }) }) }) diff --git a/pkg/providers/amifamily/types.go b/pkg/providers/amifamily/types.go index 5dae3150a1cc..bf0ccfc5c602 100644 --- a/pkg/providers/amifamily/types.go +++ b/pkg/providers/amifamily/types.go @@ -16,6 +16,7 @@ package amifamily import ( "fmt" + "math" "sort" "time" @@ -39,6 +40,7 @@ type AMI struct { Name string AmiID string CreationDate string + Deprecated bool Requirements scheduling.Requirements } @@ -48,8 +50,9 @@ type AMIs []AMI // If creation date is nil or two AMIs have the same creation date, the AMIs will be sorted by ID, which is guaranteed to be unique, in ascending order. func (a AMIs) Sort() { sort.Slice(a, func(i, j int) bool { - itime, _ := time.Parse(time.RFC3339, a[i].CreationDate) - jtime, _ := time.Parse(time.RFC3339, a[j].CreationDate) + itime := parseTimeWithDefault(a[i].CreationDate, minTime) + jtime := parseTimeWithDefault(a[j].CreationDate, minTime) + if itime.Unix() != jtime.Unix() { return itime.Unix() > jtime.Unix() } @@ -57,12 +60,21 @@ func (a AMIs) Sort() { }) } +func parseTimeWithDefault(dateStr string, defaultTime time.Time) time.Time { + if dateStr == "" { + return defaultTime + } + return lo.Must(time.Parse(time.RFC3339, dateStr)) +} + type Variant string var ( - VariantStandard Variant = "standard" - VariantNvidia Variant = "nvidia" - VariantNeuron Variant = "neuron" + VariantStandard Variant = "standard" + VariantNvidia Variant = "nvidia" + VariantNeuron Variant = "neuron" + maxTime time.Time = time.Unix(math.MaxInt64, 0) + minTime time.Time = time.Unix(math.MinInt64, 0) ) func NewVariant(v string) (Variant, error) { @@ -102,9 +114,10 @@ type DescribeImageQuery struct { func (q DescribeImageQuery) DescribeImagesInput() *ec2.DescribeImagesInput { return &ec2.DescribeImagesInput{ // Don't include filters in the Describe Images call as EC2 API doesn't allow empty filters. - Filters: lo.Ternary(len(q.Filters) > 0, q.Filters, nil), - Owners: lo.Ternary(len(q.Owners) > 0, lo.ToSlicePtr(q.Owners), nil), - MaxResults: aws.Int64(1000), + Filters: lo.Ternary(len(q.Filters) > 0, q.Filters, nil), + Owners: lo.Ternary(len(q.Owners) > 0, lo.ToSlicePtr(q.Owners), nil), + IncludeDeprecated: aws.Bool(true), + MaxResults: aws.Int64(1000), } } diff --git a/pkg/test/environment.go b/pkg/test/environment.go index 3df8b1891979..b7d90f44dbea 100644 --- a/pkg/test/environment.go +++ b/pkg/test/environment.go @@ -17,10 +17,12 @@ package test import ( "context" "net" + "time" "github.com/patrickmn/go-cache" "github.com/samber/lo" corev1 "k8s.io/api/core/v1" + clock "k8s.io/utils/clock/testing" karpv1 "sigs.k8s.io/karpenter/pkg/apis/v1" @@ -47,6 +49,9 @@ func init() { } type Environment struct { + // Mock + Clock *clock.FakeClock + // API EC2API *fake.EC2API EKSAPI *fake.EKSAPI @@ -81,6 +86,9 @@ type Environment struct { } func NewEnvironment(ctx context.Context, env *coretest.Environment) *Environment { + // Mock + clock := &clock.FakeClock{} + // API ec2api := fake.NewEC2API() eksapi := fake.NewEKSAPI() @@ -108,7 +116,7 @@ func NewEnvironment(ctx context.Context, env *coretest.Environment) *Environment versionProvider := version.NewDefaultProvider(env.KubernetesInterface, kubernetesVersionCache) instanceProfileProvider := instanceprofile.NewDefaultProvider(fake.DefaultRegion, iamapi, instanceProfileCache) ssmProvider := ssmp.NewDefaultProvider(ssmapi, ssmCache) - amiProvider := amifamily.NewDefaultProvider(versionProvider, ssmProvider, ec2api, ec2Cache) + amiProvider := amifamily.NewDefaultProvider(clock, versionProvider, ssmProvider, ec2api, ec2Cache) amiResolver := amifamily.NewDefaultResolver() instanceTypesProvider := instancetype.NewDefaultProvider(fake.DefaultRegion, instanceTypeCache, ec2api, subnetProvider, unavailableOfferingsCache, pricingProvider) launchTemplateProvider := @@ -136,6 +144,8 @@ func NewEnvironment(ctx context.Context, env *coretest.Environment) *Environment ) return &Environment{ + Clock: clock, + EC2API: ec2api, EKSAPI: eksapi, SSMAPI: ssmapi, @@ -167,6 +177,7 @@ func NewEnvironment(ctx context.Context, env *coretest.Environment) *Environment } func (env *Environment) Reset() { + env.Clock.SetTime(time.Time{}) env.EC2API.Reset() env.EKSAPI.Reset() env.SSMAPI.Reset()