adding offset to kubernetes cluster templates. Adding outputs needed … #366

JackQuincy · 2017-03-06T21:09:36Z

…to make scale down idempotent in RP

This change is

msftclas · 2017-03-06T21:09:40Z

@JackQuincy,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

acs-bot · 2017-03-06T21:10:28Z

Can one of the admins verify this patch?

JackQuincy · 2017-03-06T21:45:23Z

2 changes I have decided I want to make to this since sending it.

Remove total count and use count - offset for number of times through the loop so that the agent pool count in the template == agent pool count in the ACS API model.
Add to the Kubernetes walkthrough/documentation somewhere about using the offset for scale up scenarios and list steps to scale down.

amanohar · 2017-03-06T22:12:24Z

parts/agentparams.t

+      "metadata": {
+        "description": "The offset into the agent pool where to start creating agents.  This value can be from 0 to 100"
+      }, 
+      "type": "int"


How will this impact the case of multiple agent pools. Will this offset impact all agent pools? Shouldn't offset be applied on a per agent pool basis?

the {{.Name}} part is saying the name of the agent pool see unit test output below to see something like:
agentpool1Offset

amanohar · 2017-03-06T22:17:27Z

parts/kubernetesagentvars.t

@@ -1,7 +1,9 @@
    "{{.Name}}StorageAccountOffset": "[mul(variables('maxStorageAccountsPerAgent'),variables('{{.Name}}Index'))]",
    "{{.Name}}Count": "[parameters('{{.Name}}Count')]",
+    "{{.Name}}Offset": "[parameters('{{.Name}}Offset')]",
+    "{{.Name}}TotalCount": "[add(parameters('{{.Name}}Count'), parameters('{{.Name}}Offset'))]",


Shouldn't total count be the goal state. E.g. If current VM count is 4, and desired count is 5 (goal state). So total count will be: 5+4 =9?

agreed it is in my list of things todo to fix this PR up and get it ready for checkin

acs-bot · 2017-03-07T21:10:12Z

Can one of the admins verify this patch?

JackQuincy · 2017-03-07T21:38:45Z

Currently there is still down time on scale up. This is because our template reinputs the nsg which wipes out some rules that NRP adds. I'm looking into making it not reput the nsg potentially at the very least I'll document a work around of removing the nsg from the template before resubmitting

colemickens · 2017-03-07T21:39:52Z

The NSG rules can be made tracked sub-resources will which avoid clobbering them.

JackQuincy · 2017-03-07T21:51:20Z

@colemickens would that be the Kubernetes driver NSG rules , or the ACS NSG rules in the template, or both?

colemickens · 2017-03-07T21:54:33Z

Well, the cloudprovider code does a manual merge already, so it does the right thing by avoiding any automatic behavior. In this case, the fix is in the template. By making them sub-resources, it supposedly will use PATCH-like semantics and only add rules, rather than explicitly setting the "rules" subfield, thus causing a completely replacement of the entire set of rules, as we are doing now.

(Note, I don't have first hand knowledge of this; this is my understanding based on discussions with @yangl900 and @weinong. I hope that I'm portraying it accurately.)

weinong · 2017-03-07T22:51:55Z

Reviewed 1 of 63 files at r2.
Review status: 1 of 39 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

weinong · 2017-03-07T23:01:29Z

Reviewed 2 of 63 files at r2.
Review status: 3 of 39 files reviewed at latest revision, 3 unresolved discussions.

examples/scale-up/README.md, line 20 at r3 (raw file):

Example template kubernetes_tempalte.json
Orignal Parameters kubernetes_orignal_params.json
Scale up parameters kubernetes_scale_up_params.json
Show the reuse of a template with that was created with 6 nodes and scaled up to 15

can you reformat them a bit? they look like stuffed in single paragraph.

Comments from Reviewable

weinong · 2017-03-08T01:01:47Z

Review status: 3 of 39 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

parts/agentparams.t, line 16 at r1 (raw file):

Previously, JackQuincy (Jack) wrote…

the {{.Name}} part is saying the name of the agent pool see unit test output below to see something like:
agentpool1Offset

is 100 a valid offset?

pkg/acsengine/testdata/disks-managed/swarm-vmas_expected.json, line 216 at r3 (raw file):

        98,
        99,
        100

is 100 an valid offset?

Comments from Reviewable

JackQuincy · 2017-03-08T01:42:20Z

Review status: 3 of 39 files reviewed at latest revision, 4 unresolved discussions.

examples/scale-up/README.md, line 20 at r3 (raw file):

Previously, weinong (Weinong Wang) wrote…

Example template kubernetes_tempalte.json
Orignal Parameters kubernetes_orignal_params.json
Scale up parameters kubernetes_scale_up_params.json
Show the reuse of a template with that was created with 6 nodes and scaled up to 15

can you reformat them a bit? they look like stuffed in single paragraph.

Does this look better

parts/agentparams.t, line 16 at r1 (raw file):

Previously, weinong (Weinong Wang) wrote…

is 100 a valid offset?

No, I'm reducing to just 99 and adding comment about the change

pkg/acsengine/testdata/disks-managed/swarm-vmas_expected.json, line 216 at r3 (raw file):

Previously, weinong (Weinong Wang) wrote…

is 100 an valid offset?

no reducing to just 99 and adding comment about it needing to be less than count

Comments from Reviewable

weinong · 2017-03-08T01:49:03Z

Reviewed 10 of 63 files at r2, 2 of 2 files at r3, 24 of 24 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

anhowe

a few comments added

anhowe · 2017-03-08T19:02:48Z

parts/swarmagentresourcesvmas.t

@@ -150,20 +150,20 @@
      },
      "dependsOn": [
 {{if .IsStorageAccount}}
-        "[concat('Microsoft.Storage/storageAccounts/',variables('storageAccountPrefixes')[mod(add(div(copyIndex(),variables('maxVMsPerStorageAccount')),variables('{{.Name}}StorageAccountOffset')),variables('storageAccountPrefixesCount'))],variables('storageAccountPrefixes')[div(add(div(copyIndex(),variables('maxVMsPerStorageAccount')),variables('{{.Name}}StorageAccountOffset')),variables('storageAccountPrefixesCount'))],variables('{{.Name}}AccountName'))]",
+        "[concat('Microsoft.Storage/storageAccounts/',variables('storageAccountPrefixes')[mod(add(div(copyIndex(variables('{{.Name}}Offset')),variables('maxVMsPerStorageAccount')),variables('{{.Name}}StorageAccountOffset')),variables('storageAccountPrefixesCount'))],variables('storageAccountPrefixes')[div(add(div(copyIndex(variables('{{.Name}}Offset')),variables('maxVMsPerStorageAccount')),variables('{{.Name}}StorageAccountOffset')),variables('storageAccountPrefixesCount'))],variables('{{.Name}}AccountName'))]",


in your testing, set vms per storage account to 2, and then test a number of vms and a number of pools. For example 5 vms, 2 agent pools. That will ensure the math all checks out.

Not related to this pr. This is related to Resource Provider work to consume some of these changes

anhowe · 2017-03-08T19:21:39Z

pkg/acsengine/testdata/key-vault-params/kubernetes_expected.json

@@ -1761,6 +1981,14 @@
    }
  ],
  "outputs": {
+    "agentStorageAccountPrefixes": {


where are the agent outputs.

my bad didn't realize Kubernetes base template doesn't reference agents outputs template. Will update.

weinong · 2017-03-09T05:11:08Z

Reviewed 4 of 63 files at r2, 26 of 26 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

weinong · 2017-03-09T05:13:21Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

parts/agentparams.t, line 16 at r1 (raw file):

Previously, JackQuincy (Jack) wrote…

No, I'm reducing to just 99 and adding comment about the change

OK

Comments from Reviewable

…to make scale down idempotent in RP also updating swarm and dcos vmas agent pools and adding example file moving output definition so that we can properly format it

stale, it is several commits old and she hasn't had time to take a look and I am blocking others at this point

weinong · 2017-03-13T17:26:07Z

Reviewed 15 of 15 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

colemickens · 2017-03-13T17:31:41Z

This broke the build.

@acs-bot test this please

msftclas added the cla-not-required label Mar 6, 2017

amanohar previously requested changes Mar 6, 2017

View reviewed changes

amanohar reviewed Mar 6, 2017

View reviewed changes

JackQuincy force-pushed the scale-up-down-relability branch from e89a371 to 0294d14 Compare March 6, 2017 23:58

JackQuincy force-pushed the scale-up-down-relability branch from ade79a9 to a6ef5dd Compare March 8, 2017 01:35

anhowe reviewed Mar 8, 2017

View reviewed changes

JackQuincy force-pushed the scale-up-down-relability branch 2 times, most recently from ae35b1d to ac9199b Compare March 10, 2017 23:10

adding offset to kubernetes cluster templates. Adding outputs needed …

431b5b6

…to make scale down idempotent in RP also updating swarm and dcos vmas agent pools and adding example file moving output definition so that we can properly format it

JackQuincy force-pushed the scale-up-down-relability branch from ac9199b to 431b5b6 Compare March 13, 2017 17:14

weinong approved these changes Mar 13, 2017

View reviewed changes

JackQuincy merged commit 25cf3ac into master Mar 13, 2017

rocketraman mentioned this pull request Apr 5, 2017

Kubernetes vm scale up problem #109

Closed

JackQuincy deleted the scale-up-down-relability branch April 5, 2017 22:11

JackQuincy mentioned this pull request May 11, 2018

K8s upgrade leads to temporary unreachable services #2175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding offset to kubernetes cluster templates. Adding outputs needed … #366

adding offset to kubernetes cluster templates. Adding outputs needed … #366

JackQuincy commented Mar 6, 2017 •

edited by colemickens

msftclas commented Mar 6, 2017

acs-bot commented Mar 6, 2017

JackQuincy commented Mar 6, 2017

amanohar Mar 6, 2017

JackQuincy Mar 6, 2017

amanohar Mar 6, 2017 •

edited

JackQuincy Mar 6, 2017

acs-bot commented Mar 7, 2017

JackQuincy commented Mar 7, 2017

colemickens commented Mar 7, 2017

JackQuincy commented Mar 7, 2017

colemickens commented Mar 7, 2017

weinong commented Mar 7, 2017

weinong commented Mar 7, 2017

weinong commented Mar 8, 2017

JackQuincy commented Mar 8, 2017

weinong commented Mar 8, 2017

anhowe left a comment

anhowe Mar 8, 2017

JackQuincy Mar 8, 2017

anhowe Mar 8, 2017

JackQuincy Mar 8, 2017

weinong commented Mar 9, 2017

weinong commented Mar 9, 2017

weinong commented Mar 13, 2017

colemickens commented Mar 13, 2017

adding offset to kubernetes cluster templates. Adding outputs needed … #366

adding offset to kubernetes cluster templates. Adding outputs needed … #366

Conversation

JackQuincy commented Mar 6, 2017 • edited by colemickens

msftclas commented Mar 6, 2017

acs-bot commented Mar 6, 2017

JackQuincy commented Mar 6, 2017

amanohar Mar 6, 2017

Choose a reason for hiding this comment

JackQuincy Mar 6, 2017

Choose a reason for hiding this comment

amanohar Mar 6, 2017 • edited

Choose a reason for hiding this comment

JackQuincy Mar 6, 2017

Choose a reason for hiding this comment

acs-bot commented Mar 7, 2017

JackQuincy commented Mar 7, 2017

colemickens commented Mar 7, 2017

JackQuincy commented Mar 7, 2017

colemickens commented Mar 7, 2017

weinong commented Mar 7, 2017

weinong commented Mar 7, 2017

weinong commented Mar 8, 2017

JackQuincy commented Mar 8, 2017

weinong commented Mar 8, 2017

anhowe left a comment

Choose a reason for hiding this comment

anhowe Mar 8, 2017

Choose a reason for hiding this comment

JackQuincy Mar 8, 2017

Choose a reason for hiding this comment

anhowe Mar 8, 2017

Choose a reason for hiding this comment

JackQuincy Mar 8, 2017

Choose a reason for hiding this comment

weinong commented Mar 9, 2017

weinong commented Mar 9, 2017

weinong commented Mar 13, 2017

colemickens commented Mar 13, 2017

JackQuincy commented Mar 6, 2017 •

edited by colemickens

amanohar Mar 6, 2017 •

edited