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

Support for storage space data disks #998

Merged
merged 1 commit into from
May 18, 2021

Conversation

ambarve
Copy link
Contributor

@ambarve ambarve commented Apr 8, 2021

This commit adds support in hcsshim to mount a virtual disk backed by storage spaces as a
data disk into a container. In container config the host_path in the mount entry
should use the format space://{storage_space_pool_guid}{storage_space_disk_guid} to
specify a storage space virtual disk.

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

@ambarve ambarve requested a review from a team as a code owner April 8, 2021 20:24
@@ -63,18 +61,26 @@ func createMountsConfig(ctx context.Context, coi *createOptionsInternal) (*mount
return nil, fmt.Errorf("failed to eval symlinks for mount source %q: %s", mount.Source, err)
}
mdv2.HostPath = src
} else if mount.Type == "virtual-disk" || mount.Type == "physical-disk" || mount.Type == "ExtensibleVirtualDisk" {

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok to eliminate the existing behavior of always checking a vSMB mount first regardless of mount.Type?

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 think so. We currently have a bug that doesn't allow using the same host path as both scsi and vsmb mount because of this.

@kevpar
Copy link
Member

kevpar commented Apr 9, 2021

I think it would be more useful long-term if we can just make this feature be generic EVD support.

e.g. instead of space://{guid1}{guid2} can we just make the pattern something like evd://<type>/<path>? Then for spaces they would pass evd://space/{guid1}{guid2}, but other types would work as well without needing more work on our side.

@ambarve
Copy link
Contributor Author

ambarve commented Apr 9, 2021

@kevpar I like the idea of not having to make any other changes to support other types of EVDs. I just didn't do this because I didn't know if we wanted to keep the EVD part as an implementation detail.

@ambarve ambarve force-pushed the storage_spaces branch 2 times, most recently from 5702964 to 3b963d1 Compare April 12, 2021 17:34
@dcantah
Copy link
Contributor

dcantah commented Apr 27, 2021

This needs a rebase to grab the schema location changes (sorry 😞)

@@ -37,9 +37,7 @@ func createMountsConfig(ctx context.Context, coi *createOptionsInternal) (*mount
// TODO: Mapped pipes to add in v2 schema.
var config mountsConfig
for _, mount := range coi.Spec.Mounts {
if mount.Type != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Do we know why this check was here in the first place?

When mounting a VHD at host path `C:\data\test.vhdx` into the container over SCSI and also
sharing the same VHD inside the container over VSMB the current code just shares the VHD
inside the container for both mounts instead of actually SCSI mounting the VHD for one of
the mounts. This change fixes that.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
@ambarve ambarve marked this pull request as draft May 7, 2021 22:35
@ambarve ambarve merged commit 0feed3f into microsoft:master May 18, 2021
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