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

CA 1.12: cherry pick of #1970, #1864, #1740 and #1386 #2231

Merged

Conversation

Jeffwan
Copy link
Contributor

@Jeffwan Jeffwan commented Aug 2, 2019

#1970 Add m5ad, r5ad and t3a instance families
#1864 Upgrade CA version in aws example and fix autodiscover example
#1740 Regenerate the ec2 instance types using latest metadata
#1386 Update RBAC example to include replicasets in the apps apigroup

@k8s-ci-robot 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 2, 2019
@k8s-ci-robot k8s-ci-robot requested review from losipiuk and piosz August 2, 2019 00:46
@Jeffwan Jeffwan changed the title CA 1.13: cherry pick of #1970, #1864, #1740 and #1386 CA 1.12: cherry pick of #1970, #1864, #1740 and #1386 Aug 2, 2019
@MaciekPytel
Copy link
Contributor

Right, so the PR is lgtm. The issue is we weren't planning any more 1.12 releases - Kubernetes project no longer supports 1.12 and officially we're following the same policy. We've always been somewhat relaxed about it (ex. we recently did 1.12.7), but we shouldn't make it a rule to keep supporting versions way past their EOL.

Most of this PR is doc changes anyway and those don't need a release, none of the above applies to them. The one thing that isn't is the instances list for AWS. In this case the effort to release new version is minimal - there is no change in actual code, so no extra testing is really required. And I don't mind cutting one more 1.12 next time we do patch releases. But I'd like to avoid a situation where we keep cutting releases forever just to update instance type lists. It creates an unfortunate precedent - we no longer run tests for 1.12, and I wouldn't be as comfortable releasing a less trivial change.

At the same time I don't want to tell people that they can't use instance type because they run too old Kubernetes version. Is there some way we can stop hard-coding the list of supported instance types? Maybe at least provide a way to support fresh config file?

cc: @jaypipes

@jaypipes
Copy link
Contributor

jaypipes commented Aug 5, 2019

At the same time I don't want to tell people that they can't use instance type because they run too old Kubernetes version. Is there some way we can stop hard-coding the list of supported instance types? Maybe at least provide a way to support fresh config file?

Well, technically that list of instance types is generated :)

https://github.com/kubernetes/autoscaler/blob/100e91ba756e08ad2ad104d9950e7dea655070ab/cluster-autoscaler/cloudprovider/aws/ec2_instance_types/gen.go

For each region, the generator queries the AWS regional pricing API endpoint for instance type information in that region:

url := "https://pricing.us-east-1.amazonaws.com/offers/v1.0/aws/AmazonEC2/current/" + r.ID() + "/index.json"

Unfortunately, the aws cloudprovider implementation in CAS doesn't store a version or timestamp or anything like that for when the instance type information was generated, and the AWS pricing API itself only has a single "version" (2017-10-15).

I suppose long-term, it would be better to simply call the AWS pricing API directly via the aws-sdk-go, instead of generating this information statically. The aws-sdk-go code could be called on init() of the aws cloud provider to generate a cache of instance type information for each region:

https://github.com/aws/aws-sdk-go/blob/9ad3fb92cb115af891132a63dea7f182c32dd278/service/pricing/service.go

@MaciekPytel if you'd like, I can create a separate GH issue to track that work, if such an approach would be better, in your view.

Best,
-jay

Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in the main conversation area. I think eventually, it would be good to move away from the generated instance type information and towards an approach that calls the aws-sdk-go's pricing client (https://github.com/aws/aws-sdk-go/blob/9ad3fb92cb115af891132a63dea7f182c32dd278/service/pricing/service.go) on init() of the aws cloud provider and establishes a cache of instance type information per region that way.

But, it's up to you all whether you want to include this new static instance type information for a backported 1.12 CAS version.

@Jeffwan
Copy link
Contributor Author

Jeffwan commented Aug 5, 2019

Agree. I create an issue to track the issue and let's make list generated in runtime.
#2240

@MaciekPytel
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaciekPytel

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 2bf87cb into kubernetes:cluster-autoscaler-release-1.12 Aug 7, 2019
@Jeffwan Jeffwan deleted the fix-1.12 branch August 7, 2019 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler cherrypick cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants