OCPBUGS-100115: E2E: LLC: Restore uncore cache annotation to true

[mrniranjan]

Earlier in test_id: 77724 we disaled prefer-align-cpus-by-uncorecache but never enabled it back causing further tests to always run with prefer-align-cpus-by-uncorecache disabled

Add a deferCleanup so that prefer-align-cpus-by-uncorecache is re-enabled

Summary by CodeRabbit

• Bug Fixes
  • Improved end-to-end test cleanup to restore the cluster’s original CPU/performance configuration after the test completes.
  • Added additional checks during cleanup to ensure configuration updates complete successfully and the cluster returns to the prior tuning state.

[mrniranjan]

Thanks to @oblau for catching this

[coderabbitai]

Walkthrough

The LLC policy test now defers restoration of the original performance profile annotation, updates the profile, and waits for tuning to return to the updated state after the test completes.

Changes

LLC policy test cleanup

Layer / File(s)	Summary
Restore LLC policy after test test/e2e/performanceprofile/functests/13_llc/llc.go	Deferred cleanup restores the original prefer-align-cpus-by-uncorecache annotation, updates the performance profile with retry, and waits for tuning transitions to complete.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: yanirq, jmencak

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	No new Ginkgo titles were added or changed; the file’s It/When/Context strings are static and deterministic.
Test Structure And Quality	✅ Passed	PASS: The test remains single-purpose, adds DeferCleanup to restore the profile, and its waits go through helpers with bounded timeouts.
Microshift Test Compatibility	✅ Passed	No new Ginkgo test was added; the change only adds cleanup inside an existing test and doesn’t introduce new MicroShift-incompatible APIs.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo test was added; the existing case only updates profile annotations and checks worker nodes, with no multi-node or HA-only assumption.
Topology-Aware Scheduling Compatibility	✅ Passed	Test-only DeferCleanup restores a PerformanceProfile annotation; no manifests, operators, or controller scheduling constraints were added.
Ote Binary Stdout Contract	✅ Passed	The change only adds DeferCleanup inside an It block; no process-level stdout writes were added, and suite logging uses GinkgoWriter not stdout.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The change only adds cleanup to restore a profile annotation; it adds no IPv4 literals, IP parsing, or external connectivity calls.
No-Weak-Crypto	✅ Passed	The changed test code only adds a cleanup restoring kubelet annotations; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons were found.
Container-Privileges	✅ Passed	The PR only adds Go test cleanup logic; no container/K8s manifests or privileged settings (privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, allowPrivilegeEscalation) were introduced.
No-Sensitive-Data-In-Logs	✅ Passed	The only new log is a status message in the cleanup; the changed file contains no passwords, tokens, PII, or other sensitive data logs.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: restoring the uncore cache annotation after the E2E test.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-for-rh-openshift]

PR Summary by Qodo

E2E LLC: restore uncore cache alignment annotation after disabling test

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Add scoped cleanup to re-enable uncore-cache CPU alignment after test_id:77724.
• Prevent later LLC tests from inheriting a disabled kubelet CPUManager policy option.
• Wait for PerformanceProfile tuning rollout during cleanup to ensure cluster state is restored.

Diagram

graph TD
  T["LLC E2E test 77724"] --> D["Set option false"] --> P["Update PerformanceProfile"] --> R["Wait tuning rollout"] --> K["Kubelet policy opts"]
  T --> C["DeferCleanup restore true"] --> P

Loading

High-Level Assessment

Using Ginkgo DeferCleanup scoped to the test that mutates cluster-wide PerformanceProfile state is the most reliable approach; it guarantees restoration even on failures and prevents cross-test contamination. Alternatives like relying on AfterAll or a separate follow-up test are less safe because they allow intermediate tests to run with the wrong kubelet option.

Files changed (1) +16 / -0

Tests (1) +16 / -0

llc.goAdd DeferCleanup to restore prefer-align-cpus-by-uncorecache after disabling test +16/-0

Add DeferCleanup to restore prefer-align-cpus-by-uncorecache after disabling test

• Adds a DeferCleanup to reapply the default LLC policy annotation (prefer-align-cpus-by-uncorecache=true) after test_id:77724 sets it to false. The cleanup updates the PerformanceProfile and waits for tuning updates to start and complete so subsequent tests see restored kubelet configuration.

test/e2e/performanceprofile/functests/13_llc/llc.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/e2e/performanceprofile/functests/13_llc/llc.go`:
- Line 274: Move the DeferCleanup registration in the LLC profile test before
the first profile mutation, ensuring it is established before the update, tuning
waits, and kubelet assertions. Keep the cleanup behavior unchanged so failures
during those steps still restore prefer-align-cpus-by-uncorecache.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 396756f3-c26b-49d1-834c-fa957854e2a9

📥 Commits

Reviewing files that changed from the base of the PR and between a9d25d5 and aa7d9e1.

📒 Files selected for processing (1)

• test/e2e/performanceprofile/functests/13_llc/llc.go

[qodo-for-rh-openshift]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Cleanup registered too late ✓ Resolved 🐞 Bug ☼ Reliability

Description

In test_id:77724, the DeferCleanup that restores prefer-align-cpus-by-uncorecache is
registered only after profile rollout waits and kubelet assertions, so any failure before the
registration point will skip cleanup and can leave the option disabled for subsequent tests.

Code

test/e2e/performanceprofile/functests/13_llc/llc.go[R274-288]

+				DeferCleanup(func(cleanupCtx context.Context) {
+					By("Restoring prefer-align-cpus-by-uncorecache to true")
+					profileAnnotations["kubeletconfig.experimental"] = llcPolicy
+					perfProfile, err = profiles.GetByNodeLabels(testutils.NodeSelectorLabels)
+					Expect(err).ToNot(HaveOccurred())
+					perfProfile.Annotations = profileAnnotations
+
+					profiles.UpdateWithRetry(perfProfile)
+
+					By(fmt.Sprintf("Applying changes in performance profile and waiting until %s will start updating", poolName))
+					profilesupdate.WaitForTuningUpdating(cleanupCtx, perfProfile)
+
+					By(fmt.Sprintf("Waiting when %s finishes updates", poolName))
+					profilesupdate.WaitForTuningUpdated(cleanupCtx, perfProfile)
+				})

Relevance

⭐⭐⭐ High

LLC tests already refactored for reliable DeferCleanup; late registration contradicts that pattern
and likely will be fixed.

PR-#1562
PR-#1382

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cleanup registration happens only after a loop of kubelet config assertions; if any
Expect(...) in the loop fails, the test aborts before registering cleanup. Other tests in the same
file register DeferCleanup immediately after creating resources, demonstrating the intended
pattern for reliable cleanup.

test/e2e/performanceprofile/functests/13_llc/llc.go[253-288]
test/e2e/performanceprofile/functests/13_llc/llc.go[359-378]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The cleanup that restores `prefer-align-cpus-by-uncorecache` is registered at the end of the test, after operations/`Expect(...)` calls that can fail early. If any of those steps fails, execution aborts before `DeferCleanup(...)` is reached, and the cluster may remain with `prefer-align-cpus-by-uncorecache=false`.

### Issue Context
This test mutates a shared cluster-level PerformanceProfile. Cleanup must be registered *immediately after* deciding to mutate the profile (or right after applying the mutation) so it runs even when the test fails mid-way.

### Fix Focus Areas
- test/e2e/performanceprofile/functests/13_llc/llc.go[253-288]

### Suggested fix
1. Move the `DeferCleanup(func(cleanupCtx context.Context) { ... })` block to **immediately after** setting the disable annotation (and before `UpdateWithRetry`, waits, and kubelet assertions).
2. Optionally capture the previous annotation value at the start of the test (or fetch it from the API server) and restore that exact value in cleanup (more robust than hard-coding `llcPolicy`).
3. (Optional) Use `profilesupdate.ApplyProfileAndWait(cleanupCtx, perfProfile)` inside cleanup to reduce duplication and keep update+wait behavior consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[ffromani]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, mrniranjan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ffromani]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@mrniranjan: This pull request references Jira Issue OCPBUGS-100115, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Earlier in test_id: 77724 we disaled prefer-align-cpus-by-uncorecache but never enabled it back causing further tests to always run with prefer-align-cpus-by-uncorecache disabled

Add a deferCleanup so that prefer-align-cpus-by-uncorecache is re-enabled

Summary by CodeRabbit

• Bug Fixes
• Improved end-to-end test cleanup to restore the cluster’s original CPU/performance configuration after the test completes.
• Added additional checks during cleanup to ensure configuration updates complete successfully and the cluster returns to the prior tuning state.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[mrniranjan]

/retest-required

[mrniranjan]

/verified by @mrniranjan

[openshift-ci-robot]

@mrniranjan: This PR has been marked as verified by @mrniranjan.

Details

In response to this:

/verified by @mrniranjan

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD a9d25d5 and 2 for PR HEAD 1d1c403 in total

[openshift-ci]

@mrniranjan: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-nto-ext	1d1c403	link	false	/test e2e-aws-nto-ext
ci/prow/e2e-hypershift-pao	1d1c403	link	true	/test e2e-hypershift-pao
ci/prow/e2e-gcp-pao	1d1c403	link	true	/test e2e-gcp-pao

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.