Skip to content

Allow setting gp3 for terraform volumes without setting throughput#10572

Merged
k8s-ci-robot merged 1 commit into
kubernetes:release-1.19from
hakman:terraform-etcd-vol-gp3
Jan 15, 2021
Merged

Allow setting gp3 for terraform volumes without setting throughput#10572
k8s-ci-robot merged 1 commit into
kubernetes:release-1.19from
hakman:terraform-etcd-vol-gp3

Conversation

@hakman

@hakman hakman commented Jan 13, 2021

Copy link
Copy Markdown
Member

Until hashicorp/terraform-provider-aws#16517 is merged, TF will not be able to use the throughput arg for aws_ebs_volume.

@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jan 13, 2021
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 13, 2021
@hakman

hakman commented Jan 13, 2021

Copy link
Copy Markdown
Member Author

/cc @rifelpet @olemarkus

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 13, 2021
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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:

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 area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 13, 2021
// TODO: Remove when Terraform gets support for "throughput" with "aws_ebs_volume"
// https://github.com/hashicorp/terraform-provider-aws/pull/16517
throughput := e.VolumeThroughput
if fi.Int64Value(e.VolumeThroughput) <= 125 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why are we only setting it to nil with this condition? if the throughput field isn't yet supported at all in terraform then I'd think we should always set it to nil?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also we should have an integration test that generates this terraform output so that the verify-terraform job will catch it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is set to nil if you don't use gp3 volumes. We have integration test in master #10569, but not sure how we can "catch" it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looking at the api docs

    // The throughput to provision for a volume, with a maximum of 1,000 MiB/s.
    //
    // This parameter is valid only for gp3 volumes.
    //
    // Valid Range: Minimum value of 125. Maximum value of 1000.
    Throughput *int64 `type:"integer"`

I feel like we should fail api validation if throughput is < 125 rather than ignoring the value. We could also fail validation if RootVolumeThroughput is set for RootVolumeTypes other than gp3 (and if cloudprovider != aws )

This is the only throughput validation we have currently:

if fi.Int32Value(g.Spec.RootVolumeThroughput) < 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "rootVolumeThroughput"), g.Spec.RootVolumeThroughput, "RootVolumeThroughput must be greater than 0"))
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could, but it's not really this hack's purpose. I just want to not put the value in the TF template if the value is the minimum to not need a new kOps release when TF decides to merge the throughput arg. We set values lower than the minimum to minimum in model.

@rifelpet

Copy link
Copy Markdown
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 12ef17f into kubernetes:release-1.19 Jan 15, 2021
@hakman hakman deleted the terraform-etcd-vol-gp3 branch January 15, 2021 04:32
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. area/provider/aws Issues or PRs related to aws provider 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants