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.

Sign up for GitHub

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

Jump to bottom

Fix pricing endpoint in AWS China Region #3528

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Fix pricing endpoint in AWS China Region #3528

merged 1 commit into from
Sep 23, 2020

Conversation

qqshfox
Copy link
Contributor

@qqshfox qqshfox commented Sep 18, 2020

Fixes #3076, #3276

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2020
@qqshfox qqshfox force-pushed the patch-2 branch 2 times, most recently from 30dfb9c to 7c44ff3 Compare September 18, 2020 06:50
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 18, 2020
jordan-huangwei
jordan-huangwei approved these changes Sep 18, 2020
View reviewed changes
Copy link

@jordan-huangwei jordan-huangwei left a comment

Choose a reason for hiding this comment

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

very good

@qqshfox qqshfox changed the title WIP: Fix pricing endpoint in AWS China Region Fix pricing endpoint in AWS China Region Sep 18, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2020
@Jeffwan
Copy link
Contributor

Jeffwan commented Sep 23, 2020

Add pricing list api here.
https://docs.amazonaws.cn/en_us/aws/latest/userguide/billing-and-payment.html#price-list

Thanks for the contribution


    "AmazonEC2" : {
      "offerCode" : "AmazonEC2",
      "versionIndexUrl" : "/offers/v1.0/cn/AmazonEC2/index.json",
      "currentVersionUrl" : "/offers/v1.0/cn/AmazonEC2/current/index.json",
      "currentRegionIndexUrl" : "/offers/v1.0/cn/AmazonEC2/current/region_index.json"
    },

@Jeffwan
Copy link
Contributor

Jeffwan commented Sep 23, 2020

/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 Sep 23, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan

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 Sep 23, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7cc6e3c into kubernetes:master Sep 23, 2020
Jeffwan
Jeffwan approved these changes Sep 23, 2020
View reviewed changes
cluster-autoscaler/cloudprovider/aws/aws_util.go
staticListLastUpdateTime = "2019-10-14"
ec2MetaDataServiceUrl = "http://169.254.169.254/latest/dynamic/instance-identity/document"
ec2PricingServiceUrlTemplate = "https://pricing.us-east-1.amazonaws.com/offers/v1.0/aws/AmazonEC2/current/%s/index.json"
ec2PricingServiceUrlTemplateCN = "https://pricing.cn-north-1.amazonaws.com.cn/offers/v1.0/cn/AmazonEC2/current/%s/index.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: There're multiple partitions like AWS, AWS-cn, AWS-gov. It would be great to have all of them

cluster-autoscaler/cloudprovider/aws/aws_util.go
@@ -53,6 +54,13 @@ type productAttributes struct {

// GenerateEC2InstanceTypes returns a map of ec2 resources
func GenerateEC2InstanceTypes(region string) (map[string]*InstanceType, error) {
var pricingUrlTemplate string
if strings.HasPrefix(region, "cn-") {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I am ok with this now. It would be better to use AWS SDK to check partition

This was referenced Sep 23, 2020
Closed
Closed
@zxh326
Copy link

zxh326 commented Nov 16, 2020 •
edited
Loading

Can we cherry-pick this change to v1.17.x v1.18.x version?

@qqshfox
@Jeffwan

This was referenced Nov 16, 2020
Merged
Merged
Merged
@Jeffwan
Copy link
Contributor

Jeffwan commented Nov 16, 2020 •
edited
Loading

@zxh326 Why not to use cherry-picks scripts?

@zxh326
Copy link

zxh326 commented Nov 17, 2020

@zxh326 Why not to use cherry-picks scripts?

I don't know how to use and didn't find this script

@yongzhang
Copy link

is it available in 1.18? I still have this issue in 1.18:

W1211 05:13:25.624702       1 aws_util.go:71] Error fetching https://pricing.us-east-1.amazonaws.com/offers/v1.0/aws/AmazonEC2/current/cn-northwest-1/index.json skipping...
F1211 05:13:25.624754       1 aws_cloud_provider.go:358] Failed to generate AWS EC2 Instance Types: unable to load EC2 Instance Type list

@qqshfox
Copy link
Contributor Author

qqshfox commented Dec 11, 2020

@yongzhang Looks like the backport to 1.18 hasn't been released yet. cluster-autoscaler-1.18.3...cluster-autoscaler-release-1.18

@yongzhang
Copy link

@yongzhang Looks like the backport to 1.18 hasn't been released yet. cluster-autoscaler-1.18.3...cluster-autoscaler-release-1.18

Fine, so looks like I have to build the binary from branch cluster-autoscaler-release-1.18 for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jeffwan Jeffwan Jeffwan approved these changes

@jordan-huangwei jordan-huangwei jordan-huangwei approved these changes

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@Jeffwan Jeffwan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cluster Autoscaler 1.15.6 AWS has problem with start in region cn-northwest-1
6 participants
@qqshfox @Jeffwan @k8s-ci-robot @zxh326 @yongzhang @jordan-huangwei