Add maxPods to Kubernetes configuration #1103

ofiliz · 2017-07-25T21:55:47Z

What this PR does / why we need it:

This PR adds the feature described in issue #1102. It adds a "maxPods" field to KubernetesConfig to customize the maximum number of pods that can be scheduled on a node.

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

This PR adds the optional "maxPods" field to KubernetesConfig. It defaults to 110 (the current Kubernetes default) when using spoofed IP addresses, and 30 when using actual IP addresses with CNI plugin enabled.
Customers can set custom maxPods values appropriate for their scenarios.

This is general goodness whether CNI plugin is used or not. However when CNI plugin is enabled, maxPods gives finer control over the number of IP addresses allocated per node, allowing larger cluster sizes with fewer addresses per node (e.g. 4096/110=37 vs. 4096/64=64 vs. 4096/30=136)

Special notes for your reviewer:

Release note:

fixes #1102

This change is

acs-bot · 2017-07-25T21:56:02Z

Can one of the admins verify this patch? Say "@acs-bot test this please" to start tests.

dmitsh

Could you add an api-model example to examples/vnet/ ?

seanknox

Overall this looks good, but we need unit tests for any new functionality.

dmitsh · 2017-08-01T18:23:11Z

@acs-bot test this please

ofiliz · 2017-08-11T19:32:55Z

@seanknox Any more feedback?

lachie83 · 2017-08-14T17:32:46Z

examples/kubernetes-features/README.md

@@ -0,0 +1,9 @@
+# Microsoft Azure Container Service Engine - Kubernetes Features


I would personally change this path to kubernetes-custom-networking. Features is too generic

But maxPods is not a networking setting. It is only indirectly related because it affects number of IP addresses.

@anhowe @dmitsh Can you please advise on how this document tree should be structured?

I changed the path to "kubernetes-config" because all of these settings are under orchestratorProfile.kubernetesConfig in the API model. I thought that was a good trade-off between being too generic (as in "features") vs. being too specific (as in "networking"). Please let me know if you disagree/have a proposal.

lachie83

There are a fair few snippets of information in the code that I feel should be verbalized in docs. I'm concerned the knowledge will be lost in the code. For example your arithmetic an the minmax pod setting. We should call out everything you need to consider when using Azure CNI

ofiliz · 2017-08-14T23:27:48Z

@seanknox @lachie83 Can you please sign-off unless you have other comments? I'd love to have this merged soon so we can end-to-end test CNI plugin in large clusters.

LGTM

lachie83 · 2017-08-15T20:49:58Z

docs/clusterdefinition.md

@@ -35,6 +35,7 @@ Here are the valid values for the orchestrator types:
 |clusterSubnet|no|The IP subnet used for allocating IP addresses for pod network interfaces. The subnet must be in the VNET address space. Default value is 10.244.0.0/16.|
 |dockerBridgeSubnet|no|The specific IP and subnet used for allocating IP addresses for the docker bridge network created on the kubernetes master and agents. Default value is 172.17.0.1/16. This value is used to configure the docker daemon using the [--bip flag](https://docs.docker.com/engine/userguide/networking/default_network/custom-docker0).|
 |enableRbac|no|Enable [Kubernetes RBAC](https://kubernetes.io/docs/admin/authorization/rbac/) (boolean - default == false) |
+|maxPods|no|The maximum number of pods per node. The minimum valid value, necessary for running kube-system pods, is 5. Default value is 110.|


It also looks like the default is 30 if networkPolicy == azure. Please document.

ofiliz · 2017-08-16T21:20:21Z

@dmitsh @lachie83 Can we please merge this?

lachie83 · 2017-08-16T21:37:21Z

LGTM. Going to run e2e

lachie83 · 2017-08-18T17:58:57Z

@ofiliz can you please rebase so we can get this merged.

ofiliz · 2017-08-18T21:59:05Z

@lachie83 Rebased, again. Can we please get this merged today? These commits (as part of earlier PRs) are more than 3 months old now, and still waiting to get merged.

jackfrancis · 2017-08-18T22:10:43Z

pkg/acsengine/const.go

-	// when VNET integration is enabled. It can be overridden per pool by setting the pool's IPAdddressCount property.
-	DefaultAgentMultiIPAddressCount = 128
+	// DefaultKubernetesMaxPods is the maximum number of pods to run on a node.
+	DefaultKubernetesMaxPods = 110


Is this value consisent across all acs-engine supported k8s versions? (1.5, 1.6, and 1.7)

jackfrancis · 2017-08-18T22:44:19Z

examples/kubernetes-config/kubernetes-clustersubnet.json

@@ -14,7 +14,7 @@
    },
    "agentPoolProfiles": [
      {
-        "name": "agent",
+        "name": "agentpool1",


Changing this filename breaks our E2E tests, which consume them. Is there a good semantic reason to make a change? If so, we'll have to update our E2E test configurations.

I'll revert them to "agent". Do you need all examples in kubernetes-config directory to use "agent" as pool name?

It's actually the filename change that is breaking E2E configuration.

examples/custom-pod-cidr/kubernetes.json --> examples/kubernetes-config/kubernetes-clustersubnet.json

Do we mean to move the examples/custom-pod-cidr/kubernetes.json file around?

That was a previous CR feedback by @dmitsh. Instead of creating a new directory for every little KubernetesConfig knob we introduce, we are grouping them under one directory.

O.K., @dmitsh's recommendations makes sense. We'll have to fix the E2E configs to point to the new files then. You can do that easily thusly:

In the following three files:

test/acse-conf/acse-feature-validation.json

test/acse-conf/acse-regression.json

test/e2e/kubernetes-deployments.json

Replace all "cluster_definition" values of "custom-pod-cidr/kubernetes.json" with the new relative filepath "kubernetes-config/kubernetes-clustersubnet.json"

Added a new commit that updates the above files.

jackfrancis · 2017-08-18T22:44:30Z

examples/kubernetes-config/kubernetes-dockerbridgesubnet.json

@@ -14,7 +14,7 @@
    },
    "agentPoolProfiles": [
      {
-        "name": "agent",
+        "name": "agentpool1",


ditto comment above

jackfrancis · 2017-08-18T23:05:13Z

parts/kubernetesagentresourcesvmas.t

@@ -260,7 +260,7 @@
        "autoUpgradeMinorVersion": true,
        "settings": {},
        "protectedSettings": {
-          "commandToExecute": "[concat('/usr/bin/nohup /bin/bash -c \"/bin/bash /opt/azure/containers/provision.sh ',variables('tenantID'),' ',variables('subscriptionId'),' ',variables('resourceGroup'),' ',variables('location'),' ',variables('subnetName'),' ',variables('nsgName'),' ',variables('virtualNetworkName'),' ',variables('routeTableName'),' ',variables('primaryAvailabilitySetName'),' ',variables('servicePrincipalClientId'),' ',variables('servicePrincipalClientSecret'),' ',variables('clientPrivateKey'),' ',variables('targetEnvironment'),' ',variables('networkPolicy'),' ',variables('cloudProviderBackoff'),' ',variables('cloudProviderBackoffRetries'),' ',variables('cloudProviderBackoffExponent'),' ',variables('cloudProviderBackoffDuration'),' ',variables('cloudProviderBackoffJitter'),' ',variables('cloudProviderRatelimit'),' ',variables('cloudProviderRatelimitQPS'),' ',variables('cloudProviderRatelimitBucket'),' ', variables('useManagedIdentityExtension'),' ',variables('useInstanceMetadata'),' >> /var/log/azure/cluster-provision.log 2>&1 &\" &')]"
+          "commandToExecute": "[concat('/usr/bin/nohup /bin/bash -c \"/bin/bash /opt/azure/containers/provision.sh ',variables('tenantID'),' ',variables('subscriptionId'),' ',variables('resourceGroup'),' ',variables('location'),' ',variables('subnetName'),' ',variables('nsgName'),' ',variables('virtualNetworkName'),' ',variables('routeTableName'),' ',variables('primaryAvailablitySetName'),' ',variables('servicePrincipalClientId'),' ',variables('servicePrincipalClientSecret'),' ',variables('clientPrivateKey'),' ',variables('targetEnvironment'),' ',variables('networkPolicy'),' ',variables('maxPods'),' ',variables('cloudProviderBackoff'),' ',variables('cloudProviderBackoffRetries'),' ',variables('cloudProviderBackoffExponent'),' ',variables('cloudProviderBackoffDuration'),' ',variables('cloudProviderBackoffJitter'),' ',variables('cloudProviderRatelimit'),' ',variables('cloudProviderRatelimitQPS'),' ',variables('cloudProviderRatelimitBucket'),' ', variables('useManagedIdentityExtension'),' ',variables('useInstanceMetadata'),' >> /var/log/azure/cluster-provision.log 2>&1 &\" &')]"


We lost the change from this recent commit to master:

f477966

s/primaryAvailablitySetName/primaryAvailabilitySetName/

Thanks for catching this. I missed it during the merge. Fixed.

jackfrancis · 2017-08-18T23:05:42Z

parts/kubernetesmasterresources.t

@@ -529,7 +529,7 @@
        "autoUpgradeMinorVersion": true,
        "settings": {},
        "protectedSettings": {
-          "commandToExecute": "[concat('/usr/bin/nohup /bin/bash -c \"/bin/bash /opt/azure/containers/provision.sh ',variables('tenantID'),' ',variables('subscriptionId'),' ',variables('resourceGroup'),' ',variables('location'),' ',variables('subnetName'),' ',variables('nsgName'),' ',variables('virtualNetworkName'),' ',variables('routeTableName'),' ',variables('primaryAvailabilitySetName'),' ',variables('servicePrincipalClientId'),' ',variables('servicePrincipalClientSecret'),' ',variables('clientPrivateKey'),' ',variables('targetEnvironment'),' ',variables('networkPolicy'),' ',variables('cloudProviderBackoff'),' ',variables('cloudProviderBackoffRetries'),' ',variables('cloudProviderBackoffExponent'),' ',variables('cloudProviderBackoffDuration'),' ',variables('cloudProviderBackoffJitter'),' ',variables('cloudProviderRatelimit'),' ',variables('cloudProviderRatelimitQPS'),' ',variables('cloudProviderRatelimitBucket'),' ',variables('useManagedIdentityExtension'),' ',variables('useInstanceMetadata'),' ',variables('apiServerPrivateKey'),' ',variables('caCertificate'),' ',variables('caPrivateKey'),' ',variables('masterFqdnPrefix'),' ',variables('kubeConfigCertificate'),' ',variables('kubeConfigPrivateKey'),' ',variables('username'),' >> /var/log/azure/cluster-provision.log 2>&1\"')]"
+          "commandToExecute": "[concat('/usr/bin/nohup /bin/bash -c \"/bin/bash /opt/azure/containers/provision.sh ',variables('tenantID'),' ',variables('subscriptionId'),' ',variables('resourceGroup'),' ',variables('location'),' ',variables('subnetName'),' ',variables('nsgName'),' ',variables('virtualNetworkName'),' ',variables('routeTableName'),' ',variables('primaryAvailablitySetName'),' ',variables('servicePrincipalClientId'),' ',variables('servicePrincipalClientSecret'),' ',variables('clientPrivateKey'),' ',variables('targetEnvironment'),' ',variables('networkPolicy'),' ',variables('maxPods'),' ',variables('cloudProviderBackoff'),' ',variables('cloudProviderBackoffRetries'),' ',variables('cloudProviderBackoffExponent'),' ',variables('cloudProviderBackoffDuration'),' ',variables('cloudProviderBackoffJitter'),' ',variables('cloudProviderRatelimit'),' ',variables('cloudProviderRatelimitQPS'),' ',variables('cloudProviderRatelimitBucket'),' ',variables('useManagedIdentityExtension'),' ',variables('useInstanceMetadata'),' ',variables('apiServerPrivateKey'),' ',variables('caCertificate'),' ',variables('caPrivateKey'),' ',variables('masterFqdnPrefix'),' ',variables('kubeConfigCertificate'),' ',variables('kubeConfigPrivateKey'),' ',variables('username'),' >> /var/log/azure/cluster-provision.log 2>&1\"')]"


s/primaryAvailablitySetName/primaryAvailabilitySetName/

Fixed. Please double check.

jackfrancis · 2017-08-21T18:31:44Z

@ofiliz comments above are blocking progress of this PR, I'm ready to merge once they're addressed. Let me know if you have any questions!

msftclas added the cla-not-required label Jul 25, 2017

dmitsh suggested changes Jul 25, 2017

View reviewed changes

seanknox previously requested changes Jul 26, 2017

View reviewed changes

dmitsh approved these changes Aug 1, 2017

View reviewed changes

ofiliz force-pushed the k8s-maxpods branch from ec28eaf to d763552 Compare August 1, 2017 22:50

ofiliz force-pushed the k8s-maxpods branch 2 times, most recently from 40729d9 to eed23f1 Compare August 11, 2017 22:33

lachie83 reviewed Aug 14, 2017

View reviewed changes

lachie83 suggested changes Aug 14, 2017

View reviewed changes

ofiliz force-pushed the k8s-maxpods branch from eed23f1 to 54d77df Compare August 14, 2017 23:22

lachie83 reviewed Aug 15, 2017

View reviewed changes

ofiliz force-pushed the k8s-maxpods branch 2 times, most recently from 4aed84e to da4c75c Compare August 15, 2017 23:33

lachie83 approved these changes Aug 18, 2017

View reviewed changes

ofiliz force-pushed the k8s-maxpods branch 2 times, most recently from b4f97d3 to 11b187a Compare August 18, 2017 21:50

jackfrancis reviewed Aug 18, 2017

View reviewed changes

jackfrancis suggested changes Aug 18, 2017

View reviewed changes

ofiliz added 3 commits August 21, 2017 12:19

Add maxPods to Kubernetes configuration

3392944

Group Kubernetes config examples and add maxPods example

5b4b9bc

Update E2E configs to point to the new example files

f9828a5

ofiliz force-pushed the k8s-maxpods branch from 11b187a to f9828a5 Compare August 21, 2017 19:41

jackfrancis merged commit 53c42de into Azure:master Aug 21, 2017

ofiliz deleted the k8s-maxpods branch August 28, 2017 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add maxPods to Kubernetes configuration #1103

Add maxPods to Kubernetes configuration #1103

ofiliz commented Jul 25, 2017 •

edited by acs-bot

Loading

acs-bot commented Jul 25, 2017

dmitsh left a comment

seanknox left a comment

dmitsh commented Aug 1, 2017

ofiliz commented Aug 11, 2017

lachie83 Aug 14, 2017

ofiliz Aug 14, 2017

ofiliz Aug 14, 2017

lachie83 left a comment

ofiliz commented Aug 14, 2017

lachie83 Aug 15, 2017

ofiliz Aug 15, 2017

ofiliz commented Aug 16, 2017

lachie83 commented Aug 16, 2017

lachie83 commented Aug 18, 2017

ofiliz commented Aug 18, 2017

jackfrancis Aug 18, 2017

lachie83 Aug 18, 2017

jackfrancis Aug 18, 2017

ofiliz Aug 21, 2017

jackfrancis Aug 21, 2017

ofiliz Aug 21, 2017

jackfrancis Aug 21, 2017

ofiliz Aug 21, 2017

jackfrancis Aug 18, 2017

jackfrancis Aug 18, 2017

ofiliz Aug 21, 2017

jackfrancis Aug 18, 2017

ofiliz Aug 21, 2017

jackfrancis commented Aug 21, 2017

		@@ -0,0 +1,9 @@
		# Microsoft Azure Container Service Engine - Kubernetes Features

Add maxPods to Kubernetes configuration #1103

Add maxPods to Kubernetes configuration #1103

Conversation

ofiliz commented Jul 25, 2017 • edited by acs-bot Loading

acs-bot commented Jul 25, 2017

dmitsh left a comment

Choose a reason for hiding this comment

seanknox left a comment

Choose a reason for hiding this comment

dmitsh commented Aug 1, 2017

ofiliz commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lachie83 left a comment

Choose a reason for hiding this comment

ofiliz commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofiliz commented Aug 16, 2017

lachie83 commented Aug 16, 2017

lachie83 commented Aug 18, 2017

ofiliz commented Aug 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Aug 21, 2017

ofiliz commented Jul 25, 2017 •

edited by acs-bot

Loading