serial tests fixes #1195

shajmakh · 2025-02-19T10:51:41Z

please see commits for more details.

Problem: Reboot tests fails frequently on teardown when comparing the quantaty of the device resources. The root cause is from the sample-device plugin but haven't yet fully investigated considering we **want** to move to real devices/ sriov simulation and due to capacity. What do you want? Make the serial suite less noisy due to to known issue, until we fix the root cause or implement the alternative: https://issues.redhat.com/browse/CNF-12824 How are you fixing the problem in this PR? similarly to memory resources deviation for reboot tests, allow devices quantity to change after reboot but under the condition that the ratios are the same before and after the test in the context of allocatables and availables; That means capacities of the same device may change before vs after the test, but unequal resources consumption is not tolerated. How did you test the change? On a cluster with sample devices: - run the reboot tests to verify we no longer hit a failure but a warning: ``` make binary-e2e-serial bin/e2e-nrop-serial.test --ginkgo.v --ginkgo.focus="reboot" " - run tier-0 tests. ``` Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Add and use the MCPInfo.ToString() and print out the built info. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Previously using the MCP object that we built manually wasn't wrong because the specified config is empty anyway; still, do the correct use by pulling the updated object and use it in the MCPInfo object. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

This shouldn't be an issue if the original MCP that is configured in NROP is targeting worker nodes (MCP=worker); Still don't depend on this assumption and use the NRT nodes. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

When GinkgoHelper() was present (openshift-kni@f56658a) It was hard to follow the logs and understand which step failed while waiting for MCP update. Adding unique identifiers to each call helps track the start and end of each call and enhanced tracking down the root cause. The default identifier is `time.Now()` as a string. Additionally report config values and expectations of the call. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

openshift-ci · 2025-02-19T10:51:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shajmakh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [shajmakh]

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

With the latest selinux updates, NROP will no longer require to reboot the nodes on CR updates unless a node group annottaion is enabled. The new selinux policy is default starting 4.18. However this wasn't backported to older versions so in this commit we adapt this change based on the existance of the annotation and the openshift version. This commit fixes the expected bahavior per each case and adds missing waiting loops for MCP updating conndition. The test passed with default config on 4.18 and on older versions. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Upgrade `isDegradedInternalError` to `isDegradedWith` to check different reasons instead of checking that the reason is particularly `status.ReasonInternalError`. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

The package has already `isDegradedWith` which verifies if the degraded reason and message is as expected, let's use it instead of duplicating the same check. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

shajmakh · 2025-02-26T08:52:45Z

proposal for rewriting the test is found here: #1196

shajmakh · 2025-02-26T11:15:48Z

/retest

shajmakh · 2025-02-26T11:16:42Z

test/e2e/serial/tests/configuration.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/openshift/cluster-node-tuning-operator/pkg/performanceprofile/controller/performanceprofile/resources"


changes are not needed in this commit, should be cleanedup

shajmakh · 2025-02-26T11:18:14Z

internal/noderesourcetopology/equality.go

+}
+
+func resourceIsDevice(resName string) bool {
+	// TODO avoid duplicates with e2e/tests/internal/fixture/devices


the todo need to be removed

shajmakh · 2025-02-26T11:18:44Z

internal/noderesourcetopology/equality.go

+	return false
+}
+
+// TODO rename to EqualMemoryWithDeviation because it is dedicated for memory resource


need a cleanup

ffromani

mixed bag. Some commits are clear improvements, some other raises some question.
I can see the reason for all the changes, but some implementations can use some polishing, probably.

ffromani · 2025-02-26T11:23:20Z

internal/devices/devices.go

+const (
+	DevType1EnvVar = "E2E_NROP_DEVICE_TYPE_1"
+	DevType2EnvVar = "E2E_NROP_DEVICE_TYPE_2"
+	DevType3EnvVar = "E2E_NROP_DEVICE_TYPE_3"
+)


what's the benefit of moving here the constants?

if they were kept in test/ we cannot use them in the internal/ directory because of cycle imports because test/ directory already imports internal/. to solve this we move the common consts in internal and use them in test/ and internal helpers without issue. I believe it would help to have this change in separate commit. I'll split that.

I've grown less and less entusiast of constants-only packages (and yes, I'm guilty too) but this makes sense.

internal/devices/devices.go

ffromani · 2025-02-26T11:24:58Z

internal/noderesourcetopology/equality.go

@@ -89,6 +102,59 @@ func EqualResourceInfo(resInfoA, resInfoB nrtv1alpha2.ResourceInfo, isRebootTest
 	return true, nil
 }

+func EqualDevicesWithWarning(a, b nrtv1alpha2.ResourceInfo) (bool, error) {


no need to use "WithWarning", code is free to call the logger

doesn't seem the right name indeed, will fix that. Thanks for the comment.

ffromani · 2025-02-26T11:30:07Z

internal/noderesourcetopology/equality.go

+	// sample devices; need a way to distinguish between real vs sample devices used in the ci
+	if isRebootTest && resourceIsDevice(resInfoA.Name) {
+		// TODO: Feb 2025: NROP serial is using sample-device plugin which has a known issue that after operations like node reboot or taints update,
+		// quantity of the device reaources is messed up, so this is a workaround for reboot tests should be removed once


typo: resources

ffromani · 2025-02-26T11:31:02Z

test/e2e/serial/tests/configuration.go

@@ -98,6 +97,10 @@ type mcpInfo struct {
 	sampleNode    corev1.Node
 }

+func (i mcpInfo) ToString() string {
+	return fmt.Sprintf("name %q; config %q; sample node %q\n", i.mcpObj.Name, i.initialConfig, i.sampleNode.Name)


Stringer functions, either String() or our custom ToString() lesser format should never add the trailing "\n"

ffromani · 2025-02-26T11:33:18Z

test/e2e/serial/tests/configuration.go

+			nodesNameSet := e2enrt.AccumulateNames(nrts)
+			// nrts > 1 is checked in the setup
+			targetedNodeName, _ := nodesNameSet.PopAny()
+			targetedNode := &corev1.Node{}


why a pointer? can be a local variable

ffromani · 2025-02-26T11:36:59Z

test/e2e/serial/tests/tolerations.go

@@ -841,11 +841,18 @@ func sriovToleration() corev1.Toleration {
 }

 func waitForMcpUpdate(cli client.Client, ctx context.Context, mcpsInfo []mcpInfo, updateType MCPUpdateType) {


this is a good idea but I'm puzzled by the implementation. We don't have GinkgoHelper() calls anymore, so failures should point to the specific test, should not they?

ffromani · 2025-02-26T11:37:16Z

test/e2e/serial/tests/tolerations.go

@@ -841,11 +841,18 @@ func sriovToleration() corev1.Toleration {
 }

 func waitForMcpUpdate(cli client.Client, ctx context.Context, mcpsInfo []mcpInfo, updateType MCPUpdateType) {
+	waitForMcpUpdateWithID(cli, ctx, mcpsInfo, updateType, time.Now().String())


unnecessary, internal functions can (and should) change API freely

ffromani · 2025-02-26T11:37:44Z

test/internal/deploy/deploy.go

 			return err
 		})
 	}
-	return eg.Wait()
+	return eg.Wait() // will block the flow until goroutines are finished or return on first error


yes, this is well documented so this comment is redundant

ffromani · 2025-02-26T11:39:24Z

test/e2e/serial/tests/configuration.go

@@ -1510,3 +1523,19 @@ func getLabelRoleWorker() string {
 func getLabelRoleMCPTest() string {
 	return fmt.Sprintf("%s/%s", depnodes.LabelRole, roleMCPTest)
 }
+
+func isCustomPolicySupportEnabled(nro *nropv1.NUMAResourcesOperator) bool {
+	GinkgoHelper()


so here we do want to call GinkgoHelper?

shajmakh added 5 commits February 19, 2025 12:47

serial:test-47674 logs improvments:add MCPInfo ToString()

637ca1d

Add and use the MCPInfo.ToString() and print out the built info. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

serial:47674: use NRT nodes instead of workers

eca7687

This shouldn't be an issue if the original MCP that is configured in NROP is targeting worker nodes (MCP=worker); Still don't depend on this assumption and use the NRT nodes. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

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 Feb 19, 2025

shajmakh changed the title ~~WIP: serail tests fixes Feb 19~~ WIP: serial tests fixes Feb 19 Feb 19, 2025

openshift-ci bot requested review from mrniranjan and Tal-or February 19, 2025 10:51

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2025

shajmakh force-pushed the serial-fix-2-newversionof47674 branch 2 times, most recently from 067b1b6 to f94b584 Compare February 25, 2025 13:55

shajmakh added 2 commits February 25, 2025 15:56

serial: generalize function to check different degraded reasons

7890dda

Upgrade `isDegradedInternalError` to `isDegradedWith` to check different reasons instead of checking that the reason is particularly `status.ReasonInternalError`. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

serial: use helper function to validate degraded status

f94b584

The package has already `isDegradedWith` which verifies if the degraded reason and message is as expected, let's use it instead of duplicating the same check. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

shajmakh changed the title ~~WIP: serial tests fixes Feb 19~~ serial tests fixes Feb 19 Feb 26, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2025

shajmakh changed the title ~~serial tests fixes Feb 19~~ serial tests fixes Feb 26, 2025

shajmakh commented Feb 26, 2025

View reviewed changes

ffromani reviewed Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serial tests fixes #1195

serial tests fixes #1195

shajmakh commented Feb 19, 2025

openshift-ci bot commented Feb 19, 2025

shajmakh commented Feb 26, 2025

shajmakh commented Feb 26, 2025

shajmakh Feb 26, 2025

shajmakh Feb 26, 2025

shajmakh Feb 26, 2025

ffromani left a comment

ffromani Feb 26, 2025

shajmakh Feb 28, 2025

ffromani Feb 28, 2025

ffromani Feb 26, 2025

shajmakh Feb 28, 2025

ffromani Feb 26, 2025

ffromani Feb 26, 2025

ffromani Feb 26, 2025

ffromani Feb 26, 2025

ffromani Feb 26, 2025

ffromani Feb 26, 2025

ffromani Feb 26, 2025

		@@ -841,11 +841,18 @@ func sriovToleration() corev1.Toleration {
		}

		func waitForMcpUpdate(cli client.Client, ctx context.Context, mcpsInfo []mcpInfo, updateType MCPUpdateType) {

serial tests fixes #1195

Are you sure you want to change the base?

serial tests fixes #1195

Conversation

shajmakh commented Feb 19, 2025

openshift-ci bot commented Feb 19, 2025

shajmakh commented Feb 26, 2025

shajmakh commented Feb 26, 2025

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

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

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