Implement ICE caching for AWS cloud provider and fix bug in filtering for SubnetProvider #816

eptiger · 2021-11-19T08:11:14Z

1. Issue, if available:
#371

2. Description of changes:
Finally - the exhilirating conclusion to my set of code changes on ice caching! All the changes I've made fit nicely within the AWS cloud provider

I added a cache in the InstanceProvider. I debated a bunch with myself for where this should live and decided that it was cleaner to do that than to have the InstanceProvider pass the ICEs back to the cloudprovider to manage it. I liked the idea that the InstanceProvider would encapsulate the interactions with Fleet.
After a discussion with Ellis regarding my confusion on the constraints being passed into GetInstanceTypes, I removed the erroneous filtering in the SubnetProvider that was constraining AvailabilityZones (the only place the filtering was supposed to really happen was in the InstanceProvider when creating LT overrides for launch, so I moved the filtering into the InstanceProvider exclusively). I added brief comments to help others understand this better.
I hope it's not too controversial that I added AvailabilityZone to our LT overrides. It's redundant but it makes our lives easier for processing ICEs and given that we're not near our Fleet request size limits it feels pretty safe.

I haven't done real cluster testing yet, only unit tests (but I believe Brandon has). Will do a sanity test on a real cluster before pushing. I also didn't add a unit test yet where we fall back to another zone - I may still try to add this when addressing the next round of comments.

3. Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: link to issue
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2021-11-19T08:11:20Z

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 408a81a

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/619f044900afa600076d5236

eptiger · 2021-11-19T08:12:40Z

pkg/cloudprovider/aws/instance.go

+		}
+		if len(cleanedOfferings) > 0 {
+			instanceType.AvailableOfferings = cleanedOfferings
+			cleanedInstanceTypes = append(cleanedInstanceTypes, &instanceTypes[index])


This syntax seemed funky to me but doing I get a linting error if I just append *instanceType - there's certainly some Go idiosyncrasy I don't follow here.

I believe this is the issue I mentioned above where the range variable in a for loop uses one memory address and overrides it for each iteration. So you'd need to set another temp variable in the loop, and take the address of it when appending.

Ok, I think I've fixed this in my new commit - please let me know if there's a cleaner way that I'm spacing on for how to do this.

pkg/cloudprovider/aws/cloudprovider.go

pkg/cloudprovider/aws/errors.go

pkg/cloudprovider/aws/instance.go

bwagner5 · 2021-11-19T17:13:43Z

pkg/cloudprovider/aws/instance.go

+		}
+		if len(cleanedOfferings) > 0 {
+			instanceType.AvailableOfferings = cleanedOfferings
+			cleanedInstanceTypes = append(cleanedInstanceTypes, &instanceTypes[index])


I believe this is the issue I mentioned above where the range variable in a for loop uses one memory address and overrides it for each iteration. So you'd need to set another temp variable in the loop, and take the address of it when appending.

pkg/cloudprovider/aws/instance.go

pkg/cloudprovider/aws/cloudprovider.go

ellistarn · 2021-11-19T18:04:31Z

pkg/cloudprovider/aws/cloudprovider.go

+	if err != nil {
+		return nil, err
+	}
+	return c.instanceProvider.DiscardICEdInstanceTypes(ctx, allInstanceTypes), nil


Given that this is the only place that this is used, does it make sense to hide this behavior inside the instance provider? That is to say, instanceTypeProvider.Get() could inherently factor in unavailable offerings. Is there ever a case where we want to include unavailable offerings on this call?

I considered that, but it felt messy as I would need to get the cached instance types from instance provider and then hand it to instance type provider (like an exclusion list). Unless you had something more clever in mind? I liked the idea that the ICE cache was entirely encapsulated by the instance provider, but if folks feel strongly I'm happy to disagree and commit if you want to share how you think it should look.

In terms of possibly including unavailable offerings - I can't think of why we'd want to do that. If you have something in mind, happy to hear it out. We would have to modify the overall CloudProvider interface if wanted to make ice caching behavior optional, which then leaks that detail into the higher level abstraction, so I'm inclined to say that we probably wouldn't want to do that.

pkg/cloudprovider/aws/instance.go

ellistarn · 2021-11-19T18:08:29Z

pkg/cloudprovider/aws/instance.go

@@ -36,11 +40,30 @@ import (
 	"github.com/awslabs/karpenter/pkg/utils/options"
 )

+const (
+	iceCacheODTTL           = 15 * time.Second


Suggested change

iceCacheODTTL = 15 * time.Second

InsufficientCapacityExceptionCacheOnDemandTTL = 15 * time.Second

I think the spot and OD cache TTLs should be the same 45 seconds.

Re: the value - do you want to chat more about this over Slack? I can disagree and commit on this, but I like the idea that we control them separately and that they are actually different values.

Re: the name - just so I understand the repo style for future changes, can I get clarity on when to use short versus long variable names? I took it that the convention in Go was to use short variable names whenever possible, but I must have misunderstood this convention. Is it because ICE is an EC2 acronym whereas TTL is more widely used?

pkg/cloudprovider/aws/instance.go

pkg/cloudprovider/aws/cloudprovider.go

ellistarn · 2021-11-19T18:13:23Z

pkg/cloudprovider/aws/instance.go

@@ -105,6 +128,37 @@ func (p *InstanceProvider) Terminate(ctx context.Context, node *v1.Node) error {
 	return nil
 }

+// The intention is to remove Offerings from the provided possible InstanceTypes based on recently observed ICEs, which will
+// indirectly impact the packer to keep it from spinning its wheels on packings that are doomed to fail due to EC2 capacity constraints.
+func (p *InstanceProvider) DiscardICEdInstanceTypes(ctx context.Context, instanceTypes []InstanceType) []cloudprovider.InstanceType {


naming suggestion:

Suggested change

func (p *InstanceProvider) DiscardICEdInstanceTypes(ctx context.Context, instanceTypes []InstanceType) []cloudprovider.InstanceType {

func (p *InstanceProvider) WithoutUnavailableOfferings(ctx context.Context, instanceTypes []InstanceType) []InstanceType {

It also might be a bit more idiomatic to do type conversion at the cloud provider interface and just think in terms of aws.InstanceTypes here.

While you're probably right, it means that we'll have to loop through all the InstanceType instances in the cloud provider to convert them, IIUC - right? I didn't do this because it seemed like extra effort I could eliminate by doing the conversion here. If you think being idiomatic is more valuable than the tiny performance saving, I can change it - just LMK.

pkg/cloudprovider/aws/instance.go

ellistarn · 2021-11-19T18:18:27Z

pkg/cloudprovider/aws/instance.go

@@ -304,3 +385,7 @@ func combineReservations(reservations []*ec2.Reservation) []*ec2.Instance {
 	}
 	return instances
 }
+
+func createIceCacheKey(instanceType string, capacityType string) string {


Suggested change

func createIceCacheKey(instanceType string, capacityType string) string {

func iceCacheKey(instanceType string, capacityType string) string {

Went with unavailableOfferingsCacheKey

pkg/cloudprovider/aws/instance.go

ellistarn · 2021-11-19T18:20:08Z

pkg/cloudprovider/aws/instance.go

 type InstanceProvider struct {
 	ec2api                 ec2iface.EC2API
 	instanceTypeProvider   *InstanceTypeProvider
 	subnetProvider         *SubnetProvider
 	launchTemplateProvider *LaunchTemplateProvider
+	iceCache               *cache.Cache


Optional name suggestion

Suggested change

iceCache *cache.Cache

offeringAvailability *cache.Cache

How about unavailableOfferings?

ellistarn · 2021-11-19T18:21:40Z

pkg/cloudprovider/aws/instance.go

@@ -176,7 +237,7 @@ func (p *InstanceProvider) getLaunchTemplateConfigs(ctx context.Context, constra
 	return launchTemplateConfigs, nil
 }

-func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []*ec2.Subnet, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest {
+func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []*ec2.Subnet, constrainedZones sets.String, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest {


Suggested change

func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []*ec2.Subnet, constrainedZones sets.String, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest {

func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []*ec2.Subnet, zones sets.String, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest {

I'm gonna push back on this comment. I appreciate that we're trying to be concise, but there's 3 kinds of zones here:

The zones in the instance type offerings representing what is supported

The zones associated with the constraints that constrain what we can choose

The zone associated with the subnet

I think there's value in naming them separately as there is legitimate ambiguity. Do you feel strongly about this?

There's a simplification below that might help with this, but I'm happy either way.

Compromise: I'm adding a comment because I still think zones is confusing =P You can take your pick between the new comment or me reverting the variable name change, I think we need at least one of the two.

ellistarn · 2021-11-19T18:25:20Z

pkg/cloudprovider/aws/instance.go

@@ -185,10 +246,14 @@ func (p *InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.Inst
 				continue
 			}
 			for _, subnet := range subnets {
-				if aws.StringValue(subnet.AvailabilityZone) == offering.Zone {
+				subnetZone := aws.StringValue(subnet.AvailabilityZone)
+				if subnetZone == offering.Zone && (constrainedZones == nil || constrainedZones.Has(subnetZone)) {


You're guaranteed that constrainedZones won't be nil, since Karpenter has to produce the full set of viable zones for the requirement NotIn operator to work. e.g. us-west-2a not in nil is impossible to compute.

Suggested change

if subnetZone == offering.Zone && (constrainedZones == nil || constrainedZones.Has(subnetZone)) {

if zone := aws.StringValue(subnet.AvailabilityZone); zone == offering.Zone && zones.has(zone) {

If you are saying it's always guaranteed, I'll go ahead and remove the check. I'm hoping that we already have some unit tests that would fail in this case?

pkg/cloudprovider/aws/instance.go

ellistarn · 2021-11-19T18:28:10Z

pkg/cloudprovider/aws/instance.go

+		zones.(sets.String).Insert(zone)
+		p.iceCache.Set(cacheKey, zones, cacheTTL)
+	} else {
+		p.iceCache.Set(cacheKey, sets.String{zone: sets.Empty{}}, cacheTTL)


Don't you need to include the zone in the cache?

Suggested change

p.iceCache.Set(cacheKey, sets.String{zone: sets.Empty{}}, cacheTTL)

p.iceCache.Set(cacheKey, sets.String{zone: sets.NewString(zones)}, cacheTTL)

Hmmm, maybe I'm confused but I thought a sets.String was a hash map of strings to empty values. Looking at the source code, the insert does this:

// Insert adds items to the set. func (s String) Insert(items ...string) String { for _, item := range items { s[item] = Empty{} } return s }

Am I missing something?

ellistarn · 2021-11-19T18:29:23Z

pkg/cloudprovider/aws/instance.go

+	}
+	cacheKey := createIceCacheKey(instanceType, capacityType)
+	if zones, exists := p.iceCache.Get(cacheKey); exists {
+		zones.(sets.String).Insert(zone)


Because sets.String is a pointer (the underlying type is a map, which is a pointer), you should be able to just read and inject the zone and not call cache.Set() again

Thanks for confirming that - this was admittedly me being lazy about not verifying this suspicion I had =P That being said, don't I need to do a Set() to update the TTL?

ellistarn · 2021-11-19T18:31:35Z

pkg/cloudprovider/aws/instance.go

+	cacheKey := createIceCacheKey(instanceType, capacityType)
+	if zones, exists := p.iceCache.Get(cacheKey); exists {
+		zones.(sets.String).Insert(zone)
+		p.iceCache.Set(cacheKey, zones, cacheTTL)


Thinking about this, I think there might be a weird case

zone-a is added to the cache, 5m

4 minutes pass

zone-b is added to the cache, 5m

both zone-a and zone-b are now cached for 5m

1 minute passes

zone-a might be available again, but is cached for 4 more minutes

You could potentially store a timestamp in the cache to keep track of this, but it feels a bit complicated to me. It's probably fine in the grand scheme. Happy to discuss this more.

Yeah I, too, thought of this and decided it was an edge case not worth trying to fix.

The more I thought about it after reading your comment though the more it bothered me. I think I've come up with a solution that isn't perfect but balances ease of use with users not hitting an awful edge case. The fact that I can't make a set of structs in Go makes this very hard to solve otherwise is a nice way.

ellistarn · 2021-11-19T18:32:18Z

pkg/cloudprovider/aws/suite_test.go

@@ -75,6 +76,7 @@ var _ = BeforeSuite(func() {
 					securityGroupProvider: NewSecurityGroupProvider(fakeEC2API),
 					cache:                 launchTemplateCache,
 				},
+				cache.New(5*time.Second, 30*time.Second),


If you export these values you can reference them directly in the tests. Assuming they're not different.

You'll also likely need to use injectable.Time to test this, though.

Will play with this when I add tests. I thought lower values were better for testing, unless you think testing TTL is excessive.

eptiger

About to post the next update, just give me a few minutes to run through the checks.

eptiger · 2021-11-22T18:54:47Z

pkg/cloudprovider/aws/cloudprovider.go

+	if err != nil {
+		return nil, err
+	}
+	return c.instanceProvider.DiscardICEdInstanceTypes(ctx, allInstanceTypes), nil


I considered that, but it felt messy as I would need to get the cached instance types from instance provider and then hand it to instance type provider (like an exclusion list). Unless you had something more clever in mind? I liked the idea that the ICE cache was entirely encapsulated by the instance provider, but if folks feel strongly I'm happy to disagree and commit if you want to share how you think it should look.

In terms of possibly including unavailable offerings - I can't think of why we'd want to do that. If you have something in mind, happy to hear it out. We would have to modify the overall CloudProvider interface if wanted to make ice caching behavior optional, which then leaks that detail into the higher level abstraction, so I'm inclined to say that we probably wouldn't want to do that.

pkg/cloudprovider/aws/errors.go

eptiger · 2021-11-22T18:59:31Z

pkg/cloudprovider/aws/instance.go

@@ -36,11 +40,30 @@ import (
 	"github.com/awslabs/karpenter/pkg/utils/options"
 )

+const (
+	iceCacheODTTL           = 15 * time.Second


Re: the value - do you want to chat more about this over Slack? I can disagree and commit on this, but I like the idea that we control them separately and that they are actually different values.

Re: the name - just so I understand the repo style for future changes, can I get clarity on when to use short versus long variable names? I took it that the convention in Go was to use short variable names whenever possible, but I must have misunderstood this convention. Is it because ICE is an EC2 acronym whereas TTL is more widely used?

eptiger · 2021-11-22T19:06:43Z

pkg/cloudprovider/aws/instance.go

 type InstanceProvider struct {
 	ec2api                 ec2iface.EC2API
 	instanceTypeProvider   *InstanceTypeProvider
 	subnetProvider         *SubnetProvider
 	launchTemplateProvider *LaunchTemplateProvider
+	iceCache               *cache.Cache


How about unavailableOfferings?

eptiger · 2021-11-22T19:15:41Z

pkg/cloudprovider/aws/instance.go

@@ -105,6 +128,37 @@ func (p *InstanceProvider) Terminate(ctx context.Context, node *v1.Node) error {
 	return nil
 }

+// The intention is to remove Offerings from the provided possible InstanceTypes based on recently observed ICEs, which will
+// indirectly impact the packer to keep it from spinning its wheels on packings that are doomed to fail due to EC2 capacity constraints.
+func (p *InstanceProvider) DiscardICEdInstanceTypes(ctx context.Context, instanceTypes []InstanceType) []cloudprovider.InstanceType {


While you're probably right, it means that we'll have to loop through all the InstanceType instances in the cloud provider to convert them, IIUC - right? I didn't do this because it seemed like extra effort I could eliminate by doing the conversion here. If you think being idiomatic is more valuable than the tiny performance saving, I can change it - just LMK.

eptiger · 2021-11-22T19:37:26Z

pkg/cloudprovider/aws/instance.go

@@ -304,3 +385,7 @@ func combineReservations(reservations []*ec2.Reservation) []*ec2.Instance {
 	}
 	return instances
 }
+
+func createIceCacheKey(instanceType string, capacityType string) string {


Went with unavailableOfferingsCacheKey

eptiger · 2021-11-22T20:28:31Z

pkg/cloudprovider/aws/instance.go

+		}
+		if len(cleanedOfferings) > 0 {
+			instanceType.AvailableOfferings = cleanedOfferings
+			cleanedInstanceTypes = append(cleanedInstanceTypes, &instanceTypes[index])


Ok, I think I've fixed this in my new commit - please let me know if there's a cleaner way that I'm spacing on for how to do this.

eptiger · 2021-11-22T20:30:43Z

pkg/cloudprovider/aws/instance.go

+		zones.(sets.String).Insert(zone)
+		p.iceCache.Set(cacheKey, zones, cacheTTL)
+	} else {
+		p.iceCache.Set(cacheKey, sets.String{zone: sets.Empty{}}, cacheTTL)


Hmmm, maybe I'm confused but I thought a sets.String was a hash map of strings to empty values. Looking at the source code, the insert does this:

// Insert adds items to the set. func (s String) Insert(items ...string) String { for _, item := range items { s[item] = Empty{} } return s }

Am I missing something?

eptiger · 2021-11-22T20:31:54Z

pkg/cloudprovider/aws/suite_test.go

@@ -75,6 +76,7 @@ var _ = BeforeSuite(func() {
 					securityGroupProvider: NewSecurityGroupProvider(fakeEC2API),
 					cache:                 launchTemplateCache,
 				},
+				cache.New(5*time.Second, 30*time.Second),


Will play with this when I add tests. I thought lower values were better for testing, unless you think testing TTL is excessive.

eptiger · 2021-11-22T21:42:19Z

pkg/cloudprovider/aws/instance.go

+	cacheKey := createIceCacheKey(instanceType, capacityType)
+	if zones, exists := p.iceCache.Get(cacheKey); exists {
+		zones.(sets.String).Insert(zone)
+		p.iceCache.Set(cacheKey, zones, cacheTTL)


Yeah I, too, thought of this and decided it was an edge case not worth trying to fix.

The more I thought about it after reading your comment though the more it bothered me. I think I've come up with a solution that isn't perfect but balances ease of use with users not hitting an awful edge case. The fact that I can't make a set of structs in Go makes this very hard to solve otherwise is a nice way.

pkg/cloudprovider/aws/cloudprovider.go

pkg/cloudprovider/aws/instance.go

pkg/cloudprovider/aws/fake/ec2api.go

pkg/cloudprovider/aws/instance.go

pkg/cloudprovider/aws/instancetypes.go

… for SubnetProvider

pkg/cloudprovider/aws/instancetypes.go

pkg/cloudprovider/aws/subnets.go

pkg/test/expectations/expectations.go

ellistarn · 2021-11-25T02:00:31Z

pkg/cloudprovider/aws/suite_test.go

 				}
+				Expect(len(nodeNames)).To(Equal(1))


We test binpacking elsewhere, so you could reduce the scope of this test to not test len(nodes).

If you don't feel strongly about it, I'd rather keep these checks. I don't think it hurts to sanity test that the test is working as the author expected.

…move unnecessary expectation method

pkg/cloudprovider/aws/suite_test.go

…ouple of other tests

eptiger · 2021-11-25T05:04:40Z

pkg/cloudprovider/aws/fake/ec2api.go

-	return &ec2.CreateFleetOutput{Instances: []*ec2.CreateFleetInstance{{InstanceIds: instanceIds}}}, nil
+	result := &ec2.CreateFleetOutput{
+		Instances: []*ec2.CreateFleetInstance{{InstanceIds: instanceIds}}}
+	if len(skippedPools) > 0 {


Whoops, meant to remove this. Sorry :)

chore: Update owner alias for ellistarn

eptiger requested a review from ellistarn November 19, 2021 08:11

eptiger commented Nov 19, 2021

View reviewed changes

bwagner5 reviewed Nov 19, 2021

View reviewed changes

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 19, 2021

View reviewed changes

eptiger force-pushed the ice-cache-3 branch from 95fb50e to 4fb6c8c Compare November 23, 2021 01:00

eptiger commented Nov 23, 2021

View reviewed changes

ellistarn reviewed Nov 23, 2021

View reviewed changes

pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 23, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 23, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 24, 2021

View reviewed changes

pkg/cloudprovider/aws/fake/ec2api.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 24, 2021

View reviewed changes

pkg/cloudprovider/aws/fake/ec2api.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 24, 2021

View reviewed changes

pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 24, 2021

View reviewed changes

pkg/cloudprovider/aws/instancetypes.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 24, 2021

View reviewed changes

pkg/cloudprovider/aws/instancetypes.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 24, 2021

View reviewed changes

pkg/cloudprovider/aws/instancetypes.go Show resolved Hide resolved

Elton Pinto added 6 commits November 25, 2021 01:00

Implement ICE caching for AWS cloud provider and fix bug in filtering…

5414947

… for SubnetProvider

Addressed first round of comments - rebase

fa3ebfa

Minor logging update

f13ee96

Move ICE caching logic to InstanceTypeProvider

5a9cb81

Add unit tests, merge spot and OD ICE cache TTLs

cf8dc0e

Address last round of comments, mostly stylistic changes

b056e59

eptiger force-pushed the ice-cache-3 branch from fb1e125 to b056e59 Compare November 25, 2021 01:07

ellistarn reviewed Nov 25, 2021

View reviewed changes

pkg/cloudprovider/aws/instancetypes.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 25, 2021

View reviewed changes

pkg/cloudprovider/aws/instancetypes.go Outdated Show resolved Hide resolved

Elton Pinto added 2 commits November 25, 2021 01:28

Simplify cache expiry test for unavailable offerings caching

0bccea1

Addressed 1 comment

6dc13d1

ellistarn reviewed Nov 25, 2021

View reviewed changes

pkg/cloudprovider/aws/subnets.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 25, 2021

View reviewed changes

pkg/test/expectations/expectations.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 25, 2021

View reviewed changes

Add test for zonal fallback for insufficient capacity cache tests, re…

7e42f05

…move unnecessary expectation method

ellistarn reviewed Nov 25, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

ellistarn reviewed Nov 25, 2021

View reviewed changes

pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved

Elton Pinto added 2 commits November 25, 2021 02:54

Addressed minor comments, fixed test race condition

38e07a2

Fixed insufficient capacity cache zonal fallback test, simplified a c…

408a81a

…ouple of other tests

ellistarn approved these changes Nov 25, 2021

View reviewed changes

ellistarn merged commit 0ad68e8 into aws:main Nov 25, 2021

eptiger deleted the ice-cache-3 branch November 25, 2021 05:02

eptiger commented Nov 25, 2021

View reviewed changes

gfcroft pushed a commit to gfcroft/karpenter-provider-aws that referenced this pull request Nov 25, 2023

Merge pull request aws#816 from jonathan-innis/update-owners-aliases

58ea363

chore: Update owner alias for ellistarn

	iceCacheODTTL = 15 * time.Second
	InsufficientCapacityExceptionCacheOnDemandTTL = 15 * time.Second

	func (p *InstanceProvider) DiscardICEdInstanceTypes(ctx context.Context, instanceTypes []InstanceType) []cloudprovider.InstanceType {
	func (p *InstanceProvider) WithoutUnavailableOfferings(ctx context.Context, instanceTypes []InstanceType) []InstanceType {

	func createIceCacheKey(instanceType string, capacityType string) string {
	func iceCacheKey(instanceType string, capacityType string) string {

	func (p InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []ec2.Subnet, constrainedZones sets.String, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest {
	func (p InstanceProvider) getOverrides(instanceTypeOptions []cloudprovider.InstanceType, subnets []ec2.Subnet, zones sets.String, capacityType string) []*ec2.FleetLaunchTemplateOverridesRequest {

	if subnetZone == offering.Zone && (constrainedZones == nil \|\| constrainedZones.Has(subnetZone)) {
	if zone := aws.StringValue(subnet.AvailabilityZone); zone == offering.Zone && zones.has(zone) {

	p.iceCache.Set(cacheKey, sets.String{zone: sets.Empty{}}, cacheTTL)
	p.iceCache.Set(cacheKey, sets.String{zone: sets.NewString(zones)}, cacheTTL)

Implement ICE caching for AWS cloud provider and fix bug in filtering for SubnetProvider #816

Implement ICE caching for AWS cloud provider and fix bug in filtering for SubnetProvider #816

Conversation

eptiger commented Nov 19, 2021 • edited Loading

netlify bot commented Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eptiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eptiger commented Nov 19, 2021 •

edited

Loading

netlify bot commented Nov 19, 2021 •

edited

Loading

ellistarn Nov 19, 2021 •

edited

Loading

ellistarn Nov 19, 2021 •

edited

Loading

ellistarn Nov 19, 2021 •

edited

Loading

ellistarn Nov 19, 2021 •

edited

Loading