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

OSDOCS-3976: Add NodeTuning capability and Capability set v4.13 #57602

Merged
merged 1 commit into from Mar 27, 2023

Conversation

dagrayvid
Copy link
Contributor

@dagrayvid dagrayvid commented Mar 22, 2023

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 22, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 22, 2023

@dagrayvid: This pull request references OSDOCS-3976 which is a valid jira issue.

In response to this:

Version(s):
4.13

Issue:
OSDOCS-3976

PSAP-741

Link to docs preview:

QE review:

  • QE has approved this change.

Additional information:

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 openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 22, 2023
@dagrayvid dagrayvid changed the title OSDOCS-3976: Add NodeTuning capability and Capability set v4.13 WIP: OSDOCS-3976: Add NodeTuning capability and Capability set v4.13 Mar 22, 2023
@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 Mar 22, 2023
@dagrayvid
Copy link
Contributor Author

@sheriff-rh This is currently a draft, I'm not sure if got the asciidoc conditional statements quite right.

Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

This looks great, and the ifdefs look valid. The Travis build failed on the xref, but should pass after it's corrected!


[role="_additional-resources"]
.Additional resources
* xref:../scalability-and-performance/using-node-tuning-operator.adoc[Using the Node Tuning Operator]
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a second to catch this - folders are underscores (scalability_and_performance), files are dashes (using-node-tuning-operator.adoc). Additionally, we have to use anchors for file paths to work with our different distribution tools. I believe it's the customer portal, specifically, that requires anchors.

* xref:../scalability_and_performance/using-node-tuning-operator.adoc#using-node-tuning-operator[Using the Node Tuning Operator]

@dagrayvid dagrayvid force-pushed the optional-nto branch 2 times, most recently from a97af9b to 9ea3bf0 Compare March 23, 2023 13:22
@sheriff-rh
Copy link
Contributor

Local build previews (VPN required):
Cluster capabilities
Using the Node Tuning Operator

@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Mar 23, 2023

🤖 Updated build preview is available at:
https://57602--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/12096

@dagrayvid dagrayvid changed the title WIP: OSDOCS-3976: Add NodeTuning capability and Capability set v4.13 OSDOCS-3976: Add NodeTuning capability and Capability set v4.13 Mar 23, 2023
@dagrayvid
Copy link
Contributor Author

@jmencak @ashishkamra PTAL

@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 Mar 23, 2023
@sheriff-rh
Copy link
Contributor

@liqcui PTAL as well (We need QE review on all docs PRs except typos)

@jmencak
Copy link
Contributor

jmencak commented Mar 23, 2023

@jmencak @ashishkamra PTAL

It would help having direct links to the changed content in the PR description as required by the template. I was using old links provided by Andrew and saw a typo fixed by David's latest commit. So I assume the content is fine now.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 23, 2023

@dagrayvid: This pull request references OSDOCS-3976 which is a valid jira issue.

In response to this:

Version(s):
4.13

Issue:
OSDOCS-3976

PSAP-741

Link to docs preview:
<!--- Add direct link(s) to the exact page(s) with updated content from the preview build. --->

QE review:

  • QE has approved this change.

Additional information:

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

openshift-ci-robot commented Mar 23, 2023

@dagrayvid: This pull request references OSDOCS-3976 which is a valid jira issue.

In response to this:

Version(s):
4.13

Issue:
OSDOCS-3976

PSAP-741

Link to docs preview:
https://57602--docspreview.netlify.app/openshift-enterprise/latest/installing/cluster-capabilities.html#about-node-tuning-operator_cluster-capabilities

QE review:

  • QE has approved this change.

Additional information:

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.

@dagrayvid
Copy link
Contributor Author

It would help having direct links to the changed content in the PR description as required by the template. I was using old links provided by Andrew and saw a typo fixed by David's latest commit. So I assume the content is fine now.

Sorry about that, I meant to add it once the preview was generated but forgot to. It is added now.

@jmencak
Copy link
Contributor

jmencak commented Mar 24, 2023

Thank you for the changes, David.
/lgtm
/hold
for QE review.

@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 Mar 24, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2023
The Node Tuning Operator helps you manage node-level tuning by orchestrating the TuneD daemon and achieves low latency performance by using the Performance Profile controller. The majority of high-performance applications require some level of kernel tuning. The Node Tuning Operator provides a unified management interface to users of node-level sysctls and more flexibility to add custom tuning specified by user needs.

ifdef::cluster-caps[]
If you disable the NodeTuning capability, some tuning settings will not be applied to the control-plane Nodes which may limit the scalability and performance of large clusters with over 900 Nodes or 900 Routes.
Copy link

@liqcui liqcui Mar 24, 2023

Choose a reason for hiding this comment

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

The overall content looks good to me, but there is a little confusion in below content.

If you disable the NodeTuning capability, some tuning settings will not be applied to the control-plane Nodes which may limit the scalability and performance of large clusters with over 900 Nodes or 900 Routes.

This should be be applied to the control-plane. or control-plane Nodes? missing a period '.'

<large clusters with over 900 Nodes or 900 Routes. > should be <large clusters with over 900 nodes or 900 routes.>, does we should keep first letter capital for Node and Route ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The original sentence totally makes sense to me and I believe the capitalization is correct. However, I can imagine we can perhaps split it into two smaller sentences and end the first one with "applied to the control-plane Nodes." to make the content easier to interpret.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will confirm as part of the peer review process, but I think nodes and routes can be lower case. https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#api-object-formatting

PascalCase formatting applies when discussing Kubernetes objects: https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#object-references

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split this into two sentences and lower-cased nodes and routes.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2023
@sheriff-rh sheriff-rh removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 24, 2023

@dagrayvid: This pull request references OSDOCS-3976 which is a valid jira issue.

In response to this:

Version(s):
4.13

Issue:
OSDOCS-3976

PSAP-741

Link to docs preview:
https://57602--docspreview.netlify.app/openshift-enterprise/latest/installing/cluster-capabilities.html#about-node-tuning-operator_cluster-capabilities

QE review:

  • QE has approved this change.

Additional information:

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.

@sheriff-rh sheriff-rh added this to the Planned for 4.13 GA milestone Mar 24, 2023
@sheriff-rh sheriff-rh added the peer-review-needed Signifies that the peer review team needs to review this PR label Mar 24, 2023
@kelbrown20 kelbrown20 added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Mar 24, 2023
Copy link
Contributor

@kelbrown20 kelbrown20 left a comment

Choose a reason for hiding this comment

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

Hello! Great work! Just some nits and updates on the formating. Please let me know if you have any questions. Thank you!

ifdef::operators[]
[discrete]
== Purpose
endif::operators[]

Copy link
Contributor

Choose a reason for hiding this comment

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

All the other sections in this doc still have this Purpose title like here. I think that title should be added for consistency. It should just be something like

 [discrete]
== Purpose

on line 32. It might work adding your parameter cluster-caps to the ifdef thats there, but we'd have to check that the title size is consistent. You could also look at the other modules that do this for reference, ex: https://raw.githubusercontent.com/openshift/openshift-docs/main/modules/operator-marketplace.adoc

ifdef::operators[]
[discrete]
== Purpose
endif::operators[]

ifdef::cluster-caps[]
The Node Tuning Operator provides the features for the `NodeTuning` capability.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Node Tuning Operator provides the features for the `NodeTuning` capability.
The Node Tuning Operator provides features for the `NodeTuning` capability.

The Node Tuning Operator helps you manage node-level tuning by orchestrating the TuneD daemon and achieves low latency performance by using the Performance Profile controller. The majority of high-performance applications require some level of kernel tuning. The Node Tuning Operator provides a unified management interface to users of node-level sysctls and more flexibility to add custom tuning specified by user needs.

ifdef::cluster-caps[]
If you disable the NodeTuning capability, some default tuning settings will not be applied to the control-plane nodes. This may limit the scalability and performance of large clusters with over 900 nodes or 900 routes.
Copy link
Contributor

Choose a reason for hiding this comment

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

may -> might per IBMSG

@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 24, 2023
@liqcui
Copy link

liqcui commented Mar 26, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2023
@sheriff-rh sheriff-rh removed the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Mar 27, 2023
@sheriff-rh
Copy link
Contributor

sheriff-rh commented Mar 27, 2023

LGTM - Merging!

Edit: having technical difficulties 🤔 will merge ASAP.

@sheriff-rh sheriff-rh added peer-review-needed Signifies that the peer review team needs to review this PR peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 27, 2023
@sheriff-rh sheriff-rh merged commit 586d60c into openshift:main Mar 27, 2023
1 check passed
@sheriff-rh
Copy link
Contributor

/cherrypick enterprise-4.13

@openshift-cherrypick-robot

@sheriff-rh: new pull request created: #57769

In response to this:

/cherrypick enterprise-4.13

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.13 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. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants