[WIP] DNS nanny - Autoscale replication controller based on number of nodes #1427

girishkalele · 2016-07-23T03:55:55Z

Adaption of the addon-resizer nanny worked out pretty well.

girishkalele · 2016-07-23T03:56:26Z

roberthbailey · 2016-07-23T04:11:50Z

@Q-Lee since you wrote the addon-resizer can you take a look at this?

girishkalele · 2016-07-25T18:15:10Z

I think the failsafe checks need a little change, instead of continuing with the old params, I think it will be more correct that if we fail to parse the config, we log.Fatal and just bail out of the container.
This will immediately surface to the operator that the configmap is not formatted correctly.

fgrzadkowski · 2016-07-27T00:58:48Z

Why aren't we using addon-resizer for this?

fgrzadkowski · 2016-07-27T01:01:07Z

If we decide to write something new, please don't use a generic name like pod-autoscaler. It suggests that it's the recommended way for doing things, and I believe we all agree it's not. If this is intended to only solve dns problem, then maybe dns-nanny or dns-resizer?

girishkalele · 2016-07-27T17:05:11Z

@fgrzadkowski

The existing addon-resizer works so differently - it doesn't resize replicas, but rather the cpu and memory limits of the pod itself and it works on pods and deployments, not on RCs - that it would have been pretty much two different functionalities in the same binary with more complicated flags than to make a new one for DNS RC replica resizing.

I will change the name to be DNS specific (although the new one is written as a generic sidecar container that can resize any RC it belongs to, so it can be dropped into any RC and it will auto-scale it based on the configmap params)

thockin · 2016-08-02T04:58:32Z

dns-rc-autoscaler/README.md

+# pod-autoscaler
+
+This container image watches over the number of schedulable nodes in the cluster and resizes 
+the number of replicas in its parent object. Works only for pods which are children of RCs or RSs objects.


a short explanation of HOW would be good.

thockin · 2016-08-02T05:02:25Z

You're going to have to break this into multiple commits. One for your
code and another for deps, at least. Github basically can't load this.

As a name, can I propose "horizontal-self-scaler" ?

On Wed, Jul 27, 2016 at 10:05 AM, Girish Kalele [email protected]
wrote:

@fgrzadkowski https://github.com/fgrzadkowski

The existing addon-resizer works so differently - it doesn't resize
replicas, but rather the cpu and memory limits of the pod itself and it
works on pods and deployments, not on RCs - that it would have been pretty
much two different functionalities in the same binary with more complicated
flags than to make a new one for DNS RC replica resizing.

I will change the name to be DNS specific (although the new one is written
as a generic sidecar container that can resize any RC it belongs to, so it
can be dropped into any RC and it will auto-scale it based on the configmap
params)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1427 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVNrTJrJcX_hxEWK3untAEJAzSE9Wks5qZ4_JgaJpZM4JTRpr
.

thockin · 2016-08-02T05:03:41Z

dns-rc-autoscaler/nanny/kubernetes_client.go

+//      "resourceVersion":"20339"}}
+//
+
+func (k *kubernetesClient) GetParentRc(namespace, podname string) (string, error) {


General note: if you give a function a public name (leading capital letter) you had better have a GoDoc comment explaining it. Given that this is all one package (which is fine) I doubt this needs a public name. Same elsewhere.

Q-Lee · 2016-08-02T19:36:34Z

dns-rc-autoscaler/nanny/main/pod_nanny.go

+		log.Fatal(err)
+	}
+	k8s := nanny.NewKubernetesClient(*podNamespace, *podName, clientset)
+	log.Printf("Looking for parent/owner of pod %s/%s\n", *podNamespace, *podName)


Is there a log.Infof?

girishkalele · 2016-08-02T19:40:26Z

@Q-Lee

Will do...I am in the process of moving it to a new repo, will send out the PR for that one.

Q-Lee · 2016-08-02T20:35:01Z

dns-rc-autoscaler/nanny/scaler.go

+func (s Scaler) scaleWithNodes(numCurrentNodes uint64) uint64 {
+	newMap, err := ParseScalerParamsFile(s.ConfigFile)
+	if err != nil {
+		log.Fatalf("Parse failure: The configmap volume file is malformed (%s)\n", err)


Can you validate this at start up?

erictune · 2016-08-02T22:19:44Z

@mml this PR is trying to solve a problem that you have thought about: given a pod, find its controller (you in the context of disruption budget). You might have useful comments.

erictune · 2016-08-02T22:24:12Z

@bgrant0607 said not to use created-by, and to prefer to use Owner References. (which I think is GetOwnerReferences from pkg/api/meta/meta.go. @lavalamp and @caesarxuchao should know about Owner References.

Q-Lee · 2016-08-02T22:42:38Z

The addon-resizer was written with the intent to be extended in this fashion if the use case ever arose. It should be straight forward to extend it in this fashion.

lavalamp · 2016-08-02T22:46:14Z

I'm not a fan of doing this from every DNS pod in the cluster.

lavalamp · 2016-08-02T22:47:13Z

We hope owner references will be in beta and on-by-default in 1.4 but it's not certain at this point.

girishkalele · 2016-08-02T22:49:12Z

I created kubernetes/kubernetes#29922 to request addition of the Created-By annotation to ReplicaSets but we can close that if the OwnerReferences will work for going from ReplicaSet -> parent Deployments.

thockin · 2016-08-02T23:31:24Z

Have a simpler solution?

On Tue, Aug 2, 2016 at 3:46 PM, Daniel Smith [email protected]
wrote:

I'm not a fan of doing this from every DNS pod in the cluster.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1427 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVCfhT1_CoYaLHZGBpAV1-4uyfxwTks5qb8i4gaJpZM4JTRpr
.

lavalamp · 2016-08-02T23:36:33Z

Run one sizing thing on the master node. Alternatively, run one of these
pods in its own RS.

On Tue, Aug 2, 2016 at 4:31 PM, Tim Hockin [email protected] wrote:

Have a simpler solution?

On Tue, Aug 2, 2016 at 3:46 PM, Daniel Smith [email protected]
wrote:

I'm not a fan of doing this from every DNS pod in the cluster.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1427 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/AFVgVCfhT1_CoYaLHZGBpAV1-4uyfxwTks5qb8i4gaJpZM4JTRpr

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1427 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglgBAE44o_nOBYsZ6cZOasHyzXj58ks5qb9NRgaJpZM4JTRpr
.

Q-Lee · 2016-08-03T00:53:13Z

There should be no assumptions in the code that this container must scale its own pod. Inferring defaults is fine, but I need to be able to explicitly override that behavior. And if owner references are not a sure thing for 1.4, then we may consider dropping the inference altogether in this release.

@thockin - I think dan has a good point. If we extend the addon-resizer to another type of scaling, then we could also change it to scale multiple pods. Then it can consume a yaml object per scale, and a single pod could watch all of our addons.

The only down side is that this pod would require a deployment every time an addon is enabled or disabled.

thockin · 2016-08-03T04:36:39Z

I figured running as a sidecar was cute, but I guess it could just be a
standalone pod that takes a string argument like
"replicationcontrollers/kube-dns-v19" or "deployments/kube-dns".

What do you think Girish?

For the sake of simplicity, do we really want to jam this together with the
existing vertical sizer? They are pretty different things.

On Tue, Aug 2, 2016 at 5:53 PM, Q-Lee [email protected] wrote:

There should be no assumptions in the code that this container must scale
its own pod. Inferring defaults is fine, but I need to be able to
explicitly override that behavior. And if owner references are not a sure
thing for 1.4, then we may consider dropping the inference altogether in
this release.

@thockin https://github.com/thockin - I think dan has a good point. If
we extend the addon-resizer to another type of scaling, then we could also
change it to scale multiple pods. Then it can consume a yaml object per
scale, and a single pod could watch all of our addons.

The only down side is that this pod would require a deployment every time
an addon is enabled or disabled.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1427 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVMuVPhhEMbG70ebGEPCr47TX4mU7ks5qb-Z7gaJpZM4JTRpr
.

girishkalele · 2016-08-03T04:48:16Z

@thockin That works too - we can add another RC that provides one replica of the horizontal resizer and point it towards the rc/deployment it should be scaling.

The addon-resizer scales the fluentd pod vertically and the DNS use-case is horizontal scaling. My first look at it, I figured combining them would make it overly complex for a single component, and would be better as two independent modules.
@Q-Lee suggested we can combine them into a single all-purpose scaler. Perhaps we can look at the complexity of the configuration to combine them and decide if combining is simpler.

thockin · 2016-08-03T05:18:54Z

I'd say - IF we want to combine them, we do it later. Let's get something
implemented. If this just takes an arg, it's even simpler.

On Tue, Aug 2, 2016 at 9:48 PM, Girish Kalele [email protected]
wrote:

@thockin https://github.com/thockin That works too - we can add another
RC that provides one replica of the horizontal resizer and point it towards
the rc/deployment it should be scaling.

The addon-resizer scales the fluentd pod vertically and the DNS use-case
is horizontal scaling. My first look at it, I figured combining them would
make it overly complex for a single component, and would be better as two
independent modules.
@Q-Lee https://github.com/Q-Lee suggested we can combine them into a
single all-purpose scaler. Perhaps we can look at the complexity of the
configuration to combine them and decide if combining is simpler.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1427 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVCB4XZRmVgxCHXWATPapdOl8wL0Fks5qcB2SgaJpZM4JTRpr
.

girishkalele · 2016-08-03T16:43:03Z

To summarize a few of the points:

Current config file format is ugly, switch to a standard format - switching to JSON. (Embedding yaml strings inside the configmap yaml seems to give me indentation headaches).
Use ConfigMap ResourceVersion to optimize refresh logic.
Remove created-by and/or OwnerReference magic and point to the target RC/RS/Deployment via cmd line args. This scaler will run as an RC independent from the controller it is scaling.
Implement horizontal scaler independently for now, combine them later if possible.
Close this PR and create a PR from the new repo.

girishkalele · 2016-08-09T21:55:08Z

Moved out of contrib repo.

googlebot added the cla: yes label Jul 23, 2016

girishkalele self-assigned this Jul 23, 2016

girishkalele force-pushed the dns_nanny branch 4 times, most recently from 917d8c9 to cd4c32b Compare July 23, 2016 04:03

roberthbailey assigned Q-Lee and unassigned girishkalele Jul 23, 2016

girishkalele force-pushed the dns_nanny branch 3 times, most recently from 862276c to a17f110 Compare July 26, 2016 00:40

girishkalele force-pushed the dns_nanny branch 2 times, most recently from 0dc925c to fc7f812 Compare July 27, 2016 22:02

DNS Replication Controller resizer nanny

0bc392d

girishkalele force-pushed the dns_nanny branch from fc7f812 to 0bc392d Compare July 27, 2016 23:25

thockin reviewed Aug 2, 2016
View reviewed changes

Q-Lee reviewed Aug 2, 2016
View reviewed changes

This was referenced Aug 3, 2016

Amount of skydns pods does not scale with cluster needs kubernetes/kubernetes#28497

Closed

Horizontal Scaling RC to scale another controller based on number of cores and nodes kubernetes-sigs/cluster-proportional-autoscaler#2

Closed

girishkalele closed this Aug 9, 2016

MrHohn mentioned this pull request Sep 8, 2016

Autoscaler container to scale Deployment/RC/RS based on cluster size kubernetes-sigs/cluster-proportional-autoscaler#3

Merged

cblecker unassigned Q-Lee Apr 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] DNS nanny - Autoscale replication controller based on number of nodes #1427

[WIP] DNS nanny - Autoscale replication controller based on number of nodes #1427

girishkalele commented Jul 23, 2016

girishkalele commented Jul 23, 2016

roberthbailey commented Jul 23, 2016

girishkalele commented Jul 25, 2016

fgrzadkowski commented Jul 27, 2016

fgrzadkowski commented Jul 27, 2016

girishkalele commented Jul 27, 2016

thockin Aug 2, 2016

thockin commented Aug 2, 2016

thockin Aug 2, 2016

Q-Lee Aug 2, 2016

girishkalele commented Aug 2, 2016

Q-Lee Aug 2, 2016

erictune commented Aug 2, 2016

erictune commented Aug 2, 2016

Q-Lee commented Aug 2, 2016

lavalamp commented Aug 2, 2016

lavalamp commented Aug 2, 2016

girishkalele commented Aug 2, 2016

thockin commented Aug 2, 2016

lavalamp commented Aug 2, 2016

Q-Lee commented Aug 3, 2016

thockin commented Aug 3, 2016

girishkalele commented Aug 3, 2016

thockin commented Aug 3, 2016

girishkalele commented Aug 3, 2016

girishkalele commented Aug 9, 2016

[WIP] DNS nanny - Autoscale replication controller based on number of nodes #1427

[WIP] DNS nanny - Autoscale replication controller based on number of nodes #1427

Conversation

girishkalele commented Jul 23, 2016

girishkalele commented Jul 23, 2016

roberthbailey commented Jul 23, 2016

girishkalele commented Jul 25, 2016

fgrzadkowski commented Jul 27, 2016

fgrzadkowski commented Jul 27, 2016

girishkalele commented Jul 27, 2016

thockin Aug 2, 2016

Choose a reason for hiding this comment

thockin commented Aug 2, 2016

thockin Aug 2, 2016

Choose a reason for hiding this comment

Q-Lee Aug 2, 2016

Choose a reason for hiding this comment

girishkalele commented Aug 2, 2016

Q-Lee Aug 2, 2016

Choose a reason for hiding this comment

erictune commented Aug 2, 2016

erictune commented Aug 2, 2016

Q-Lee commented Aug 2, 2016

lavalamp commented Aug 2, 2016

lavalamp commented Aug 2, 2016

girishkalele commented Aug 2, 2016

thockin commented Aug 2, 2016

lavalamp commented Aug 2, 2016

Q-Lee commented Aug 3, 2016

thockin commented Aug 3, 2016

girishkalele commented Aug 3, 2016

thockin commented Aug 3, 2016

girishkalele commented Aug 3, 2016

girishkalele commented Aug 9, 2016