-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add size and type support for aws volumes #1079
Conversation
@wking addressed comments, let me know if there's anything else you want me to change, thanks! |
/test e2e-aws |
/retest |
/test e2e-aws |
1 similar comment
/test e2e-aws |
pkg/asset/machines/aws/machines.go
Outdated
BlockDevices: []awsprovider.BlockDeviceMappingSpec{ | ||
{ | ||
EBS: &awsprovider.EBSBlockDeviceSpec{ | ||
VolumeType: &volumeType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use k8s.io/utils/pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet, thanks!
pkg/asset/machines/worker.go
Outdated
@@ -94,6 +94,11 @@ func (w *Worker) Generate(dependencies asset.Parents) error { | |||
switch ic.Platform.Name() { | |||
case awstypes.Name: | |||
mpool := defaultAWSMachinePoolPlatform() | |||
// worker pool default volume settings | |||
mpool.EC2RootVolume = awstypes.EC2RootVolume{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we move this to defaultAWSMachinePoolPlatform and only override the Size
in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks!
9ba64ee
to
10eb32d
Compare
/approve /hold /cc @crawford @derekwaynecarr for approval |
@abhinavdahiya: GitHub didn't allow me to request PR reviews from the following users: for, approval. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
we have approval for this. /hold cancel |
Needs rebasing around #1069. |
Since we added support on the aws actuator API openshift/cluster-api-provider-aws#132 we want to allow this to be configurable for the users through the upper level installer API config
rebased, thanks |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, enxebre, 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 |
Card filed to support in Hive API: https://jira.coreos.com/browse/CO-190 |
…aults Since 9a9cd6d (origin/pr/1076) 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.
…aults 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.
…aults 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.
…aults 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.
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.
Since we added support on the aws actuator API openshift/cluster-api-provider-aws#132 we want to allow this to be configurable for the users through the upper level installer API config