Add support for Container CPU Utilization Resource Monitor #37792

nix1n · 2024-12-23T08:03:07Z

Risk Level: Low
Testing: Unit tests
Docs Changes:
Release Notes:
Platform Specific Features: Today this is only implemented for Linux

In my org we use envoy in containerised K8s environment, this PR will allow us to trigger overload actions based on the container cpu utilization of the pod where envoy is running.

repokitteh-read-only · 2024-12-23T08:03:12Z

Hi @nix1n, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #37792 was opened by nix1n.

see: more, trace.

repokitteh-read-only · 2024-12-23T08:03:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37792 was opened by nix1n.

see: more, trace.

…edding in K8s environment Signed-off-by: nix1n <[email protected]>

Signed-off-by: nix1n <[email protected]>

KBaichoo

Thank you for working on this! I think we can try to reuse some of the existing components from the other cpu monitor to make this even better.

KBaichoo · 2024-12-24T16:02:08Z

source/extensions/resource_monitors/envoy_container_cpu_utilization/container_stats_reader.h

+namespace EnvoyContainerCpuUtilizationMonitor {
+
+
+struct EnvoyContainerStats {


Can we fold this into the existing cpu utilization monitor?

For example if we were to modify the proto to have an enum or WKT for the reader to instantiate?

we could then modify:

envoy/source/extensions/resource_monitors/cpu_utilization/config.cc

Line 19 in e22980c

// In the future, the below can be configurable based on the operating system.

to create a cpu monitor that uses either one of the stat readers ( the container one or the host one)

This would lead to us having less code to own in general :).

@KBaichoo can you check the PR now ? I tried to add container support in the existing cpu_utilization resource monitor. Have NOT rewritten tests and docs yet , will write once we finalise the actual implementation first.

KBaichoo · 2024-12-24T16:03:39Z

...resource_monitors/envoy_container_cpu_utilization/envoy_container_cpu_utilization_monitor.cc

+  previous_envoy_container_stats_ = envoy_container_stats_reader_->getEnvoyContainerStats();
+}
+
+void EnvoyContainerCpuUtilizationMonitor::updateResourceUsage(Server::ResourceUpdateCallbacks& callbacks) {


If we did the folding this file would effectively merge with the existing https://github.com/envoyproxy/envoy/blob/main/source/extensions/resource_monitors/cpu_utilization/cpu_utilization_monitor.h which it looks to me to be very similar.

KBaichoo · 2024-12-24T16:20:08Z

/wait

Signed-off-by: nix1n <[email protected]>

markdroth · 2025-01-03T00:41:02Z

api/envoy/extensions/resource_monitors/cpu_utilization/v3/cpu_utilization.proto

@@ -12,8 +13,12 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;

 // [#protodoc-title: CPU utilization]
 // [#extension: envoy.resource_monitors.cpu_utilization]
-
+enum UtilizationComputeStrategy {


Any reason not to put this enum inside of the CpuUtilizationConfig message?

Also, please document.

Updated docs, and added enum inside the CpuUtilizationConfig message.

Signed-off-by: nix1n <[email protected]>

markdroth · 2025-01-06T16:40:11Z

api/envoy/extensions/resource_monitors/cpu_utilization/v3/cpu_utilization.proto

@@ -13,7 +14,16 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // [#protodoc-title: CPU utilization]
 // [#extension: envoy.resource_monitors.cpu_utilization]

-// The CPU utilization resource monitor reports the Envoy process the CPU Utilization of the entire host.
+// The CPU utilization resource monitor reports the Envoy process the,


I'm having trouble parsing this sentence. Can you please clarify?

markdroth · 2025-01-06T16:41:20Z

api/envoy/extensions/resource_monitors/cpu_utilization/v3/cpu_utilization.proto

@@ -13,7 +14,16 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 // [#protodoc-title: CPU utilization]
 // [#extension: envoy.resource_monitors.cpu_utilization]

-// The CPU utilization resource monitor reports the Envoy process the CPU Utilization of the entire host.
+// The CPU utilization resource monitor reports the Envoy process the,
+// CPU Utilization of the entire host if HOST mode is selected, reports


Please document the enum values right above where each one is defined, rather than in a comment above the entire message.

KBaichoo

Thanks for working on simplifying this, this looks much better.

/wait

source/extensions/resource_monitors/cpu_utilization/cpu_stats_reader.h

source/extensions/resource_monitors/cpu_utilization/cpu_utilization_monitor.cc

KBaichoo · 2025-01-06T18:31:39Z

source/extensions/resource_monitors/cpu_utilization/cpu_stats_reader.h

@@ -22,13 +22,26 @@ struct CpuTimes {
  uint64_t total_time;
 };

+struct CgroupStats {


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.

@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.

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.

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-1183c2c3937672e9d4c85d700d1e54ef13d360b4b517a0c643a8e46c13c3eb79R120

we 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.

@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?

I didn't want to try unnecessary derived calculations and lose accuracy. Cputimes would need to have then double data type instead of integer. But let me see this once in daytime. These two fields I couldn't merge logically and definition wise initially that's why I started with another unique resource monitor initially.

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.

have checked the calculation strategy, It should work. But trying now to pushdown timesource from context in stats reader.

@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 ?

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.

Have implemented all tests and functionality using proc/uptime. @KBaichoo . This should be sufficient for us.

source/extensions/resource_monitors/cpu_utilization/linux_cpu_stats_reader.cc

KBaichoo · 2025-01-06T18:37:14Z

source/extensions/resource_monitors/cpu_utilization/linux_cpu_stats_reader.h

@@ -20,6 +22,16 @@ class LinuxCpuStatsReader : public CpuStatsReader {
  const std::string cpu_stats_filename_;
 };

+class LinuxContainerCpuStatsReader: public CgroupStatsReader {


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.

source/extensions/resource_monitors/cpu_utilization/cpu_utilization_monitor.h

Signed-off-by: nix1n <[email protected]>

KBaichoo

/wait

KBaichoo · 2025-01-07T22:48:08Z

source/extensions/resource_monitors/cpu_utilization/cpu_utilization_monitor.h

@@ -35,7 +35,7 @@ class CpuUtilizationMonitor : public Server::ResourceMonitor {
  std::unique_ptr <CgroupStatsReader> cgroup_stats_reader_;
  TimeSource& time_source_;
  MonotonicTime last_update_time_;
-  envoy::extensions::resource_monitors::cpu_utilization::v3::CpuUtilizationConfig_UtilizationComputeStrategy mode_;
+  int16_t mode_ = -1; // Will be updated in Resource Monitor Class Constructor


to be more specific I meant to use the non-mangled named like:

envoy::extensions::resource_monitors::cpu_utilization::v3::CpuUtilizationConfig::UtilizationComputeStrategy

I did try this earlier already, it won't build. Previous two ways were only compiling successfully for me. If we use this we have to use default in switch statement then only it builds which you said to avoid.

we can try something like this then, container is specific mode. rest for default computeHostUsage()

KBaichoo · 2025-01-07T23:12:05Z

source/extensions/resource_monitors/cpu_utilization/cpu_stats_reader.h

@@ -22,13 +22,26 @@ struct CpuTimes {
  uint64_t total_time;
 };

+struct CgroupStats {


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-1183c2c3937672e9d4c85d700d1e54ef13d360b4b517a0c643a8e46c13c3eb79R120

we 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.

Signed-off-by: nix1n <[email protected]>

nix1n

/ready

Signed-off-by: nix1n <[email protected]>

repokitteh-read-only bot added the api label Dec 23, 2024

repokitteh-read-only bot assigned markdroth Dec 23, 2024

nix1n added 4 commits December 23, 2024 15:08

Add support for Container CPU Utilization Resource Monitor for loadhs…

8d1863c

…edding in K8s environment Signed-off-by: nix1n <[email protected]>

remove trailing whitespaces & add last newline

8c24c52

Signed-off-by: nix1n <[email protected]>

correct doc reference

3de82b3

Signed-off-by: nix1n <[email protected]>

fix changelog yamllint character limit

174111d

Signed-off-by: nix1n <[email protected]>

nix1n force-pushed the main branch from cc1471e to 174111d Compare December 23, 2024 09:38

Add CGROUP in dictionary

26d1e99

Signed-off-by: nix1n <[email protected]>

KBaichoo self-assigned this Dec 23, 2024

Merge branch 'envoyproxy:main' into main

59f6dd3

KBaichoo reviewed Dec 24, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 24, 2024

Merge branch 'envoyproxy:main' into main

b607d8c

repokitteh-read-only bot removed the waiting label Dec 30, 2024

nix1n added 2 commits January 1, 2025 14:06

add container cpu utilization type monitor

4315e3f

Signed-off-by: nix1n <[email protected]>

add myself as codeowner in cpu_utilization

354bec1

Signed-off-by: nix1n <[email protected]>

markdroth reviewed Jan 3, 2025

View reviewed changes

nix1n and others added 3 commits January 6, 2025 15:06

correct proto config message

beafb4b

Signed-off-by: nix1n <[email protected]>

Merge branch 'envoyproxy:main' into main

b6a619b

docs and changelogs update

e623cf4

Signed-off-by: nix1n <[email protected]>

markdroth reviewed Jan 6, 2025

View reviewed changes

KBaichoo reviewed Jan 6, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 6, 2025

correct mode initialisation,and proto comments

8979e8b

Signed-off-by: nix1n <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 7, 2025

refactor cpu stats error handling

060118f

Signed-off-by: nix1n <[email protected]>

KBaichoo reviewed Jan 7, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 7, 2025

change time diff calculation strategy without timesource

a32c1f6

Signed-off-by: nix1n <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 8, 2025

nix1n and others added 6 commits January 8, 2025 17:41

Merge branch 'main' into main

ccd9456

Signed-off-by: nix1n <[email protected]>

fix test header

3d7bbd1

Signed-off-by: nix1n <[email protected]>

fix proto message comments

153a2a8

Signed-off-by: nix1n <[email protected]>

fix proto message comments

ca96425

Signed-off-by: nix1n <[email protected]>

report error from container cpu usage monitor

114f444

Signed-off-by: nix1n <[email protected]>

add tests for missing cases

b93f3d8

Signed-off-by: nix1n <[email protected]>

nix1n commented Jan 9, 2025

View reviewed changes

nix1n and others added 3 commits January 9, 2025 14:48

Simplify Monitor class implementaion

346cb53

Signed-off-by: nix1n <[email protected]>

Merge branch 'envoyproxy:main' into main

9f90f9c

add missing library in BUIL

64320f4

Signed-off-by: nix1n <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Container CPU Utilization Resource Monitor #37792

Add support for Container CPU Utilization Resource Monitor #37792

nix1n commented Dec 23, 2024

repokitteh-read-only bot commented Dec 23, 2024

repokitteh-read-only bot commented Dec 23, 2024

KBaichoo left a comment

KBaichoo Dec 24, 2024

nix1n Jan 1, 2025

KBaichoo Dec 24, 2024

KBaichoo commented Dec 24, 2024

markdroth Jan 3, 2025

nix1n Jan 6, 2025

markdroth Jan 6, 2025

markdroth Jan 6, 2025

KBaichoo left a comment

KBaichoo Jan 6, 2025

nix1n Jan 7, 2025

nix1n Jan 7, 2025

KBaichoo Jan 7, 2025

nix1n Jan 7, 2025

nix1n Jan 7, 2025

nix1n Jan 8, 2025

nix1n Jan 8, 2025

nix1n Jan 8, 2025

nix1n Jan 8, 2025

KBaichoo Jan 6, 2025

KBaichoo left a comment

KBaichoo Jan 7, 2025

nix1n Jan 8, 2025

nix1n Jan 8, 2025

nix1n Jan 8, 2025

KBaichoo Jan 7, 2025

nix1n left a comment

		namespace EnvoyContainerCpuUtilizationMonitor {


		struct EnvoyContainerStats {

Add support for Container CPU Utilization Resource Monitor #37792

Are you sure you want to change the base?

Add support for Container CPU Utilization Resource Monitor #37792

Conversation

nix1n commented Dec 23, 2024

repokitteh-read-only bot commented Dec 23, 2024

repokitteh-read-only bot commented Dec 23, 2024

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ref: google/cadvisor#2026 (comment)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nix1n left a comment

Choose a reason for hiding this comment