Added option to SkipUpgradeSameTypeAndVersion #3529

mikanyg · 2017-02-02T22:12:55Z

This PR gives the option to make upgrades idempotent by skipping the upgrade if the following error 'Continue on 'Application type and version is still in use' occurs during upgrade validation. Solves #3346

…NewServiceFabricApplication cmdlet. Placed in new Advanced group and defaults to SameAppTypeAndVersion to keep backwards compatability.

…ock call.

- Incremented task patch number - removed trailing whitespace - moved test constants to variable

- taking latest Patch

…nd skip the entire upgrade, making the upgrade process idempotent.

… skipUpgrade logic

msftclas · 2017-02-02T22:12:59Z

Hi @mikanyg, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

mikanyg · 2017-02-09T22:24:27Z

@mikkelhegn @ericsciple Would you consider this PR in regards to #3346 or do you have other plans for ServiceFabricDeploy task based on the comment by @mikkelhegn

ericsciple · 2017-02-09T22:53:24Z

@NCarlsonMSFT to consider

NCarlsonMSFT · 2017-02-10T00:22:26Z

Tasks/ServiceFabricDeploy/ServiceFabricSDK/Publish-UpgradedServiceFabricApplication.ps1

+            }
+            catch [System.Fabric.FabricException] {
+                ## If SkipUpgrade param is set when existing version already deployed, then back out of upgrade
+                if($SkipUpgradeSameTypeAndVersion -And $Error[0].Exception.Message -like '*Application type and version is still in use*') {                    


Checking based on the error message won't work on a non-english machine and will break if the Service Fabric platform changes their error messages. Have you looked at the Error Code property? It looks like the value ApplicationAlreadyInTargetVersion may be what you are looking for.

True, missed the ErrorCode property before.

Regarding the enum value it would ideally be ApplicationAlreadyInTargetVersion thrown during upgrade, but the exception occurs prior to the upgrade, instead it is ApplicationTypeInUse thrown when the app type and version is already found and is being unregistered.

I never truly understood why an app would be unregistered before an upgrade, but thought is was some sort of pre-validation step, which would cause the error to be thrown, hopefully intentionaly. I don't know if the pre-validation step is really needed if the upgrade actally throws ApplicationAlreadyInTargetVersion if it finds the same app type and version as in the upgrade package. Then the catch block could just wrap the upgrade step.

Just talked with an Engineer on the Platform. His recommendation is to call Get-ServiceFabricApplication -ApplicationName and check the ApplicationTypeVersion property on that to decide whether or not to skip deployment.

ok, will look into that

@NCarlsonMSFT Changed the logic from being exception handling to pre-validation using the existing code that checked for existing of app in the cluster. Was actually much simpler than the first implementation.

But seems like my new VS Code settings removed a lot trailing whitespace from the file. Should I revert and re-apply my changed to keep PR to only new features or do you prefer the removal of trailing whitespace?

I'm not worried about removing the extra white-space (I'm personally glad it's gone, always been a pet peeve of mine)

The new logic does look cleaner and I think the change looks good. I need to manually test it before signing off though.

Do take it for a spin, remember its on redeployments that the skip upgrade part kicks in.

The functionality looks good, I added one more comment in task.json based on the UI experience.

NCarlsonMSFT · 2017-02-10T00:24:08Z

Tasks/ServiceFabricDeploy/ServiceFabricSDK/Publish-UpgradedServiceFabricApplication.ps1

+                else {                    
+                    throw   
+                }                
+            }            


This block has a bunch of stray whitespace on the end of the lines

Really don't know why the trailing spaces appear. I am just using VS Code on Win10 with default settings. The diff tool does not show them untill I actually remove them after a commit.

NCarlsonMSFT · 2017-02-10T00:25:15Z

Tasks/ServiceFabricDeploy/deploy.ps1

@@ -130,7 +131,8 @@ try {
    {
        $publishParameters['Action'] = "RegisterAndUpgrade"
        $publishParameters['UpgradeParameters'] = $upgradeParameters
-        $publishParameters['UnregisterUnusedVersions'] = $true
+        $publishParameters['UnregisterUnusedVersions'] = $true        


The only change to this line is stray whitespace, please remove

NCarlsonMSFT · 2017-02-10T23:00:03Z

VS Code tips: you can turn on viewing white space with the setting:
"editor.renderWhitespace": "all"

and you can make VS Code auto-remove trailing white space with the settings:
"files.trimTrailingWhitespace": true

mikanyg · 2017-02-10T23:10:24Z

Moved my VS Code comment to the review. Still a bit new to the PR and review experience in github

NCarlsonMSFT · 2017-02-10T23:29:12Z

Don't worry, I'm still learning Git Hub PRs too.

…n error handling

NCarlsonMSFT · 2017-02-16T23:07:20Z

Tasks/ServiceFabricDeploy/task.json

+            "defaultValue": "false",
+            "required": false,
+            "groupname": "upgrade",
+            "helpMarkDown": "Indicates whether an upgrade will be skipped if the same application type and version already exists in the cluster, otherwise the upgrade fails during validation. If enabled, re-deployments are idempotent."


Once I deployed this I noticed the UI around this check-box gets a bit funky with the behavior of the "Override All Publish Profile Upgrade Settings" check-box. could you please move this to the advanced group.

Sure, I placed it in the upgrade group since it was related to upgrades, but yes it looks and behaves a bit odd, when selecting/deselecting the override publish settings and upgrade checkbox.

NCarlsonMSFT · 2017-02-17T18:43:20Z

@ericsciple does anyone else need to sign-off before merging?

mikanyg · 2017-03-06T19:12:12Z

@NCarlsonMSFT is this PR considered dead, or is it just busy time at MS?

NCarlsonMSFT · 2017-03-07T21:07:37Z

@mikanyg sorry, it's not dead but it has been a busy time. I'm still trying to verify if any other sign-off is needed before merging.

bishal-pdMSFT · 2017-03-09T06:16:09Z

Tasks/ServiceFabricDeploy/ServiceFabricSDK/Publish-UpgradedServiceFabricApplication.ps1


+        [Parameter(ParameterSetName="ApplicationParameterFilePath")]
+        [Parameter(ParameterSetName="ApplicationName")]
+        [Switch]$SkipUpgradeSameTypeAndVersion


include this parameter's description in function doc above.

mikanyg added 10 commits January 7, 2017 00:41

Added new input for controlling the OverwriteBehavior of the Publish-…

e3d2612

…NewServiceFabricApplication cmdlet. Placed in new Advanced group and defaults to SameAppTypeAndVersion to keep backwards compatability.

Fixed unit tests for ServiceFabricDeploy, by registering additional m…

f32a9b9

…ock call.

Improvements based on PR review

9d228a2

- Incremented task patch number - removed trailing whitespace - moved test constants to variable

Merged with upstream/master

63511bc

- taking latest Patch

Incremented patch for task

5c8e892

Merge remote-tracking branch 'upstream/master'

f083291

Merge branch 'master' of https://github.com/Microsoft/vsts-tasks

dcd4730

Added upgrade option to ignore errors when redeploying same version a…

4a20097

…nd skip the entire upgrade, making the upgrade process idempotent.

added explicit filter for exception type and message when determining…

8c19e4b

… skipUpgrade logic

updated legacy tests for ServiceFabricDeploy, registering new mock call

079e22b

msftclas added the cla-already-signed label Feb 2, 2017

merged with upstream, bumped patch number

bfcc0b9

ericsciple added the Area: ServiceFabric label Feb 9, 2017

ericsciple assigned NCarlsonMSFT Feb 9, 2017

NCarlsonMSFT requested changes Feb 10, 2017

View reviewed changes

Improvements based on PR review

e5d2fef

mikanyg added 2 commits February 12, 2017 20:54

Merge branch 'master' of https://github.com/Microsoft/vsts-tasks

39af721

changed logic around skip upgrade to be upfront validation rather tha…

2bdb178

…n error handling

NCarlsonMSFT reviewed Feb 16, 2017

View reviewed changes

mikanyg added 2 commits February 17, 2017 01:16

Merge branch 'master' of https://github.com/Microsoft/vsts-tasks

f70bd1b

moved option to advanced group

d7d06ec

NCarlsonMSFT approved these changes Feb 17, 2017

View reviewed changes

bishal-pdMSFT approved these changes Mar 9, 2017

View reviewed changes

mikanyg added 2 commits March 10, 2017 00:23

Merge branch 'master' of https://github.com/Microsoft/vsts-tasks

24d2124

Updated function doc with parameter description

2a968c3

NCarlsonMSFT merged commit 80bc9ec into microsoft:master Mar 10, 2017

mikanyg mentioned this pull request Mar 13, 2017

[ServiceFabric] Continue on 'Application type and version is still in use' #3346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to SkipUpgradeSameTypeAndVersion #3529

Added option to SkipUpgradeSameTypeAndVersion #3529

mikanyg commented Feb 2, 2017

msftclas commented Feb 2, 2017

mikanyg commented Feb 9, 2017

ericsciple commented Feb 9, 2017

NCarlsonMSFT Feb 10, 2017

mikanyg Feb 10, 2017

NCarlsonMSFT Feb 10, 2017

mikanyg Feb 10, 2017

mikanyg Feb 12, 2017

NCarlsonMSFT Feb 15, 2017

mikanyg Feb 15, 2017

NCarlsonMSFT Feb 16, 2017

NCarlsonMSFT Feb 10, 2017

mikanyg Feb 10, 2017

NCarlsonMSFT Feb 10, 2017

NCarlsonMSFT commented Feb 10, 2017

mikanyg commented Feb 10, 2017

NCarlsonMSFT commented Feb 10, 2017 •

edited

Loading

NCarlsonMSFT Feb 16, 2017

mikanyg Feb 17, 2017

NCarlsonMSFT commented Feb 17, 2017

mikanyg commented Mar 6, 2017

NCarlsonMSFT commented Mar 7, 2017

bishal-pdMSFT Mar 9, 2017

mikanyg Mar 9, 2017

Added option to SkipUpgradeSameTypeAndVersion #3529

Added option to SkipUpgradeSameTypeAndVersion #3529

Conversation

mikanyg commented Feb 2, 2017

msftclas commented Feb 2, 2017

mikanyg commented Feb 9, 2017

ericsciple commented Feb 9, 2017

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

NCarlsonMSFT commented Feb 10, 2017

mikanyg commented Feb 10, 2017

NCarlsonMSFT commented Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NCarlsonMSFT commented Feb 17, 2017

mikanyg commented Mar 6, 2017

NCarlsonMSFT commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NCarlsonMSFT commented Feb 10, 2017 •

edited

Loading