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.

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

Allow setting gp3 for terraform volumes without setting throughput #10572

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

hakman
Copy link
Member

@hakman hakman commented Jan 13, 2021

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
Copy link
Member Author

hakman commented Jan 13, 2021

/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
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

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
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
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
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
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
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
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