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

Add CPU Pinning document #639

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Conversation

brandboat
Copy link
Member

related to harvester/harvester#6148, add cpu pinning document.

Copy link

github-actions bot commented Sep 9, 2024

Name Link
🔨 Latest commit 0caee10
😎 Deploy Preview https://66ed39e4fe793624f03b84c4--harvester-preview.netlify.app


### Limitations

- Only one master node can enable/disable the CPU Manager at a time.
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to rephrase this, since the CPU manager can be enable on any node in the cluster except the witness node

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, already refine the limitations part, also add description about what harvester behaves after user checks the enable CPU pinning checkbox in virtual machine page. As you mentioned offline, this would help user understand how to enable cpu pinning via YAML.

Signed-off-by: Cooper Tseng <cooper.tseng@suse.com>
Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

Let me know if you have concerns about the changes.

docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
Comment on lines 64 to 66
- Only one master node can enable or disable the CPU Manager at a time. Note that the witness node cannot perform these actions.
- You must wait for an operation (enable/disable) to finish before starting another on the same node.
- Any VMs with CPU pinning enabled must be stopped before disabling CPU Manager on that node.
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
- Only one master node can enable or disable the CPU Manager at a time. Note that the witness node cannot perform these actions.
- You must wait for an operation (enable/disable) to finish before starting another on the same node.
- Any VMs with CPU pinning enabled must be stopped before disabling CPU Manager on that node.
- The CPU Manager cannot be enabled on the witness node.
- You can enable or disable the CPU Manager on only one management node at a time.
- You must wait for an operation (enabling or disabling) to be completed before starting another.
- VMs with CPU pinning enabled must be stopped before CPU Manager is disabled on the corresponding node.

Copy link
Contributor

Choose a reason for hiding this comment

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

CPU manager is not limited to management/controlplane node, it can be enable on any node in the cluster. We shoudl rephrase this to better reflect this

You can enable or disable the CPU Manager on only one management node at a time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment @ibrokethecloud ! What I want to emphasize here is that users could only enable/disable cpu manager on one management node at the same time.

Just a note, I add this limitation since enable/disable cpu manager requires restarting rke2-server on management node, if all rke2-server are restarting in the same time, that would cause whole cluster unavailable.

docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
Copy link

@a110605 a110605 left a comment

Choose a reason for hiding this comment

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

The doc looks clear to me.


- The CPU Manager cannot be enabled on the witness node.

- You can enable or disable the CPU Manager on only one management node at a time.
Copy link
Member

@bk201 bk201 Sep 19, 2024

Choose a reason for hiding this comment

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

This sounds like when a user enables CPU manager on a management node and after the operation finishes, he/she can't enable another management node. It would be nice to re-phase it more clearly.

Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

I responded to the feedback and recently added/modified text.

docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
docs/vm/cpu-pinning.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jillian-maroket jillian-maroket left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for rephrasing.

@jillian-maroket jillian-maroket merged commit 1afd3a1 into harvester:main Oct 8, 2024
3 checks passed
@brandboat brandboat deleted the HARV-6148 branch October 8, 2024 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants