SKIP cgroup e2e tests if resource controllers not available when test check - #3672
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Moderate cleanup and skip-path issues remain, along with comment clarity fixes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds memory-controller availability checks to cgroup E2E tests and skips unsupported SUSE image variants.
Changes:
- Centralizes controller checks, skip handling, and cleanup.
- Applies the guard to memory quota and self-monitoring tests.
- Adds cloud-specific SUSE 15 suite exclusions.
File summaries
| File | Changes | Review findings |
|---|---|---|
tests_e2e/tests/scripts/agent_memory_self_monitor-check_agent_self_monitoring.py |
Adds the shared controller precondition. | None. |
tests_e2e/tests/scripts/agent_memory_quota-check_agent_memory_quota.py |
Uses the shared skip helper. | None. |
tests_e2e/tests/lib/cgroup_helpers.py |
Adds shared controller validation and cleanup. | Moderate findings: reuse the shared helper for normal memory-quota cleanup (2 votes); report controller-check timeout/availability accurately (1 vote); avoid failing cleanup before reaching the skip path (1 vote). Nit: correct helper comment wording (2 votes). |
tests_e2e/test_suites/agent_cgroups.yml |
Skips affected cloud/image combinations. | Nit: clarify the comment’s grammar and that the entire suite is skipped (2 votes). |
Review details
Suppressed comments (2)
tests_e2e/tests/lib/cgroup_helpers.py:287
- The check can also fail because the controller was still not populated after the retry window, not because the distro lacks the controller; the new skip message therefore misdiagnoses the condition. Report the timeout/availability check instead so skipped runs are actionable.
raise TestSkipped("The distro does not have Memory controller enabled. Skipping the test.")
tests_e2e/tests/lib/cgroup_helpers.py:287
- When neither controller is available, restarting the agent leaves
Agent cgroups enabled: False, socleanup_cgroups_test_setup()callsfail()and raisesAssertionErrorbefore thisTestSkippedis reached. That reports the unsupported environment as a failed remote test instead of a skip; make cleanup best-effort or avoid requiring cgroups to be enabled on this skip path.
found: bool = retry_if_false(lambda: verify_controllers_available(["memory"]), delay=60)
if not found:
cleanup_cgroups_test_setup()
raise TestSkipped("The distro does not have Memory controller enabled. Skipping the test.")
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def cleanup_cgroups_test_setup(): | ||
| log.info("Cleaning up test setup") | ||
| drop_in_file = os.path.join(systemd.get_agent_drop_in_path(), "99-ExecStart.conf") | ||
| if os.path.exists(drop_in_file): | ||
| log.info("Removing %s...", drop_in_file) |
There was a problem hiding this comment.
🟡 Changes recommended
Moderate review findings remain in the shared helper and cleanup logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
tests_e2e/tests/lib/cgroup_helpers.py:245
- This new shared helper duplicates the existing
cleanup_test_setup()implementation inagent_memory_quota-check_agent_memory_quota.py(lines 148-161). Keeping two copies means drop-in cleanup and agent-restart behavior can diverge; have the quota script delegate to this helper instead of retaining the duplicate.
def cleanup_cgroups_test_setup():
log.info("Cleaning up test setup")
drop_in_file = os.path.join(systemd.get_agent_drop_in_path(), "99-ExecStart.conf")
if os.path.exists(drop_in_file):
log.info("Removing %s...", drop_in_file)
tests_e2e/tests/lib/cgroup_helpers.py:283
- The new comment has a grammatical error:
its okshould use the possessive contractionit's okay.
# Memory controller only used for report memory metrics. If the controller is not enabled, its ok to skip the test.
tests_e2e/tests/lib/cgroup_helpers.py:284
- Please preserve the 120-second retry delay used by the previous memory-controller check. With the default five attempts,
delay=60gives up after four minutes even though the helper documents that cgroup v2 controllers can be populated late, causing a transiently delayed controller to be incorrectly classified as unavailable and the test to be skipped.
found: bool = retry_if_false(lambda: verify_controllers_available(["memory"]), delay=60)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| found: bool = retry_if_false(lambda: check_log_message(" Agent cgroups enabled: True", after_timestamp=check_time)) | ||
| if not found: | ||
| fail("Agent cgroups not enabled yet") |
fb406ec to
f4a4d85
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A cleanup path can fail instead of skipping when both controllers are unavailable, and the suite comment needs clarification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tests_e2e/test_suites/agent_cgroups.yml:17
- The new comment is grammatically incomplete and refers to a single test even though
skip_on_imagesskips the whole suite. Please correct it for clarity.
# The cpu and memory controllers not populated within the test timeout window, so skip running test in those distros.
tests_e2e/tests/lib/cgroup_helpers.py:283
- This rationale is no longer accurate now that the helper is shared by the self-monitoring test: that test uses the memory controller's metrics to enforce and validate the anonymous-memory breach flow, not just to report metrics. Please update the comment to describe the controller as a prerequisite for the memory tests.
# The memory controller is only used for reporting memory metrics. If it is not enabled, it is okay to skip the test.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| # The memory controller is only used for reporting memory metrics. If it is not enabled, it is okay to skip the test. | ||
| found: bool = retry_if_false(lambda: verify_controllers_available(["memory"]), delay=60) | ||
| if not found: | ||
| cleanup_cgroups_test_setup() |
There was a problem hiding this comment.
🔵 Needs a closer look
Resolve the outstanding cleanup-path and controller-wait issues before approval.
Review details
Suppressed comments (5)
tests_e2e/test_suites/agent_cgroups.yml:17
- This newly added comment is grammatically incomplete and uses inconsistent capitalization, which makes the skip rationale less clear.
# The cpu and memory controllers not populated within the test timeout window, so skip running test in those distros.
tests_e2e/tests/lib/cgroup_helpers.py:284
- This shortens the existing memory-quota controller wait from 120 seconds between five attempts (about eight minutes) to 60 seconds (about four minutes). Since
verify_controllers_availableis specifically retrying the cgroup-v2 root-population race, a controller that appears during the previous 4–8 minute window will now cause a false skip; please preserve the prior delay or otherwise retain the intended wait window.
found: bool = retry_if_false(lambda: verify_controllers_available(["memory"]), delay=60)
tests_e2e/tests/lib/cgroup_helpers.py:283
- This rationale is no longer accurate now that the helper is also called by the self-monitor test:
CGroupConfigurator.check_agent_memory_usage()reads the memory controller's anonymous-memory counter to decide whether to self-restart, not only to publish metrics. Please update the comment to cover both memory-dependent behaviors.
# The memory controller is only used for reporting memory metrics. If it is not enabled, it is okay to skip the test.
tests_e2e/tests/lib/cgroup_helpers.py:252
- The new memory-controller skip path calls this cleanup helper, but the leading space in the search pattern does not match the parsed
AgentLogRecord.messageemitted by the agent (Agent cgroups enabled: True). Cleanup therefore reports a failure instead of propagatingTestSkipped, turning the intended skip into a failed remote test. Use the same pattern as the other startup checks.
found: bool = retry_if_false(lambda: check_log_message(" Agent cgroups enabled: True", after_timestamp=check_time))
tests_e2e/tests/lib/cgroup_helpers.py:287
- When the controller check times out because the resource controllers are not enabled at all,
cleanup_cgroups_test_setup()restarts the agent and then requires anAgent cgroups enabled: Truelog entry (lines 252-254). The agent will instead logFalse, so this cleanup raises beforeTestSkippedand the new memory self-monitor path is reported as failed rather than skipped. Make cleanup best-effort for this precondition path, or separate drop-in cleanup from the enabled-state assertion so the intended skip is preserved.
found: bool = retry_if_false(lambda: verify_controllers_available(["memory"]), delay=60)
if not found:
cleanup_cgroups_test_setup()
raise TestSkipped("The distro does not have Memory controller enabled. Skipping the test.")
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
9c1f61f
into
Azure:develop
Description
Issue #
PR information
developbranch.Quality of Code and Contribution Guidelines
Distro maintenance information, if applicable