Implemented Linear Controller and added unit test. #5

MrHohn · 2016-10-19T04:09:35Z

The autoscaler are now supporting one more control pattern: linear mode. An example for the ConfigMap params as below:

data:
  linear: |-
    { 
      "coresPerReplica": 2,
      "nodesPerReplica": 1,
      "min": 1,
      "max": 100
    }

Because serious performance evaluation of kube-dns might not be finished soon, I would like to use this simple pattern for the DNS horizontal scaling for now (will tune the specific params). Details for this new pattern are updated in README.

@bowei @thockin @bprashanth
cc @matchstick

thockin · 2016-10-20T00:30:50Z

this is net simpler, which is good. I forget why Girish wanted the ladder mode. Do we need to keep it?

MrHohn · 2016-10-20T00:46:48Z

I don't think he gave out the reason. But I think it does not harm to keep it.

thockin · 2016-10-20T01:44:48Z

Dead code is a liability - keeping it around means we support it and debug
it and compile it and test it. If we really think linear is sufficient,
and I don't see why not, we should remove the ladder. YAGNI.

https://dougseven.com/2014/04/17/knightmare-a-devops-cautionary-tale/

On Wed, Oct 19, 2016 at 5:46 PM, Zihong Zheng notifications@github.com
wrote:

I don't think he gave out the reason. But I think it does not harm to keep
it.

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

MrHohn · 2016-10-20T06:38:28Z

Your concern totally makes sense, but I vote for keeping the ladder mode:

I won't say ladder mode is completely useless. It is, in my opinion, another common and intuitive control pattern.
linear mode might be sufficient for kube-dns scaling. But I would like to see other applications also use this autoscaler and they may have different requirements.
ladder mode does something that linear mode can not do. It provides users more flexibility and customization. Before we have better substitute for it, I would like to support this pattern.

bowei · 2016-10-20T22:24:37Z

I would agree with keeping ladder as it is a way out for people to implement their own arbitrary scaling function

thockin · 2016-10-25T18:12:07Z

OK.

On Thu, Oct 20, 2016 at 3:24 PM, Bowei Du notifications@github.com wrote:

I would agree with keeping ladder as it is a way out for people to
implement their own arbitrary scaling function

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

bowei · 2016-10-20T21:25:00Z

pkg/autoscaler/controller/linearcontroller/linear_controller.go

+	Max             int `json:"max"`
+}
+
+func (c *LinearController) SyncConfig(configMap k8sclient.ConfigMap) error {


this seems to be duplicated from ladder controller. Can we move this to a common base struct or move the version change logic out.

See https://play.golang.org/p/fBLkWIZR01

Moved out the version change logic.

bowei · 2016-11-02T23:17:03Z

pkg/autoscaler/controller/linearcontroller/linear_controller.go

+}
+
+// parseParams Parse the params from JSON string
+func parseParams(data []byte) (params *linearParams, err error) {


you don't reference params or err, probably should leave them anonymous

bowei · 2016-11-02T23:32:07Z

pkg/autoscaler/controller/linearcontroller/linear_controller_test.go

+		numResources int
+		expReplicas  int
+	}{
+		{testController, 0, 2},


Why is testController the same for all the testcases? can we remove this field from the struct?

bowei · 2016-11-02T23:32:39Z

pkg/autoscaler/controller/linearcontroller/linear_controller_test.go

+	}
+}
+
+func TestControllerScaler(t *testing.T) {


no testing of max between linear on nodes and linear on pods

Added one more test.

bowei

some doc nits

bowei · 2016-11-03T06:19:37Z

README.md

+    }
+```
+
+The idea of linear control mode is simple: how many cores or nodes could be taking care by one replica?


I think you should put the equation and then give the example

replicas = max( cores * 1/coresPerReplica , nodes * 1/nodesPerReplica )

Yeah, an equation will explain the idea more clearly. Added.

bowei · 2016-11-03T06:20:20Z

autoscaler-ladder-example.yaml

@@ -77,7 +77,7 @@ spec:
        app: autoscaler
    spec:
      containers:
-        - image: gcr.io/google_containers/cluster-proportional-autoscaler-amd64:0.7.2
+        - image: gcr.io/google_containers/cluster-proportional-autoscaler-amd64:0.8.2


why is the patch number still .2? I would think this will be 0.8.0

bowei · 2016-11-03T06:20:39Z

autoscaler-linear-example.yaml

+        app: autoscaler
+    spec:
+      containers:
+        - image: gcr.io/google_containers/cluster-proportional-autoscaler-amd64:0.8.2


fix it here as well

bowei

LGTM

MrHohn · 2016-11-03T18:38:16Z

Thanks! Self merging.

coredns: Add customlabels to configmap

This was referenced Oct 19, 2016

Support switching control pattern on-the-fly. #6

Closed

More control patterns are needed beside ladder mode. #4

Closed

thockin assigned bowei Oct 20, 2016

MrHohn added the priority/P1 label Oct 25, 2016

MrHohn mentioned this pull request Oct 25, 2016

Supports default ConfigMap params #8

Merged

MrHohn mentioned this pull request Oct 31, 2016

Adds doc for DNS horizontal autoscaling feature kubernetes/kubernetes#35931

Closed

bowei reviewed Nov 2, 2016

View reviewed changes

Implement linear controller and unit test

e837db0

MrHohn force-pushed the linear_controller branch from 582390b to f6e1447 Compare November 3, 2016 03:23

bowei reviewed Nov 3, 2016

View reviewed changes

Update README and example yamls for linear controller

7f3d5c0

MrHohn force-pushed the linear_controller branch from f6e1447 to 7f3d5c0 Compare November 3, 2016 16:54

bowei approved these changes Nov 3, 2016

View reviewed changes

MrHohn merged commit 7a0a4e2 into kubernetes-sigs:master Nov 3, 2016

MrHohn deleted the linear_controller branch October 5, 2017 00:59

sftim pushed a commit to sftim/cluster-proportional-autoscaler that referenced this pull request Feb 27, 2023

Merge pull request kubernetes-sigs#5 from mrueg/labels

7a3ec21

coredns: Add customlabels to configmap

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Linear Controller and added unit test. #5

Implemented Linear Controller and added unit test. #5

MrHohn commented Oct 19, 2016 •

edited

Loading

thockin commented Oct 20, 2016

MrHohn commented Oct 20, 2016

thockin commented Oct 20, 2016

MrHohn commented Oct 20, 2016 •

edited

Loading

bowei commented Oct 20, 2016

thockin commented Oct 25, 2016

bowei Oct 20, 2016

MrHohn Nov 3, 2016

bowei Nov 2, 2016

MrHohn Nov 3, 2016

bowei Nov 2, 2016

MrHohn Nov 3, 2016

bowei Nov 2, 2016

MrHohn Nov 3, 2016

bowei left a comment

bowei Nov 3, 2016

MrHohn Nov 3, 2016

bowei Nov 3, 2016

MrHohn Nov 3, 2016

bowei Nov 3, 2016

MrHohn Nov 3, 2016

bowei left a comment

MrHohn commented Nov 3, 2016

Implemented Linear Controller and added unit test. #5

Implemented Linear Controller and added unit test. #5

Conversation

MrHohn commented Oct 19, 2016 • edited Loading

thockin commented Oct 20, 2016

MrHohn commented Oct 20, 2016

thockin commented Oct 20, 2016

MrHohn commented Oct 20, 2016 • edited Loading

bowei commented Oct 20, 2016

thockin commented Oct 25, 2016

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

bowei left a 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

bowei left a comment

Choose a reason for hiding this comment

MrHohn commented Nov 3, 2016

MrHohn commented Oct 19, 2016 •

edited

Loading

MrHohn commented Oct 20, 2016 •

edited

Loading