Skip to content
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.

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

CNF-9173: e2e: mixedcpus test #892

Merged
merged 8 commits into from Feb 12, 2024

Conversation

Tal-or
Copy link
Contributor

@Tal-or Tal-or commented Dec 19, 2023

This is a first PR (more to come) that tests the mixed-cpus e2e.
It contains the following tests:

  1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
    its cgroup cpuset.

  2. Check that cpu-quota.crio.io: disable annotation works along with mixedcpus.

  3. Check that OPENSHIFT_ISOLATED_CPUS and OPENSHIFT_SHARED_CPUS env variables are under the container and configured properly.

  4. Verify that updates to shared CPUs under the profile are reflect under the containers correctly.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 19, 2023

@Tal-or: This pull request references CNF-9173 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:

This is a first PR (more to come) that tests the mixed-cpus e2e.
It contains the following tests:

  1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
    its cgroup cpuset.

  2. Check that cpu-quota.crio.io: disable annotation works along with mixedcpus.

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/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 19, 2023
@Tal-or
Copy link
Contributor Author

Tal-or commented Dec 19, 2023

depends on cri-o/cri-o#7608

@Tal-or Tal-or force-pushed the mixed_cpu_plugin_e2e branch 2 times, most recently from 9194a1b to a57ec98 Compare December 20, 2023 14:32
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 21, 2023

@Tal-or: This pull request references CNF-9173 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:

This is a first PR (more to come) that tests the mixed-cpus e2e.
It contains the following tests:

  1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
    its cgroup cpuset.

  2. Check that cpu-quota.crio.io: disable annotation works along with mixedcpus.

  3. Check that OPENSHIFT_ISOLATED_CPUS and OPENSHIFT_SHARED_CPUS env variables are under the container and configured properly.

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/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 2, 2024

@Tal-or: This pull request references CNF-9173 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:

This is a first PR (more to come) that tests the mixed-cpus e2e.
It contains the following tests:

  1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
    its cgroup cpuset.

  2. Check that cpu-quota.crio.io: disable annotation works along with mixedcpus.

  3. Check that OPENSHIFT_ISOLATED_CPUS and OPENSHIFT_SHARED_CPUS env variables are under the container and configured properly.

  4. Check if shared CPUs survive Kubelet restart.

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/test-infra repository.

@Tal-or
Copy link
Contributor Author

Tal-or commented Jan 9, 2024

/hold
depends on #906

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2024
@Tal-or
Copy link
Contributor Author

Tal-or commented Jan 9, 2024

/test e2e-aws-ovn-techpreview

@Tal-or Tal-or force-pushed the mixed_cpu_plugin_e2e branch 2 times, most recently from 8356653 to 257c1d7 Compare January 10, 2024 09:32
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 10, 2024

@Tal-or: This pull request references CNF-9173 which is a valid jira issue.

In response to this:

This is a first PR (more to come) that tests the mixed-cpus e2e.
It contains the following tests:

  1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
    its cgroup cpuset.

  2. Check that cpu-quota.crio.io: disable annotation works along with mixedcpus.

  3. Check that OPENSHIFT_ISOLATED_CPUS and OPENSHIFT_SHARED_CPUS env variables are under the container and configured properly.

  4. Verify that updates to shared CPUs under the profile are reflect under the containers correctly.

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.

@Tal-or Tal-or force-pushed the mixed_cpu_plugin_e2e branch 4 times, most recently from 2dad186 to db4015d Compare January 11, 2024 13:23
@Tal-or
Copy link
Contributor Author

Tal-or commented Jan 11, 2024

/retest

@Tal-or Tal-or force-pushed the mixed_cpu_plugin_e2e branch 2 times, most recently from 2527030 to aa4c3b0 Compare January 16, 2024 16:13
Copy link
Contributor

openshift-ci bot commented Feb 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, Tal-or

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024
@mrniranjan
Copy link
Contributor

@Tal-or @shajmakh if these tests are important, we need to add test case id's and add it to our test case management systems. Its not something very urgent can be addressed in another PR

This test changes the shared cpuset under the profile,
wait for mcp to catchup, and checks if the container
has been updated to reflect the new shared cpuset.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
Use `mustParse` to make sure we assert in case of invalid
cpuset string

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
The controller file names on cgroupv1 are different than the one on cgroupv2.
This is a pure copy-paste mistake.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
Copy link
Contributor Author

Tal-or commented Feb 7, 2024

@Tal-or @shajmakh if these tests are important, we need to add test case id's and add it to our test case management systems. Its not something very urgent can be addressed in another PR

Yes we can add id's later on, you're welcome to provide list of valid ids and we shall add them.

@ffromani
Copy link
Contributor

ffromani commented Feb 7, 2024

LGTM

* Check the len returned from split
* Use `GinkgoHelper()`

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
Copy link
Contributor Author

Tal-or commented Feb 8, 2024

/cherry-pick release-4.15

@openshift-cherrypick-robot

@Tal-or: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.15

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/test-infra repository.

@shajmakh
Copy link
Contributor

shajmakh commented Feb 8, 2024

@Tal-or @shajmakh if these tests are important, we need to add test case id's and add it to our test case management systems. Its not something very urgent can be addressed in another PR

Yes we can add id's later on, you're welcome to provide list of valid ids and we shall add them.

indeed they are, and they're part of the TP so when imported to Polarion the ids will get generated and I'll consider to add them accordingly.

/lgtm
/hold
for other reviewers
@Tal-or feel free to unhold as you see fit.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2024
@Tal-or
Copy link
Contributor Author

Tal-or commented Feb 8, 2024

Thanks @shajmakh I'm doing some local tests on 4.15 and will remove the hold if they are passing

@Tal-or
Copy link
Contributor Author

Tal-or commented Feb 12, 2024

/hold cancel
Tests are passing on 4.15 cgroupv1

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2024
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD f68b3a2 and 2 for PR HEAD 9bd0341 in total

Copy link
Contributor

openshift-ci bot commented Feb 12, 2024

@Tal-or: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 836b327 into openshift:master Feb 12, 2024
15 checks passed
@openshift-cherrypick-robot

@Tal-or: #892 failed to apply on top of branch "release-4.15":

Applying: e2e: allowed annotation to ns
Applying: e2e: mixedcpus: add e2e tests
Using index info to reconstruct a base tree...
M	test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
CONFLICT (content): Merge conflict in test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 e2e: mixedcpus: add e2e tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.15

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/test-infra repository.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-node-tuning-operator-container-v4.16.0-202402121340.p0.g836b327.assembly.stream.el9 for distgit cluster-node-tuning-operator.
All builds following this will include this PR.

Tal-or added a commit to Tal-or/cluster-node-tuning-operator that referenced this pull request Feb 13, 2024
* e2e: allowed annotation to ns

The the allowed mixedcpus annotation to the testing namespace.
Without the annotation the api admission pliugin will reject the
request for the workload creation.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: mixedcpus: add e2e tests

1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
its cgroup cpuset.

2. Check that `cpu-quota.crio.io: disable` annotation works along with mixedcpus.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: check env variable exsitance

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: cancel SMT alignment

On some environments we have very few resources.
On order to be able to run tests on those type of environment, we
must narrow down the resources required by the test pod, but yet keep their guaranteed QoS class.

This means that one is the minimal possible, but the pod will failed with SMT alignment error,
hence in such cases we want to cancel the  SMT alignment.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: test updates to shared cpus under profile

This test changes the shared cpuset under the profile,
wait for mcp to catchup, and checks if the container
has been updated to reflect the new shared cpuset.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: use `mustParse`

Use `mustParse` to make sure we assert in case of invalid
cpuset string

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: fix controller file naming

The controller file names on cgroupv1 are different than the one on cgroupv2.
This is a pure copy-paste mistake.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: addressed reviewers comments

* Check the len returned from split
* Use `GinkgoHelper()`

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

---------

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or Tal-or deleted the mixed_cpu_plugin_e2e branch February 19, 2024 09:46
openshift-merge-bot bot pushed a commit that referenced this pull request Feb 19, 2024
* CNF-9173: e2e: mixedcpus test (#892)

* e2e: allowed annotation to ns

The the allowed mixedcpus annotation to the testing namespace.
Without the annotation the api admission pliugin will reject the
request for the workload creation.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: mixedcpus: add e2e tests

1. Basic test to verify that pod which asks for shared cpus, has the shared cpus under
its cgroup cpuset.

2. Check that `cpu-quota.crio.io: disable` annotation works along with mixedcpus.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: check env variable exsitance

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: cancel SMT alignment

On some environments we have very few resources.
On order to be able to run tests on those type of environment, we
must narrow down the resources required by the test pod, but yet keep their guaranteed QoS class.

This means that one is the minimal possible, but the pod will failed with SMT alignment error,
hence in such cases we want to cancel the  SMT alignment.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: test updates to shared cpus under profile

This test changes the shared cpuset under the profile,
wait for mcp to catchup, and checks if the container
has been updated to reflect the new shared cpuset.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: use `mustParse`

Use `mustParse` to make sure we assert in case of invalid
cpuset string

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: fix controller file naming

The controller file names on cgroupv1 are different than the one on cgroupv2.
This is a pure copy-paste mistake.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: addressed reviewers comments

* Check the len returned from split
* Use `GinkgoHelper()`

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

---------

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: check if shared CPUs survive Kubelet restart

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: remove ctx arg

Leftovers from the cherry-pick which was done from 4.16.
On 4.16 we already updated most of our functions to accept context as the first argument.
On 4.15 we don't, hence we shall remove it.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

* e2e: `UpdateWithRetry` only update spec

The function contains a subtle bug on which the update
is done only for fileds under the `Spec`

This means that changes to annotations and labels
field outside the `Spec` won't take affect.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>

---------

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants