e2e: Add housekeeping IRQ load balancing tests #1456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

SargunNarula wants to merge 1 commit into openshift:main from SargunNarula:irq_housekeeping_2

+503 −83

Contributor

SargunNarula commented Jan 15, 2026 •

edited

Loading

Added e2e tests for IRQ load balancing with housekeeping pods:

[86346] Verify housekeeping works correctly with single hyperthread allocation
[86348] Verify irqbalance does not overwrite on TuneD restart
(housekeeping annotation)
[86347] Verify housekeeping selects single CPU when SMT is disabled
Added baseload calculation functions for determining available pod capacity on nodes

Contributor

openshift-ci bot commented Jan 15, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SargunNarula
Once this PR has been reviewed and has the lgtm label, please assign jmencak for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci bot requested review from jmencak and yanirq

January 15, 2026 13:25

Contributor Author

SargunNarula commented Jan 21, 2026

/retest

1 similar comment

Contributor

yanirq commented Jan 25, 2026

/retest

yanirq reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

yanirq reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

SargunNarula force-pushed the irq_housekeeping_2 branch from ff2d905 to bdafc69 Compare

January 27, 2026 09:49

MarSik reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

+              			By("Verifying housekeeping CPUs equal the container's assigned CPU")
+              			Expect(housekeepingCPUSet.Equals(containerCpuSet)).To(BeTrue(),
+              				"Housekeeping CPUs %v should match container CPUs %v for single hyperthread allocation",

Contributor

MarSik Jan 27, 2026

Hmm, this will pass, but only because the container itself has just one cpu available. The same should actually happen with 2 cpus when HT is enabled.

The edge case test is fine, but the description and logs do not accurately represent what is tested here. full-pcpus-only is actually not important for the logic, it just allows you to allocate a single cpu only when HT is enabled.

Contributor Author

SargunNarula Jan 27, 2026

Thanks for pointing this, I have updated the descriptions and logs to reflect hyperthread allocation rather than full-pcpus-only. Let me know if you think any other changes are needed.

MarSik reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

+              		})
+              	})
+              	Context("Verify TuneD restart preserves IRQ configuration for multiple housekeeping pods", Label(string(label.Tier2)), func() {

Contributor

MarSik Jan 27, 2026

I believe we have a similar test for non-housekeeping case, right? Would it make sense to coalesce the code into a single test with a parameter? To avoid code duplication.

Contributor Author

SargunNarula Jan 27, 2026

Thanks for insight, yes we already have a very similar test case. Refactored the test as Describe Table in latest commit.

MarSik reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

+              			smtActive := strings.TrimSpace(testutils.ToString(smtOutput))
+              			Expect(smtActive).To(Equal("0"), "SMT should be disabled (smt/active should be 0)")
+              			By("Creating a housekeeping pod with 1 CPU")

Contributor

MarSik Jan 27, 2026

Use more cpus, otherwise the test never uncovers anything even if the logic breaks.

Contributor Author

SargunNarula Jan 27, 2026

Yes, Indeed. Updated the test.

SargunNarula force-pushed the irq_housekeeping_2 branch from bdafc69 to f663ea0 Compare

January 27, 2026 14:04

MarSik reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

+              			Expect(smtActive).To(Equal("0"), "SMT should be disabled (smt/active should be 0)")
+              			cpuRequest := 2
+              			if cpuRequest > newIsolatedCPUs.Size() {

Contributor

MarSik Jan 27, 2026

This will most likely fail to schedule if you have exactly 2 isolated cpus. There will be some burstable pods as well. You need to check the currently available resources or "assume" some burstable and compare with newIsolatedCPUs - 1.

Contributor

shajmakh Jan 27, 2026

+1

Contributor

shajmakh Jan 27, 2026

you can add a similar helper as there is in NROP:
https://github.com/openshift-kni/numaresources-operator/blob/main/internal/baseload/baseload.go#L37

Contributor Author

SargunNarula Jan 28, 2026

@shajmakh @MarSik Thanks for pointing this, have a look at the baseload helper implementation introduced with latest commit.

shajmakh reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

+              			if currentReserved != originalReserved {
+              				currentProfile.Spec.CPU = originalProfile.Spec.CPU
+              				profiles.UpdateWithRetry(currentProfile)

Contributor

shajmakh Jan 27, 2026

same here regarding the profile update

Contributor Author

SargunNarula Jan 28, 2026 •

edited

Loading

Since removing nosmt needs 2 step profile update, removing kernel arg initially and then updating cpusets, it has been resolved by introducing a context level profile flag at initial update itself - here

test/e2e/performanceprofile/functests/1_performance/irqbalance.go

+              		})
+              	})
+              	Context("Verify housekeeping CPU assignment when SMT is disabled", Label(string(label.Tier2)), func() {

Contributor

shajmakh Jan 27, 2026

profile update comment apply to this context as well. there could be 2 way I think of ATM to do the profile update in AfetrEach:

first update is in beforeEach
first update is in the test spec but with a flag to indicate that an update is done to the profile and it needs to be reverted.

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

+              			Expect(smtActive).To(Equal("0"), "SMT should be disabled (smt/active should be 0)")
+              			cpuRequest := 2
+              			if cpuRequest > newIsolatedCPUs.Size() {

Contributor

shajmakh Jan 27, 2026

+1

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

SargunNarula force-pushed the irq_housekeeping_2 branch from f663ea0 to 0ce6e8c Compare

January 28, 2026 13:43

shajmakh reviewed

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated Show resolved Hide resolved

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Outdated

Comment on lines 611 to 614

+              			By("Restoring original CPU configuration")
+              			currentProfile, err = profiles.GetByNodeLabels(testutils.NodeSelectorLabels)
+              			Expect(err).ToNot(HaveOccurred())
+              			currentReserved := string(*currentProfile.Spec.CPU.Reserved)

Contributor

shajmakh Jan 28, 2026

why updating PP twice?

Contributor Author

SargunNarula Jan 29, 2026

While restoring, we update Profile twice to handle the CPU topology transition safely.

We cannot update PP in a single phase because removing nosmt changes the online CPU
topology only after reboot, while kubelet immediately validates reservedSystemCPUs/isolatedCPUs
against the current online CPU set during CPU Manager initialization. A single update would
apply cpusets that reference offline SMT siblings, causing kubelet config validation to fail.

See kubelet CPU manager validation:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cm/cpumanager/policy_static.go#L251-L270

test/e2e/performanceprofile/functests/utils/baseload/baseload.go Show resolved Hide resolved

test/e2e/performanceprofile/functests/utils/baseload/baseload.go

+              // CPURequestedCores returns the total CPU requested in whole cores (rounded up)
+              func (l Load) CPURequestedCores() int {
+              	millis := l.Resources.Cpu().MilliValue()
+              	return int((millis + 999) / 1000)

Contributor

shajmakh Jan 28, 2026

perhaps use a roundup:
retCpu := *resource.NewQuantity(roundUp(cpu.Value(), 2), resource.DecimalSI)


          e2e: Add housekeeping IRQ load balancing tests

bf2f688

Added e2e tests for IRQ load balancing with housekeeping pods:
- [86346] Verify housekeeping works correctly with single hyperthread allocation
- [86348] Verify irqbalance does not overwrite on TuneD restart
  (housekeeping annotation)
- [86347] Verify housekeeping selects single CPU when SMT is disabled

Added baseload calculation functions for determining available pod capacity on nodes

Signed-off-by: Sargun Narula <snarula@redhat.com>

SargunNarula force-pushed the irq_housekeeping_2 branch from 0ce6e8c to bf2f688 Compare

January 29, 2026 13:11

Contributor

openshift-ci bot commented Jan 29, 2026

@SargunNarula: 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-hypershift-pao	`bf2f688`	link	true	`/test e2e-hypershift-pao`
ci/prow/e2e-gcp-pao	`bf2f688`	link	true	`/test e2e-gcp-pao`
ci/prow/e2e-hypershift	`bf2f688`	link	true	`/test e2e-hypershift`
ci/prow/e2e-aws-ovn	`bf2f688`	link	true	`/test e2e-aws-ovn`

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet