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

CNF-16267: Add kernel page size field #1262

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

CNF-16267: Add kernel page size field #1262

wants to merge 4 commits into from

Conversation

rbaturov
Copy link
Contributor

@rbaturov rbaturov commented Dec 22, 2024 •
edited
Loading

This PR introduces the following changes:

  • KernelPageSize field has been added to performance profile API.
    This field defines the kernel page size: for x86/amd64, the only valid value is 4k, while for aarch64, the valid values are 4k, 64k.
    The default value is 4k if none is specified.
  • Validation has been added
  • Modified kernel Type selection process, adding the new kernel type 64k-pages alongside default and realtime
  • Documented this

Edit: This has been verified on an ARM cluster.

@openshift-ci 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 Dec 22, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 22, 2024

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

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 22, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rbaturov
Once this PR has been reviewed and has the lgtm label, please assign ffromani for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@rbaturov
Copy link
Contributor Author

/test all

@rbaturov rbaturov force-pushed the add-kernel-page-size-field branch from fc168a5 to c2e7ab1 Compare December 22, 2024 14:42
@rbaturov
Copy link
Contributor Author

/test all

@rbaturov rbaturov force-pushed the add-kernel-page-size-field branch from c2e7ab1 to 9534531 Compare December 22, 2024 19:14
@rbaturov
Copy link
Contributor Author

/test all

@rbaturov rbaturov marked this pull request as ready for review December 23, 2024 08:59
@openshift-ci 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 Dec 23, 2024
@openshift-ci openshift-ci bot requested review from jmencak and yanirq December 23, 2024 09:00
@yanirq
Copy link
Contributor

yanirq commented Dec 23, 2024

Add the section also to docs : https://github.com/openshift/cluster-node-tuning-operator/blob/master/docs/performanceprofile/performance_profile.md#realtimekernel

@rbaturov
Copy link
Contributor Author

/cc @MarSik @Tal-or

@openshift-ci openshift-ci bot requested review from MarSik and Tal-or December 23, 2024 14:17
@rbaturov
Copy link
Contributor Author

/retest-required

@rbaturov rbaturov changed the title Add kernel page size field CNF-16267: Add kernel page size field Dec 25, 2024
@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 25, 2024
@openshift-ci-robot
Copy link
Contributor

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

@rbaturov: This pull request references CNF-16267 which is a valid jira issue.

In response to this:

This PR introduces the following changes:

  • KernelPageSize field has been added to performance profile API.
    This field defines the kernel page size: for x86/amd64, the only valid value is 4K, while for aarch64, the valid values are 4K, 64K.
    The default value is 4K if none is specified.
  • Validation has been added
  • Modified kernel Type selection process, adding the new kernel type 64k-pages alongside default and realtime
  • Documented this

Edit: This has been verified on an ARM cluster.

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
api: added KernelPageSize to performance profile API
1049027 
KernelPageSize defines the size of the kernel page.
For x86/amd64, the only valid value is 4k.
For aarch64, the valid values are 4k, 64k.
The default value is 4k, if none specified.

Signed-off-by: Ronny Baturov <[email protected]>
@rbaturov
validation: added validation to KernelPageSize field
0c3613f 
This update introduces the following validation checks for the KernelPageSize field:

* On x86/amd64 systems, only 4k is accepted.
* On aarch64 systems, 4k is accepted unconditionally, while 64k is accepted only if the real-time kernel is disabled (current supported behavior).
* Any other values are rejected.

Note: the validation ensures that all nodes in the MCP have the same architecture and determines the CPU architecture before running the validation checks mentioned above.
Signed-off-by: Ronny Baturov <[email protected]>
@rbaturov
controller: add support for 64k-pages kernel on aarch64
ebd3f73 
This commit consist of the following changes:
* Enhanced the performanceprofile controller to choose 64k-pages MC kernelType, when 64k pages specified.
* Added unit tests to cover the following scenarios:
 RealTime kernel disabled + 64k => 64-pages kernelType
 RealTime kernel disabled + 4k => default kernelType
 RealTime kernel enabled + 4k => realtime kernelType

Note that the case of RealTime kernel enabled + 64k is not tested, as it is expected to be rejected by validation.

Signed-off-by: Ronny Baturov <[email protected]>
@rbaturov
docs: documenting kernelPageSize
194a3c7 
Signed-off-by: Ronny Baturov <[email protected]>
@rbaturov rbaturov force-pushed the add-kernel-page-size-field branch from 1828054 to 194a3c7 Compare January 2, 2025 10:46
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 2, 2025 •
edited by openshift-ci bot
Loading

@rbaturov: This pull request references CNF-16267 which is a valid jira issue.

In response to this:

This PR introduces the following changes:

  • KernelPageSize field has been added to performance profile API.
    This field defines the kernel page size: for x86/amd64, the only valid value is 4k, while for aarch64, the valid values are 4k, 64k.
    The default value is 4k if none is specified.
  • Validation has been added
  • Modified kernel Type selection process, adding the new kernel type 64k-pages alongside default and realtime
  • Documented this

Edit: This has been verified on an ARM cluster.

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.

swatisehgal
swatisehgal reviewed Jan 3, 2025
View reviewed changes
docs/performanceprofile/performance_profile.md
## KernelPageSize

KernelPageSize defines the kernel page size that will be used by the kernel.
4k is the default value, 64k is only supported on aarch64.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add here or in the PerformanceProfileSpec section that real time kernel with 64k kernel page size is not supported with on aarch64. WDYT?

@swatisehgal
Copy link
Contributor

/test okd-scos-e2e-aws-ovn

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 3, 2025

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swatisehgal swatisehgal swatisehgal left review comments

@MarSik MarSik MarSik left review comments

@jmencak jmencak Awaiting requested review from jmencak

@yanirq yanirq Awaiting requested review from yanirq

@Tal-or Tal-or Awaiting requested review from Tal-or

Assignees
No one assigned
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rbaturov @yanirq @openshift-ci-robot @swatisehgal @MarSik