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

Change iops params directly convert string to int64 #1128

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Change iops params directly convert string to int64 #1128

merged 1 commit into from
Feb 9, 2023

Conversation

sunnylovestiramisu
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
After discussion, the iops fields should not be handling GB/MB etc. because PD is treating this value as a configuration for the whole disk

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
The change of HdX and HdT is not released yet.

NONE

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 8, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 8, 2023
@sunnylovestiramisu
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 8, 2023
@sunnylovestiramisu
Copy link
Contributor Author

/assign @mattcary

msau42
msau42 reviewed Feb 8, 2023
View reviewed changes
pkg/common/utils.go Outdated
last := string(str[len(str)-1])

switch last {
case "k":
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to account for base 2 vs base 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we care about base 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

In SI notation, k = 1000 and Ki = 2^10

https://physics.nist.gov/cuu/Units/binary.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I was asking why do we need support for base 2 input, because it is unnatural for humans. Do our customers ask for that?

pkg/common/utils.go
// ConvertGiBStringToInt64 converts a GiB string to int64
func ConvertGiBStringToInt64(str string) (int64, error) {
// ConvertStringToInt64 converts a string to int64
func ConvertStringToInt64(str string) (int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be better to use kubernetes resource quantity parsing instead of rolling your own: https://pkg.go.dev/k8s.io/apimachinery/pkg/api/resource#Quantity

Copy link
Contributor Author

@sunnylovestiramisu sunnylovestiramisu Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But they do not have our use case. They do not convert these values to both million and thousand?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand. It looks like this function is converting from a string, possibly abbreviated in SI notation, to the raw number right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and we want to support:

"1000" -> int64(1000)
"1K"-> int64(1000)
"1M" -> int64(1,000,000)
Maybe "1G" -> int64(1,000,000,000)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "K" is not supported in resource#Quantity. But other than that mostly covered. We do not need to support the "K"?

@sunnylovestiramisu sunnylovestiramisu changed the title Change iops params to not handle GB/MB etc. Change iops params to not handle Gi/Mi etc. Feb 8, 2023
@sunnylovestiramisu sunnylovestiramisu changed the title Change iops params to not handle Gi/Mi etc. Change iops params directly convert string to int64 Feb 8, 2023
@sunnylovestiramisu sunnylovestiramisu force-pushed the fixExtreme branch 2 times, most recently from f23141f to 1afd9ed Compare February 8, 2023 18:50
@sunnylovestiramisu sunnylovestiramisu marked this pull request as ready for review February 8, 2023 19:12
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2023
leiyiz
leiyiz reviewed Feb 8, 2023
View reviewed changes
@sunnylovestiramisu sunnylovestiramisu force-pushed the fixExtreme branch 3 times, most recently from 3ff0e83 to e8d9a5c Compare February 9, 2023 01:09
@mattcary
Copy link
Contributor

mattcary commented Feb 9, 2023

\o/

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary, sunnylovestiramisu

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 Feb 9, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0a39f68 into kubernetes-sigs:master Feb 9, 2023
@leiyiz leiyiz mentioned this pull request Feb 13, 2023
Merged
@sunnylovestiramisu sunnylovestiramisu deleted the fixExtreme branch February 14, 2023 21:57
This was referenced May 26, 2023
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Jul 21, 2023
@k8s-ci-robot
Merge pull request #1243 from sunnylovestiramisu/automated-cherry-pic…
cc2fea0 
…k-of-#1079-#1101-#1128-upstream-release-1.7

Automated cherry pick of #1079: Add provisionedIops for pd-extreme #1128: Change iops params directly convert string to int64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@leiyiz leiyiz leiyiz left review comments

@mattcary mattcary Awaiting requested review from mattcary

@amacaskill amacaskill Awaiting requested review from amacaskill

Assignees

@mattcary mattcary

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. kind/feature Categorizes issue or PR as related to a new feature. 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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sunnylovestiramisu @k8s-ci-robot @mattcary @msau42 @leiyiz