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 cpu_utilization larger than 1.0. #60

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

yousukseung
Copy link
Contributor

Signed-off-by: Yousuk Seung ysseung@google.com

@yousukseung yousukseung marked this pull request as ready for review April 26, 2023 18:51
@yousukseung
Copy link
Contributor Author

yousukseung commented Apr 26, 2023

@markdroth @ejona86 @dfawley

@yousukseung
Copy link
Contributor Author

@htuch PTAL.

@markdroth markdroth changed the title Allow cpu_utilizatoin larger than 1.0. Allow cpu_utilization larger than 1.0. Apr 26, 2023
@markdroth
Copy link
Contributor

I think this is the right thing to do, since there are a lot of cluster systems out there that have varying levels of oversubscription and opportunistic provisioning, so I think it will not be uncommon to need values greater than 1.0 here.

// should be derived from the latest sample or measurement.
double cpu_utilization = 1 [(validate.rules).double.gte = 0, (validate.rules).double.lte = 1];
// should be derived from the latest sample or measurement. The value may be
// larger than 1.0 when the usage exceeds the soft limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

"exceeds a reporter dependent notion of soft limits". I think it's legit, just want to be clear who is specifying the soft limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@htuch
Copy link
Contributor

htuch commented Apr 27, 2023

Needs DCO fix (or possibly new PR).

Signed-off-by: Yousuk Seung <ysseung@google.com>
@yousukseung
Copy link
Contributor Author

I rebased to fix the DCO error per the link above.

@htuch htuch merged commit 4003588 into cncf:main Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants