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.

Sign up for GitHub

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

Jump to bottom

Topology Manager and Memory manager e2e tests on multi-NUMA machines #28369

Merged
merged 2 commits into from
Jan 11, 2023
Merged

Topology Manager and Memory manager e2e tests on multi-NUMA machines #28369

merged 2 commits into from
Jan 11, 2023

Conversation

swatisehgal
Copy link
Contributor

As Topology Manager and Memory Manager are both candidates for GA graduation, it is important to make sure that
e2e tests are not skipped due to lack of multi-NUMA machines in the test infrastructure.

This PR proposes a new job definition image-config-serial-multi-numa.yaml with image config specifying n2d-standard-32 as machine. Both Topology Manager and memory manager then subsequently refer to this file for execution of e2e test on multi-numa machines.

Issue: #28211.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/config Issues or PRs related to code in /config area/jobs sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 5, 2023
@k8s-ci-robot k8s-ci-robot requested review from dims and sjenning January 5, 2023 17:18
@swatisehgal
Copy link
Contributor Author

/sig node

@swatisehgal
Copy link
Contributor Author

/cc @BenTheElder @SergeyKanzhelev

@bart0sh
Copy link
Contributor

bart0sh commented Jan 9, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2023
SergeyKanzhelev
SergeyKanzhelev reviewed Jan 11, 2023
View reviewed changes
jobs/e2e_node/image-config-serial-multi-numa.yaml Outdated
ubuntu:
image: ubuntu-gke-2204-1-24-v20220623
project: ubuntu-os-gke-cloud
# Using `n2d-standard-32` to enable Topology manager node e2e tests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Using `n2d-standard-32` to enable Topology manager node e2e tests.
# Using `n2d-standard-32` that has 2 numa nodes to enable Topology manager node e2e tests.

SergeyKanzhelev
SergeyKanzhelev reviewed Jan 11, 2023
View reviewed changes
jobs/e2e_node/image-config-serial-multi-numa.yaml Outdated
image_family: cos-89-lts # deprecated after March 2023 (https://cloud.google.com/container-optimized-os/docs/release-notes)
project: cos-cloud
metadata: "user-data<test/e2e_node/jenkins/cos-init-live-restore.yaml,gci-update-strategy=update_disabled"
# Using `n2d-standard-32` to enable Topology manager node e2e tests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Using `n2d-standard-32` to enable Topology manager node e2e tests.
# Using `n2d-standard-32` that has 2 numa nodes to enable Topology manager node e2e tests.

SergeyKanzhelev
SergeyKanzhelev approved these changes Jan 11, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

with the small in in comments

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023
@SergeyKanzhelev
Copy link
Member

/hold

to address comments update

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2023
@swatisehgal
node: e2e: ci: Allow Topology-mgr tests to run on multi-NUMA machines
eb672a6 
Topology Manager e2e tests are skipped if they are not running
on multi-NUMA hardware as can be seen here:
https://github.com/kubernetes/kubernetes/blob/release-1.26/test/e2e_node/topology_manager_test.go#L957-L959.

This change allows for tests that rely on multi-NUMA to run on
a machine (n2d-standard-32) that provides this capability.

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
@swatisehgal
node: e2e: ci: Allow memory-mgr tests to run on multi-NUMA machines
bb75fad 
Memory manager e2e checks if the underlying machine has multiple
NUMA nodes as can be seen here: https://github.com/kubernetes/kubernetes/blob/release-1.26/test/e2e_node/memory_manager_test.go#L571-L574.
In case the system doesn't have multiple NUMA nodes, the tests
are skipped.

Similar to the previous commit, this change allows memory manager
e2e tests (which rely on multi-NUMA) to run on a machine
(n2d-standard-32) that provides this capability.

Signed-off-by: Swati Sehgal <swsehgal@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2023
SergeyKanzhelev
SergeyKanzhelev approved these changes Jan 11, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/unhold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 11, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SergeyKanzhelev, swatisehgal

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

@k8s-ci-robot k8s-ci-robot merged commit 45fa74a into kubernetes:master Jan 11, 2023
@k8s-ci-robot
Copy link
Contributor

@swatisehgal: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key sig-node-presubmit.yaml using file config/jobs/kubernetes/sig-node/sig-node-presubmit.yaml

In response to this:

As Topology Manager and Memory Manager are both candidates for GA graduation, it is important to make sure that
e2e tests are not skipped due to lack of multi-NUMA machines in the test infrastructure.

This PR proposes a new job definition image-config-serial-multi-numa.yaml with image config specifying n2d-standard-32 as machine. Both Topology Manager and memory manager then subsequently refer to this file for execution of e2e test on multi-numa machines.

Issue: #28211.

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.

@swatisehgal swatisehgal deleted the e2e-on-multi-numa branch January 19, 2023 11:08
@swatisehgal swatisehgal mentioned this pull request Jun 7, 2023
Merged
@swatisehgal swatisehgal mentioned this pull request Jun 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@dims dims Awaiting requested review from dims

@sjenning sjenning Awaiting requested review from sjenning

@BenTheElder BenTheElder Awaiting requested review from BenTheElder

Assignees

@SergeyKanzhelev SergeyKanzhelev

@bart0sh bart0sh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/jobs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@swatisehgal @bart0sh @SergeyKanzhelev @k8s-ci-robot