Add support for upgrading compose applications #6256
Conversation
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "id": "19C02B15-D377-40E0-9EFA-3168506E0933", | |||
| "name": "ServiceFabricComposeDeploy", | |||
| "friendlyName": "Service Fabric Compose Deploy", | |||
| "friendlyName": "Service Fabric Compose Deploy (PREVIEW)", | |||
There was a problem hiding this comment.
We can't make it preview once it has gone out of preview. Also, this task has been shipped in on-premise TFS release and hence should not be preview
There was a problem hiding this comment.
It never should have gone out of preview, someone else made that change with out reference to the fact that the platform feature is still in preview.
There was a problem hiding this comment.
It was taken out of preview as a task should not be in preview when it is shipping in on-prem TFS. Can we make assumption that task itself is stable enough to be non-preview regardless of target platform?
@kmkumaran do you have a suggestion here?
| { | ||
| if ($newApplication -eq $null) | ||
| $upgradeParameters = New-Object 'System.Collections.Hashtable' $deployParameters | ||
| $upgradeParameters.Monitored = $True |
There was a problem hiding this comment.
Don't we need to take knobs as task inputs- just like in ServiceFabricDeploy task?
There was a problem hiding this comment.
Long term we may need to. The feature is still in preview on the platform side so we are getting the golden path working and waiting for feedback on what knobs user's want/need.
| # Wait a minute before checking on the upgrade to avoid getting the status of the last upgrade. | ||
| Start-Sleep -Seconds 60 | ||
| $upgradeStatus = Get-ServiceFabricComposeDeploymentUpgradeHelper -ApiVersion $apiVersion -GetUpgradeParameters $getStatusParameters | ||
| while (($upgradeStatus -eq $null) -or (IsUpgradeRunning $upgradeStatus.UpgradeState)) |
There was a problem hiding this comment.
Will ($upgradeStatus -eq $null) be true if upgrade completes within 60 seconds we waited earlier - this will put us in infinite loop
There was a problem hiding this comment.
No, once the upgrade is complete the Get-ServiceFabricComposeDeploymentUpgradeHelper will continue to return the results of the last upgrade. This can only be $null when no upgrade has ever been performed on the application or within a narrow window after starting the first upgrade.
| { | ||
| Write-Host (Get-VstsLocString -Key CurrentStatus -ArgumentList $upgradeStatus.UpgradeState ) | ||
| } | ||
| Start-Sleep -Seconds 3 |
| # Wait a minute before checking on the upgrade to avoid getting the status of the last upgrade. | ||
| Start-Sleep -Seconds 60 | ||
| $upgradeStatus = Get-ServiceFabricComposeDeploymentUpgradeHelper -ApiVersion $apiVersion -GetUpgradeParameters $getStatusParameters | ||
| while (($upgradeStatus -eq $null) -or (IsUpgradeRunning $upgradeStatus.UpgradeState)) |
There was a problem hiding this comment.
Should there be a max count of iterations?
There was a problem hiding this comment.
The standard Service Fabric deploy task waits Int32.MaxValue times. I can add similar logic here if you'd like for consistency
| Write-Host (Get-VstsLocString -Key CurrentStatus -ArgumentList $upgradeStatus.UpgradeState ) | ||
| } | ||
| Start-Sleep -Seconds 3 | ||
| $upgradeStatus = Get-ServiceFabricComposeDeploymentUpgradeHelper -ApiVersion $apiVersion -GetUpgradeParameters $getStatusParameters |
There was a problem hiding this comment.
When does $upgradeStatus become null after upgrade is complete? Can it happen in 3 seconds window here?
There was a problem hiding this comment.
The docs don't give a specific time to live that I've found. But I have accessed the results days later.
|
@NCarlsonMSFT are you planning to take this PR forward? |
|
@bishal-pdMSFT are there any other questions/concerns you have before merging this? |
|
Hi everyone, is this going to be released soon? |
| Write-Host (Get-VstsLocString -Key CurrentStatus -ArgumentList $existingApplication.Status) | ||
| Start-Sleep -Seconds 3 | ||
| $existingApplication = Get-ServiceFabricComposeApplicationStatusHelper -ApiVersion $apiVersion -GetStatusParameters $getStatusParameters | ||
| $upgrading = $true |
There was a problem hiding this comment.
Should a new task input be added which specifies whether to upgrade or not? We should be consistent with Deploy task.
There was a problem hiding this comment.
The original task has suffered from a great deal of over complexity that has led to user confusion. I'd prefer to wait for user feedback requesting a flag rather then speculatively adding flags just in case.
On top of that the underlying platform feature remains in preview with preiodic breaking changes so I'd like to keep the options focused to reduce the maintence cost.
|
@NCarlsonMSFT @bishal-pdMSFT, any chance this will be merged in the near future? we are currently using our own fork of this to get rolling upgrade, but would of course much prefer to just use the built in task. Also +1 for having an upgrade flag. Our own fork has one, as we use the same task to deploy to dev/test/prod clusters, but we only care about rolling upgrade on production. |
|
@bishal-pdMSFT any outstanding objections? |
|
Looks good to me. |
Fixes #5760