WIP: performance: e2e: configure adaptive cpu in profile #884

shajmakh · 2023-12-14T10:14:52Z

We've been observing lately that some tests that involve disabling load balancing are failing (like 32646) because the expected result does not have specific anticipated CPUs. After investigation, it turns out that one factor is the profile configuration of the CPU distribution.

PAO functional tests configure fixed CPU values under the PP. This is considered misconfiguration, especially when the system has more than 4 CPUs, and there is no guarantee that the functionality of the performance profile controller will work adequately with not all cpus reflected in the CPU section in the PP.

To fix this, we check the actual CPU capacity on a worker node and divide that between reserved and isolated (required cpu fields in PP).

shajmakh · 2023-12-14T10:15:06Z

/hold

openshift-ci · 2023-12-14T10:15:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

Tal-or · 2023-12-14T10:29:59Z

test/e2e/performanceprofile/functests/0_config/config.go

@@ -3,6 +3,7 @@ package __performance_config
 import (
 	"context"
 	"fmt"
+	"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/nodes"


Please move it down along with the other relevant imports

Tal-or · 2023-12-14T10:38:57Z

test/e2e/performanceprofile/functests/0_config/config.go

+		return nil, fmt.Errorf("insufficient cpus on a node to create a valid performanceprofile for the tests, found %d cpus, expects at least 4", capacityCPU)
+	}
+	reserved := performancev2.CPUSet(fmt.Sprintf("0,%d", capacityCPU/2))
+	isolated := performancev2.CPUSet(fmt.Sprintf("1-%d,%d-%d", capacityCPU/2-1, capacityCPU/2+1, capacityCPU-1))


Are 2 reserved CPUs are enough for machine with a big amount of cpus?

This will cause issues on BM because on HT enabled if we don't pick cpus from the same core.

Also the same for isolated, In terms of upstream where there are only 4 cpus, No Numa, this works , but as we use this on multinuma systems with HT enabled, this gets complicated. I strongly suggest to use PPC . Otherwise you will have to duplicate what ppc does here.

Using PPC can work here, even though it will make the lane setup significantly more complex. But I agree we're reimplementing PPC.
The best IMO would be reuse part of PPC code, but we would need more detailed HW information to enable PPC.

Tal-or · 2023-12-14T10:40:43Z

test/e2e/performanceprofile/functests/0_config/config.go

+	//Get cpus capacity on a node; typically it is same for all nodes
+	workerNodes, err := nodes.GetByLabels(testutils.NodeSelectorLabels)
+	Expect(err).ToNot(HaveOccurred())
+	capacityCPU, _ := workerNodes[0].Status.Capacity.Cpu().AsInt64()


I would validate that all workers are homogeneous and failed other wise.

yes please; also, as unlikely it could be, let's handle the error here

ffromani

I side with @mrniranjan here. The main problem is we're reimplementing PPC.
Best would be to either reuse (part of) PPC code or just use PPC as prerequiste before the testsuite starts.

I don't have strong opinions

ffromani · 2023-12-14T14:16:31Z

test/e2e/performanceprofile/functests/0_config/config.go

+	//Get cpus capacity on a node; typically it is same for all nodes
+	workerNodes, err := nodes.GetByLabels(testutils.NodeSelectorLabels)
+	Expect(err).ToNot(HaveOccurred())
+	capacityCPU, _ := workerNodes[0].Status.Capacity.Cpu().AsInt64()


yes please; also, as unlikely it could be, let's handle the error here

ffromani · 2023-12-14T14:17:11Z

test/e2e/performanceprofile/functests/0_config/config.go

+func testProfile() (*performancev2.PerformanceProfile, error) {
+	//Get cpus capacity on a node; typically it is same for all nodes
+	workerNodes, err := nodes.GetByLabels(testutils.NodeSelectorLabels)
+	Expect(err).ToNot(HaveOccurred())


please either use Expect in the code (but please use ExpectWithOffset) OR return error and let the caller handle - but let's not do both

ffromani · 2023-12-14T14:19:32Z

test/e2e/performanceprofile/functests/0_config/config.go

+		return nil, fmt.Errorf("insufficient cpus on a node to create a valid performanceprofile for the tests, found %d cpus, expects at least 4", capacityCPU)
+	}
+	reserved := performancev2.CPUSet(fmt.Sprintf("0,%d", capacityCPU/2))
+	isolated := performancev2.CPUSet(fmt.Sprintf("1-%d,%d-%d", capacityCPU/2-1, capacityCPU/2+1, capacityCPU-1))


Using PPC can work here, even though it will make the lane setup significantly more complex. But I agree we're reimplementing PPC.
The best IMO would be reuse part of PPC code, but we would need more detailed HW information to enable PPC.

ffromani · 2023-12-14T14:19:56Z

test/e2e/performanceprofile/functests/0_config/config.go

+		//should never reach but still
+		return nil, fmt.Errorf("insufficient cpus on a node to create a valid performanceprofile for the tests, found %d cpus, expects at least 4", capacityCPU)
+	}
+	reserved := performancev2.CPUSet(fmt.Sprintf("0,%d", capacityCPU/2))


I strongly recommend to use CPUSets and do logic with that instead of using just numbers

ffromani · 2023-12-14T14:50:58Z

another option we could explore is somehow a middleground:

get the cpu topology (same format as PPC) as parameter of the config lane.
iff the cpu topology is given, THEN the code will assume all the workers have the same topology. It will be responsability of the lane maintainer to enforce this and to ship up to date topology data
reuse (possibly refactoring the code) the PPC code to consume the given data to optimally compute (parts of) the PPC profile.

It is not fully automated (e.g. just run must-gather and let PPC do its magic) but would be a step in the right direction, would be more automated than it is now and should be deliverable quickly as fix.

ffromani · 2023-12-14T14:54:41Z

another option we could explore is somehow a middleground:
1. get the cpu topology (same format as PPC) as parameter of the config lane.

2. iff the cpu topology is given, THEN the code will assume all the workers have the same topology. It will be responsability of the lane maintainer to enforce this and to ship up to date topology data

3. reuse (possibly refactoring the code) the PPC code to consume the given data to optimally compute (parts of) the PPC profile.
It is not fully automated (e.g. just run must-gather and let PPC do its magic) but would be a step in the right direction, would be more automated than it is now and should be deliverable quickly as fix.

however we will need to check how to actually ship the data and make it accessible to the 0_config suite

shajmakh · 2023-12-15T09:27:35Z

@Tal-or @mrniranjan @ffromani thank you for the valuable reviews and comments. you highlighted legit points about using PPC. however, this PR is intended to address the blocker CI failure on cnf-feature-deploy, considering that d/s CI it is in fact using PPC to generate the profile. I'm researching to see if we can enhance this even more to satisfy all needs taking into account the urgency of this for u/s.

ffromani · 2023-12-15T09:30:25Z

@Tal-or @mrniranjan @ffromani thank you for the valuable reviews and comments. you highlighted legit points about using PPC. however, this PR is intended to address the blocker CI failure on cnf-feature-deploy, considering that d/s CI it is in fact using PPC to generate the profile. I'm researching to see if we can enhance this even more to satisfy all needs taking into account the urgency of this for u/s.

The urgency IS a factor indeed, but the main blocker for this approach is that we need to take into account HT and NUMA cpu affinity when deciding the reserved cpus. Otherwise the perfprof will be incorrect for other reasons, perhaps lesser reasons than the current state but not much better; the NUMA allocation is perhaps debatable, but HT is a requirement for which we have e2e tests for.

I understand both camps and I don't have strong opinions yet.

We've been observing lately that some tests that involve disabling load balancing are failing (like 32646) because the expected result does not have specific anticipated CPUs. After investigation, it turns out that one factor is the profile configuration of the CPU distribution. PAO functional tests configure fixed CPU values under the PP. This is considered misconfiguration, especially when the system has more than 4 CPUs, and there is no guarantee that the functionality of the performance profile controller will work adequately with not all cpus reflected in the CPU section in the PP. To fix this, we check the actual CPU capacity on a worker node and divide that between reserved and isolated (required cpu fields in PP). Signed-off-by: shajmakh <shajmakh@redhat.com>

shajmakh · 2024-01-09T12:01:02Z

After reassessing the cost of the approach described here, I am closing this in favor of #909

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2023

openshift-ci bot requested review from dagrayvid and Tal-or December 14, 2023 10:15

Tal-or reviewed Dec 14, 2023

View reviewed changes

ffromani reviewed Dec 14, 2023

View reviewed changes

shajmakh changed the title ~~performance: e2e: configure adaptive cpu in profile~~ WIP: performance: e2e: configure adaptive cpu in profile Dec 15, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2023

shajmakh added 4 commits December 21, 2023 18:07

use ppc.systemInfo to calculate the cpu distribution

87d2f63

temp

840a5e7

process /proc/cpuinfo data

60b74db

shajmakh force-pushed the make-adaptive-pp branch from ab19aa9 to 60b74db Compare December 21, 2023 16:08

shajmakh marked this pull request as draft December 21, 2023 16:09

shajmakh closed this Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: performance: e2e: configure adaptive cpu in profile #884

WIP: performance: e2e: configure adaptive cpu in profile #884

shajmakh commented Dec 14, 2023

shajmakh commented Dec 14, 2023

openshift-ci bot commented Dec 14, 2023

Tal-or Dec 14, 2023

Tal-or Dec 14, 2023

mrniranjan Dec 14, 2023

mrniranjan Dec 14, 2023

ffromani Dec 14, 2023

Tal-or Dec 14, 2023

ffromani Dec 14, 2023

ffromani left a comment

ffromani Dec 14, 2023

ffromani Dec 14, 2023 •

edited

ffromani Dec 14, 2023

ffromani Dec 14, 2023

ffromani commented Dec 14, 2023

ffromani commented Dec 14, 2023

shajmakh commented Dec 15, 2023

ffromani commented Dec 15, 2023 •

edited

shajmakh commented Jan 9, 2024

WIP: performance: e2e: configure adaptive cpu in profile #884

WIP: performance: e2e: configure adaptive cpu in profile #884

Conversation

shajmakh commented Dec 14, 2023

shajmakh commented Dec 14, 2023

openshift-ci bot commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani Dec 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani commented Dec 14, 2023

ffromani commented Dec 14, 2023

shajmakh commented Dec 15, 2023

ffromani commented Dec 15, 2023 • edited

shajmakh commented Jan 9, 2024

ffromani Dec 14, 2023 •

edited

ffromani commented Dec 15, 2023 •

edited