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

Document etcd volume options + fail fast if ratio is too high #6035

Merged
merged 2 commits into from
Nov 11, 2018
Merged

Document etcd volume options + fail fast if ratio is too high #6035

merged 2 commits into from
Nov 11, 2018

Conversation

Vlaaaaaaad
Copy link
Contributor

This PR adds documentation around the options for the etcd volumes: volumeType and volumeIops, in addition to the already existing documentation for volumeSize

Fixes #4557

@Vlaaaaaaad
Document etcd volume options
b2b07cf 
This commit adds documentation around the options for the etcd volumes:
volumeType and volumeIops, in addition to the already existing documentation for
volumeSize

Fixes #4557
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 2, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @Vlaaaaaaad. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 2, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 2, 2018
@Vlaaaaaaad
Copy link
Contributor Author

CLA refresh comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 2, 2018
@Vlaaaaaaad
Fail fast if io2 iops to size ratio is too high
098266e 
In AWS the ratio between volume IOPS and volume size must be at most 50,
otherwise volume will fail creating. See
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSVolumeTypes.html,
specifically "_The maximum ratio of provisioned IOPS to requested volume size
(in GiB) is 50:1. For example, a 100 GiB volume can be provisioned with up to
5,000 IOPS._"

This commit adds the option of failing fast when creating a new cluster if the
ratio is higher than 50. Previously kops would send the API request to AWS, fail
and repeat until the timeout was reached.
@Vlaaaaaaad Vlaaaaaaad changed the title Document etcd volume options Document etcd volume options + fail fast if ratio is too high Nov 6, 2018
justinsb
justinsb reviewed Nov 10, 2018
View reviewed changes
docs/cluster_spec.md
@@ -85,19 +85,23 @@ etcdClusters:

> __Note:__ The images for etcd that kops uses are from the Google Cloud Repository. Google doesn't release every version of etcd to the gcr. Check that the version of etcd you want to use is available [at the gcr](https://console.cloud.google.com/gcr/images/google-containers/GLOBAL/etcd?gcrImageListsize=50) before using it in your cluster spec.

By default, the Volumes created for the etcd clusters are 20GB each. They can be adjusted via the `volumeSize` parameter.
By default, the Volumes created for the etcd clusters are `gp2` and 20GB each. The volume size, type and Iops( for `io1`) can be configured via their parameters. Conversion between `gp2` and `io1` is not supported, nor are size changes.
Copy link
Member

Choose a reason for hiding this comment

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

Docs look great - much clearer, thank you!

pkg/model/master_volumes.go
@@ -160,9 +163,16 @@ func (b *MasterVolumeBuilder) addAWSVolume(c *fi.ModelBuilderContext, name strin
}
if volumeType == "io1" {
t.VolumeIops = i64(int64(volumeIops))

// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSVolumeTypes.html
Copy link
Member

Choose a reason for hiding this comment

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

So it would probably be better to do this validation earlier - you will only get told about this when you do kops update cluster, but it would be nice to tell people earlier. (Just as k8s, validates objects and tries to prevent invalid changes - for example you can't update pod limits to a negative resource value ... I think!).

In kops, this would mean putting it here: https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/aws.go#L39

But it's fine to have both validations, and it's a bit more tricky to add it to the validation because the etcd validation is currently spread out across a few files and not comprehensive (as you've found!) So we can leave this here.

I do have another concern, which is that we're hard-coding an AWS limit, and that limit might change, but we can add it and see if/when it changes. It likely won't change too often :-)

@justinsb
Copy link
Member

Would be great for us to clean up our etcd validation, but that shouldn't block this PR :-)

I also have some concerns over hard-coding an AWS limit which seems like it could change, but I'm happy to put it in and see how much of a problem that is, as I don't think AWS changes these sorts of limits too often.

/approve
/lgtm
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 10, 2018
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, Vlaaaaaaad

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2018
@k8s-ci-robot k8s-ci-robot merged commit 892d26e into kubernetes:master Nov 11, 2018
@Vlaaaaaaad Vlaaaaaaad deleted the etcd-volume-docs branch November 11, 2018 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@justinsb justinsb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

None yet
3 participants
@Vlaaaaaaad @k8s-ci-robot @justinsb