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

updated 'deploy-gpu-node-pool.md' with instructions for using Headless OS #3171

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mgodfre3
Copy link
Contributor

The instructions for uninstalling drivers was written for a Desktop Experience Server, not Azure Stack HCI, which is intended OS, I updated docs to include PowerShell examples.

Copy link
Contributor

@mgodfre3 : Thanks for your contribution! The author(s) have been notified to review your proposed change.

Copy link
Contributor

Learn Build status updates of commit 0018650:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@Court72
Copy link
Contributor

Court72 commented Jun 11, 2024

@sethmanheim

Can you review the proposed changes?

Important: When the changes are ready for publication, adding a #sign-off comment is the best way to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator bot added the aq-pr-triaged Tracking label for the PR review team label Jun 11, 2024
Copy link
Contributor

Learn Build status updates of commit 412c583:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 03d48f0:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Run the following command in your Powershell session, and replace `.\oem1.inf` with the value in **Published Name** from the previous **PNPUTIL Enum-Devices** output:

```powershell
pnputil /delete-driver .\oem1.inf /uninstall /reboot

Choose a reason for hiding this comment

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

We'd better replace .\oem1.inf to something like <oem#.inf>, avoiding someone copy, paste, run without noticing your guideline above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I call this out on line 89 and suggest that you replace the value with a value from the output.

Choose a reason for hiding this comment

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

Yes. But oem1.inf is a real driver. If someone just copy, paste and run without replacing the value, a driver will be removed unexpectedly. By given <oem#.inf> in sample code, the command with fail with noop to system. It's safer.

@@ -82,7 +107,8 @@ When you uninstall the host driver, the physical GPU goes into an error state. Y
For each GPU (3D Video Controller) device, run the following commands in PowerShell. Copy the instance ID; for example, `PCI\VEN_10DE&DEV_1EB8&SUBSYS_12A210DE&REV_A1\4&32EEF88F&0&0000` from the previous command output:

```powershell
$id1 = "<Copy and paste GPU instance id into this string>"
$gpu=Get-PnpDevice -FriendlyName "3D Video Controller"
$id1 =$gpu[0].InstanceId

Choose a reason for hiding this comment

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

These works for single GPU device. But if there're multiple GPU installed on one host, we need a loop. Moreover, end users may want to choose to setup some, but not all of devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called out on line 107 with the statement "For Each GPU...., run the following commands in Powershell." Do we need to make it even more clear?

Choose a reason for hiding this comment

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

In line 110 the $gpu gets all devices named as "3D Video Controller", so in line 111 the $id1 will always be set to the instance id of the first GPU device.

Copy link
Contributor

@Karl-WE Karl-WE left a comment

Choose a reason for hiding this comment

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

Would it be possible to avoid mix and match legacy shell commands with PowerShell?
See line 73 piping to findstr. I am no expert but believe PowerShell can do this natively.

Any idea @JamesLiang29 ?

@JamesLiang29
Copy link

Would it be possible to avoid mix and match legacy shell commands with PowerShell? See line 73 piping to findstr. I am no expert but believe PowerShell can do this natively.

Any idea @JamesLiang29 ?

Get-PnpDevice | Where-Object FriendlyName -Like '3D Video*' | Select-Object Status, FriendlyName, InstanceId

@Karl-WE
Copy link
Contributor

Karl-WE commented Jun 20, 2024

I was asking for your help as I cannot test the result.

Would you mind to create a request for change of line 74?

@mgodfre3
Copy link
Contributor Author

Added the requested changes and added a new GPU command for additional GPUs.

Copy link
Contributor

Learn Build status updates of commit ca8cf70:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@@ -104,11 +104,21 @@ Error 3D Video Controller PCI\VEN_10DE&DEV_1EB8&SUBSYS_1

When you uninstall the host driver, the physical GPU goes into an error state. You must dismount all the GPU devices from the host.

For each GPU (3D Video Controller) device, run the following commands in PowerShell. Copy the instance ID; for example, `PCI\VEN_10DE&DEV_1EB8&SUBSYS_12A210DE&REV_A1\4&32EEF88F&0&0000` from the previous command output:
For each GPU (3D Video Controller) device, run the following commands in PowerShell. This command will create a variable named "ID1" and "lp1" and populate the instance ID of the first GPU; for example, `PCI\VEN_10DE&DEV_1EB8&SUBSYS_12A210DE&REV_A1\4&32EEF88F&0&0000` from the previous command output.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For each GPU (3D Video Controller) device, run the following commands in PowerShell. This command will create a variable named "ID1" and "lp1" and populate the instance ID of the first GPU; for example, `PCI\VEN_10DE&DEV_1EB8&SUBSYS_12A210DE&REV_A1\4&32EEF88F&0&0000` from the previous command output.
For each GPU (3D Video Controller) device, run the following commands in PowerShell. This command creates variables named `ID1` and `lp1` and populates the instance ID of the first GPU; for example, `PCI\VEN_10DE&DEV_1EB8&SUBSYS_12A210DE&REV_A1\4&32EEF88F&0&0000` from the previous command output.

Copy link
Contributor

Learn Build status updates of commit 8da0cdd:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Comment on lines +71 to +76
Open an elevated PowerShell prompt and run the following command:

```powershell
Get-PnpDevice | Where-Object FriendlyName -Like '3D Video*' | Select-Object Status, FriendlyName, InstanceId
```

Choose a reason for hiding this comment

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

Could you please move these lines (L71-76) to that between L95 and L96.

Comment on lines 111 to 112
$id0 =$gpu[0].InstanceId
$lp0 = (Get-PnpDeviceProperty -KeyName DEVPKEY_Device_LocationPaths -InstanceId $id1).Data[0]

Choose a reason for hiding this comment

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

You renamed the variables to $id0 and $lp0 here, but subsequence lines (112-114) are still using $id1 and $lp1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see the issue, quick fix.

Copy link
Contributor

Learn Build status updates of commit 0557b31:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit b447080:

✅ Validation status: passed

File Status Preview URL Details
AKS-Hybrid/deploy-gpu-node-pool.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

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

Successfully merging this pull request may close these issues.

6 participants