Attempt to use cgroup v2 prior to cgroup v1 in path resolver #1978

ddelnano · 2024-08-08T14:13:40Z

Summary: Attempt to use cgroup v2 prior to cgroup v1 in path resolver

For certain environments, like OVH cloud's managed kubernetes service, the auto discovery mechanism fails due to a confusion between cgroup v1 and v2. This is fixed by attempting to resolve the cgroup v2 path before v1. This change also introduces a new flag --force_cgroup1_mode, which can restore the old ordering if this causes any unforeseen issues.

Relevant Issues: N/A

Type of change: /kind bug

Test Plan: Community user deployed this change and verified their OVH MKS cluster is working now

Changelog Message: Update cgroup path resolution to prefer cgroup v2 over v1 to fix an issue with OVH cloud Kubernetes clusters

Signed-off-by: Dom Del Nano <[email protected]>

ddelnano · 2024-08-08T14:17:20Z

src/shared/metadata/cgroup_path_resolver.cc

+  // TODO(ddelnano): this aids in testing the LegacyCGroupPathResolver. Determine what
+  // to do with this.
  if (cgroupv2 || FLAGS_force_cgroup2_mode) {
    return cgv2_base_path;
  }


It's unfortunate that this code branch needs to stay to support the LeagcyCGroupPathResolverTest.Cgroup2Format test. Was the intention to remove the legacy resolver at some point? Is that something that could be considered now?

Alegrowin · 2024-08-08T17:59:35Z

Tested on OVH Cloud - MKS 1.29.3

I20240808 17:36:06.891670 4155112 cgroup_path_resolver.cc:147] Auto-discovered CGroup base path: /sys/fs/cgroup
I20240808 17:36:06.897358 4155112 cgroup_path_resolver.cc:150] Auto-discovered example path: /sys/fs/cgroup/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-pod3e2d9410_d57d_45b1_ab92_d6d7b1741dcd.slice/cri-containerd-6472e8d8acae8bfb89ce35421a93cd34955f2e48a1ce041f4fdf0983e4ecfce4.scope/cgroup.procs
I20240808 17:36:06.897461 4155112 cgroup_path_resolver.cc:154] Auto-discovered template: /sys/fs/cgroup/kubepods.slice/kubepods-$2.slice/kubepods-$2-pod$0.slice/cri-containerd-$1.scope/cgroup.procs
I20240808 17:36:06.897475 4155112 cgroup_metadata_reader.cc:41] Using path_resolver with configuration: template=/sys/fs/cgroup/kubepods.slice/kubepods-$2.slice/kubepods-$2-pod$0.slice/cri-containerd-$1.scope/cgroup.procs pod_id_separators=_ qos_spearator=-
I20240808 17:36:06.899402 4156184 stirling.cc:792] Stirling is running.

oazizi000 · 2024-08-12T21:54:30Z

src/shared/metadata/cgroup_path_resolver.cc

@@ -28,11 +28,21 @@
 #include "src/shared/metadata/cgroup_path_resolver.h"

 DEFINE_bool(force_cgroup2_mode, true, "Flag to force assume cgroup2 fs for testing purposes");
+DEFINE_bool(force_cgroup1_mode, false, "Flag to force use of cgroup v1 fs");


We used to do this more, but using flags for testing is generally error prone, because they persist across tests. I'd recommend finding a way to avoid flags like this. This goes for the force_cgroup2_mode as well, if we're going to try to fix this.

The idea behind this was if it's worth extensively testing the new logic ordering (v2 before v1), then this flag would be used to restore the old behavior in the event there was a distro/system that had issues. Since the tests expected that the check would happen in the previous ordering, it also needed to be used there as well.

oazizi000 · 2024-08-12T21:55:42Z

src/shared/metadata/cgroup_path_resolver.cc


 namespace px {
 namespace md {

 StatusOr<std::string> CGroupBasePath(std::string_view sysfs_path) {
+  std::string cgv2_base_path = absl::StrCat(sysfs_path, "/cgroup");


We need to test extensively if we change this. Systems can have both cgroup and cgroup2 if I recall correctly, and this change has to potential to break things.

Unfortunately the OVH case is one where cgroup v1 and v2 are both present, but our logic fails for it.

I agree that this needs to be tested very carefully. After thinking about this more, I think it will be difficult to be confident that all edge cases are covered even if time is invested in testing the known cases.

I'm going to think about this more and see if there is a safer option for handling the OVH case. If the cgroup v1 code doesn't short circuit and allows reaching the cgroup v2 logic, then testing this reordering can be avoided.

oazizi000 · 2024-08-12T21:56:53Z

src/shared/metadata/cgroup_path_resolver.cc

+  // TODO(ddelnano): this aids in testing the LegacyCGroupPathResolver. Determine what
+  // to do with this.


Should the default value of FLAGS_force_cgroup2_mode be false? I would think that makes more sense. Maybe that helps with cleaning up the tests.

But I also think using flags in tests is error prone, as mentioned above, so we should try to get away from that.

I was worried about the possibility of FLAGS_force_cgroup2_mode's default true value meant this code path could be used in production clusters. In theory, the caller should not parse a cgv2_base_path correctly (since the filesystem check didn't indicate cgroup v2). However, since both v1 and v2 can be active and this OVH issue shows that these cases are complicated, I'm a little worried about removing this.

oazizi000 · 2024-08-12T22:02:00Z

src/shared/metadata/cgroup_path_resolver_test.cc

@@ -32,6 +32,7 @@ constexpr std::string_view kContainerID =
    "a7638fe3934b37419cc56bca73465a02b354ba6e98e10272542d84eb2014dd62";

 TEST(CGroupPathResolver, GKEFormat) {
+  FLAGS_force_cgroup1_mode = true;


If we must set flags in test, then use PX_SET_FOR_SCOPE to make sure it doesn't leak out.

ddelnano · 2024-08-15T23:09:50Z

#1981 is the approach we will be going with. I'm working on addressing the final comments.

Summary: Update cgroup resolver to try all possible base paths For certain environments, like OVH cloud's managed kubernetes service, the auto discovery mechanism fails due to a confusion between cgroup v1 and v2. This is fixed by attempting all cgroup base paths rather than trying the first to match and failing if it doesn't work. This is the second iteration of this change (#1978 was the earlier attempt). Relevant Issues: N/A Type of change: /kind bug Test Plan: Community user deployed this change and verified their OVH MKS cluster is working now --------- Signed-off-by: Dom Del Nano <[email protected]>

Summary: Update cgroup resolver to try all possible base paths For certain environments, like OVH cloud's managed kubernetes service, the auto discovery mechanism fails due to a confusion between cgroup v1 and v2. This is fixed by attempting all cgroup base paths rather than trying the first to match and failing if it doesn't work. This is the second iteration of this change (pixie-io#1978 was the earlier attempt). Relevant Issues: N/A Type of change: /kind bug Test Plan: Community user deployed this change and verified their OVH MKS cluster is working now --------- Signed-off-by: Dom Del Nano <[email protected]> GitOrigin-RevId: ff83032

Attempt to use cgroup v2 prior to cgroup v1 in path resolver

96bf2cb

Signed-off-by: Dom Del Nano <[email protected]>

ddelnano requested a review from a team as a code owner August 8, 2024 14:13

ddelnano commented Aug 8, 2024

View reviewed changes

oazizi000 reviewed Aug 12, 2024

View reviewed changes

ddelnano mentioned this pull request Aug 13, 2024

Update cgroup resolver to try all possible base paths #1981

Merged

ddelnano closed this Aug 15, 2024

ddelnano deleted the ddelnano/update-cgroupv2-ordering branch August 15, 2024 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to use cgroup v2 prior to cgroup v1 in path resolver #1978

Attempt to use cgroup v2 prior to cgroup v1 in path resolver #1978

ddelnano commented Aug 8, 2024

ddelnano Aug 8, 2024

Alegrowin commented Aug 8, 2024 •

edited

Loading

oazizi000 Aug 12, 2024

ddelnano Aug 13, 2024 •

edited

Loading

oazizi000 Aug 12, 2024

ddelnano Aug 13, 2024

oazizi000 Aug 12, 2024

ddelnano Aug 13, 2024

oazizi000 Aug 12, 2024

ddelnano commented Aug 15, 2024

		// TODO(ddelnano): this aids in testing the LegacyCGroupPathResolver. Determine what
		// to do with this.

Attempt to use cgroup v2 prior to cgroup v1 in path resolver #1978

Attempt to use cgroup v2 prior to cgroup v1 in path resolver #1978

Conversation

ddelnano commented Aug 8, 2024

ddelnano Aug 8, 2024

Choose a reason for hiding this comment

Alegrowin commented Aug 8, 2024 • edited Loading

oazizi000 Aug 12, 2024

Choose a reason for hiding this comment

ddelnano Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

oazizi000 Aug 12, 2024

Choose a reason for hiding this comment

ddelnano Aug 13, 2024

Choose a reason for hiding this comment

oazizi000 Aug 12, 2024

Choose a reason for hiding this comment

ddelnano Aug 13, 2024

Choose a reason for hiding this comment

oazizi000 Aug 12, 2024

Choose a reason for hiding this comment

ddelnano commented Aug 15, 2024

Alegrowin commented Aug 8, 2024 •

edited

Loading

ddelnano Aug 13, 2024 •

edited

Loading