[Service fabric deploy] Re-tries for copy and register commands #7555

bishal-pdMSFT · 2018-06-25T09:38:48Z

No description provided.

sachinma · 2018-06-25T10:05:20Z

Tasks/Common/PowershellHelpers/Helpers.ps1

@@ -43,7 +46,14 @@ function Invoke-ActionWithRetries
        {
            if (($null -eq $RetryableExceptions) -or (Test-RetryableException -Exception $_.Exception -AllowedExceptions $RetryableExceptions))
            {
+                $shouldRetry = $OnException.Invoke($_.Exception)
+                if (!$shouldRetry)
+                {


Lets add a verbose statement indicating retry

Or a telemetry!

There is a log statement indicating retry at line no 81

I will add telemetry seprately, once I am done with all command re-tries

sachinma · 2018-06-25T11:14:57Z

Tasks/Common/PowershellHelpers/Helpers.ps1

+        $RetryMessage,
+
+        [scriptblock]
+        $OnException = { $true }


Consider renaming like ShouldRetryHandler

Renamed to ShouldRetryOnException

sachinma · 2018-06-25T11:16:34Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

+        -RetryMessage (Get-VstsLocString -Key SFSDK_RetryingGetApplicationType)
+}
+
+function Invoke-CommandWithRetries


Invoke-CommandWithDefaultRetries

sachinma · 2018-06-25T11:17:01Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

+    )
+
+    $global:operationId = $SF_Operations.GetApplicationUpgradeStatus
+    return Get-ServiceFabricApplicationUpgrade -ApplicationName $ApplicationName


No retry here?

This function has not changed. Github is not showing diff properly.

sachinma · 2018-06-25T11:19:38Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

+        }
+
+        # if provisioning failed, throw and don't retry
+        throw (Get-VstsLocString -Key SFSDK_RegisterAppTypeFailedWithStatus -ArgumentList @($appType.Status, $appType.StatusDetails))


Better to log this and return false

OnException has three possible outcomes:

true - in this case retry is attempted

false - in this case it comes out of retry, but without failure. e.g. if get-applicationtype returns that application type is provisioned, we should come out of register-applicationtype retry loop

throw - it comes out of retry with failure .e.g if get-applicationtype returns that application provisioning failed

sachinma · 2018-06-25T11:23:16Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

+    $appTypeStatusValidator = {
+        param($appType)
+        # if app type does not exist (i.e it got unprovisioned) or if its status has changed to a terminal one, stop wait
+        if ((!$appType) -or (($appType.Status -ne [System.Fabric.Query.ApplicationTypeStatus]::Provisioning) -and ($appType.Status -ne [System.Fabric.Query.ApplicationTypeStatus]::Unprovisioning)))


this statement doesn't look correct

this is correct. if app type is in terminal state, then retry should not be attempted. This is ActionSuccessValidator delegate and hence $true means no further retry.

sachinma · 2018-06-26T06:54:22Z

Tasks/Common/PowershellHelpers/Helpers.ps1

+        $RetryMessage,
+
+        [scriptblock]
+        $ShouldRetryOnException = { $true }


Consider renaming the success handler to ShouldRetryOnSuccess or ShouldRetry

kuvinodms · 2018-06-28T04:06:50Z

Tasks/Common/PowershellHelpers/Helpers.ps1

@@ -43,15 +48,22 @@ function Invoke-ActionWithRetries
        {
            if (($null -eq $RetryableExceptions) -or (Test-RetryableException -Exception $_.Exception -AllowedExceptions $RetryableExceptions))


Lets update the parameter name for Test-RetryableException cmdlet from AllowedExceptions to RetryableExceptions

kuvinodms · 2018-06-28T04:15:30Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Publish-NewServiceFabricApplication.ps1

@@ -328,8 +328,7 @@
            }

            Write-Host (Get-VstsLocString -Key SFSDK_RegisterAppType)
-            $global:operationId = $SF_Operations.RegisterApplicationType
-            Register-ServiceFabricApplicationType @registerParameters
+            Register-ServiceFabricApplicationTypeAction -RegisterParameters $registerParameters -ApplicationTypeName $names.ApplicationTypeName -ApplicationTypeVersion $names.ApplicationTypeVersion
            if (!$?)


You can remove this since Register-ServiceFabricApplicationTypeAction will never Fail.
This check is unnecessary.

Same as above

kuvinodms · 2018-06-28T04:15:45Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Publish-NewServiceFabricApplication.ps1

-            $global:operationId = $SF_Operations.CopyApplicationPackage
-            Copy-ServiceFabricApplicationPackage @copyParameters
+
+            Copy-ServiceFabricApplicationPackageAction -CopyParameters $copyParameters
            if (!$?)


This check is unnecessary.

I am not sure whether it is necessary or not. Since it existed before my change, I am not going to remove this. May be it is there as Copy command throws a non-terminating error.

Please go through this article https://blogs.technet.microsoft.com/heyscriptingguy/2011/05/12/powershell-error-handling-and-why-you-should-care/
We've used it because earlier we were directly calling SDK cmdlets (like Copy-ServiceFabricApplicationPackage) so there was some scope of call failures but now we're calling our own utility functions (like Copy-ServiceFabricApplicationPackageAction in this case) so there is no chance for call failure.
My suggestion is to remove this check from here and move it to Copy-ServiceFabricApplicationPackageAction function right after where you're calling Copy-ServiceFabricApplicationPackage cmdlet.

Do the same for all such checks!

Yes, it would make sense to move them to action itself

kuvinodms · 2018-06-28T04:22:34Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Publish-NewServiceFabricApplication.ps1

@@ -328,8 +328,7 @@
            }

            Write-Host (Get-VstsLocString -Key SFSDK_RegisterAppType)
-            $global:operationId = $SF_Operations.RegisterApplicationType
-            Register-ServiceFabricApplicationType @registerParameters
+            Register-ServiceFabricApplicationTypeAction -RegisterParameters $registerParameters -ApplicationTypeName $names.ApplicationTypeName -ApplicationTypeVersion $names.ApplicationTypeVersion
            if (!$?)
            {
                throw (Get-VstsLocString -Key SFSDK_RegisterAppTypeFailed)


Once you remove if(!$?) check then please do delete SFSDK_RegisterAppTypeFailed as well in case if its not being used anywhere else.

kuvinodms · 2018-06-28T04:27:40Z

Tasks/Common/PowershellHelpers/Helpers.ps1

        [scriptblock]
-        $ActionSuccessValidator = { $true },
+        $ResultRetryEvaluator = { $false },


ActionRetryEvaluator will be better name for this parameter

Both ResultRetryEvaluator and ExceptionRetryEvaluator will evaluate whether to re-try action or not - one evaluates if action has successfully completed and other will evaluate if action has thrown exception.
Naming just one as ActionRetryEvaluator will be mis-leading.

kuvinodms · 2018-06-28T04:32:27Z

Tasks/Common/PowershellHelpers/Helpers.ps1

            }
            else
            {
                throw
            }
        }

-        if (!$exception -and (!$result -or $ActionSuccessValidator.Invoke($result)))
+        if (!$exception -and (!$result -or !$ResultRetryEvaluator.Invoke($result)))


Don't you think it should be
if (!$exception -and ($result -and !$ResultRetryEvaluator.Invoke($result)))

$result can be $null if action return void. In such case, we should not retry. Only if $result is non-null, we should evaluate whether to retry or not

kuvinodms · 2018-06-28T04:44:09Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

+    $parameters = @{
+        Action                 = $Action
+        MaxTries               = 3;
+        RetryIntervalInSeconds = 10;


Any specific reason for fixing RetryIntervalInSeconds as 10. This is huge. Why not 5 or 3?

I don't have any data to have a specific number. This is just a guess as we want to give enough time for any network glitch

kuvinodms · 2018-06-28T04:45:58Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Publish-UpgradedServiceFabricApplication.ps1

@@ -373,9 +371,9 @@ function Publish-UpgradedServiceFabricApplication

        Write-Host (Get-VstsLocString -Key SFSDK_WaitingForUpgrade)
        $upgradeStatusFetcher = { Get-ServiceFabricApplicationUpgradeAction -ApplicationName $ApplicationName }
-        $upgradeStatusValidator = { param($upgradeStatus) return ($upgradeStatus.UpgradeState -eq "RollingBackCompleted" -or $upgradeStatus.UpgradeState -eq "RollingForwardCompleted") }
+        $upgradeRetryEvaluator = { param($upgradeStatus) return ($upgradeStatus.UpgradeState -ne "RollingBackCompleted" -and $upgradeStatus.UpgradeState -ne "RollingForwardCompleted") }


Please move this logic in Get-ServiceFabricApplicationUpgradeAction similar to what you've done for other cmdlets like Copy and Register.
Currently it is inconsistent!

It is consistent. In upgradeRetryEvaluator we are checking the result to decide whether to retry. In case of Register, we need to call Get-ServiceFabricApplicationType only in case of exception and hence that is handled in $exceptionRetryEvaluator

I mean to point out the logic of calling Invoke-ActionWithRetries in this file.
This call should ideally be done in Get-ServiceFabricApplicationUpgradeAction to as we've done for all other cmdlets.
I've raised a quick draft PR #7593 for the same, let me know if this seems fine to you so that I can go ahead test out the change and merge

kuvinodms · 2018-06-28T04:52:18Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

 }

-function Get-ServiceFabricApplicationUpgradeAction
+function Wait-ServiceFabricApplicationTypeStatusChange


function name is very generic however you're checking only for status Provisioning and Unprovisioning.
Consider changing function name.

Will do in next PR

kuvinodms · 2018-06-28T05:03:53Z

Tasks/ServiceFabricDeployV1/ServiceFabricSDK/Utilities.ps1

+
+    $exceptionRetryEvaluator = {
+        param($ex)
+        $appType = Get-ServiceFabricApplicationTypeAction -ApplicationTypeName $ApplicationTypeName -ApplicationTypeVersion $ApplicationTypeVersion


This is Incorrect. You might enter into infinite loop. There will be no bound of MaxRetries.

Get-ServiceFabricApplicationTypeAction has a max tries of 3. Or are you pointing to something different? Can you explain how it will go into infinit?

Oops my mistake. I thought function name is also Register-ServiceFabricApplicationTypeAction.
This is correct!

kuvinodms · 2018-06-28T05:27:06Z

Tasks/Common/PowershellHelpers/Helpers.ps1

@@ -43,15 +48,22 @@ function Invoke-ActionWithRetries
        {
            if (($null -eq $RetryableExceptions) -or (Test-RetryableException -Exception $_.Exception -AllowedExceptions $RetryableExceptions))
            {
+                $shouldRetry = $ExceptionRetryEvaluator.Invoke($_.Exception)


No need to pass the parameter $_.Exception since you're not using it anywhere for any ExceptionRetryEvaluator

Using it in next PR.

Re-tries for copy and register commands

0220334

bishal-pdMSFT requested review from kuvinodms and sachinma June 25, 2018 09:38

sachinma requested changes Jun 25, 2018

View reviewed changes

PR comments

7ea3836

sachinma approved these changes Jun 26, 2018

View reviewed changes

PR comments and few suggestion as per Oana

b041944

bishal-pdMSFT merged commit 1d75fa8 into master Jun 26, 2018

kuvinodms reviewed Jun 28, 2018

View reviewed changes

		@@ -43,15 +48,22 @@ function Invoke-ActionWithRetries
		{
		if (($null -eq $RetryableExceptions) -or (Test-RetryableException -Exception $_.Exception -AllowedExceptions $RetryableExceptions))

[Service fabric deploy] Re-tries for copy and register commands #7555

[Service fabric deploy] Re-tries for copy and register commands #7555

Conversation

bishal-pdMSFT commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuvinodms Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuvinodms Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuvinodms Jun 28, 2018 •

edited

Loading

kuvinodms Jun 28, 2018 •

edited

Loading