Skip to content

Conversation

@Malika133
Copy link

@Malika133 Malika133 commented Jan 13, 2025

This PR closes #

The changes in this PR are as follows:

We want to provide RunCommand based experience for ElasticSAN customers to expand their ElasticSAN datastore, once they expand their ElasticSAN volume in Azure Portal.
Creates a new module, and a new cmdlet Resize-iSCSIDatastore for expanding ISCSI based datastores for AVS.

  1. We do have a Resize-VmfsVolume cmdlet in Microsoft.AVS.VMFS, but the GA version of the package is tied to Pure Storage GA. This GA version only allows customers to execute the cmdlets necessary for Pure Storage CBS appliance integration. Hence it can't be used for ElasticSAN use cases.
  2. Resize-VmfsDatastore uses NAAID, which might not be readily available of the attached LUN, this will allow them to expand datastore using ClusterName and DatastoreName only

The new Resize-iSCSIDatastore accepts ElasticSAN and Pure datastore name and cluster name to identify the datastore and expand it, internally we are calling Resize-VMfsVolume cmdlet

Screenshot of Testing -
image

Increased Volume size from 125 GB to 150 GB
image

Tested failure scenarios, gracefully handled
image

Tested again with changes from 100 to 120 GB

image

image

srajmohan24
srajmohan24 previously approved these changes Jan 22, 2025
Copy link
Member

@et1975 et1975 left a comment

Choose a reason for hiding this comment

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

The stated reason for this PR is not reflected in the implementation. If the intent is simply to add a version of "resize" function that takes datastore name then it should say so.

throw "Datastore $DatastoreName does not exist."
}

if ($Datastore.Type -ne "VMFS") {
Copy link
Member

Choose a reason for hiding this comment

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

I believe the the test does not correspond to the conclusion - I don't see what's testing the VMFS provider specifically for Elastic.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is pointless and redundant to resize-vmfsdatastore.

Copy link
Author

@Malika133 Malika133 Jan 28, 2025

Choose a reason for hiding this comment

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

Hi @et1975 and @Zsoldier, thank you for taking out time to review the PR, I have renamed it to Resize-iSCSIDatastore, as we want it to available for both iSCSI-based storage solutions, ElasticSAN and Pure. It differs from Resize-VmfsDatastore in the way that
1.GA version of the package of Resize-VmfsDatastore is tied to Pure Storage GA. This GA version only allows customers to execute the cmdlets necessary for Pure Storage CBS appliance integration. Hence it can't be used for ElasticSAN use cases.
2. Customers might not have NAA ID readily available of the attached LUN, this will allow them to expand datastore using ClusterName and DatastoreName only,
adding same details in the PR description as well.
This check will ensure it is not implemented for other Datastore Types- vvol and NFS

#>
function Resize-ElasticSANDatastore {
[CmdletBinding()]
[AVSAttribute(10, UpdatesSDDC = $false, AutomationOnly = $true)]
Copy link
Member

Choose a reason for hiding this comment

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

What automation is going to call this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is pointless and redundant to resize-vmfsdatastore as that is designed to be called by automation only as well.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing out, I have removed AutomationOnly flag, we want this Run Command to be available for Customers on Azure Portal as we have been receiving numerous requests for support for Volume Expansion for Elastic SAN

@et1975 et1975 requested a review from Zsoldier January 27, 2025 14:04
@et1975
Copy link
Member

et1975 commented Jan 28, 2025

I think if we want to offer to resize arbitrary VMFS via the portal and using the datastore name we should modify the existing function to take either ID or name parameter, making them both optional and validating in the implementation that at least one is specified.

@Malika133
Copy link
Author

Hi @srajmohan24, can you provide your insight on @et1975 suggestion, is it feasible to use run command currently used by Pure for Elastic SAN as well with optional parameters.

@srajmohan24
Copy link
Contributor

Hi @srajmohan24, can you provide your insight on @et1975 suggestion, is it feasible to use run command currently used by Pure for Elastic SAN as well with optional parameters.

Yes, the existing Resize-VmfsVolume function should be feasible for Elastic SAN devices as well. Currently device NAA ID is a required parameter, but I'll open a PR shortly where we make it optional instead and also add another optional parameter for the datastore name

@Malika133
Copy link
Author

Hi @srajmohan24, can you provide your insight on @et1975 suggestion, is it feasible to use run command currently used by Pure for Elastic SAN as well with optional parameters.

Yes, the existing Resize-VmfsVolume function should be feasible for Elastic SAN devices as well. Currently device NAA ID is a required parameter, but I'll open a PR shortly where we make it optional instead and also add another optional parameter for the datastore name

Can this be triggered by Customer via Azure Portal as well ? Given it has AutomationOnly flag enabled as we want to give this functionality to our customers.

@srajmohan24
Copy link
Contributor

Hi @srajmohan24, can you provide your insight on @et1975 suggestion, is it feasible to use run command currently used by Pure for Elastic SAN as well with optional parameters.

Yes, the existing Resize-VmfsVolume function should be feasible for Elastic SAN devices as well. Currently device NAA ID is a required parameter, but I'll open a PR shortly where we make it optional instead and also add another optional parameter for the datastore name

Can this be triggered by Customer via Azure Portal as well ? Given it has AutomationOnly flag enabled as we want to give this functionality to our customers.

We would need to remove the AutomationOnly flag in this case, I'll also include this change as part of the PR

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.

4 participants