Add File Share Snapshot Support #4832

blueww · 2017-10-23T09:31:20Z

Description

Support Azure File Share Snapshot in Powershell.
The design doc is reviewed and signed off by OneSDK team.

We also change the File/Dir/Share Uri to SnapshotQuanlifiedUri, in other storage File Cmdlets, to handle Share Snapshot.

The change already pass the storage regression.

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

[Y ] I have read the contribution guidelines.
[Y ] If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
[ Y] If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

[ Y] Title of the pull request is clear and informative.
[Y ] There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
[ Y] The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

[Y ] Pull request includes test coverage for the included changes.
[Y ] PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

[ Y] Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
[ Y] Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
[Y ] Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

blueww · 2017-10-23T09:43:24Z

on demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/33

I have rerun the on-demand with code change from comments, but seems it failed since some on-demand system issue.

maddieclayton · 2017-10-23T18:17:13Z

Hi Wei Wei- Could you fill out a design review template for these changes and send it to our review alias (azdevxpsdr)? Thanks.

blueww · 2017-10-24T08:36:10Z

The design is reviewed and signed off by OneSDK team (Mark) long ago.
Anyway, I have resend it in mail.

maddieclayton

Some initial comments. I also don't see any tests for these changes - can you add tests to cover these changes to this PR?

maddieclayton · 2017-10-25T01:08:59Z

src/Storage/ChangeLog.md

@@ -18,7 +18,11 @@
        - Additional information about change #1
 -->
 ## Current Release
-
+* Upgrade to Azure Storage Client Library 8.5.0 and Azure Storage DataMovement Library 0.6.3
+* Add File Share Snapshot Support 


Can you add a little more information here? See example here.

Change it to:

Current Release

Upgrade to Azure Storage Client Library 8.5.0 and Azure Storage DataMovement Library 0.6.3

Add File Share Snapshot Support Feature

Add 'SnapshotTime' parameter to Get-AzureStorageShare

Add 'IncludeAllSnapshot' parameter to Remove-AzureStorageShare

maddieclayton · 2017-10-25T01:13:46Z

src/Storage/Commands.Storage/File/Cmdlet/GetAzureStorageShare.cs

+        Position = 1,
+        Mandatory = false,
+        ParameterSetName = Constants.SpecificParameterSetName,
+        HelpMessage = "Snapshot Time of the file share snapshot to be gotten.")]


Fix spacing

Change to: "SnapshotTime of the file share snapshot to be gotten."

maddieclayton · 2017-10-25T01:21:23Z

src/Storage/Commands.Storage/File/Cmdlet/RemoveAzureStorageShare.cs

+                            if (e.IsConflictException() && retryDeleteSnapshot)
+                            {
+                                //If x-ms-delete-snapshots is not specified on the request and the share has associated snapshots, the File service returns status code 409 (Conflict).
+                                retryDeleteSnapshot = true;


I don't understand this if statement. If the retryDeleteSnapshot variable is true, it will set retryDeleteSnapshot to true?

Actually, here mean when the exception is ConflictException and need retry, we won't throw the exception, else throw the exception. (If share has snapshot and no "IncludeSnapshots" flag, will report ConflictException in delete share, in this case will retry.)
Change the code to make it more straightforward.

maddieclayton · 2017-10-25T01:22:28Z

src/Storage/Commands.Storage/File/Cmdlet/RemoveAzureStorageShare.cs

+
+                        if (retryDeleteSnapshot)
+                        {
+                            if (force || await OutputStream.ConfirmAsync(string.Format("This share might has snapshots, Remove the share and all snapshots?: {0}", share.Name)).ConfigureAwait(false))


What is the difference in behavior between -IncludeAllSnapshots and -Force?

-Force means no prompt, we will try our best to delete the share without prompt.

-IncludeAllSnapshot, just means will add "IncludeSnapshots" flag when delete the share. So delete the share with snapshot won't fail since has snapshot. (Anyway, it still can fail for other reason like server timeout)

maddieclayton · 2017-10-25T01:33:02Z

src/Storage/Commands.Storage/help/Remove-AzureStorageShare.md

@@ -37,6 +38,13 @@ PS C:\>Remove-AzureStorageShare -Name "ContosoShare06"

 This command removes the file share named ContosoShare06.

+### Example 2: Remove a file share and all its snapshots
+```
+PS C:\>Remove-AzureStorageShare -Name "ContosoShare06" -IncludeSnapshot


IncludeAllSnapshot

Good catch! The help has rebuild after change the parameter name from "IncludeSnapshot" to "IncludeAllSnaspshot" (agreed by feature team and OneSDK team), but the sample is missed.
I have checked all other help change, no missing now.

blueww · 2017-10-25T01:52:26Z

@maddieclayton

The Data Plan storage cmdlet are based on XSCL, which can't be record by Unit test. So we get agreed will test the scenario in Storage Team functional test. And the test already passed.

maddieclayton · 2017-10-26T18:44:48Z

src/Storage/Commands.Storage/File/Cmdlet/RemoveAzureStorageShare.cs

+                        catch (StorageException e)
+                        {
+                            //If x-ms-delete-snapshots is not specified on the request and the share has associated snapshots, the File service returns status code 409 (Conflict).
+                            if (!(e.IsConflictException() && retryDeleteSnapshot))


Is there ever a case where DeleteShareAsync will throw a conflictexception when the deleteShareSnapshotsOption is set to IncludeSnapshots? I'm just curious about whether you need to check if retryDeleteSnapshot is true.

There are cases to throw 419 (conflict) error when the share is being deleted, and try to delete again.

So we need to check the retryDeleteSnapshot is set and the error is 419 error (try to delete a share with snapshot without IncludSnapshot flag will get 419 error). This the best we can do.

Okay, sounds good.

maddieclayton · 2017-10-26T18:49:12Z

src/Storage/Commands.Storage/File/Cmdlet/RemoveAzureStorageShare.cs

+
+                        if (retryDeleteSnapshot)
+                        {
+                            if (force || await OutputStream.ConfirmAsync(string.Format("This share might has snapshots, Remove the share and all snapshots?: {0}", share.Name)).ConfigureAwait(false))


Should be: "might have snapshots" and "remove" should not be capitalized.

Thanks for the catch!
Modified.

maddieclayton · 2017-10-26T18:51:28Z

src/Storage/Commands.Storage/File/Cmdlet/RemoveAzureStorageShare.cs

+                            }
+                            else
+                            {
+                                string result = string.Format("The remove operation of  share '{0}' is cancelled.", share.Name);


I think "The remove operation of share '{0}' has been cancelled" makes more sense. Also remove extra whitespace.

cormacpayne

@blueww some minor additional changes

cormacpayne · 2017-10-26T19:36:05Z

src/Storage/Commands.Storage/Commands.Storage.csproj

    </Reference>
+    <Reference Include="Newtonsoft.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL" />


@blueww please remove this reference to Newtonsoft.Json - it is already referenced in the Common.Dependencies.targets file that this .csproj file references at the bottom of this file

cormacpayne · 2017-10-26T19:37:36Z

src/Storage/Commands.Storage/File/Cmdlet/GetAzureStorageShare.cs

@@ -28,7 +29,7 @@ public class GetAzureStorageShare : AzureStorageFileCmdletBase
            Position = 0,
            Mandatory = true,
            ParameterSetName = Constants.SpecificParameterSetName,
-            HelpMessage = "Name of the file share to be listed.")]
+            HelpMessage = "Name of the file share to be gotten.")]


@blueww I think we should change this to "Name of the file share to be received"

cormacpayne · 2017-10-26T19:37:56Z

src/Storage/Commands.Storage/File/Cmdlet/GetAzureStorageShare.cs

+        Position = 1,
+        Mandatory = false,
+        ParameterSetName = Constants.SpecificParameterSetName,
+        HelpMessage = "SnapshotTime of the file share snapshot to be gotten.")]


@blueww same comment about changing "gotten" to "received"

cormacpayne · 2017-10-26T19:42:59Z

src/Storage/Commands.Storage/packages.config

  <package id="Microsoft.Data.Edm" version="5.8.2" targetFramework="net452" />
  <package id="Microsoft.Data.OData" version="5.8.2" targetFramework="net452" />
  <package id="Microsoft.Data.Services.Client" version="5.8.2" targetFramework="net452" />
+  <package id="Microsoft.WindowsAzure.ConfigurationManager" version="1.8.0.0" targetFramework="net452" />
+  <package id="Newtonsoft.Json" version="6.0.8" targetFramework="net452" />


@blueww similar comment to the .csproj file: remove this reference to Newtonsoft.Json as it can be found in the packages.config file of the Commands.ResourceManager.Common project.

cormacpayne · 2017-10-30T03:17:53Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/56/

maddieclayton · 2017-10-30T20:36:25Z

New on-demand: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/78/

azuresdkci added the needs-review label Oct 23, 2017

azuresdkci assigned shahabhijeet Oct 23, 2017

blueww force-pushed the fsh branch from ea04556 to ce411cb Compare October 23, 2017 10:11

markcowl assigned maddieclayton and unassigned shahabhijeet Oct 23, 2017

markcowl added the Cmdlet Review Required ⚠️ label Oct 23, 2017

maddieclayton assigned blueww and unassigned maddieclayton Oct 23, 2017

maddieclayton self-requested a review October 23, 2017 18:17

blueww force-pushed the fsh branch from ce411cb to eb0e948 Compare October 24, 2017 08:34

maddieclayton suggested changes Oct 25, 2017

View reviewed changes

maddieclayton assigned maddieclayton and blueww and unassigned blueww and maddieclayton Oct 25, 2017

maddieclayton added Cmdlet Review Complete and removed Cmdlet Review Required ⚠️ labels Oct 26, 2017

maddieclayton reviewed Oct 26, 2017

View reviewed changes

maddieclayton assigned cormacpayne and unassigned maddieclayton Oct 26, 2017

cormacpayne suggested changes Oct 26, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Oct 26, 2017

cormacpayne removed their assignment Oct 26, 2017

cormacpayne assigned blueww Oct 26, 2017

blueww force-pushed the fsh branch 2 times, most recently from 09c87b7 to 26034d4 Compare October 27, 2017 01:41

blueww added 2 commits October 27, 2017 10:00

Add File Share Snapshot Support

f224c14

Update for File Share Snapshot feature per feedback

561f754

blueww force-pushed the fsh branch from 26034d4 to 561f754 Compare October 27, 2017 02:00

maddieclayton removed the needs-revision label Oct 27, 2017

maddieclayton approved these changes Oct 27, 2017

View reviewed changes

maddieclayton assigned cormacpayne and unassigned blueww Oct 27, 2017

maddieclayton added the needs-review label Oct 27, 2017

cormacpayne approved these changes Oct 30, 2017

View reviewed changes

cormacpayne added Waiting for On Demand and removed needs-review labels Oct 30, 2017

cormacpayne removed their assignment Oct 30, 2017

markcowl merged commit a6f2ca0 into Azure:preview Oct 30, 2017

		</Reference>
		<Reference Include="Newtonsoft.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL" />

Add File Share Snapshot Support #4832

Add File Share Snapshot Support #4832

Uh oh!

Conversation

blueww commented Oct 23, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

blueww commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maddieclayton commented Oct 23, 2017

Uh oh!

blueww commented Oct 24, 2017

Uh oh!

maddieclayton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Current Release

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueww Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueww commented Oct 25, 2017

Uh oh!

maddieclayton Oct 26, 2017 • edited by blueww Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueww Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented Oct 30, 2017

Uh oh!

blueww commented Oct 23, 2017 •

edited by azuresdkci

Loading

blueww commented Oct 23, 2017 •

edited

Loading

blueww Oct 25, 2017 •

edited

Loading

maddieclayton Oct 26, 2017 •

edited by blueww

Loading

blueww Oct 27, 2017 •

edited

Loading