-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
docs: correct the dominator of memory and examples in resource-durati… #11432
Conversation
…on.md - The dominator of memory should be `100Mi` instead of `1Gi`. [Source code](https://github.com/argoproj/argo-workflows/blob/7f22085d16316bdd77bdb1060455140d9d6e1664/pkg/apis/workflow/v1alpha1/workflow_types.go#L2012) - Fix the wrong examples in the document - Remove "Rounding Down" section, because it's not likely to happen Signed-off-by: Jay Lee <jayin920805@gmail.com>
@agilgur5 Can help me review this? |
Follow up from Slack where OP found this inconsistency (nice find!) and I found the correct amounts in the source code 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great find again! Two small changes requested, otherwise LGTM
For example, `memory` means "amount of time a resource requested 1Gi of memory." If a container only | ||
uses 100Mi, each second it runs will only count as a tenth-second of `memory`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this example could also be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #11449
|
||
## Rounding Down | ||
|
||
For short running pods (<10s), the memory value may be 0s. This is because the default is `100Mi`, | ||
but the denominator is `1Gi`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I still think this section is useful. I definitely have Pods that run for less than 10s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #11449
Oops, @terrytangyuan we got an IRL race condition; you happened to merge while I was still reviewing 😅 |
Oops feel free to submit follow up PR! |
I will file another PR later for the review @agilgur5 metioned above. |
Followed up in #11449. Thanks! |
Motivation
The dominator of memory should be
100Mi
instead of1Gi
. Source codeModifications
Verification
These changes are consisitent with the source code.