Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for Container CPU Utilization Resource Monitor #37792
base: main
Are you sure you want to change the base?
Add support for Container CPU Utilization Resource Monitor #37792
Changes from 9 commits
8d1863c
8c24c52
3de82b3
174111d
26d1e99
59f6dd3
b607d8c
4315e3f
354bec1
beafb4b
b6a619b
e623cf4
8979e8b
060118f
a32c1f6
ccd9456
3d7bbd1
153a2a8
ca96425
114f444
b93f3d8
346cb53
9f90f9c
64320f4
ef793ba
607050e
e4817dc
1213a6c
72da693
2523785
71bd67a
44188b2
c817300
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is the reason we can't use the existing cpu times structure given it has effectively the same fields?
This would simplify the utilization monitor as it could then just use the CpuStatsReader interface vs the concrete implementation class.
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.
@KBaichoo actually, for Host CPU Utilization, we just need /proc/stat file to read cpu_work and cpu_times both of the field in CpuTimes.
While to calculate container cpu utilization we need to read two different files to read allocated quota at some time, usage seconds total in nanoseconds at that time, and the time_difference from the timer only I could find to calculate. This method we are using in our ambassador edge stack service also, but to update injected resource pressure from a parallely running python script using same calculation strategy.
ref: google/cadvisor#2026 (comment)
So I created another class to read cgroup stats, which read allocated quota, total cpu time in nanoseconds.
This can be merged with CpuTimes class itself but not sure what meaningful naming would suffice for these two though both are uint64_t data type only. But yeah then the reader class also would need to access config mode and based on config mode it should calculate and return stats accordingly.
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.
Time difference can be calculated by timeSource only in case of cgroup metrics . Which we don't need while calculating usage of host from /proc/stat file since all the data in this is time dependent already.
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 is a good point, if we were to push down the time source into the cgroup stats reader (along with some other logic) we would be able to produce the same format for
CpuTimes
-- e.g. https://github.com/envoyproxy/envoy/pull/37792/files#diff-1183c2c3937672e9d4c85d700d1e54ef13d360b4b517a0c643a8e46c13c3eb79R120we could then de-dup a lot of the implementation details from being brought up at the monitor layer to avoid the monitoring layer having to have implementations for all of the different readers.
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.
@KBaichoo in that case, in cgroupstatsreader, I have to derive some calculation to incorporate timing also to allocated millicores and usage seconds total metrics to make it equivalent to , cputimes fields total time and work time ? Something like that you are meaning ? And then the resource monitor would still using the original strategy without checking config mode and switching the calculation strategy?
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.
have checked the calculation strategy, It should work. But trying now to pushdown timesource from context in stats reader.
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.
@KBaichoo /proc/uptime has precision till 100th of a second. We can use this without using timesource if refresh_interval is not selected below 0.01 seconds. But where to set it's limit ? How much less refresh_interval we can set ? Is it given ? We use 5 second of refresh_interval for loadshedding in our ambassador edgestack. Will there be any usecase that devs would use this for even less than 0.01 second refresh interval ?
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.
Using proc/uptime metrics to calculate will get us rid of timesource in reader class and much simpler implementaion. Just with 1 limitation, refresh_interval less than 0.01 second won't work properly, but might update resource pressure as soon as the monitor's loop interval crosses 0.01 second. For refresh_interval greater than 0.01 seconds, it would be perfect. Please let me know if you will allow this. For us it should be sufficient.
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.
Have implemented all tests and functionality using proc/uptime. @KBaichoo . This should be sufficient for us.
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.
I might be missing something, why do we need to use uptime vs the timesource? ISTM it might be cheaper to use timesource e.g. no file open, etc. and no limitation on uptime granularity.
100% agree that for most use cases polling resource monitors faster is a great way to eat up CPU.
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.
as mentioned above in https://github.com/envoyproxy/envoy/pull/37792/files#diff-9281e66aafccb8196311602044a32a4ac53a877d7bae55cc591df8f30ae15810R25 if we go that route this can then just inherit directly from CpuStatsReader interface.
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.
@KBaichoo have got rid of timesource and removed multiple redundancies and simplified the monitor as well , our org would not use refresh_interval smaller than even 1 second. And envoy's cpu overhead again increases if we use more smaller refresh_interval . So sticking with /proc/uptime stats which has precision of 0.01 second. Which is more than enough for most usecases and from now on linux cpu stats reader will be sending stats to monitor based on the strategy configured.