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

Add disk sharing functionality (v35.0) and UUID (v36.0) #383

Merged
merged 2 commits into from
Feb 18, 2022

Conversation

arunmk
Copy link
Contributor

@arunmk arunmk commented Jun 11, 2021

IMPORTANT

To help us process your pull request efficiently, please follow the
guidelines shown below.

A Pull Request should be associated with an Issue

Created a PR without an issue after discussion with the contributors. The description should make it self-evident.

Description

Related issue: (<URL or #NUMBER of your Issue>)

  • This PR adds basic support to create shareable disks. That is done by essentially adding the Shareable field to the Disk structure. Additional other fields such as Encrypted and SharingType, which were added in v35 have also been pulled in. These new fields have been added in v35. If these are not present, the default value is false, i.e., the disks are not shared. If Shareable is set to True, the disk can be attached to multiple VMs. That functionality is provided in VCD 10.2 and does not need any other API change in order to attach to multiple disks. Note that all of the parameters mentioned here are optional, and sensible defaults are mentioned in VCD API to keep them backward compatible. Hence they are specified as omitempty in the API.

  • Another UUID parameter was added in v36.0 to help discover the disk that was attached. When a disk is attached to a VM, there is no way to know which of the /dev/sd* devices correspond to the device being attached. However, there is a UUID parameter for each disk available from v36. When the command /lib/udev/scsi_id --page=0x83 --whitelisted --device=</dev/sdX> is run on the VM host, there is a UUID emitted that can be checked against the UUID obtained from the API, and hence the right disk can be discovered. (Note that for the host command to work, the extra option disk.enableUUID must be set on the VM using vmomi).Once the right device /dev/sdx is discovered, the disk can be mounted etc. This parameter is only returned from VCD and hence there is no impact to the callers apart from the additional field in the struct.

  • (Optional) @Didainius could you please take a look?

Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Sounds quite clear - just if you could improve description of this PR and also add a few lines to CHANGELOG.md.

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

I'm approving in advance, but as requested by @Didainius we need a proper PR description (which will be commit message too) and an entry in the changelog.

@arunmk arunmk force-pushed the ak/disk-36.0 branch 2 times, most recently from 149ed76 to 166ac0e Compare June 25, 2021 09:18
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

I think that we should not add these fields without any function that supports them or tests to make sure the addition doesn't break anything

@lvirbalas
Copy link
Collaborator

CSE

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

Last change and we merge this :)

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