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

data/aws/variables-aws: Drop aws_master_ec2_type and similar defaults #1128

Merged
merged 1 commit into from
Feb 15, 2019
Merged

data/aws/variables-aws: Drop aws_master_ec2_type and similar defaults #1128

merged 1 commit into from
Feb 15, 2019

Conversation

wking
Copy link
Member

@wking wking commented Jan 25, 2019

Since 9a9cd6d (#1076) and cbad1a4 (#1079), we should always be providing these in the Terraform variables generated by the asset graph.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 25, 2019
@wking wking force-pushed the drop-terraform-master-instance-type branch from ab9fb3f to 580a34b Compare January 25, 2019 18:17
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2019
staebler
staebler reviewed Jan 25, 2019
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

looks good

data/data/aws/variables-aws.tf Show resolved Hide resolved
@wking wking force-pushed the drop-terraform-master-instance-type branch from 580a34b to 4dfa11f Compare January 25, 2019 19:37
@wking
Copy link
Member Author

wking commented Jan 25, 2019

Obviously I'm missing something, because e2e-aws is giving:

level=info msg="Creating cluster..."
level=error
level=error msg="Error: Required variable not set: aws_master_ec2_type"
level=error
level=error
level=error
level=error msg="Error: Required variable not set: aws_master_root_volume_type"
level=error
level=error
level=error
level=error msg="Error: Required variable not set: aws_master_root_volume_size"
level=error
level=error
level=error
level=error msg="Error: Required variable not set: aws_master_root_volume_iops"

@staebler
Copy link
Contributor

Obviously I'm missing something, because e2e-aws is giving:

level=info msg="Creating cluster..."
level=error
level=error msg="Error: Required variable not set: aws_master_ec2_type"
level=error
level=error
level=error
level=error msg="Error: Required variable not set: aws_master_root_volume_type"
level=error
level=error
level=error
level=error msg="Error: Required variable not set: aws_master_root_volume_size"
level=error
level=error
level=error
level=error msg="Error: Required variable not set: aws_master_root_volume_iops"

@wking The default values are not added to the install config for those fields.

@wking
Copy link
Member Author

wking commented Jan 25, 2019

🤦‍♂️ ok, so blocked on #792.

wking referenced this pull request in eparis/installer Jan 31, 2019
@eparis
Define a map of region to instance "classes" for regions that don't s…
bc58267 
…upport 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 2 date.
@staebler staebler mentioned this pull request Feb 4, 2019
Merged
@wking wking changed the title data/data/aws/variables-aws: Drop aws_master_ec2_type and similar defaults data/aws/variables-aws: Drop aws_master_ec2_type and similar defaults Feb 4, 2019
wking added a commit to wking/openshift-installer that referenced this pull request Feb 4, 2019
@wking
pkg/tfvars: Pull from []Machine instead of InstallConfig
42a8a10 
The previous implementation pulled from the install-config, but that
missed downstream changes like AWS instance type defaults being
applied in pkg/asset/machines.  With this commit, we pull that
information from the cluster-API types, since they're the last touch
point for that data.  Some remaining information still comes from the
InstallConfig, but I've split it into explicit arguments to avoid
confusion about where data is coming from when InstallConfig's machine
pools, etc. overlap with clusterapi.Machine fields.

This commit also splits the platform-specific Terraform variable
generation into separate functions generating separate files.  I've
used *.auto.tfvars filenames because Terraform loads those
automatically from the current directory [1].  But that only helps
folks who are trying to run our generated Terraform by hand; as
described in d19cad5 (destroy/bootstrap: explicit pass
`disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the
installer runs outside the Terraform directory and needs to pass this
through to Terraform explicitly.

I'd prefer if FileList could be an internal property (.files?), but we
currently require public fields for reloading from disk during
multiple-invocation creation [2].

The AWS volume defaults are from data/data/aws/variables-aws.tf.
We'll be able to drop the Terraform defaults soon [3].

[1]: https://www.terraform.io/docs/configuration/variables.html#variable-files
[2]: openshift#792 (comment)
[3]: openshift#1128
@wking wking force-pushed the drop-terraform-master-instance-type branch from 4dfa11f to 90e56bf Compare February 7, 2019 21:54
@wking
Copy link
Member Author

wking commented Feb 7, 2019

Rebased onto master with 4dfa11f -> 90e56bf now that #792 has landed.

@wking
data/aws/variables-aws: Drop aws_master_ec2_type and similar defaults
55a88f3 
Since 9a9cd6d (pkg/tfvars: Respect install-config AWS machine pools,
2019-01-15, openshift#1076) and cbad1a4 (Add size and type support for aws
volumes, 2019-01-16, openshift#1079), we should always be providing these in
the Terraform variables generated by the asset graph.

There's still a default for the bootstrap machine, but we plan on
removing that from Terraform shortly.

Dropping omitempty for the master IOPS allows the zero value (if IOPS
is nil or zero in the machine config) to generate a non-empty
Terraform value, avoiding:

  Error: Required variable not set: aws_master_root_volume_iops"

I've left omitempty on the other AWS properties, because in most cases
their zero values will not produce a functional cluster and we want to
fail fast.  In the user-tags case, we have a sane (empty set) default,
so omitting the nil map is fine.
@wking wking force-pushed the drop-terraform-master-instance-type branch from 90e56bf to 55a88f3 Compare February 7, 2019 23:34
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 7, 2019
@staebler
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-merge-robot openshift-merge-robot merged commit 9e137c7 into openshift:master Feb 15, 2019
@wking wking deleted the drop-terraform-master-instance-type branch February 16, 2019 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staebler staebler staebler left review comments

@russellb russellb Awaiting requested review from russellb

@tomassedovic tomassedovic Awaiting requested review from tomassedovic

Assignees

@staebler staebler

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. platform/aws 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.
4 participants
@wking @staebler @openshift-ci-robot @openshift-merge-robot