diff --git a/src/shared/metadata/cgroup_path_resolver.cc b/src/shared/metadata/cgroup_path_resolver.cc index 6086ef69417..bf71cb9a2b9 100644 --- a/src/shared/metadata/cgroup_path_resolver.cc +++ b/src/shared/metadata/cgroup_path_resolver.cc @@ -27,12 +27,14 @@ #include "src/common/fs/fs_wrapper.h" #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(test_only_force_cgroup2_mode, false, + "Flag to force assume cgroup2 fs for testing purposes"); namespace px { namespace md { -StatusOr CGroupBasePath(std::string_view sysfs_path) { +StatusOr> CGroupBasePaths(std::string_view sysfs_path) { + std::vector base_paths; // Different hosts may mount different cgroup dirs. Try a couple for robustness. const std::vector cgroup_dirs = {"cpu,cpuacct", "cpu", "pids"}; @@ -43,7 +45,7 @@ StatusOr CGroupBasePath(std::string_view sysfs_path) { std::string base_path = absl::StrCat(sysfs_path, "/cgroup/", cgroup_dir); if (fs::Exists(base_path)) { - return base_path; + base_paths.push_back(base_path); } } @@ -52,12 +54,18 @@ StatusOr CGroupBasePath(std::string_view sysfs_path) { auto fs_status = statfs(cgv2_base_path.c_str(), &info); bool cgroupv2 = (fs_status == 0) && (info.f_type == CGROUP2_SUPER_MAGIC); - if (cgroupv2 || FLAGS_force_cgroup2_mode) { - return cgv2_base_path; + if (cgroupv2 || FLAGS_test_only_force_cgroup2_mode) { + if (FLAGS_test_only_force_cgroup2_mode) { + return std::vector{cgv2_base_path}; + } + base_paths.push_back(cgv2_base_path); } // (TODO): This check for cgroup2FS is eventually to be moved above the cgroupv1 check. - return error::NotFound("Could not find CGroup base path"); + if (base_paths.empty()) { + return error::NotFound("Could not find CGroup base path"); + } + return base_paths; } StatusOr FindSelfCGroupProcs(std::string_view base_path) { @@ -137,17 +145,32 @@ StatusOr CreateCGroupTemplateSpecFromPath(std::string_view p } StatusOr AutoDiscoverCGroupTemplate(std::string_view sysfs_path) { - PX_ASSIGN_OR_RETURN(std::string base_path, CGroupBasePath(sysfs_path)); - LOG(INFO) << "Auto-discovered CGroup base path: " << base_path; - - PX_ASSIGN_OR_RETURN(std::string self_cgroup_procs, FindSelfCGroupProcs(base_path)); - LOG(INFO) << "Auto-discovered example path: " << self_cgroup_procs; - - PX_ASSIGN_OR_RETURN(CGroupTemplateSpec cgroup_path_template, - CreateCGroupTemplateSpecFromPath(self_cgroup_procs)); - LOG(INFO) << "Auto-discovered template: " << cgroup_path_template.templated_path; + PX_ASSIGN_OR_RETURN(std::vector base_paths, CGroupBasePaths(sysfs_path)); + for (const auto& base_path : base_paths) { + LOG(INFO) << "Auto-discovered CGroup base path: " << base_path; + + auto self_cgroup_procs_status = FindSelfCGroupProcs(base_path); + if (!self_cgroup_procs_status.ok()) { + LOG(WARNING) << "Could not find self in cgroup procs. Trying next base path."; + continue; + } + auto self_cgroup_procs = self_cgroup_procs_status.ConsumeValueOrDie(); + LOG(INFO) << "Auto-discovered example path: " << self_cgroup_procs; + + auto cgroup_path_template_status = CreateCGroupTemplateSpecFromPath(self_cgroup_procs); + if (!cgroup_path_template_status.ok()) { + LOG(WARNING) << absl::Substitute( + "Failed to create cgroup template spec from path $0. Trying next base path.", + self_cgroup_procs); + continue; + } + auto cgroup_path_template = cgroup_path_template_status.ConsumeValueOrDie(); + LOG(INFO) << "Auto-discovered template: " << cgroup_path_template.templated_path; - return cgroup_path_template; + return cgroup_path_template; + } + return error::NotFound("Unable to auto discover cgroup template from $0", + absl::StrJoin(base_paths, ", ")); } StatusOr> CGroupPathResolver::Create( @@ -222,51 +245,52 @@ Status LegacyCGroupPathResolver::Init(std::string_view sysfs_path) { // $2 = container runtime // These template parameters are resolved by calls to PodPath. // Different hosts may mount different cgroup dirs. Try a couple for robustness. - PX_ASSIGN_OR_RETURN(std::string cgroup_dir, CGroupBasePath(sysfs_path)); - - // Attempt assuming naming scheme #1. - std::string cgroup_kubepods_base_path = absl::Substitute("$0/kubepods", cgroup_dir); - if (fs::Exists(cgroup_kubepods_base_path)) { - cgroup_kubepod_guaranteed_path_template_ = - absl::StrCat(cgroup_kubepods_base_path, "/pod$0/$1/cgroup.procs"); - cgroup_kubepod_besteffort_path_template_ = - absl::StrCat(cgroup_kubepods_base_path, "/besteffort/pod$0/$1/cgroup.procs"); - cgroup_kubepod_burstable_path_template_ = - absl::StrCat(cgroup_kubepods_base_path, "/burstable/pod$0/$1/cgroup.procs"); - cgroup_kubepod_convert_dashes_ = false; - return Status::OK(); - } + PX_ASSIGN_OR_RETURN(std::vector cgroup_dirs, CGroupBasePaths(sysfs_path)); - // Attempt assuming naming scheme #3. - // Must be before the scheme below, since there have been systems that have both paths, - // but this must take priority. - cgroup_kubepods_base_path = absl::Substitute("$0/system.slice/containerd.service", cgroup_dir); - if (fs::Exists(cgroup_kubepods_base_path)) { - cgroup_kubepod_guaranteed_path_template_ = - absl::StrCat(cgroup_kubepods_base_path, "/kubepods-pod$0.slice:$2:$1/cgroup.procs"); - cgroup_kubepod_besteffort_path_template_ = absl::StrCat( - cgroup_kubepods_base_path, "/kubepods-besteffort-pod$0.slice:$2:$1/cgroup.procs"); - cgroup_kubepod_burstable_path_template_ = absl::StrCat( - cgroup_kubepods_base_path, "/kubepods-burstable-pod$0.slice:$2:$1/cgroup.procs"); - cgroup_kubepod_convert_dashes_ = true; - return Status::OK(); - } + for (const auto& cgroup_dir : cgroup_dirs) { + // Attempt assuming naming scheme #1. + std::string cgroup_kubepods_base_path = absl::Substitute("$0/kubepods", cgroup_dir); + if (fs::Exists(cgroup_kubepods_base_path)) { + cgroup_kubepod_guaranteed_path_template_ = + absl::StrCat(cgroup_kubepods_base_path, "/pod$0/$1/cgroup.procs"); + cgroup_kubepod_besteffort_path_template_ = + absl::StrCat(cgroup_kubepods_base_path, "/besteffort/pod$0/$1/cgroup.procs"); + cgroup_kubepod_burstable_path_template_ = + absl::StrCat(cgroup_kubepods_base_path, "/burstable/pod$0/$1/cgroup.procs"); + cgroup_kubepod_convert_dashes_ = false; + return Status::OK(); + } - // Attempt assuming naming scheme #2. - cgroup_kubepods_base_path = absl::Substitute("$0/kubepods.slice", cgroup_dir); - if (fs::Exists(cgroup_kubepods_base_path)) { - cgroup_kubepod_guaranteed_path_template_ = - absl::StrCat(cgroup_kubepods_base_path, "/kubepods-pod$0.slice/$2-$1.scope/cgroup.procs"); - cgroup_kubepod_besteffort_path_template_ = absl::StrCat( - cgroup_kubepods_base_path, - "/kubepods-besteffort.slice/kubepods-besteffort-pod$0.slice/$2-$1.scope/cgroup.procs"); - cgroup_kubepod_burstable_path_template_ = absl::StrCat( - cgroup_kubepods_base_path, - "/kubepods-burstable.slice/kubepods-burstable-pod$0.slice/$2-$1.scope/cgroup.procs"); - cgroup_kubepod_convert_dashes_ = true; - return Status::OK(); - } + // Attempt assuming naming scheme #3. + // Must be before the scheme below, since there have been systems that have both paths, + // but this must take priority. + cgroup_kubepods_base_path = absl::Substitute("$0/system.slice/containerd.service", cgroup_dir); + if (fs::Exists(cgroup_kubepods_base_path)) { + cgroup_kubepod_guaranteed_path_template_ = + absl::StrCat(cgroup_kubepods_base_path, "/kubepods-pod$0.slice:$2:$1/cgroup.procs"); + cgroup_kubepod_besteffort_path_template_ = absl::StrCat( + cgroup_kubepods_base_path, "/kubepods-besteffort-pod$0.slice:$2:$1/cgroup.procs"); + cgroup_kubepod_burstable_path_template_ = absl::StrCat( + cgroup_kubepods_base_path, "/kubepods-burstable-pod$0.slice:$2:$1/cgroup.procs"); + cgroup_kubepod_convert_dashes_ = true; + return Status::OK(); + } + // Attempt assuming naming scheme #2. + cgroup_kubepods_base_path = absl::Substitute("$0/kubepods.slice", cgroup_dir); + if (fs::Exists(cgroup_kubepods_base_path)) { + cgroup_kubepod_guaranteed_path_template_ = + absl::StrCat(cgroup_kubepods_base_path, "/kubepods-pod$0.slice/$2-$1.scope/cgroup.procs"); + cgroup_kubepod_besteffort_path_template_ = absl::StrCat( + cgroup_kubepods_base_path, + "/kubepods-besteffort.slice/kubepods-besteffort-pod$0.slice/$2-$1.scope/cgroup.procs"); + cgroup_kubepod_burstable_path_template_ = absl::StrCat( + cgroup_kubepods_base_path, + "/kubepods-burstable.slice/kubepods-burstable-pod$0.slice/$2-$1.scope/cgroup.procs"); + cgroup_kubepod_convert_dashes_ = true; + return Status::OK(); + } + } return error::NotFound("Could not find kubepods slice under sysfs ($0)", sysfs_path); } diff --git a/src/shared/metadata/cgroup_path_resolver.h b/src/shared/metadata/cgroup_path_resolver.h index b3aca9a5ea9..fd123ac981b 100644 --- a/src/shared/metadata/cgroup_path_resolver.h +++ b/src/shared/metadata/cgroup_path_resolver.h @@ -27,7 +27,7 @@ #include "src/common/system/system.h" #include "src/shared/metadata/k8s_objects.h" -DECLARE_bool(force_cgroup2_mode); +DECLARE_bool(test_only_force_cgroup2_mode); namespace px { namespace md { diff --git a/src/shared/metadata/cgroup_path_resolver_test.cc b/src/shared/metadata/cgroup_path_resolver_test.cc index 8046908c80b..9e57b0aa0b0 100644 --- a/src/shared/metadata/cgroup_path_resolver_test.cc +++ b/src/shared/metadata/cgroup_path_resolver_test.cc @@ -316,7 +316,8 @@ TEST(LegacyCGroupPathResolverTest, StandardFormat) { ContainerType::kContainerd)); } -TEST(LeagcyCGroupPathResolverTest, Cgroup2Format) { +TEST(LegacyCGroupPathResolverTest, Cgroup2Format) { + PX_SET_FOR_SCOPE(FLAGS_test_only_force_cgroup2_mode, true); ASSERT_OK_AND_ASSIGN( auto path_resolver, LegacyCGroupPathResolver::Create(GetSysFsPathFromTestDataFile( @@ -326,7 +327,6 @@ TEST(LeagcyCGroupPathResolverTest, Cgroup2Format) { "cgroup.procs", "testdata/sysfs3"))); - FLAGS_force_cgroup2_mode = true; EXPECT_EQ( GetPathToTestDataFile( "testdata/sysfs3/cgroup/kubepods.slice/kubepods-besteffort.slice/" @@ -379,5 +379,16 @@ TEST(CGroupPathResolver, Cgroup2Format) { "docker-a7638fe3934b37419cc56bca73465a02b354ba6e98e10272542d84eb2014dd62.scope/cgroup.procs"); } +/** + * TODO(ddelnano): Refactor the cgroup resolver code so that logic within AutoDiscoverCGroupPath + * and CGroupBasePaths can be tested. OVH's managed k8s failed to work with our logic because + * it enables cgroup v1 and v2 while the PEM existed in a v2 cgroup. Ideally our tests would + * cover the following scenarios for both LegacyCGroupPathResolver and CGroupPathResolver: + * 1. cgroup1 only + * 2. cgroup2 only + * 3. cgroup1+cgroup2 w/ cgroup1 failing (gh#XXX bug) + * 4. cgroup1+cgroup2 w/ cgroup1 succeeding + */ + } // namespace md } // namespace px