Enabling custom script extension support for VMSS deployment #4645

bishal-pdMSFT · 2017-06-27T07:51:43Z

Setting extension is done separately from updating image(earlier it was part of same PATCH call). Flow is:

List VM extensions for VMSS
Check if custom script extension exists
If so, delete existing extension
Install extension
Update VMSS image

tejasd1990 · 2017-06-27T09:17:36Z

Tasks/AzureResourceGroupDeployment/task.json

@@ -380,6 +380,7 @@
        "OnlyTokenAuthAllowed": "Endpoint can be of Token authorization type only.",
        "DeploymentGroupEndpointUrlCannotBeEmpty": "Deployment group endpoint url cannot be empty",
        "DeploymentGroupEndpointPatTokenCannotBeEmpty": "Deployment group endpoint personal access token cannot be empty",
-        "ErrorWhileParsingParameter": "There was an error while overriding '%s' parameter because of '%s'."
+        "ErrorWhileParsingParameter": "There was an error while overriding '%s' parameter because of '%s'.",
+        "ResourceNameCannotBeNull": "Resource name cannot be null."


Is this string used in ARG task?

this is used in common azure-arm-rest. currently, common modules' resource strings are also loaded from task.json.

tejasd1990 · 2017-06-27T09:20:02Z

Tasks/Common/azure-arm-rest/azure-arm-compute.ts

+        var httpRequest = new azureServiceClient.WebRequest();
+        httpRequest.method = 'GET';
+        httpRequest.headers = this.client.setCustomHeaders(options);
+        httpRequest.uri = this.client.getRequestUri('//subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/{resourceType}/{resourceName}/extensions',


Should the strings "virtualMachines" and "virtualMachineScaleSets" be local to methods in this file only, rather than each of their callers?

I will add an enum

sachinma · 2017-06-27T10:26:48Z

Tasks/AzureResourceGroupDeployment/operations/DeploymentGroupExtensionHelper.ts

                if (result && result.properties.provisioningState === "Failed") {
                    await this.tryDeleteFailedExtension(vm);
                }
                console.log(tl.loc("AddExtension", extensionName, vmName));
-                this.computeClient.virtualMachineExtensions.createOrUpdate(this.taskParameters.resourceGroupName, vmName, extensionName, parameters, async (error, result, request, response) => {
+                this.computeClient.virtualMachineExtensions.createOrUpdate(this.taskParameters.resourceGroupName, vmName, "virtualMachines",extensionName, parameters, async (error, result, request, response) => {


define "virtualmachines" as a constant

will add an enum

sachinma · 2017-06-27T10:32:32Z

Tasks/AzureVmssDeployment/tsconfig.json

-        "module": "commonjs"
+        "module": "commonjs",
+        "sourceMap": true,
+        "sourceRoot": "C:\\github\\vsts-tasks\\Tasks\\AzureVmssDeployment"


what is the significance of this hard-coded path. Is this only for IDE. Should we check-in this?

this is only for debugging. will remove.

sachinma · 2017-06-27T10:34:40Z

Tasks/AzureVmssDeployment/task.json

+            "name": "customScriptUrl",
+            "type": "string",
+            "label": "Custom script url",
+            "defaultValue": "",


I thought you were going with the local file assumption

that will come later.

sachinma · 2017-06-27T10:36:40Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

@@ -62,21 +62,74 @@ export default class VirtualMachineScaleSet {
                                }
                            }
                        }
+
+                        client.virtualMachineExtensions.list(resourceGroupName, this.taskParameters.vmssName, "virtualMachineScaleSets", null, (error, result, request, response) => {


consider splitting it into another method for better read-ability

sachinma · 2017-06-28T06:44:19Z

Tasks/Common/azure-arm-rest/azure-arm-compute.ts

@@ -386,7 +386,60 @@ export class VirtualMachineExtensions {
        this.client = client;
    }

-    public get(resourceGroupName, vmName, vmExtensionName, options, callback) {
+    public list(resourceGroupName, resourceName, resourceType, options, callback: azureServiceClient.ApiCallback) {


define types for the parameters

sachinma · 2017-06-28T06:52:56Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

                    break;
                }
            });
        });
    }

+    private _getResourceGroupForVmss(client, resolve, reject, callback): void {


Consider modeling this function as returning a promise instead of using callbacks. We used callback in the library to match the pattern with arm sdk

public method in this class already uses promise. For other private methods which are calling SDK, I don't see any benefit in wrapping then in promise.

You are right; however I think that will improve the code readability. For example your main method will not have nested statements and be more readable and less error-prone like following.
var resourceGroupDetails = await this.getResourceGroupName;
var extensionStatus = await this.getExistingExtensions();

yes, readability will increase.

sachinma · 2017-06-28T06:58:10Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

+
+            var vmssList: azureModel.VMSS[] = result;
+            if (vmssList.length == 0) {
+                console.log(tl.loc("NoVMSSFound", this.taskParameters.vmssName));


Shouldn't you reject with this message?

I think no. If no VMSS found, task will not do anything. I will ask Atul.

I will prefer the task to fail if the vmss is not foun

sachinma · 2017-06-28T09:43:49Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

-                                    this._updateImageInternal(client, resourceGroupName, resolve, reject);
-                                });
+                                await this._installCustomScripExtension(client, resourceGroupName, customScriptExtension);
+                                await this._updateImageInternal(client, resourceGroupName);


P2: Consider not repeating it now. Since this is all await. You can call it once (outside of all conditions)

sachinma · 2017-06-28T09:44:55Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

-                                this._deleteAndInstallCustomScriptExtension(client, resourceGroupName, matchingExtension, customScriptExtension, resolve, reject, (error, matchingExtension, request, response) => {
-                                    this._updateImageInternal(client, resourceGroupName, resolve, reject);
-                                });
+                                await this._deleteAndInstallCustomScriptExtension(client, resourceGroupName, matchingExtension, customScriptExtension);


P2: similarly consider if you need to merge this method now. Or you can delete if a matching extension is found. Install extension always. And then update image.

sachinma · 2017-06-28T09:46:03Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

        var client = new armCompute.ComputeManagementClient(this.taskParameters.credentials, this.taskParameters.subscriptionId);
-        return new Promise<void>((resolve, reject) => {
-            this._getResourceGroupForVmss(client,resolve, reject, (error, result, request, response) => {
+        return new Promise<void>(async (resolve, reject) => {


you can remove creating and returning this promise as well since your method is async

you are right. infact removed try-catch also

sachinma · 2017-06-28T09:48:11Z

Tasks/AzureVmssDeployment/operations/VirtualMachineScaleSet.ts

-                                this._installCustomScripExtension(client, resourceGroupName, customScriptExtension, resolve, reject, (error, result, request, response) => {
-                                    this._updateImageInternal(client, resourceGroupName, resolve, reject);
-                                });
+                                await this._installCustomScripExtension(client, resourceGroupName, customScriptExtension);


For my knowledge, is this the right way to update image (to do it in two steps). Would it lead to smooth upgrade; I thought earlier logic of merged update was better for upgrade; i.e. would it lead to zero downtime?

there were few reasons why I did like this:

deleting & installing extension before image update will not cause any downtime. The custom script is supposed to run at start-up only. Removing extension is not going to have any adverse affect.

I was seeing some reliability issues when earlier I was doing both in same PATCH call. it used to hang sometimes especially when extension was failed previously.

Discussed with Roopesh and this looks better way to achieve blue-green deployment. A cloned VMSS will be used to update extension and image and then vip swap will be done. So no downtime.

Anyways we have to support setting extension as a separate functionality. This makes it re-usable to enable that easily.

sachinma

Rest looks fine. However, lets discuss the upgrade approach that you shared tomorrow

bishal-pdMSFT added 2 commits June 26, 2017 16:14

delete existing extension and install separately

124079d

Move custom script extension to a separate flow from updating image.

7a2a392

msftclas added the cla-not-required label Jun 27, 2017

bishal-pdMSFT requested review from thesattiraju, sachinma and tejasd1990 June 27, 2017 07:51

tejasd1990 reviewed Jun 27, 2017

View reviewed changes

tejasd1990 approved these changes Jun 27, 2017

View reviewed changes

sachinma reviewed Jun 27, 2017

View reviewed changes

bishal-pdMSFT added 2 commits June 28, 2017 10:42

PR comments

f3f630f

Changing task version to 0.0.1 as it is preview version

cb9b451

sachinma reviewed Jun 28, 2017

View reviewed changes

replace callback with promise

2fb49fa

sachinma reviewed Jun 28, 2017

View reviewed changes

code cleanup

f1e5702

sachinma approved these changes Jun 28, 2017

View reviewed changes

bishal-pdMSFT merged commit ad914d3 into master Jun 28, 2017

bryanmacfarlane deleted the users/biprasad/VmssExtension branch January 31, 2018 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling custom script extension support for VMSS deployment #4645

Enabling custom script extension support for VMSS deployment #4645

bishal-pdMSFT commented Jun 27, 2017

tejasd1990 Jun 27, 2017

bishal-pdMSFT Jun 27, 2017

tejasd1990 Jun 27, 2017 •

edited

Loading

bishal-pdMSFT Jun 27, 2017

sachinma Jun 27, 2017

bishal-pdMSFT Jun 27, 2017

sachinma Jun 27, 2017

bishal-pdMSFT Jun 27, 2017

sachinma Jun 27, 2017

bishal-pdMSFT Jun 27, 2017

sachinma Jun 27, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017

sachinma Jun 28, 2017

bishal-pdMSFT Jun 28, 2017 •

edited

Loading

sachinma left a comment

Enabling custom script extension support for VMSS deployment #4645

Enabling custom script extension support for VMSS deployment #4645

Conversation

bishal-pdMSFT commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejasd1990 Jun 27, 2017 • 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

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

bishal-pdMSFT Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

sachinma left a comment

Choose a reason for hiding this comment

tejasd1990 Jun 27, 2017 •

edited

Loading

bishal-pdMSFT Jun 28, 2017 •

edited

Loading