-
Notifications
You must be signed in to change notification settings - Fork 1.8k
CNV#64706: [DOC] Add IOMMU verification step to OpenShift Virtualization documentation. | Version 4.18 #98467
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
Conversation
|
/label CNV |
|
Hi @vladikr, I'm hoping that you can help me. I'm working on this doc bug and asked Sarika Patil, the reporter, some clarifying questions. She said the following in Jira comments: "Apologies for the delay. I believe the engineering team would be better equipped to answer your questions more precisely, so please check with them. Thanks for your understanding." Are you able to assist with doc updates for this? I found a Closed doc PR that you had helped with back in 2022 and hope that you are the right contact again. If you are not, can you please suggest a more appropriate person? Thanks very much. |
|
Hi @vladikr, Can you review or suggest someone else to take a look? Thanks. |
|
Hi vladikr, Can you take another look at this PR when you have a chance? Here is the link to the actual heading: https://98467--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/managing_vms/advanced_vm_management/virt-configuring-virtual-gpus.html#virt-adding-kernel-arguments-enable-IOMMU_virt-configuring-virtual-gpus Also, I wondered about the version number showing as 3.2. Should that change? Thanks! |
|
@aspauldi Thank you! It looks good to me. |
3.2.0 is the ignition version. I'm not sure what the most recent one is. I think OCP 4.19 supports 3.4.0 as well, but I'm not sure. |
abrennan89
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of suggestions to make this closer to the standard format https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#source-tags-for-terminal-commands-and-output
These are not blockers though and otherwise lgtm
| . Verify that IOMMU is enabled at the operating system (OS) level by entering the following command: | ||
| + | ||
| [source,terminal] | ||
| ---- | ||
| $ dmesg | grep -i iommu | ||
| ---- | ||
| * If IOMMU is enabled, output is displayed as shown in the following example: | ||
| + | ||
| .Example output | ||
| [source,terminal] | ||
| ---- | ||
| Intel: [ 0.000000] DMAR: Intel(R) IOMMU Driver | ||
| AMD: [ 0.000000] AMD-Vi: IOMMU Initialized | ||
| ---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| . Verify that IOMMU is enabled at the operating system (OS) level by entering the following command: | |
| + | |
| [source,terminal] | |
| ---- | |
| $ dmesg | grep -i iommu | |
| ---- | |
| * If IOMMU is enabled, output is displayed as shown in the following example: | |
| + | |
| .Example output | |
| [source,terminal] | |
| ---- | |
| Intel: [ 0.000000] DMAR: Intel(R) IOMMU Driver | |
| AMD: [ 0.000000] AMD-Vi: IOMMU Initialized | |
| ---- | |
| . Verify that IOMMU is enabled at the operating system (OS) level by entering the following command and observing the output: | |
| + | |
| [source,terminal] | |
| ---- | |
| $ dmesg | grep -i iommu | |
| ---- | |
| + | |
| .Example output | |
| [source,terminal] | |
| ---- | |
| Intel: [ 0.000000] DMAR: Intel(R) IOMMU Driver | |
| AMD: [ 0.000000] AMD-Vi: IOMMU Initialized | |
| ---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aspauldi Are you planning to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sjhala-ccs, what do you mean? Sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aspauldi, I meant the suggestion (highlighted in green) to remove this line - * If IOMMU is enabled, output is displayed as shown in the following example: and rewording the procedure step to Verify that IOMMU is enabled at the operating system (OS) level by entering the following command and observing the output:.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had opted to leave the content as is and should have left a comment about that.
| .Verification | ||
|
|
||
| * Verify that the new `MachineConfig` object was added. | ||
| . Verify that the new `MachineConfig` object was added by entering the following command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| . Verify that the new `MachineConfig` object was added by entering the following command: | |
| . Verify that the new `MachineConfig` object was added by entering the following command and observing the output: |
I would recommend standardizing these and calling out the need to observe the output as part of the command step. I would also add an example output here if possible, similar to the next step
|
Hi @vladikr, our docs go through a documentation peer review after SME and QE input has been added. The reviewer who looked at this PR suggested adding example output for this step: . Verify that the new
|
@aspauldi It should probably look something like: |
|
Hi @lkladnit, Are you able to test this, as Vladik mentioned regarding his not having a cluster for testing? Thanks. |
|
@aspauldi , i don't have cluster with GPU. But adding the MachineConfig works anyway, so i can confirm In OCP v4.20 version 3.5.0 is the default, i guess |
|
@aspauldi: 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. |
|
Hi @sjhala-ccs, I meant to request a merge review on this rather than a peer review. Sorry for any confusion! |
|
/cherrypick enterprise-4.18 |
|
/cherrypick enterprise-4.19 |
|
/cherrypick enterprise-4.20 |
|
@sjhala-ccs: new pull request created: #99880 In response to this:
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. |
|
@sjhala-ccs: new pull request created: #99881 In response to this:
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. |
|
@sjhala-ccs: new pull request created: #99882 In response to this:
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. |
Version(s): 4.18+
Issue: CNV-64706
Link to docs preview: https://98467--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/managing_vms/advanced_vm_management/virt-configuring-virtual-gpus.html#virt-adding-kernel-arguments-enable-IOMMU_virt-configuring-virtual-gpus
QE review: