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

Add constants and documentation around AWS magic numbers #31115

Merged
merged 1 commit into from
Aug 24, 2016

Conversation

jsafrane
Copy link
Member

@jsafrane jsafrane commented Aug 22, 2016

Also, bumped max IOPS/GB to 50, it changed from 30 since last time I checked.

Source: http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSVolumeTypes.html

@kubernetes/sig-storage


This change is Reviewable

Also, remove check for IOPS per GB, AWS checks it on its own.
@jsafrane jsafrane added sig/storage Categorizes an issue or PR as relevant to SIG Storage. team/cluster release-note-none Denotes a PR that doesn't merit a release note. labels Aug 22, 2016
@jsafrane jsafrane added this to the v1.4 milestone Aug 22, 2016
@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 22, 2016
@MHBauer
Copy link
Contributor

MHBauer commented Aug 22, 2016

Yay variables and reuse.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016
const (
MaxIOPSPerGB = 50
MinTotalIOPS = 100
MaxTotalIOPS = 20000
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the EBS link, but I see the numbers are different for SSD and HDD drivers. Are we only considering SSD?

Copy link
Member Author

Choose a reason for hiding this comment

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

IOPS can be set by user only for io1 volumes, IOPS for the rest of volumes is preset by AWS.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack

@jsafrane jsafrane changed the title Add constants and documentation aroung AWS magic numbers Add constants and documentation around AWS magic numbers Aug 23, 2016
if volumeOptions.IOPSPerGB <= 0 || volumeOptions.IOPSPerGB > 30 {
return "", fmt.Errorf("invalid iopsPerGB value %d, must be 0 < IOPSPerGB <= 30", volumeOptions.IOPSPerGB)
if volumeOptions.IOPSPerGB <= 0 || volumeOptions.IOPSPerGB > MaxIOPSPerGB {
return "", fmt.Errorf("invalid iopsPerGB value %d, must be 0 < IOPSPerGB <= %d", volumeOptions.IOPSPerGB, MaxIOPSPerGB)
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid --> Invalid?

@jsafrane
Copy link
Member Author

removed checking of iopsPerGB, aws will do it on our behalf. Left capping at 20 000 IOPS.

@jsafrane
Copy link
Member Author

@jingxu97, PTAL.

@k8s-bot
Copy link

k8s-bot commented Aug 24, 2016

GCE e2e build/test passed for commit a596668.

@jingxu97
Copy link
Contributor

LGTM

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 24, 2016

GCE e2e build/test passed for commit a596668.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 49ff2e8 into kubernetes:master Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants