Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ext): Add choco extension with param support #2707

Merged
merged 2 commits into from May 15, 2018

Conversation

@sylus
Copy link
Contributor

commented Apr 17, 2018

What this PR does / why we need it: Adds a Windows extension example; in this case an extension which installs packages passed as parameters via the Choco package manager. Very similar to how the one that works in DevTestLabs.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2709

Special notes for your reviewer: Confirmed and tested with microsoft-build-tools and it provisioned by Windows with the package.

I tested this on a generated cluster definition on top of v0.15.2

I did have to fix an entry in azuredeploy.json to get everything to work so think still need a fix in code.

"name": "[concat(variables('windowspool1VMNamePrefix'), copyIndex(variables('windowspool1Offset')), '/cse')]",

Should be this instead:

 "name": "[concat(variables('windowspool1VMNamePrefix'), copyIndex(variables('windowspool1Offset')),'/cse', copyIndex(variables('windowspool1Offset')))]",

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

@msftclas

This comment has been minimized.

Copy link

commented Apr 17, 2018

CLA assistant check
All CLA requirements met.

@CecileRobertMichon

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@sylr is this PR ready for review? Or does this mean that the code is not ready:

I did have to fix an entry in azuredeploy.json to get everything to work so think still need a fix in code.

?

@sylus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@CecileRobertMichon there is still the one manual adjustment that needs to be made, i can try to fix this myself but was hoping someone could point me to where that line is being generated?

Thanks so much!

@CecileRobertMichon

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

This is the place where the Windows agent custom script extension name is set:

"name": "[concat(variables('{{.Name}}VMNamePrefix'), copyIndex(variables('{{.Name}}Offset')), '/cse')]",

Not sure how that has to do with your PR and why you need to change it though?

If you are going to change, please change it to something like

"name": "[concat(variables('windowspool1VMNamePrefix'), copyIndex(variables('windowspool1Offset')),'/cse', '-win-agent-`, copyIndex(variables('windowspool1Offset')))]",

to be consistent with #2713

@sylus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

Hey @CecileRobertMichon

If the artifact has only the following line:

"name": "[concat(variables('windowspool1VMNamePrefix'), copyIndex(variables('windowspool1Offset')), '/cse')]",

Then the choco installation won't install saying can't find cse extension. This is why I need to change the line to:

 "name": "[concat(variables('windowspool1VMNamePrefix'), copyIndex(variables('windowspool1Offset')),'/cse', copyIndex(variables('windowspool1Offset')))]",

Thanks for pointing me to the line, i'll send an updated P.R. shortly! Appreciate your guidance as always!

@CecileRobertMichon

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

@sylus can please share the exact error message you're seeing? It's curious that the choco extension relies on the CSE

@sylus

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@CecileRobertMichon here you go, please let me know if can provide any more info. ^_^

❯ az group deployment create --name "acs-default" \
                           --resource-group "acs-default" \
                           --template-file "./_output/k8s-acs-default/azuredeploy.json" \
                           --parameters "./_output/k8s-acs-default/azuredeploy.parameters.json"
Deployment template validation failed: 'The resource 'Microsoft.Compute/virtualMachines/42410k8s9010/extensions/cse0' is not defined in the template. Please see https://aka.ms/arm-template for usage details.'.

With the mentioned fix everything then deploys correctly with choco installing the specified package.

Here was the base cluster def I used:

{
  "apiVersion": "vlabs",
  "properties": {
    "orchestratorProfile": {
      "orchestratorType": "Kubernetes",
      "orchestratorRelease": "1.9",
      "orchestratorVersion": "1.9.6",
      "kubernetesConfig": {
        "kubernetesImageBase": "k8s-gcrio.azureedge.net/",
        "addons": [
          {
            "name": "tiller",
            "enabled" : true
          },
          {
            "name": "kubernetes-dashboard",
            "enabled" : true
          },
          {
            "name": "rescheduler",
            "enabled" : true
          }
        ]
      }
    },
    "masterProfile": {
      "count": 1,
      "dnsPrefix": "k8s-acs-default",
      "vmSize": "Standard_D4s_v3",
      "OSDiskSizeGB": 200
    },
    "agentPoolProfiles": [
      {
        "name": "linuxpool1",
        "count": 1,
        "customNodeLabels": {
          "os": "linux"
        },
        "vmSize": "Standard_D4s_v3",
        "OSDiskSizeGB": 200,
        "storageProfile" : "ManagedDisks",
        "availabilityProfile": "AvailabilitySet"
      },
      {
        "name": "windowspool1",
        "count": 1,
        "osType": "Windows",
        "customNodeLabels": {
          "os": "windows"
        },
        "vmSize": "Standard_D4s_v3",
        "OSDiskSizeGB": 200,
        "storageProfile" : "ManagedDisks",
        "availabilityProfile": "AvailabilitySet",
        "extensions": [
          {
            "name": "choco"
          }
        ]
      }
    ],
    "linuxProfile": {
      "adminUsername": "azureuser",
      "ssh": {
        "publicKeys": [
          {
            "keyData": "ssh-rsa XXXX"
          }
        ]
      }
    },
    "extensionProfiles": [
      {
        "name": "choco",
        "version": "v1",
        "extensionParameters": "microsoft-build-tools",
        "rootURL": "https://raw.githubusercontent.com/govcloud/acs-engine/extension-choco/"
      }
    ],
    "windowsProfile": {
      "adminUsername": "azureuser",
      "adminPassword": "XXXX",
      "imageVersion": "2016.127.20180220",
      "WindowsImageSourceUrl": ""
    },
    "servicePrincipalProfile": {
      "clientId": "XXXX",
      "secret": "XXXX"
    }
  }
}
"type": "Microsoft.Resources/deployments",
"apiVersion": "[variables('apiVersionLinkDefault')]",
"dependsOn": [
"[concat('Microsoft.Compute/virtualMachines/', EXTENSION_TARGET_VM_NAME_PREFIX, copyIndex(EXTENSION_LOOP_OFFSET), '/extensions/cse', copyIndex(EXTENSION_LOOP_OFFSET))]"

This comment has been minimized.

Copy link
@CecileRobertMichon

CecileRobertMichon Apr 20, 2018

Member

This is what is causing your error, you need to make sure that the name of the VM extension is the same as it is defined in parts\k8s\kuberneteswinagentresourcesvmas.t

"dependsOn": [],
"location": "[resourceGroup().location]",
"type": "Microsoft.Compute/virtualMachines/extensions",
"name": "[concat(parameters('targetVMName'),'/cse', parameters('vmIndex'))]",

This comment has been minimized.

Copy link
@CecileRobertMichon

CecileRobertMichon Apr 20, 2018

Member

This should be named something more specific to chocolatey not to be confused with the already existing CSE

@CecileRobertMichon

This comment has been minimized.

Copy link
Member

commented May 4, 2018

@sylus what is the status of this PR?

@sylus

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

Ah thanks for reminding me, I will take a look at this over the weekend and have it rdy to merge given for fix before the start of Monday! @CecileRobertMichon

@sylus

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2018

@CecileRobertMichon thanks a bunch for your help I made the correction by calling the correct template, and everything works now without any manual changes needed.

Thanks a bunch!

This is ready to review from my end. ^_^

@CecileRobertMichon

This comment has been minimized.

Copy link
Member

commented May 7, 2018

Deployed a test cluster w/ the extension at https://jenkins.azure-containers.io/view/acs-engine%20ad-hoc/job/k8s-deployment/102

@JiangtianLi could you kindly review this when you have time?

@CecileRobertMichon CecileRobertMichon requested a review from JiangtianLi May 7, 2018

@CecileRobertMichon
Copy link
Member

left a comment

lgtm

@jackfrancis
Copy link
Member

left a comment

lgtm, thanks @sylus!

@jackfrancis jackfrancis merged commit 7b89830 into Azure:master May 15, 2018

14 of 15 checks passed

tide Not mergeable. Needs approved, lgtm labels.
Details
ci/circleci: build_and_test_pr/pr-e2e-hold Your job was approved on CircleCI!
Details
ci/circleci: dcos-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-1.10-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-1.7-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-1.8-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-1.9-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-windows-1.10-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-windows-1.7-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-windows-1.8-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: k8s-windows-1.9-release-e2e Your tests passed on CircleCI!
Details
ci/circleci: swarm-e2e Your tests passed on CircleCI!
Details
ci/circleci: swarmmode-e2e Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
license/cla All CLA requirements met.
Details

@ghost ghost removed the awaiting review label May 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.