Generate EC2 Instance Type list in runtime #2249

Jeffwan · 2019-08-12T07:51:10Z

We have some discussion here. #2231

Consider the case not all k8s clusters has network to aws pricing API, I still keep the static instance type list. If network is not accessible, it will fall back to static dict.

dynamic loading will reuse code snippet from gen.go

I am trying reach out to folks who worked on this, not sure if we have any internal IP like EC2 metadata service. If so, we can use that and assume it can always get updated list in the same network.

@jaypipes

mwielgus

Tests are failing

FAIL	k8s.io/autoscaler/cluster-autoscaler/cloudprovider/aws	0.114s

Jeffwan · 2019-08-14T09:20:48Z

Tests are failing

FAIL	k8s.io/autoscaler/cluster-autoscaler/cloudprovider/aws	0.114s

I will update the test case.

Jeffwan · 2019-08-19T17:20:19Z

@mwielgus Tests have been fixed. Please have a review.

Jeffwan · 2019-08-19T17:20:41Z

/cc @MaciekPytel

jaypipes

👍

cluster-autoscaler/cloudprovider/aws/aws_util_test.go

jaypipes

still LGTM.

cluster-autoscaler/cloudprovider/aws/aws_util.go

cluster-autoscaler/cloudprovider/aws/aws_util_test.go

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

stefansedich · 2019-10-01T15:50:28Z

@Jeffwan any idea when this one might get in? We are aiming to move to g4dn instances and would be great to get either this or #2401 merged!

Jeffwan · 2019-10-02T01:14:19Z

Sorry for late response. I will clean it up and submit revision

Jeffwan · 2019-10-09T23:11:03Z

/hold

jaypipes

++ lgtm. Would still prefer a const for the pricing URL, but that's something to do in a follow up.

Jeffwan · 2019-10-10T23:47:45Z

++ lgtm. Would still prefer a const for the pricing URL, but that's something to do in a follow up.

Move to const in latest change.

Jeffwan · 2019-10-10T23:50:52Z

/hold cancel

In the latest change, we expose option to users whose network is restricted to call EC2 pricing service.

jaypipes

Yup, 👍

cluster-autoscaler/cloudprovider/aws/aws_util.go

losipiuk

Thanks. A few cleanup comments. Looks good otherwise.

losipiuk · 2019-10-14T08:04:03Z

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

+	var instanceTypes map[string]*InstanceType
+	if opts.StaticInstanceList {
+		klog.Warning("Use static EC2 Instance Types, list could be outdated")
+		instanceTypes = GetStaticEC2InstanceTypes()


Possibly this method can also return the date when the list was last updated. Then you could add it to warning message to give clear singal how up-to-date data is.

This is great suggestion.
I create a const and add constructions in the doc that everytime the list is updated, people need to update date correspondingly.

Updated logs:

W1014 17:25:22.612552 75153 aws_cloud_provider.go:354] Use static EC2 Instance Types and list could be outdated. Last update time: 2019-10-14 I1014 17:25:22.612701 75153 reflector.go:120] Starting reflector *v1.PersistentVolumeClaim (0s) from k8s.io/client-go/informers/factory.go:134

cluster-autoscaler/cloudprovider/aws/aws_util_test.go

losipiuk · 2019-10-14T08:13:31Z

cluster-autoscaler/cloudprovider/aws/aws_util_test.go

+
+func TestGetCurrentAwsRegionWithRegionEnv(t *testing.T) {
+	region := "us-west-2"
+	os.Setenv("AWS_REGION", region)


To be more correct please change to:

if oldRegion, found := os.LookupEnv("AWS_REGION"); found { defer os.Setenv("AWS_REGION", oldRegion) } else { defer os.Unsetenv("AWS_REGION") } os.Setenv("AWS_REGION", region)

Possibly not needed if we have just above two places. Yet you may consider writing a short wrapper with signature:

func withEnv(variable string, variableValue string, f func())

Great! This is precise. I make the changes. I didn't see any other place test env here. I will do refactor if I see more tests with similar setups.

cluster-autoscaler/FAQ.md

losipiuk · 2019-10-15T07:52:11Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2019-10-15T07:52:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: losipiuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster-autoscaler/OWNERS~~ [losipiuk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cablespaghetti · 2019-12-20T13:08:05Z

Any chance of this making it over to the 1.14-1.16 branches? I've got a couple of clusters I want to build on t5a instances which aren't in the static list.

jaypipes · 2019-12-20T14:48:46Z

@Jeffwan can you look into porting to the older branches please?

Jeffwan · 2019-12-26T21:57:09Z

@jaypipes I mark this as a backport candidate in next release.

Cherry pick #2249 #2737 #2929 #2931 to 1.15

Cherry pick #2249 #2737 #2931 #2929 to 1.16

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 12, 2019

k8s-ci-robot requested review from aleksandra-malinowska and piosz August 12, 2019 07:51

mwielgus reviewed Aug 14, 2019

View reviewed changes

Jeffwan force-pushed the dynamic_ec2_list branch from bf7debb to 42c0797 Compare August 15, 2019 21:37

k8s-ci-robot requested a review from MaciekPytel August 19, 2019 17:20

jaypipes approved these changes Aug 20, 2019

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_util_test.go Outdated Show resolved Hide resolved

Jeffwan force-pushed the dynamic_ec2_list branch 2 times, most recently from c28fd36 to eedd1db Compare August 21, 2019 14:06

jaypipes approved these changes Aug 21, 2019

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_util.go Outdated Show resolved Hide resolved

cluster-autoscaler/cloudprovider/aws/aws_util_test.go Show resolved Hide resolved

losipiuk reviewed Aug 26, 2019

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go Show resolved Hide resolved

seh mentioned this pull request Sep 24, 2019

Cluster Autoscaler's AWS EC2 instance type catalog needs to be updated #2376

Closed

Jeffwan mentioned this pull request Oct 2, 2019

Add g4dn instance type for AWS #2401

Merged

Jeffwan force-pushed the dynamic_ec2_list branch 2 times, most recently from d0bbe7e to 27cf672 Compare October 9, 2019 22:01

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2019

Jeffwan force-pushed the dynamic_ec2_list branch from 27cf672 to 9179390 Compare October 10, 2019 00:28

jaypipes approved these changes Oct 10, 2019

View reviewed changes

Jeffwan force-pushed the dynamic_ec2_list branch from 9179390 to 87a7c28 Compare October 10, 2019 23:02

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2019

Jeffwan force-pushed the dynamic_ec2_list branch from 87a7c28 to 6389b11 Compare October 10, 2019 23:04

Jeffwan force-pushed the dynamic_ec2_list branch from 6389b11 to f52678e Compare October 10, 2019 23:48

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2019

jaypipes approved these changes Oct 10, 2019

View reviewed changes

seh reviewed Oct 11, 2019

View reviewed changes

cluster-autoscaler/cloudprovider/aws/aws_util.go Show resolved Hide resolved

Jeffwan requested review from losipiuk and mwielgus October 11, 2019 19:02

losipiuk reviewed Oct 14, 2019

View reviewed changes

Load AWS EC2 Instance Types dynamically

7eb8645

Jeffwan force-pushed the dynamic_ec2_list branch from f52678e to 7eb8645 Compare October 15, 2019 00:26

k8s-ci-robot assigned losipiuk Oct 15, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019

k8s-ci-robot merged commit 31b5118 into kubernetes:master Oct 15, 2019

Jeffwan deleted the dynamic_ec2_list branch December 26, 2019 21:57

This was referenced Mar 27, 2020

Cluster Autoscaler releases 1.18.1, 1.17.2, 1.16.5, 1.15.6, 1.14.8 #2988

Closed

Cherry pick #2249 #2737 #2931 #2929 to 1.16 #2993

Merged

Cherry pick #2249 #2737 #2929 #2931 to 1.15 #2994

Merged

k8s-ci-robot added a commit that referenced this pull request Mar 30, 2020

Merge pull request #2994 from Jeffwan/cherry-pick-1.15

aae9171

Cherry pick #2249 #2737 #2929 #2931 to 1.15

k8s-ci-robot added a commit that referenced this pull request Mar 30, 2020

Merge pull request #2993 from Jeffwan/cherry-pick-1.16

277f19e

Cherry pick #2249 #2737 #2931 #2929 to 1.16

gjtempleton mentioned this pull request May 4, 2020

[Bug] - CA AWS Dynamic InstanceTypes Generation Ignored #3109

Closed

timothyb89 mentioned this pull request Sep 11, 2020

Cluster Autoscaler on AWS is OOM killed on startup in GenerateEC2InstanceTypes #3506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate EC2 Instance Type list in runtime #2249

Generate EC2 Instance Type list in runtime #2249

Jeffwan commented Aug 12, 2019 •

edited

Loading

mwielgus left a comment •

edited

Loading

Jeffwan commented Aug 14, 2019

Jeffwan commented Aug 19, 2019

Jeffwan commented Aug 19, 2019

jaypipes left a comment

jaypipes left a comment

stefansedich commented Oct 1, 2019

Jeffwan commented Oct 2, 2019

Jeffwan commented Oct 9, 2019

jaypipes left a comment

Jeffwan commented Oct 10, 2019

Jeffwan commented Oct 10, 2019

jaypipes left a comment

losipiuk left a comment

losipiuk Oct 14, 2019

Jeffwan Oct 15, 2019

Jeffwan Oct 15, 2019 •

edited

Loading

losipiuk Oct 14, 2019

Jeffwan Oct 14, 2019

losipiuk commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

cablespaghetti commented Dec 20, 2019

jaypipes commented Dec 20, 2019

Jeffwan commented Dec 26, 2019

Generate EC2 Instance Type list in runtime #2249

Generate EC2 Instance Type list in runtime #2249

Conversation

Jeffwan commented Aug 12, 2019 • edited Loading

mwielgus left a comment • edited Loading

Choose a reason for hiding this comment

Jeffwan commented Aug 14, 2019

Jeffwan commented Aug 19, 2019

Jeffwan commented Aug 19, 2019

jaypipes left a comment

Choose a reason for hiding this comment

jaypipes left a comment

Choose a reason for hiding this comment

stefansedich commented Oct 1, 2019

Jeffwan commented Oct 2, 2019

Jeffwan commented Oct 9, 2019

jaypipes left a comment

Choose a reason for hiding this comment

Jeffwan commented Oct 10, 2019

Jeffwan commented Oct 10, 2019

jaypipes left a comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

losipiuk Oct 14, 2019

Choose a reason for hiding this comment

Jeffwan Oct 15, 2019

Choose a reason for hiding this comment

Jeffwan Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

losipiuk Oct 14, 2019

Choose a reason for hiding this comment

Jeffwan Oct 14, 2019

Choose a reason for hiding this comment

losipiuk commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

cablespaghetti commented Dec 20, 2019

jaypipes commented Dec 20, 2019

Jeffwan commented Dec 26, 2019

Jeffwan commented Aug 12, 2019 •

edited

Loading

mwielgus left a comment •

edited

Loading

Jeffwan Oct 15, 2019 •

edited

Loading