From a88e82fa4ab00f336e2575c029e72a4d28c01502 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Thu, 29 Feb 2024 14:20:59 -0800 Subject: [PATCH] Deflake cpuacct cgroup test. - Cgroup::PollControlFileForChange() is only used by cpuacct tests to check that CPU usage for the test's containing cgroups increases over time. In this context, sleeping between checks is counterproductive because the test uses no CPU while sleeping. Remove the sleep. - Don't assume that the root cgroup's usage is initially non-zero, due to granularity issues. PiperOrigin-RevId: 611594418 --- test/syscalls/linux/cgroup.cc | 23 +++++++++++++---------- test/util/cgroup_util.cc | 10 ++++------ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc index 78108df58..2db528f14 100644 --- a/test/syscalls/linux/cgroup.cc +++ b/test/syscalls/linux/cgroup.cc @@ -550,8 +550,11 @@ TEST(CPUAcctCgroup, HierarchicalAccounting) { Cgroup root = Cgroup::RootCgroup("/sys/fs/cgroup/cpuacct"); Cgroup child = ASSERT_NO_ERRNO_AND_VALUE(root.CreateChild("child1")); - // Root should have non-zero CPU usage since the test itself will be running - // in the root cgroup. + // The test starts in the root cgroup, so its CPU usage should be accounted + // there. Since the granularity of cpuacct.usage is unspecified and the test + // may not have run for very long yet, wait for it to be accounted. + ASSERT_NO_ERRNO( + root.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); EXPECT_THAT(root.ReadIntegerControlFile("cpuacct.usage"), IsPosixErrorOkAndHolds(Gt(0))); @@ -564,7 +567,7 @@ TEST(CPUAcctCgroup, HierarchicalAccounting) { ASSERT_NO_ERRNO_AND_VALUE(root.ReadIntegerControlFile("cpuacct.usage")); ASSERT_NO_ERRNO(child.Enter(getpid())); ASSERT_NO_ERRNO( - child.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + child.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); EXPECT_THAT(child.ReadIntegerControlFile("cpuacct.usage"), IsPosixErrorOkAndHolds(Gt(0))); @@ -577,7 +580,7 @@ TEST(CPUAcctCgroup, HierarchicalAccounting) { // Root should continue to gain usage after the move since child is a // subcgroup. ASSERT_NO_ERRNO( - child.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + child.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); EXPECT_THAT(root.ReadIntegerControlFile("cpuacct.usage"), IsPosixErrorOkAndHolds(Ge(after_move))); } @@ -592,7 +595,7 @@ TEST(CPUAcctCgroup, IndirectCharge) { ASSERT_NO_ERRNO(child1.Enter(getpid())); ASSERT_NO_ERRNO( - child1.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + child1.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); // Only root and child1 should have usage. for (auto const& cg : {root, child1}) { @@ -606,7 +609,7 @@ TEST(CPUAcctCgroup, IndirectCharge) { ASSERT_NO_ERRNO(child2a.Enter(getpid())); ASSERT_NO_ERRNO( - child2a.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + child2a.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); const int64_t snapshot_root = ASSERT_NO_ERRNO_AND_VALUE(root.ReadIntegerControlFile("cpuacct.usage")); @@ -618,7 +621,7 @@ TEST(CPUAcctCgroup, IndirectCharge) { child2a.ReadIntegerControlFile("cpuacct.usage")); ASSERT_NO_ERRNO( - child2a.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + child2a.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); // Root, child2 and child2a should've accumulated new usage. Child1 should // not. @@ -647,15 +650,15 @@ TEST(CPUAcctCgroup, NoDoubleAccounting) { ASSERT_NO_ERRNO(a.Enter(getpid())); ASSERT_NO_ERRNO( - a.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + a.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); ASSERT_NO_ERRNO(b.Enter(getpid())); ASSERT_NO_ERRNO( - b.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + b.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); ASSERT_NO_ERRNO(root.Enter(getpid())); ASSERT_NO_ERRNO( - root.PollControlFileForChange("cpuacct.usage", absl::Seconds(60))); + root.PollControlFileForChange("cpuacct.usage", absl::Seconds(5))); // The usage for parent, a & b should now be frozen, since they no longer have // any tasks. Root will continue to accumulate usage. diff --git a/test/util/cgroup_util.cc b/test/util/cgroup_util.cc index ca9bc4d29..090420cd0 100644 --- a/test/util/cgroup_util.cc +++ b/test/util/cgroup_util.cc @@ -93,7 +93,6 @@ PosixError Cgroup::PollControlFileForChange(absl::string_view name, PosixError Cgroup::PollControlFileForChangeAfter( absl::string_view name, absl::Duration timeout, std::function body) const { - const absl::Duration poll_interval = absl::Milliseconds(10); const absl::Time deadline = absl::Now() + timeout; const std::string alias_path = absl::StrFormat("[cg#%d]/%s", id_, name); @@ -107,6 +106,10 @@ PosixError Cgroup::PollControlFileForChangeAfter( // resource usage this function is often waiting for, resulting in timeouts. const DisableSave ds; + std::cerr << absl::StreamFormat( + "Waiting for control file '%s' to change from '%d'...", + alias_path, initial_value) + << std::endl; while (true) { ASSIGN_OR_RETURN_ERRNO(const int64_t current_value, ReadIntegerControlFile(name)); @@ -121,11 +124,6 @@ PosixError Cgroup::PollControlFileForChangeAfter( return PosixError(ETIME, absl::StrCat(alias_path, " didn't change in ", absl::FormatDuration(timeout))); } - std::cerr << absl::StreamFormat( - "Waiting for control file '%s' to change from '%d'...", - alias_path, initial_value) - << std::endl; - absl::SleepFor(poll_interval); } }