CNF-11815: e2e: Added node inspector for inspecting nodes configuration #1008

rbaturov · 2024-04-01T07:44:11Z

On hypershift there is no MCO, hence there are no machine-config-daemon pods.
A different resolution is needed for accessing the underlying node for inspecting configurations.
This commit introduces a node inspector implemented as a daemonset.
Upon execution of test suites, a pod with elevated privileges and host filesystem mounted will be deployed on every node.
Also I have added Z-deconfig suite ('Z' prefix, will guarantee that it will be the last suite run) that will be used for cleanup.
This API will be used for both hypershift and non-hypershift systems.

ffromani

LGTM, possible improvements inside

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

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

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

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

openshift-ci-robot · 2024-04-02T11:33:19Z

@rbaturov: This pull request references CNF-11815 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.16.0" version, but no target version was set.

In response to this:

On hypershift there is no MCO, hence there are no machine-config-daemon pods. A different resolution is needed for accessing the underlying node for inspecting configurations. This commit adds a daemonset that will start upon test suites - on every node we will deploy a pod with escalated privileges and host fs mounted. This API will be used for both hypershift and non-hypershift systems.

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.

rbaturov · 2024-04-02T11:39:08Z

/hold
Depends on #1004

test/e2e/performanceprofile/functests/Z_deconfig/deconfig.go

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

Tal-or · 2024-04-14T12:39:50Z

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

 }

-// ExecCommandOnMachineConfigDaemon returns the output of the command execution on the machine-config-daemon pod that runs on the specified node
-func ExecCommandOnMachineConfigDaemon(ctx context.Context, node *corev1.Node, command []string) ([]byte, error) {


Both ExecCommandOnTestingDaemonPod and ExecCommandOnNode have the same purpose.
Unless there is a very good reason to have both functions, I would merge them into one named:
ExecCommandOnNode which declare explicitly where the command is about to be running.

@Tal-or
ExecCommandOnTestingDaemonPod is being used as an underlying function for ExecCommandOnNode.
The difference between the two is that ExecCommandOnNode returns a string (after some formatting) rather than a bytes slice (ExecCommandOnTestingDaemonPod).
As throughout the repo, developers have chosen to use both of them, I think we should keep both of these functions and just rename them appropriately. (Another option is to unify them to one function that returns []bytes and change all calls to target this func, following a call if needed to a util function that will do the string format that is left). But again the latter option would require some work for refactoring.
WDYT?

Then let's keep them and just rename ExecCommandOnTestingDaemonPod.
We don't want to steer too much from the scope of this work

@Tal-or I'll need to rename ExecCommandOnNode to ExecCommandOnNodeToString and ExecCommandOnTestingDaemonPod which returns a []byte to ExecCommandOnNode.
This however, may cause a slight confusion for developers that got used that ExecCommandOnNode returns a string. but I think this is not a big deal.
Another option could be to change ExecCommandOnTestingDaemonPod to ExecCommandOnNodeToByteSlice. I personally think we should go with the first option, even if it might cause a bit of confusion.

since the name of the package is nodes I would rename ExecCommandOnTestingDaemonPod to ExecCommand and call it a day.
From the caller site it looks like:
nodes.ExecCommand() The package name provides a clear indication about where this command is about to be running.

But what will be the name of the second func?
Anyways I was thinking that we should move these functions to the node inspector package, as they rely on it running.
So it's going to be more of nodeInspector.ExecCommand...

The node inspector is an implementation detail. you can call nodeInspector.ExecCommand from nodes.ExecCommand

openshift-ci-robot · 2024-04-15T12:03:49Z

@rbaturov: This pull request references CNF-11815 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.16.0" version, but no target version was set.

In response to this:

On hypershift there is no MCO, hence there are no machine-config-daemon pods.
A different resolution is needed for accessing the underlying node for inspecting configurations.
This commit introduces a node inspector implemented as a daemonset.
Upon execution of test suites, a pod with elevated privileges and host filesystem mounted will be deployed on every node.
Also I have added Z-deconfig suite ('Z' prefix, will guarantee that it will be the last suite run) that will be used for cleanup.
This API will be used for both hypershift and non-hypershift systems.

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.

rbaturov · 2024-04-15T14:29:07Z

/retest-required

rbaturov · 2024-04-16T09:41:15Z

@Tal-or ready for another review iteration

rbaturov · 2024-05-27T13:13:20Z

/retest-required

ffromani

/approve

I like the direction and we surely want this change. A few inline comments/questions

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

test/e2e/performanceprofile/functests/utils/node_inspector/inspector.go

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

openshift-ci · 2024-05-30T10:30:53Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

test/e2e/performanceprofile/functests/Z_deconfig/deconfig.go

test/e2e/performanceprofile/functests/utils/node_inspector/inspector.go

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

rbaturov · 2024-06-02T10:27:54Z

@Tal-or @ffromani
Did some modifications:

Calling Z_deconfig as part of a trap command in the run-test.sh script will ensure cleanup occurs, even if we run through failures mid-way.
Changed the node-inspector delete function, to delete namespace (which will implicitly delete service account, cluster-role-binding), thereby reducing API calls. Clusterrole requires an additional delete request as it is not namespaced.

Makefile

test/e2e/performanceprofile/functests/Z_deconfig/deconfig.go

test/e2e/performanceprofile/functests/utils/node_inspector/inspector.go

hack/run-test.sh

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

On hypershift there is no MCO, hence there are no machine-config-daemon pods. A different resolution is needed for accessing the underlying node for inspecting configurations. This commit introduces a node inspector implemented as a daemonset. Upon execution of test suites, a pod with elevated privileges and host filesystem mounted will be deployed on every node. Also I have added Z-deconfig suite ('Z' prefix, will guarantee that it will be the last suite run) that will be used for cleanup. This API will be used for both hypershift and non-hypershift systems. Signed-off-by: Ronny Baturov <[email protected]>

Adding trap command in the run-test.sh script will ensure the Z_deconfig suite will run, thereby deleting the node inspector. Signed-off-by: Ronny Baturov <[email protected]>

Updated these functions to use the node inspector rather than the MCD. Furthermore, I have renamed these functions to better reflect their purposes and usage. Signed-off-by: Ronny Baturov <[email protected]>

rbaturov · 2024-06-04T14:45:49Z

/retest-required

rbaturov · 2024-06-04T19:48:47Z

/retest-required

rbaturov · 2024-06-05T06:30:31Z

/retest-required

Refactor all calls to the unified function: ExecCommand. Added a util ToString function that will be used for the calls that require the output in a string format. Signed-off-by: Ronny Baturov <[email protected]>

rbaturov · 2024-06-05T10:35:26Z

/retest-required

openshift-ci · 2024-06-05T12:41:06Z

@rbaturov: all tests passed!

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.

Tal-or · 2024-06-05T13:04:28Z

/lgtm
Thank you for all your work on this

ffromani · 2024-06-05T13:13:31Z

/label acknowledge-critical-fixes-only
test-only code and this is an improvement anyway. Plus, we depend on this work for a critical feature

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 Apr 1, 2024

openshift-ci bot requested review from kpouget and Tal-or April 1, 2024 07:44

rbaturov force-pushed the add-daemonset-for-tests branch from 56d838d to 43743a9 Compare April 1, 2024 08:29

ffromani reviewed Apr 2, 2024

View reviewed changes

rbaturov force-pushed the add-daemonset-for-tests branch from 43743a9 to 7c6d163 Compare April 2, 2024 11:23

rbaturov changed the title ~~WIP: Added daemonset for inspecting nodes configuration~~ CNF-11815: Added daemonset for inspecting nodes configuration Apr 2, 2024

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

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 Apr 2, 2024

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

rbaturov force-pushed the add-daemonset-for-tests branch 2 times, most recently from d61ea00 to 294c4af Compare April 2, 2024 13:17

rbaturov changed the title ~~CNF-11815: Added daemonset for inspecting nodes configuration~~ CNF-11815: e2e: Added daemonset for inspecting nodes configuration Apr 2, 2024

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

Tal-or reviewed Apr 14, 2024

View reviewed changes

rbaturov force-pushed the add-daemonset-for-tests branch from 294c4af to 3648ece Compare April 15, 2024 07:28

rbaturov changed the title ~~CNF-11815: e2e: Added daemonset for inspecting nodes configuration~~ WIP: CNF-11815: e2e: Added daemonset for inspecting nodes configuration Apr 15, 2024

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 Apr 15, 2024

rbaturov force-pushed the add-daemonset-for-tests branch from 3648ece to ddddb84 Compare April 15, 2024 11:48

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

rbaturov force-pushed the add-daemonset-for-tests branch from ddddb84 to 1247887 Compare April 15, 2024 12:00

rbaturov changed the title ~~WIP: CNF-11815: e2e: Added daemonset for inspecting nodes configuration~~ WIP: CNF-11815: e2e: Added node inspector for inspecting nodes configuration Apr 15, 2024

rbaturov force-pushed the add-daemonset-for-tests branch from 1247887 to ee990a0 Compare April 16, 2024 08:18

rbaturov changed the title ~~WIP: CNF-11815: e2e: Added node inspector for inspecting nodes configuration~~ CNF-11815: e2e: Added node inspector for inspecting nodes configuration Apr 16, 2024

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 Apr 16, 2024

rbaturov force-pushed the add-daemonset-for-tests branch from 90cba92 to 14f169e Compare May 27, 2024 07:28

rbaturov force-pushed the add-daemonset-for-tests branch from 14f169e to 390015c Compare May 30, 2024 08:28

ffromani reviewed May 30, 2024

View reviewed changes

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

Tal-or reviewed May 30, 2024

View reviewed changes

rbaturov force-pushed the add-daemonset-for-tests branch from 390015c to fd8cb0c Compare June 2, 2024 10:17

Tal-or reviewed Jun 3, 2024

View reviewed changes

rbaturov force-pushed the add-daemonset-for-tests branch 3 times, most recently from dc71b58 to a7b20dc Compare June 3, 2024 17:10

rbaturov added 3 commits June 4, 2024 14:54

Added Z_deconfig cleanup call in run-test.sh script

f8badd8

Adding trap command in the run-test.sh script will ensure the Z_deconfig suite will run, thereby deleting the node inspector. Signed-off-by: Ronny Baturov <[email protected]>

Refactor node command execution functions

21653f0

Updated these functions to use the node inspector rather than the MCD. Furthermore, I have renamed these functions to better reflect their purposes and usage. Signed-off-by: Ronny Baturov <[email protected]>

rbaturov force-pushed the add-daemonset-for-tests branch from 639d431 to 41e92a5 Compare June 4, 2024 11:54

Unified ExecCommand and ExecCommandToString

362196e

Refactor all calls to the unified function: ExecCommand. Added a util ToString function that will be used for the calls that require the output in a string format. Signed-off-by: Ronny Baturov <[email protected]>

rbaturov force-pushed the add-daemonset-for-tests branch from 41e92a5 to 362196e Compare June 5, 2024 09:12

openshift-ci bot assigned Tal-or Jun 5, 2024

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

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Jun 5, 2024

openshift-merge-bot bot merged commit b057528 into openshift:master Jun 5, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-11815: e2e: Added node inspector for inspecting nodes configuration #1008

CNF-11815: e2e: Added node inspector for inspecting nodes configuration #1008

rbaturov commented Apr 1, 2024 •

edited

Loading

ffromani left a comment

openshift-ci-robot commented Apr 2, 2024 •

edited by openshift-ci bot

Loading

rbaturov commented Apr 2, 2024 •

edited

Loading

Tal-or Apr 14, 2024

rbaturov Apr 15, 2024

Tal-or Apr 15, 2024

rbaturov Apr 15, 2024

Tal-or Apr 15, 2024 •

edited

Loading

rbaturov Apr 15, 2024 •

edited

Loading

Tal-or Apr 16, 2024

openshift-ci-robot commented Apr 15, 2024 •

edited by openshift-ci bot

Loading

rbaturov commented Apr 15, 2024

rbaturov commented Apr 16, 2024

rbaturov commented May 27, 2024

ffromani left a comment

openshift-ci bot commented May 30, 2024

rbaturov commented Jun 2, 2024 •

edited

Loading

rbaturov commented Jun 4, 2024

rbaturov commented Jun 4, 2024

rbaturov commented Jun 5, 2024

rbaturov commented Jun 5, 2024

openshift-ci bot commented Jun 5, 2024

Tal-or commented Jun 5, 2024

ffromani commented Jun 5, 2024

CNF-11815: e2e: Added node inspector for inspecting nodes configuration #1008

CNF-11815: e2e: Added node inspector for inspecting nodes configuration #1008

Conversation

rbaturov commented Apr 1, 2024 • edited Loading

ffromani left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 2, 2024 • edited by openshift-ci bot Loading

rbaturov commented Apr 2, 2024 • edited Loading

Tal-or Apr 14, 2024

Choose a reason for hiding this comment

rbaturov Apr 15, 2024

Choose a reason for hiding this comment

Tal-or Apr 15, 2024

Choose a reason for hiding this comment

rbaturov Apr 15, 2024

Choose a reason for hiding this comment

Tal-or Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

rbaturov Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Tal-or Apr 16, 2024

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 15, 2024 • edited by openshift-ci bot Loading

rbaturov commented Apr 15, 2024

rbaturov commented Apr 16, 2024

rbaturov commented May 27, 2024

ffromani left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 30, 2024

rbaturov commented Jun 2, 2024 • edited Loading

rbaturov commented Jun 4, 2024

rbaturov commented Jun 4, 2024

rbaturov commented Jun 5, 2024

rbaturov commented Jun 5, 2024

openshift-ci bot commented Jun 5, 2024

Tal-or commented Jun 5, 2024

ffromani commented Jun 5, 2024

rbaturov commented Apr 1, 2024 •

edited

Loading

openshift-ci-robot commented Apr 2, 2024 •

edited by openshift-ci bot

Loading

rbaturov commented Apr 2, 2024 •

edited

Loading

Tal-or Apr 15, 2024 •

edited

Loading

rbaturov Apr 15, 2024 •

edited

Loading

openshift-ci-robot commented Apr 15, 2024 •

edited by openshift-ci bot

Loading

rbaturov commented Jun 2, 2024 •

edited

Loading