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

topologymanager: document topology manager policy options #37668

Merged

Conversation

PiotrProkop
Copy link
Contributor

@PiotrProkop PiotrProkop commented Nov 2, 2022

Enhancement link: kubernetes/enhancements#3545
Implementation link: kubernetes/kubernetes#112914

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 2, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @PiotrProkop!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 2, 2022
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 2, 2022
@netlify
Copy link

netlify bot commented Nov 2, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 61dd008
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/636a22fdccb1c100082ead74
😎 Deploy Preview https://deploy-preview-37668--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks @PiotrProkop

Please don't update content/en/docs/reference/command-line-tools-reference/kubelet.md in this PR. That document is semi-manually autogenerated and we'll update it separately.

@PiotrProkop PiotrProkop force-pushed the topology-manager-policy-options branch 2 times, most recently from c8df1df to 155766f Compare November 7, 2022 08:33
@PiotrProkop
Copy link
Contributor Author

@sftim updated.

@klueska
Copy link
Contributor

klueska commented Nov 7, 2022

You need to retarget this to the dev-1.26 branch.

@PiotrProkop PiotrProkop changed the base branch from main to dev-1.26 November 8, 2022 09:25
@PiotrProkop PiotrProkop changed the base branch from dev-1.26 to main November 8, 2022 09:25
@PiotrProkop PiotrProkop force-pushed the topology-manager-policy-options branch from 155766f to 61dd008 Compare November 8, 2022 09:35
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022
@PiotrProkop PiotrProkop changed the base branch from main to dev-1.26 November 8, 2022 09:36
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2022
@PiotrProkop PiotrProkop force-pushed the topology-manager-policy-options branch from 61dd008 to d4dd86c Compare November 8, 2022 09:52
@netlify
Copy link

netlify bot commented Nov 8, 2022

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 701ed98
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6372158ea4b014000ae25373

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2022
@PiotrProkop PiotrProkop force-pushed the topology-manager-policy-options branch from d4dd86c to c6a794d Compare November 10, 2022 08:34
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2022
@ffromani
Copy link
Contributor

LGTM about the technical content from sig-node perspective.

Copy link
Contributor

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

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

Noticed a few minor NITs but looks good overall from node-perspective.

@PiotrProkop PiotrProkop force-pushed the topology-manager-policy-options branch from c6a794d to 13967ac Compare November 14, 2022 08:37
@@ -727,6 +730,15 @@ Each feature gate is designed for enabling/disabling a specific feature:
- `TopologyManager`: Enable a mechanism to coordinate fine-grained hardware resource
assignments for different components in Kubernetes. See
[Control Topology Management Policies on a node](/docs/tasks/administer-cluster/topology-manager/).
- `TopologyManagerPolicyAlphaOptions`: Allow fine-tuning of topology manager policies,
experimental, Alpha-quality options.
This feature gate guards *a group* of topology manager options whose quality level is alpha.
Copy link
Contributor

Choose a reason for hiding this comment

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

So the features behind this gate are designed to be a mystery, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I've contribute a near-identical feature for cpumanager). The intention here is AFACT not to hide the specific names nor to confuse the users. However we need to deal with the fact that the set of features guarded by these FGs are designed to rotate. We describe the FG in detail and their maturity level in the component-specific docs: https://github.com/kubernetes/website/blob/main/content/en/docs/tasks/administer-cluster/cpu-management-policies.md#static-policy-options

We can replicate the details here if this is less confusing for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now the only implemented policy option is prefer-closest-numa-nodes and it is an alpha option. I think the list of options and its stage should be documented in topology-manager specific docs not here. I will add a section which feature-gates has to be enabled to test out this option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

section added, please let me know if it helps 😄

- `TopologyManagerPolicyBetaOptions`: Allow fine-tuning of topology manager policies,
experimental, Beta-quality options.
This feature gate guards *a group* of topology manager options whose quality level is alpha.
This feature gate will never graduate to stable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. We don't want users to know what are behind the gate, and we want them to try them out?

Copy link
Contributor

Choose a reason for hiding this comment

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

The specific options gated by this feature gate is described in the relevant documentation for the TopologyManager itself, e.g.
content/en/docs/tasks/administer-cluster/topology-manager.md.

There is no precedent for listing out the specific options here (if that is what you are suggesting).

Signed-off-by: PiotrProkop <pprokop@nvidia.com>
@PiotrProkop PiotrProkop force-pushed the topology-manager-policy-options branch from 13967ac to 701ed98 Compare November 14, 2022 10:16
@k8s-ci-robot k8s-ci-robot 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 Nov 14, 2022
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/lgtm
technical content from sig-node perspective

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1531e21dc41be72021e66eb7fd65adaa880bf504

@krol3
Copy link
Contributor

krol3 commented Nov 28, 2022

Hi @PiotrProkop ! This PR needs a doc review by Mon Nov 28th to get this into the release. Please reach out to required SIGs to get their review. Thank you!

@ffromani
Copy link
Contributor

/assign @sftim @tengqm

@PiotrProkop
Copy link
Contributor Author

@krol3 I've got lgtm label from sig/node

@klueska
Copy link
Contributor

klueska commented Nov 28, 2022

/lgtm
/approve
(as sig-node-approver)

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

We should add a list of alpha options and beta options into a new page within https://kubernetes.io/docs/reference/node/
(and not https://kubernetes.io/docs/tasks/administer-cluster/topology-manager/ which is a task page; task pages are the wrong place to put reference docs)

Although that change is really important, for me it doesn't block merging this PR.

@PiotrProkop / @klueska would you be willing to file a docs issue about adding that page, so we don't forget? Once the work is at least tracked, I think this is good to go.

@PiotrProkop
Copy link
Contributor Author

@sftim #38119. done 😄

@sftim
Copy link
Contributor

sftim commented Nov 28, 2022

Thanks
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: klueska, sftim

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8531bd1 into kubernetes:dev-1.26 Nov 28, 2022
@k8s-ci-robot k8s-ci-robot added this to the 1.26 milestone Nov 28, 2022
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. 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.

8 participants