Skip to content

CNV-49612: Document CPU hotplug #83243

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
Closed
wants to merge 1 commit into from
Closed

CNV-49612: Document CPU hotplug #83243

wants to merge 1 commit into from

Conversation

sbeskin-redhat
Copy link
Contributor

@sbeskin-redhat sbeskin-redhat commented Oct 9, 2024 •
edited
Loading

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

openshift-ci-robot commented Oct 9, 2024 •
edited by openshift-ci bot
Loading

@sbeskin-redhat: This pull request references CNV-49612 which is a valid jira issue.

In response to this:

Resolves: https://issues.redhat.com/browse/CNV-49612

OCP 4.17
CNV 4.17

Preview:

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.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 9, 2024
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Oct 9, 2024 •
edited
Loading

🤖 Tue Oct 15 16:33:41 - Prow CI generated the docs preview:

https://83243--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/virtual_machines/virt-edit-vms.html
https://83243--ocpdocs-pr.netlify.app/openshift-rosa/latest/virt/virtual_machines/virt-edit-vms.html

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 9, 2024 •
edited by openshift-ci bot
Loading

@sbeskin-redhat: This pull request references CNV-49612 which is a valid jira issue.

In response to this:

Resolves: https://issues.redhat.com/browse/CNV-49612

OCP 4.17
CNV 4.17

Preview: https://83243--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/virtual_machines/virt-edit-vms.html#virt-hot-plugging-cpu_virt-edit-vms

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.

vsibirsk
vsibirsk suggested changes Oct 9, 2024
View reviewed changes
stu-gott
stu-gott reviewed Oct 10, 2024
View reviewed changes
Copy link

@stu-gott stu-gott left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of questions.

modules/virt-hot-plugging-cpu.adoc Outdated
. Select the *vCPU* radio button.
. Enter the desired number of vCPU sockets and click *Save*.

The system applies these changes immediately. If the VM is migratable, a live migration is triggered. If not, or if the changes cannot be live-updated, a `RestartRequired` condition is added to the VM.
Copy link

Choose a reason for hiding this comment

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

Could the words "applies changes immediately" be misinterpreted to mean there's no delay at all from when the change is requested until the CPUs are available? I'm assuming you meant the changes to the spec are immediate.

modules/virt-hot-plugging-cpu.adoc Outdated

= Hot plugging CPUs on a virtual machine

You can add or remove the number of CPU sockets allocated to a virtual machine (VM) without having to restart the VM by using the {product-title} web console.
Copy link

Choose a reason for hiding this comment

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

@jean-edouard the word "sockets" is completely correct, but is it needed in this context?

Copy link

Choose a reason for hiding this comment

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

I'm not sure I understand the question, but yes I think it's important to tell the user we hotplug sockets and not CPUs, because that means hotplugging 1 (socket) will most likely result in more than 1 CPU getting hotplugged.

@sbeskin-redhat
Copy link
Contributor Author

@stu-gott @vsibirsk
I made some minor changes, please review. Let's finalize the PR, we need to merge it now.

jean-edouard
jean-edouard approved these changes Oct 15, 2024
View reviewed changes
Copy link

@jean-edouard jean-edouard left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple questions, very minor, feel free to ignore.
Thanks!

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Oct 15, 2024
@sbeskin-redhat sbeskin-redhat mentioned this pull request Oct 15, 2024
Merged
@sjhala-ccs sjhala-ccs added peer-review-in-progress Signifies that the peer review team is reviewing this PR CNV Label for all CNV PRs branch/enterprise-4.17 branch/enterprise-4.18 and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Oct 15, 2024
@sjhala-ccs sjhala-ccs added this to the Continuous Release milestone Oct 15, 2024
sjhala-ccs
sjhala-ccs reviewed Oct 15, 2024
View reviewed changes
Copy link
Contributor

@sjhala-ccs sjhala-ccs left a comment

Choose a reason for hiding this comment

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

The content LGTM but this PR should be based off the main branch (and cherry picked to 4.17 and 4.18) and not enterprise-4.17.

@sjhala-ccs sjhala-ccs added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Oct 15, 2024
@sbeskin-redhat
Copy link
Contributor Author

@sjhala-ccs But the main branch is 4.18 - why should it be there?

@sjhala-ccs
Copy link
Contributor

@sjhala-ccs But the main branch is 4.18 - why should it be there?

main is different from the specific version branches such as enterprise-4.17 or enterprise-4.18. If your content applies to 4.17 and later versions, then the PR should be based off main and the content cherry picked to 4.17 and 4.18 (latest version).
The OpenShift Docs Manual likely has more information on repo branching.

@sbeskin-redhat
Copy link
Contributor Author

@sjhala-ccs I see, thank you. Should i redo the PR? Will you be able to merge it?

@sbeskin-redhat sbeskin-redhat mentioned this pull request Oct 15, 2024
Merged
@sbeskin-redhat
Copy link
Contributor Author

@sjhala-ccs New PR: #83558

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 15, 2024

@sbeskin-redhat: 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.

@sbeskin-redhat
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Dec 31, 2024
@sbeskin-redhat
Copy link
Contributor Author

Replaced by #83558

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

Reviewers

@sjhala-ccs sjhala-ccs sjhala-ccs left review comments

+3 more reviewers

@stu-gott stu-gott stu-gott left review comments

@jean-edouard jean-edouard jean-edouard approved these changes

@vsibirsk vsibirsk vsibirsk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

branch/enterprise-4.17 branch/enterprise-4.18 CNV Label for all CNV PRs jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. peer-review-done Signifies that the peer review team has reviewed this PR peer-review-needed Signifies that the peer review team needs to review this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

Continuous Release

Development

Successfully merging this pull request may close these issues.

7 participants

@sbeskin-redhat @openshift-ci-robot @ocpdocs-previewbot @sjhala-ccs @stu-gott @jean-edouard @vsibirsk