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

Support for multiple SCSI controllers #1328

Merged
merged 12 commits into from
Mar 28, 2022
Merged

Support for multiple SCSI controllers #1328

merged 12 commits into from
Mar 28, 2022

Conversation

ambarve
Copy link
Contributor

@ambarve ambarve commented Mar 14, 2022

Enable using upto 4 SCSI controllers for LCOW UVMs. HCS currently doesn't respect the
SCSI controller number provided with the Add SCSI disk requests. Hence, the SCSI disk can
show up at some different controller inside the LCOW UVM. To avoid this, now we use GUIDs
to represent each controller and use that GUID with the Add SCSI disk request.
GCS code is also modified to identify the controller number from the controller GUID.

Signed-off-by: Amit Barve ambarve@microsoft.com

@ambarve ambarve requested a review from a team as a code owner March 14, 2022 16:26
@ambarve ambarve mentioned this pull request Mar 14, 2022
Closed
@ambarve
Support for multiple SCSI controllers
7a2b364 
Enable using upto 4 SCSI controllers for LCOW UVMs. HCS currently doesn't respect the
SCSI controller number provided with the Add SCSI disk requests. Hence, the SCSI disk can
show up at some different controller inside the LCOW UVM. To avoid this, now we use GUIDs
to represent each controller and use that GUID with the Add SCSI disk request.
GCS code is also modified to identify the controller number from the controller GUID.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve
Add test and go mod vendor
ab2f28d 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve ambarve marked this pull request as draft March 14, 2022 17:09
@ambarve
Update create scratch code to keep SCSI controllers to 1
9883272 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve ambarve marked this pull request as ready for review March 14, 2022 22:30
@ambarve
Fix scsi tests
a953641 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
@dcantah dcantah self-assigned this Mar 15, 2022
dcantah
dcantah reviewed Mar 15, 2022
View reviewed changes
internal/uvm/constants.go Outdated Show resolved Hide resolved
kevpar
kevpar reviewed Mar 15, 2022
View reviewed changes
internal/uvm/constants.go Outdated Show resolved Hide resolved
kevpar
kevpar reviewed Mar 15, 2022
View reviewed changes
internal/uvm/constants.go Outdated Show resolved Hide resolved
@ambarve
PR suggestions
717a275 
Use Camel case for GUID array, include comment above each GUID to make it easier to
search. Add a check to make sure WCOW is never created with zero SCSI controllers.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
anmaxvl
anmaxvl reviewed Mar 16, 2022
View reviewed changes
internal/guest/storage/scsi/scsi.go Show resolved Hide resolved
internal/uvm/constants.go Outdated Show resolved Hide resolved
internal/uvm/constants.go Outdated Show resolved Hide resolved
@ambarve
Avoid changing GCS protocol
8442283 
Earlier changes to support several SCSI controllers modified the GCS protocol. This commit
modifies the code to keep the same GCS protocol and maintain the list of controller GUIDs
inside GCS.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
kevpar
kevpar reviewed Mar 17, 2022
View reviewed changes
internal/uvm/create.go Outdated Show resolved Hide resolved
@ambarve
Fix broken late clone tests
fc8af69 
Also, enable 4 SCSI controllers on LCOW only if no vpmem multimapping is enabled.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve
go mod vendor & tidy
a3c72ca 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
kevpar
kevpar reviewed Mar 21, 2022
View reviewed changes
internal/uvm/create_lcow.go Outdated Show resolved Hide resolved
kevpar
kevpar reviewed Mar 21, 2022
View reviewed changes
internal/uvm/create.go Outdated Show resolved Hide resolved
@ambarve
Use initrd for LCOW
935edfc 
Earlier we had disabled VPMEM devices for container layer but the UVM was still using
VPMEM for rootfs. This commit changes that and forces the UVM to use initrd for rootfs.
Also, adds an error if SCSI controller count for WCOW is not set to 1.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve
go mod vendor and go mod tidy
ec13c2d 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve
Use rootfs.vhd instead of initrd even if vpmem is disabled
754c55b 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve
go mod vendor and go mod tidy
0797a13 
Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve
Copy link
Contributor Author

ambarve commented Mar 28, 2022

@kevpar , @dcantah & @anmaxvl Can you PTAL?

kevpar
kevpar approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

anmaxvl
anmaxvl approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@anmaxvl anmaxvl left a comment

Choose a reason for hiding this comment

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

LGTM

dcantah
dcantah approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM

@ambarve ambarve merged commit d36cc7c into microsoft:master Mar 28, 2022
@ambarve ambarve deleted the scsi_controller_guid branch August 9, 2022 04:06
@ambarve ambarve restored the scsi_controller_guid branch August 9, 2022 04:06
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
@ambarve
Merged PR 5895108: update ADO to d36cc7c
6a5be6f 
Related work items: #1240, #1241, #1258, #1260, #1270, #1271, #1272, #1279, #1286, #1288, #1289, #1292, #1293, #1294, #1295, #1297, #1298, #1299, #1301, #1305, #1307, #1308, #1309, #1310, #1313, #1314, #1315, #1318, #1320, #1321, #1322, #1324, #1326, #1328, #1332, #1333, #1334, #1335, #1337
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
@ambarve
Support for multiple SCSI controllers (microsoft#1328)
4656282 
* Support for multiple SCSI controllers

Enable using upto 4 SCSI controllers for LCOW UVMs. HCS currently doesn't respect the
SCSI controller number provided with the Add SCSI disk requests. Hence, the SCSI disk can
show up at some different controller inside the LCOW UVM. To avoid this, now we use GUIDs
to represent each controller and use that GUID with the Add SCSI disk request.
GCS code is also modified to identify the controller number from the controller GUID. Now if a LCOW pod is created with an annotation that sets VPMEM device count to 0, we will automatically enable 4 SCSI controllers. Even the rootfs.vhd will be attached via SCSI in that scenario. 

Signed-off-by: Amit Barve <ambarve@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jterry75 jterry75 jterry75 left review comments

@kevpar kevpar kevpar approved these changes

@anmaxvl anmaxvl anmaxvl approved these changes

@dcantah dcantah dcantah approved these changes

Assignees

@kevpar kevpar

@dcantah dcantah

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ambarve @kevpar @anmaxvl @jterry75 @dcantah