Support regions without m4 machines #1163

eparis · 2019-01-31T15:28:40Z

This does not dynamically pick machine types for the bootstrap node. Instead it just uses i3.large. i3 is the only machine type in all regions of AWS. It costs a little extra (about $0.05/hour) but we only run it for like 20 minutes.

Fixes: #1127

wking · 2019-01-31T19:52:20Z

I've pushed https://github.com/wking/openshift-installer.git fixup-bootstrap-default-type (c3d322c) with the code needed to get the bootstrap type from Go through Terraform to its eventual resource consumer. We still need to update the Go to feed in the dynamic default, but with the logic internal to pkg/asset/machines and no bootstrap machine object coming out, that's difficult at the moment. I don't think we want an install-config knob for this if we can avoid it, so how about a Bootstrap asset in pkg/asset/machines that just holds the dynamic value?

abhinavdahiya · 2019-02-01T00:15:17Z

pkg/asset/machines/master.go

+var (
+	_ asset.Asset = (*Master)(nil)
+
+	defaultMachineClass = map[string]string{


aws specific code should live in pkg/asset/machines/aws

also WDYT using m4 by default and overriding only some regions, so that we only maintain override list?

aws specific code should live in pkg/asset/machines/aws

This and some other feedback attached to eparis/installer@bc58267. I dunno why @eparis didn't just push that to this PR for feedback ;).

your hope was that I only keep non-m4 regions in this map instead of every region? I can probably do that, and fix CI.

abhinavdahiya · 2019-02-01T00:18:00Z

pkg/asset/machines/master_test.go

+		}
+		return true
+	}
+	err := svc.GetProductsPages(input, handlePage)


our unit tests are going to get rate limited :P

our unit tests are going to get rate limited :P

We shouldn't be running that many installer PRs in CI at once, should we? Are product rate limits especially tight?

wking · 2019-02-01T03:55:43Z

Vendor conflict; needs a rebase.

eparis · 2019-02-01T14:44:53Z

pushed my second stab at this

wking · 2019-02-14T08:41:14Z

I've picked this up and pushed ebd0559 -> 1275b04 to:

Rebase onto master and resolve conflicts.
Drop the m4 entries from the map and only keep entries for regions which needed an override.
Added additional pricing filters to address rate-limiting concerns (and we don't need to check for instance types that support Windows, etc., anyway).
Drop the complete regionMap from the unit tests, instead relying on validation.Regions for this information (although AWS doesn't seem to be particularly consistent about their display names, so I needed two special-cases).
Drop the InstanceClasses type. Using maps directly seemed about as expressive and saves readers from the mental overhead of absorbing a new type.
Added t.Run subtests to make it easier to isolate errors when things go wrong for a given region (this also lets us be a bit more generous with t.Fatal calls).

wking · 2019-02-14T08:54:32Z

unit:

�[0G�[2K�[1;92m? �[0m�[1;99mAWS Access Key ID �[0m�[36m[? for help]�[0m �[?25l�7�[999;999f�[6n�[6n--- FAIL: TestGetDefaultInstanceClass (0.00s)
	platform_test.go:18: EOF
FAIL

Looks like our CI user lacks pricing:GetProducts. Or we're running the unit tests without AWS credentials at all, since an earlier unit job has:

--- FAIL: TestDefaultRegionInstanceTypeMap (0.01s)
	master_test.go:123: Error fetching from AWS: NoCredentialProviders: no valid providers in chain. Deprecated.
			For verbose messaging see aws.Config.CredentialsChainVerboseErrors
FAIL

Do we want to set AWS credentials for unit tests? Or should we skip this test when we don't have credentials? If the latter, is the idea that maintainers would periodically run the test manually?

wking · 2019-02-14T19:33:20Z

I've pushed 147e63d -> f9a381e, pushing the test out into a new tests/aws directory where folks can run it manually (so we don't have to give our usual unit tests AWS creds). CC @staebler

wking · 2019-02-15T07:53:25Z

I've pushed f9a381e -> 3fb0ff6, rebasing onto master again, and moving tests/aws -> t/aws so dep ensure will find the tests and add their dependencies to the project vendor/. More on why in the 71e7f8580acf commit message.

eparis · 2019-02-16T15:02:00Z

the AWS bulk pricing api (instead of being able to filter) does not require creds, though then we can't use the aws library and have to just get the raw url. Probably could decode it into the aws library types, but they aren't actually very helpful...

eparis · 2019-02-16T15:05:13Z

pkg/types/aws/validation/platform.go

 		"eu-west-1":      "Ireland",
 		"eu-west-2":      "London",
 		"eu-west-3":      "Paris",
 		"sa-east-1":      "São Paulo",
 		"us-east-1":      "N. Virginia",
 		"us-east-2":      "Ohio",
+		"us-gov-east-1":  "AWS GovCloud (US-East)",
+		"us-gov-west-1":  "AWS GovCloud (US-West)",


You sure about this? The pricing API calls this AWS GovCloud (US) I think...

The pricing API calls this AWS GovCloud (US) I think...

It does, but I have an entry for decoding that here. They use US-West in other places, and now that US-East exists, I think the qualified name makes more sense here.

wking · 2019-02-16T15:21:58Z

the AWS bulk pricing api (instead of being able to filter) does not require creds, though then we can't use the aws library and have to just get the raw url.

I'm fine with a transition here, but am also fine punting that to follow-up work.

crawford · 2019-02-19T03:27:11Z

From the logs:

level=error msg="Error: Required variable not set: aws_master_root_volume_iops"

I've never seen that one before.

From Trevor: For regions like eu-west-3 (Paris) that don't support m4. The only instance type available in all AWS regions today is i3. We prefer m4 because of the higher EBS attach limit. But for regions that don't support m4 we use m5. This also adds a CI job which checks the AWS pricing API to keep our region map up to date. The eu-north-1 region is new as of 2018-12-12 [1], and it show up in: $ aws ec2 describe-regions --query 'Regions[].RegionName' --output json | jq -r 'sort[]' ... eu-central-1 eu-north-1 eu-west-1 ... The GovCloud entries didn't show up in that query (maybe they are only listed for accounts that have access?), but they are listed on [2]. The new test is in platformtests/aws to keep it out of our default unit tests. Maintainers can run it manually to verify that the default fallbacks we hard-code are still appropriate for the current AWS offerings. It's not in tests/aws, because 'dep ensure' seems to ignore tests. I think it's fine to have both platformtests and tests in parallel, with the former holding tests who share the project vendor/ and the latter holding tests which have a distinct vendor/. bdd-smoke is in tests with its own vendor/ intentionally to keep gomega and other test-specific libraries out of the project vendor/ [3]. But for these tests we're using Go's built-in test framework, so it's less maintenance to layer our minimal additional dependencies into the project vendor/. [1]: https://aws.amazon.com/blogs/aws/now-open-aws-europe-stockholm-region/ [2]: https://aws.amazon.com/about-aws/global-infrastructure/regional-product-services/ [3]: openshift#552 (comment)

wking · 2019-02-19T06:58:19Z

level=error msg="Error: Required variable not set: aws_master_root_volume_iops"

Oops. Fixed with 3745dcf -> 354554d, fixing an omitempty that had snuck back in during a broken rebase (and rebasing again onto master).

wking · 2019-02-19T09:17:09Z

e2e-aws:

Failing tests:

The bootstrap user should successfully login with password decoded from kubeadmin secret [Suite:openshift/conformance/parallel]
[Conformance][Area:Networking][Feature:Router] The HAProxy router should expose prometheus metrics for a route [Suite:openshift/conformance/parallel/minimal]

/retest

wking · 2019-02-19T10:39:24Z

e2e-aws:

error: unable to read image registry.svc.ci.openshift.org/ci-op-hdsq08kg/stable@sha256:cd30230b8b0fbd2cfcca3040f99a376c86f5f8abca441fb85165d48d1e23ba97: received unexpected HTTP status: 504 Gateway Time-out

/retest

droslean · 2019-02-19T11:43:05Z

/retest

crawford · 2019-02-19T17:16:48Z

Same error as before:

error: unable to read image registry.svc.ci.openshift.org/ci-op-hdsq08kg/stable@sha256:b3f34b64b11c1554b47f9b455972ce8da79367a26bc12ee20da7e330a128d0de: received unexpected HTTP status: 504 Gateway Time-out

/retest

crawford · 2019-02-19T20:24:50Z

Flaky tests:

[Feature:DeploymentConfig] deploymentconfigs when run iteratively [Conformance] should only deploy the last deployment [Suite:openshift/conformance/parallel/minimal]

Failing tests:

The bootstrap user should successfully login with password decoded from kubeadmin secret [Suite:openshift/conformance/parallel]

Sigh Blocked on openshift/origin#22070.

crawford · 2019-02-20T01:56:11Z

/retest

crawford · 2019-02-20T02:00:44Z

2019/02/20 01:57:48 Build src failed, printing logs:
E0220 01:57:58.864097      13 event.go:203] Server rejected event '&v1.Event{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:v1.ObjectMeta{Name:".1584efd716f757a1", GenerateName:"", Namespace:"ci-op-4r3lp5bf", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Initializers:(*v1.Initializers)(nil), Finalizers:[]string(nil), ClusterName:""}, InvolvedObject:v1.ObjectReference{Kind:"", Namespace:"ci-op-4r3lp5bf", Name:"", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}, Reason:"CiJobFailed", Message:"Running job pull-ci-openshift-installer-master-e2e-aws for PR https://github.com/openshift/installer/pull/1163 in namespace ci-op-4r3lp5bf from author eparis", Source:v1.EventSource{Component:"ci-op-4r3lp5bf", Host:""}, FirstTimestamp:v1.Time{Time:time.Time{wall:0xbf134ae9aff55ba1, ext:19501311214, loc:(*time.Location)(0x1a064c0)}}, LastTimestamp:v1.Time{Time:time.Time{wall:0xbf134ae9aff55ba1, ext:19501311214, loc:(*time.Location)(0x1a064c0)}}, Count:1, Type:"Warning", EventTime:v1.MicroTime{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, Series:(*v1.EventSeries)(nil), Action:"", Related:(*v1.ObjectReference)(nil), ReportingController:"", ReportingInstance:""}': 'namespaces "ci-op-4r3lp5bf" not found' (will not retry!)

will not retry!

We'll see about that...

/retest

wking · 2019-02-20T04:14:41Z

e2e-aws:

level=fatal msg="failed to initialize the cluster: Cluster operator network has not yet reported success"

/retest

crawford · 2019-02-20T20:34:32Z

I haven't seen this failure before:

failed to create imagestreamtag for input image: imagestreamtags.image.openshift.io "pipeline:base" is forbidden: unable to create new content in namespace ci-op-4r3lp5bf because it is being terminated

It's an issue with CI; not OpenShift.

/retest

crawford · 2019-02-21T05:05:32Z

/retest

droslean · 2019-02-21T08:15:20Z

/test e2e-aws

droslean · 2019-02-21T13:28:52Z

/test e2e-aws

crawford · 2019-02-21T22:36:00Z

/retest

abhinavdahiya · 2019-02-22T00:07:53Z

green ci :yay:

/lgtm

openshift-ci-robot · 2019-02-22T00:08:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, crawford, eparis

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:

~~OWNERS~~ [abhinavdahiya,crawford]

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2019

eparis changed the title ~~WIP: Non m4 machines~~ Support regions without m4 machines Jan 31, 2019

openshift-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 Jan 31, 2019

abhinavdahiya reviewed Feb 1, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2019

eparis force-pushed the non-m4-machines branch from 775a729 to ebd0559 Compare February 1, 2019 14:43

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2019

wking force-pushed the non-m4-machines branch from ebd0559 to 1275b04 Compare February 14, 2019 08:34

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2019

wking force-pushed the non-m4-machines branch from 1275b04 to 147e63d Compare February 14, 2019 08:44

wking mentioned this pull request Feb 14, 2019

*: support no worker nodes #1231

Merged

wking force-pushed the non-m4-machines branch from 147e63d to f9a381e Compare February 14, 2019 19:31

wking force-pushed the non-m4-machines branch 2 times, most recently from d9a52d5 to 3fb0ff6 Compare February 15, 2019 07:53

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2019

eparis commented Feb 16, 2019

View reviewed changes

wking force-pushed the non-m4-machines branch from 3fb0ff6 to 3745dcf Compare February 16, 2019 18:06

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2019

eparis added 2 commits February 18, 2019 22:56

dep ensure: we need to add the aws pricing dependancy

354554d

wking force-pushed the non-m4-machines branch from 3745dcf to 354554d Compare February 19, 2019 06:57

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2019

eparis mentioned this pull request Feb 19, 2019

eu-west-3 doesn't have m4.xlarge #1127

Closed

openshift-ci-robot assigned abhinavdahiya Feb 22, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019

openshift-merge-robot merged commit e8121e2 into openshift:master Feb 22, 2019

abhinavdahiya mentioned this pull request Feb 22, 2019

*: use a human friendly ID to tag and name cloud resources. #1280

Merged

eparis deleted the non-m4-machines branch March 12, 2019 20:41

wking mentioned this pull request Apr 3, 2019

update instance family from m4 to m5 #1087

Closed

wking mentioned this pull request Apr 22, 2019

AWS UPI draft openshift/openshift-docs#14241

Merged

cgwalters mentioned this pull request Jan 14, 2020

aws: Use modern instance types by default #2919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support regions without m4 machines #1163

Support regions without m4 machines #1163

eparis commented Jan 31, 2019 •

edited

Loading

wking commented Jan 31, 2019

abhinavdahiya Feb 1, 2019

abhinavdahiya Feb 1, 2019

wking Feb 1, 2019

eparis Feb 1, 2019

abhinavdahiya Feb 1, 2019

wking Feb 1, 2019 •

edited

Loading

wking commented Feb 1, 2019

eparis commented Feb 1, 2019

wking commented Feb 14, 2019

wking commented Feb 14, 2019

wking commented Feb 14, 2019

wking commented Feb 15, 2019

eparis commented Feb 16, 2019

eparis Feb 16, 2019

wking Feb 16, 2019

wking commented Feb 16, 2019

crawford commented Feb 19, 2019

wking commented Feb 19, 2019

wking commented Feb 19, 2019

wking commented Feb 19, 2019

droslean commented Feb 19, 2019

crawford commented Feb 19, 2019

crawford commented Feb 19, 2019

crawford commented Feb 20, 2019

crawford commented Feb 20, 2019

wking commented Feb 20, 2019

crawford commented Feb 20, 2019

crawford commented Feb 21, 2019

droslean commented Feb 21, 2019

droslean commented Feb 21, 2019

crawford commented Feb 21, 2019

abhinavdahiya commented Feb 22, 2019

openshift-ci-robot commented Feb 22, 2019

Support regions without m4 machines #1163

Support regions without m4 machines #1163

Conversation

eparis commented Jan 31, 2019 • edited Loading

wking commented Jan 31, 2019

abhinavdahiya Feb 1, 2019

Choose a reason for hiding this comment

abhinavdahiya Feb 1, 2019

Choose a reason for hiding this comment

wking Feb 1, 2019

Choose a reason for hiding this comment

eparis Feb 1, 2019

Choose a reason for hiding this comment

abhinavdahiya Feb 1, 2019

Choose a reason for hiding this comment

wking Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

wking commented Feb 1, 2019

eparis commented Feb 1, 2019

wking commented Feb 14, 2019

wking commented Feb 14, 2019

wking commented Feb 14, 2019

wking commented Feb 15, 2019

eparis commented Feb 16, 2019

eparis Feb 16, 2019

Choose a reason for hiding this comment

wking Feb 16, 2019

Choose a reason for hiding this comment

wking commented Feb 16, 2019

crawford commented Feb 19, 2019

wking commented Feb 19, 2019

wking commented Feb 19, 2019

wking commented Feb 19, 2019

droslean commented Feb 19, 2019

crawford commented Feb 19, 2019

crawford commented Feb 19, 2019

crawford commented Feb 20, 2019

crawford commented Feb 20, 2019

wking commented Feb 20, 2019

crawford commented Feb 20, 2019

crawford commented Feb 21, 2019

droslean commented Feb 21, 2019

droslean commented Feb 21, 2019

crawford commented Feb 21, 2019

abhinavdahiya commented Feb 22, 2019

openshift-ci-robot commented Feb 22, 2019

eparis commented Jan 31, 2019 •

edited

Loading

wking Feb 1, 2019 •

edited

Loading