CNF-16063: E2E: Add hypershift support to container runtime config tests #1229

SargunNarula · 2024-11-27T13:55:17Z

The container runtime configuration test was omitted during the HyperShift adjustments, as it was initially deemed unsuitable for HyperShift. This change introduces support for it.

openshift-ci · 2024-11-27T13:55:21Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

SargunNarula · 2024-11-28T04:40:31Z

/retest

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

rbaturov

Thank you for this work Sargun!
I realize that my removal of the container runtime type in this switch case has impacted the e2e, and it resulted with creating manually configmaps for ctrcfg.
Since we are no longer want to interact with ctrcfg from the controller perspective my deletion was correct. However, since we use the func GetObjectConfigMapDataKey for testing purposes aswell we might want to retain that line.
@Tal-or WDYT?

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

rbaturov · 2024-12-04T15:35:37Z

/test e2e-pao-updating-profile-hypershift

Let's not forget to test the work here

rbaturov · 2024-12-04T15:37:14Z

/retest-required

The e2e-gcp-pao-updating-profile has reached the 5h timeout, we should consider to extend the time.

SargunNarula · 2024-12-06T13:49:13Z

/test e2e-pao-updating-profile-hypershift

SargunNarula · 2024-12-08T12:13:20Z

/retest

SargunNarula · 2024-12-11T06:44:25Z

/test e2e-pao-updating-profile-hypershift

SargunNarula · 2024-12-11T06:44:49Z

/retest

Tal-or · 2024-12-11T14:19:08Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

@@ -442,7 +444,7 @@ var _ = Describe("[rfe_id:28761][performance] Updating parameters in performance

 			By("Creating new MachineConfigPool")
 			mcp = mcps.New(newRole, newNodeSelector)
-			err = testclient.ControlPlaneClient.Create(context.TODO(), mcp)
+			err = testclient.Client.Create(context.TODO(), mcp)


We should not use bare Client anymore. Is there a reason for this change?

In the context of "Updating the nodeSelector parameter and node labels," there are still several instances using Client. Previously, I updated some occurrences with the appropriate client, but @rbaturov advised that we should handle the cleanup in a separate PR, as this PR is focused specifically on containerRuntimeConfig tests.

Tal-or · 2024-12-11T14:28:20Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

+				ctrcfg, err = getContainerRuntimeConfigFrom(context.TODO(), profile, mcp)
+				Expect(err).ToNot(HaveOccurred(), "failed to get ContainerRuntimeConfig from profile %q mcp %q", profile.Name, mcp.Name)
+				if ctrcfg != nil {
+					Skip(fmt.Sprintf("ContainerRuntimeConfig %s exist in the cluster and not expected", ctrcfg.Name))


Why these checks are needed?
The tests are suppose to be running under our CI, and noone but the test is suppose to pollute the cluster state and create undesired ContainerRuntimeConfig object.

@Tal-or Well this check was from the original test before even my refactor.
If we plan to use this e2e solely on CI and therefore no ctrcfg is expected, then I guess it is safe to remove this.
However, if this test is somehow executed on a cluster with container runtime config it might run into trouble since it is not designed to start this way. I would not remove this check.

Our tests are intended to run on clean, persisted and consistent environment.
As I suggested to @SargunNarula we should not skip but log with error because having unexpected objects on the cluster, might hide even a bigger problem.

Logging an error and running this test could potentially fail or not test the desired functionality. IOW, this test is incorrect if it runs on a system with ctrcfg pre-installed.
I think the best way to go forward here to ensure we do not have "unexpected objects on the cluster, might hide even a bigger problem." is to fail the test right away if a ctrcfg is found with a proper message

Tal-or · 2024-12-11T15:41:29Z

test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go

-					By(fmt.Sprintf("creating ContainerRuntimeConfig %q", ctrcfg.Name))
-					Expect(testclient.Client.Create(context.TODO(), ctrcfg)).ToNot(HaveOccurred(), "failed to create ctrcfg %#v", ctrcfg)
+					if hypershift.IsHypershiftCluster() {
+						ctrcfg_configmap, err = containerRuntimeConfigConfigMap(ContainerRuntimeConfigName, profile, mcp)


You don't need that.
You're already get the for free by in the Client.Create call

cluster-node-tuning-operator/pkg/performanceprofile/controller/performanceprofile/hypershift/hypershift.go

Line 65 in 8aa2a81

func (ci *ControlPlaneClientImpl) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {

Well he now actually needs since I removed that support.
@Tal-or take a look at this comment.

Tal-or · 2024-12-17T11:20:48Z

Thank you for this work Sargun! I realize that my removal of the container runtime type in this switch case has impacted the e2e, and it resulted with creating manually configmaps for ctrcfg. Since we are no longer want to interact with ctrcfg from the controller perspective my deletion was correct. However, since we use the func GetObjectConfigMapDataKey for testing purposes aswell we might want to retain that line. @Tal-or WDYT?

If we're still testing the affect of ContainerRuntimeConfig on the controller, we should revert this change

rbaturov · 2024-12-17T11:44:16Z

Thank you for this work Sargun! I realize that my removal of the container runtime type in this switch case has impacted the e2e, and it resulted with creating manually configmaps for ctrcfg. Since we are no longer want to interact with ctrcfg from the controller perspective my deletion was correct. However, since we use the func GetObjectConfigMapDataKey for testing purposes aswell we might want to retain that line. @Tal-or WDYT?

If we're still testing the affect of ContainerRuntimeConfig on the controller, we should revert this change

Let's do that. @SargunNarula just add this line that I removed back again

SargunNarula · 2024-12-17T11:50:45Z

Thank you for this work Sargun! I realize that my removal of the container runtime type in this switch case has impacted the e2e, and it resulted with creating manually configmaps for ctrcfg. Since we are no longer want to interact with ctrcfg from the controller perspective my deletion was correct. However, since we use the func GetObjectConfigMapDataKey for testing purposes aswell we might want to retain that line. @Tal-or WDYT?

If we're still testing the affect of ContainerRuntimeConfig on the controller, we should revert this change

Let's do that. @SargunNarula just add this line that I removed back again

Alright, I will include that revert change in this PR itself.

Signed-off-by: Sargun Narula <[email protected]> Revert: ContainerRuntimeConfig handling for test purposes This reverts the removal of ContainerRuntimeConfig handling in the switch case, which inadvertently impacted e2e tests. While the deletion was correct from the controller's perspective (as we no longer had to interact with ContainerRuntimeConfig), the function `GetObjectConfigMapDataKey` is still used for testing purposes. To ensure e2e tests function correctly and to continue testing the effects of ContainerRuntimeConfig, this change is being reverted. Signed-off-by: Sargun Narula <[email protected]>

rbaturov · 2024-12-17T14:45:01Z

/test e2e-pao-updating-profile-hypershift

Lets not forget this test
It is not automatically triggered

SargunNarula · 2024-12-17T21:13:26Z

/retest

rbaturov · 2024-12-18T07:47:50Z

/lgtm

openshift-ci · 2024-12-18T15:11:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik, SargunNarula

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~~ [MarSik]

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 · 2024-12-18T15:19:10Z

@SargunNarula: This pull request references CNF-16063 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The container runtime configuration test was omitted during the HyperShift adjustments, as it was initially deemed unsuitable for HyperShift. This change introduces support for it.

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.

SargunNarula · 2024-12-18T15:48:30Z

/refresh

openshift-ci-robot · 2024-12-18T18:13:35Z

/retest-required

Remaining retests: 0 against base HEAD c0171d3 and 2 for PR HEAD 961e5b8 in total

openshift-ci · 2024-12-18T20:05:07Z

@SargunNarula: The following test 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/okd-scos-e2e-aws-ovn	`961e5b8`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

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.

openshift-ci-robot · 2024-12-18T20:13:40Z

/retest-required

Remaining retests: 0 against base HEAD 48153be and 1 for PR HEAD 961e5b8 in total

openshift-bot · 2024-12-19T17:53:41Z

[ART PR BUILD NOTIFIER]

Distgit: cluster-node-tuning-operator
This PR has been included in build cluster-node-tuning-operator-container-v4.19.0-202412191309.p0.g61c94c4.assembly.stream.el9.
All builds following this will include this PR.

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 Nov 27, 2024

SargunNarula mentioned this pull request Nov 27, 2024

CNF-15505: Remove container runtime selection #1219

Merged

SargunNarula marked this pull request as ready for review November 27, 2024 19:51

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 Nov 27, 2024

openshift-ci bot requested review from MarSik and yanirq November 27, 2024 19:52

SargunNarula force-pushed the hypershift_update_tests branch from 2174fae to d60c1fe Compare November 27, 2024 20:01

rbaturov reviewed Dec 1, 2024

View reviewed changes

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

rbaturov reviewed Dec 1, 2024

View reviewed changes

test/e2e/performanceprofile/functests/utils/nodepools/nodepools.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2024

SargunNarula force-pushed the hypershift_update_tests branch from d60c1fe to 5d7fe67 Compare December 4, 2024 09:59

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2024

rbaturov reviewed Dec 4, 2024

View reviewed changes

SargunNarula force-pushed the hypershift_update_tests branch from 5d7fe67 to 8fc68f5 Compare December 6, 2024 13:26

SargunNarula force-pushed the hypershift_update_tests branch from 8fc68f5 to 9e7da4d Compare December 10, 2024 11:35

Tal-or reviewed Dec 11, 2024

View reviewed changes

SargunNarula force-pushed the hypershift_update_tests branch from 9e7da4d to 961e5b8 Compare December 17, 2024 14:10

openshift-ci bot assigned rbaturov Dec 18, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024

MarSik approved these changes Dec 18, 2024

View reviewed changes

openshift-ci bot assigned MarSik Dec 18, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2024

SargunNarula changed the title ~~E2E: Add hypershift support to container runtime config tests~~ CNF-16063: E2E: Add hypershift support to container runtime config tests Dec 18, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 18, 2024

openshift-merge-bot bot merged commit 61c94c4 into openshift:master Dec 19, 2024
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-16063: E2E: Add hypershift support to container runtime config tests #1229

CNF-16063: E2E: Add hypershift support to container runtime config tests #1229

SargunNarula commented Nov 27, 2024 •

edited

Loading

openshift-ci bot commented Nov 27, 2024

SargunNarula commented Nov 28, 2024

rbaturov left a comment

rbaturov commented Dec 4, 2024

rbaturov commented Dec 4, 2024

SargunNarula commented Dec 6, 2024

SargunNarula commented Dec 8, 2024

SargunNarula commented Dec 11, 2024

SargunNarula commented Dec 11, 2024

Tal-or Dec 11, 2024

SargunNarula Dec 17, 2024

Tal-or Dec 11, 2024

rbaturov Dec 17, 2024

Tal-or Dec 17, 2024

rbaturov Dec 17, 2024 •

edited

Loading

Tal-or Dec 11, 2024

rbaturov Dec 17, 2024

Tal-or commented Dec 17, 2024

rbaturov commented Dec 17, 2024

SargunNarula commented Dec 17, 2024

rbaturov commented Dec 17, 2024

SargunNarula commented Dec 17, 2024

rbaturov commented Dec 18, 2024

openshift-ci bot commented Dec 18, 2024

openshift-ci-robot commented Dec 18, 2024 •

edited by openshift-ci bot

Loading

SargunNarula commented Dec 18, 2024

openshift-ci-robot commented Dec 18, 2024

openshift-ci bot commented Dec 18, 2024 •

edited

Loading

openshift-ci-robot commented Dec 18, 2024

openshift-bot commented Dec 19, 2024

CNF-16063: E2E: Add hypershift support to container runtime config tests #1229

CNF-16063: E2E: Add hypershift support to container runtime config tests #1229

Conversation

SargunNarula commented Nov 27, 2024 • edited Loading

openshift-ci bot commented Nov 27, 2024

SargunNarula commented Nov 28, 2024

rbaturov left a comment

Choose a reason for hiding this comment

rbaturov commented Dec 4, 2024

rbaturov commented Dec 4, 2024

SargunNarula commented Dec 6, 2024

SargunNarula commented Dec 8, 2024

SargunNarula commented Dec 11, 2024

SargunNarula commented Dec 11, 2024

Tal-or Dec 11, 2024

Choose a reason for hiding this comment

SargunNarula Dec 17, 2024

Choose a reason for hiding this comment

Tal-or Dec 11, 2024

Choose a reason for hiding this comment

rbaturov Dec 17, 2024

Choose a reason for hiding this comment

Tal-or Dec 17, 2024

Choose a reason for hiding this comment

rbaturov Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Tal-or Dec 11, 2024

Choose a reason for hiding this comment

rbaturov Dec 17, 2024

Choose a reason for hiding this comment

Tal-or commented Dec 17, 2024

rbaturov commented Dec 17, 2024

SargunNarula commented Dec 17, 2024

rbaturov commented Dec 17, 2024

SargunNarula commented Dec 17, 2024

rbaturov commented Dec 18, 2024

openshift-ci bot commented Dec 18, 2024

openshift-ci-robot commented Dec 18, 2024 • edited by openshift-ci bot Loading

SargunNarula commented Dec 18, 2024

openshift-ci-robot commented Dec 18, 2024

openshift-ci bot commented Dec 18, 2024 • edited Loading

openshift-ci-robot commented Dec 18, 2024

openshift-bot commented Dec 19, 2024

SargunNarula commented Nov 27, 2024 •

edited

Loading

rbaturov Dec 17, 2024 •

edited

Loading

openshift-ci-robot commented Dec 18, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Dec 18, 2024 •

edited

Loading