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

Refactor and add new tests to hugepages e2e tests #91243

Merged
merged 1 commit into from
Jun 8, 2020
Merged

Refactor and add new tests to hugepages e2e tests #91243

merged 1 commit into from
Jun 8, 2020

Conversation

cynepco3hahue
Copy link

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Add e2e tests to cover usage of multiple hugepages with different
page sizes under the same pod.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
It should be merged together with new prow jobs - kubernetes/test-infra#17634

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [Usage]: https://github.com/kubernetes/enhancements/blob/8137e8370a55b789f17fbe5e674a690063a5b735/keps/sig-node/20190129-hugepages.md#pod-specification

Signed-off-by: Artyom Lukianov alukiano@redhat.com

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 19, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @cynepco3hahue. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 19, 2020
@cynepco3hahue
Copy link
Author

I unsure what kind should I set.

@cynepco3hahue cynepco3hahue mentioned this pull request May 19, 2020
Merged
@Joseph-Irving
Copy link
Member

/kind feature
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. kind/feature Categorizes issue or PR as related to a new feature. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 19, 2020
@cynepco3hahue cynepco3hahue force-pushed the e2e_multiply_hugepages branch from e7001d5 to cc1db0a Compare May 19, 2020 15:15
@cynepco3hahue
Copy link
Author

/retest

2 similar comments
@cynepco3hahue
Copy link
Author

/retest

@cynepco3hahue
Copy link
Author

/retest

@bart0sh
Copy link
Contributor

bart0sh commented May 26, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2020
@cynepco3hahue
Copy link
Author

/test pull-kubernetes-node-kubelet-serial-hugepages

1 similar comment
@cynepco3hahue
Copy link
Author

/test pull-kubernetes-node-kubelet-serial-hugepages

@dims
Copy link
Member

dims commented Jun 1, 2020

/assign @sjenning @derekwaynecarr

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 7, 2020
@cynepco3hahue
Copy link
Author

@bart0sh Can you please review again?

@cynepco3hahue
Copy link
Author

/retest

odinuge
odinuge approved these changes Jun 7, 2020
View reviewed changes
Copy link
Member

@odinuge odinuge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the cleanup! As other said, it would also be nice with support for other sizes, but this is a great start. Ref. #78495 arm64 support 4 different sizes. As far as I remember the CI infra on gce only support 2MiB huge pages, but that would only skip the 1GiB pages, so that should be no issue.

I haven't run the tests locally, but the changes looks good to me. Since this is already approved, ill add a hold so it isn't merged right away. In case some other reviewer or you @cynepco3hahue would like to merge it, feel free to cancel the hold. 😄

/lgtm
/kind cleanup
/hold

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 7, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2020
@cynepco3hahue
Copy link
Author

cynepco3hahue commented Jun 7, 2020 •
edited
Loading

@odinuge Thanks for the review, in case, if you want to test hugepages under the CI, we have now a separate job for it - pull-kubernetes-node-kubelet-serial-hugepages, currently it supports 2Mi and 1Gi hugepages.

@odinuge
Copy link
Member

odinuge commented Jun 7, 2020

You want to test hugepages under the CI, we have now a separate job for it - `pull-kubernetes-node-kubelet-serial-hugepages, currently it supports 2Mi and 1Gi hugepages.

Yes, saw that just after leaving the review! Fantastic!

Here is the testgridlink for those who are looking for it: https://testgrid.k8s.io/sig-node-kubelet#kubelet-serial-gce-e2e-hugepages 😄

Refactor and add new tests to hugepages e2e tests
a4b367a 
Add tests to cover usage of multiple hugepages with different
page sizes under the same pod.

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
@cynepco3hahue cynepco3hahue force-pushed the e2e_multiply_hugepages branch from 23307ee to a4b367a Compare June 8, 2020 08:24
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2020
@cynepco3hahue
Copy link
Author

/test pull-kubernetes-node-kubelet-serial-hugepages

@cynepco3hahue
Copy link
Author

/retest

@cynepco3hahue cynepco3hahue requested review from odinuge and bsdnet June 8, 2020 10:47
odinuge
odinuge approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Another lgtm from me, as this looks good! 😄 Agree that it is smart to move it to a NodeSpecialFeature so we don't "help" ci-kubernetes-node-kubelet-serial to flake due to memory pressure.

I think this is good to go, so no hold this time.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2020
@odinuge
Copy link
Member

odinuge commented Jun 8, 2020

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 8, 2020
@odinuge
Copy link
Member

odinuge commented Jun 8, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2020
@cynepco3hahue
Copy link
Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 0e2220b into kubernetes:master Jun 8, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 8, 2020
@hasheddan hasheddan mentioned this pull request Jun 8, 2020
Closed
@bart0sh bart0sh removed their assignment Jun 12, 2020
@cynepco3hahue cynepco3hahue mentioned this pull request Feb 9, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odinuge odinuge odinuge approved these changes

@tallclair tallclair Awaiting requested review from tallclair

@yifan-gu yifan-gu Awaiting requested review from yifan-gu

@bsdnet bsdnet Awaiting requested review from bsdnet

Assignees

@sjenning sjenning

@odinuge odinuge

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
9 participants
@cynepco3hahue @k8s-ci-robot @Joseph-Irving @bart0sh @dims @derekwaynecarr @odinuge @bsdnet @sjenning