Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented May 16, 2019

The Hong Kong region is new 2019-04-25 and does not support the older m4 types. This pull-request adds it (commented out in validation, because we don't push RHCOS AMIs there, see also #1528). It also expands the seeded regions in TestGetDefaultInstanceClass to gets the tests to pass:

platformtests/aws$ AWS_PROFILE=openshift-dev go test .
ok  							 github.com/openshift/installer/platformtests/aws			26.282s

The Hong Kong region is new 2019-04-25 and does not support the older
m4 types [1].  This pull-request adds it (commented out in validation,
because we don't push RHCOS AMIs there).  It also expands the seeded
regions in TestGetDefaultInstanceClass to gets the tests to pass:

  platformtests/aws$ AWS_PROFILE=openshift-dev go test .
  ok  							 github.com/openshift/installer/platformtests/aws			26.282s

[1]: https://aws.amazon.com/blogs/aws/now-open-aws-asia-pacific-hong-kong-region/
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 16, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2019
@trown
Copy link

trown commented May 16, 2019

/test e2e-openstack

@abhinavdahiya abhinavdahiya changed the title pkg/types/aws/defaults/platform: Default to m5 in ap-east-1 aws/defaults/platform: Default to m5 in ap-east-1 May 16, 2019
@cgwalters
Copy link
Member

because we don't push RHCOS AMIs there

I suspect it'd be nearly as fast for us to just copy AMIs from another region as to have it pre-uploaded.

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,wking]

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

@openshift-merge-robot openshift-merge-robot merged commit 187ce5e into openshift:master May 22, 2019
@wking wking deleted the ap-east-1-m5-default branch May 22, 2019 13:21
@wking
Copy link
Member Author

wking commented Jul 26, 2019

/cherrypick release-4.1

We need this so #1935 can be backported cleanly.

@openshift-cherrypick-robot

@wking: new pull request created: #2108

Details

In response to this:

/cherrypick release-4.1

We need this so #1935 can be backported cleanly.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@abhinavdahiya
Copy link
Contributor

/cherrypick release-4.1

We need this so #1935 can be backported cleanly.

I think the bar for entry to release-4.1 is higher than need this for clean cherry-pick, all PRs in t release-4.1 need a bug and approval from Architects (or more)

I would rather suggest we backport manually..

@wking
Copy link
Member Author

wking commented Jul 26, 2019

I think the bar for entry to release-4.1 is higher than need this for clean cherry-pick...

I agree in theory, but this is the same issue as #1935, right? If #1935 passes the bar, then why wouldn't this PR pass that same bar? On the other hand, we'd also be internally consistent saying that neither this PR nor #1935 passes the 4.1.z bar, and closing rhbz#1725524 as WONTFIX for 4.1.z.

@abhinavdahiya
Copy link
Contributor

I'm not saying this PR doesn't meet the bar for release-4.1 but the reason this needs to be backported cannot be "needs by another PR for backport"

@wking
Copy link
Member Author

wking commented Jul 26, 2019

I'm not saying this PR doesn't meet the bar for release-4.1 but the reason this needs to be backported cannot be "needs by another PR for backport"

So how do you feel about expanding the scope of rhbz#1725524 to include ap-east-1?

@abhinavdahiya
Copy link
Contributor

I think we can create another one for ap-east-1 and get both in individually serially

wking pushed a commit to wking/openshift-installer that referenced this pull request Aug 14, 2019
AWS allows m4 class for reserved instances in all AZs for ap-northeast-2

```console
aws --region ap-northeast-2 ec2 describe-reserved-instances-offerings --instance-tenancy default --instance-type m4.large --product-description 'Linux/UNIX' --filters Name=scope,Values='Availability Zone' | jq -r '[.ReservedInstancesOfferings[].AvailabilityZone] | sort | unique[]'
ap-northeast-2a
ap-northeast-2b
ap-northeast-2c
```

But the on-demand instances in ap-norteast-2b was failing with error
```
Unsupported: Your requested instance type (m4.xlarge) is not supported in your requested Availability Zone (ap-northeast-2b). Please retry your request by not specifying an Availability Zone or choosing ap-northeast-2a, ap-northeast-2c.
```

So moving to m5 class for ap-northeast-2 should allow creating control plane instances in all Zones. (the default configuration)

This commit cherry-picks 3085753 (Bug 1725524: types/aws/default:
move ap-northeast-2 to m5 instance class, 2019-07-02, openshift#1935).  It's
not a clean cherry-pick, because we decided to not backport the
ap-east-1 addition from cdd6689 (pkg/types/aws/defaults/platform:
Default to m5 in ap-east-1, 2019-05-16, openshift#1755) to 4.1.z [1].

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1734136#c4
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. lgtm 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

Development

Successfully merging this pull request may close these issues.

7 participants