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

node: start moving the resource management docs to concepts #48797

Merged
merged 1 commit into from
Nov 26, 2024
Merged

node: start moving the resource management docs to concepts #48797

merged 1 commit into from
Nov 26, 2024
+289 −197

Conversation

ffromani
Copy link
Contributor

@ffromani ffromani commented Nov 21, 2024 •
edited
Loading

Description

Move the cpu management policies and options docs from tasks to concepts
xref: #48340 (review)

In the 1.32 cycle I don't have capacity to move except for CPU manager, which is the worst offender anyway. Other managers should follow suite, hopefully in the 1.33 cycle already.

Issue

Closes: #38121 (albeit in a different and IMO better way)

@k8s-ci-robot k8s-ci-robot added this to the 1.32 milestone Nov 21, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2024
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Nov 21, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 21, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 21, 2024 •
edited
Loading

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

Name Link
🔨 Latest commit 96a2e17
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6745b5744a6dbc00088b46ea

@ffromani ffromani mentioned this pull request Nov 21, 2024
Merged
@ffromani ffromani changed the title node: move resource management docs to concepts node: start moving the resource management docs to concepts Nov 21, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 21, 2024

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit dbdd3ec
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/673f1c378d677600096cf597
😎 Deploy Preview https://deploy-preview-48797--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 configuration.

@netlify Netlify
Copy link

netlify bot commented Nov 21, 2024 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 96a2e17
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6745b5747384020008c98dd0
😎 Deploy Preview https://deploy-preview-48797--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 configuration.

@tengqm
Copy link
Contributor

tengqm commented Nov 21, 2024

Totally agree to this refactoring, however, I'd suggest we postpone this because 1.32 is about to be released soon. Propagating this change to localization teams takes some time.

@ffromani
Copy link
Contributor Author

Totally agree to this refactoring, however, I'd suggest we postpone this because 1.32 is about to be released soon. Propagating this change to localization teams takes some time.

cc @sftim so we go ahead with #48340 as-is?

@ffromani
Copy link
Contributor Author

also note this change will impact PRs #48469 and #48356 other than #48340

@ffromani
Copy link
Contributor Author

Totally agree to this refactoring, however, I'd suggest we postpone this because 1.32 is about to be released soon. Propagating this change to localization teams takes some time.

If this helps, what I want to do here is 90% content movement (which I honestly believe helps anyway). @sftim could you please provide your take here about @tengqm 's concern, also considering #48797 (comment) ?

@sftim
Copy link
Contributor

sftim commented Nov 22, 2024

Totally agree to this refactoring, however, I'd suggest we postpone this because 1.32 is about to be released soon. Propagating this change to localization teams takes some time.

The decision is about what to postpone

  • adding the new code
  • fixing the documentation

In an ideal world, we avoid adding features where we don't also have capacity to document them well. That's what commercial product docs often aim for. In the same ideal world, we planned in that refactoring early.

See #48340 (comment) for a compromise option. How does that sound?

@sftim
Copy link
Contributor

sftim commented Nov 22, 2024

If we can get the refactoring done within a few days of now, I'd prefer to land it and redo the feature PRs.

@ffromani
Copy link
Contributor Author

If we can get the refactoring done within a few days of now, I'd prefer to land it and redo the feature PRs.

ok, fair point. Let's timebox this attempt. I'll need some sig-node reviews and I asked for them: https://kubernetes.slack.com/archives/C0BP8PW9G/p1732284959539839
If i can get them and get #48797 in good shape we can see how we can serialize the merges here. Otherwise I'll push to make it ready for early 1.33 merge (or when the floodgates open anyway)

sftim
sftim reviewed Nov 23, 2024
View reviewed changes
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.

/lgtm

content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
content/en/docs/tasks/administer-cluster/cpu-management-policies.md Outdated Show resolved Hide resolved
content/en/docs/tasks/administer-cluster/cpu-management-policies.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
content/en/docs/concepts/policy/node-resource-managers.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: df88e29f6338dc3d61435521fba556715bf73816

@ffromani
Copy link
Contributor Author

thanks @sftim ! I'm adopting all your suggestions

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2024
@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 26, 2024
@ffromani
node: start moving resource management docs to concepts
96a2e17 
We have reached a point where the existing CPU management task page is quite hard to follow.
Start moving the resource management concepts to the concept page.

We begin with the CPU management policies, the worst offender right now.
Over time, the plan is to move all the concepts from tasks in the
concepts page.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@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 26, 2024
@sftim
Copy link
Contributor

sftim commented Nov 26, 2024

/label refactor

@k8s-ci-robot k8s-ci-robot added the refactor Indicates a PR with large refactoring changes e.g. removes files or moves content label Nov 26, 2024
sftim
sftim reviewed Nov 26, 2024
View reviewed changes
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.

Informal LGTM
/approve

Ready for LGTM

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 26, 2024
@katcosgrove
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: 3dc8f3cd22192f0a110f9017cbad3dfe2459bc09

@k8s-ci-robot k8s-ci-robot merged commit 58c8107 into kubernetes:dev-1.32 Nov 26, 2024
6 checks passed
@ffromani ffromani deleted the issue-38121-cpu-manager branch November 26, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@katcosgrove katcosgrove

@sftim sftim

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. refactor Indicates a PR with large refactoring changes e.g. removes files or moves content size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.32
Development

Successfully merging this pull request may close these issues.
5 participants
@ffromani @tengqm @sftim @k8s-ci-robot @katcosgrove