Enable HorizontalPodAutoscaler Support for DeploymentConfigs #5310

DirectXMan12 · 2015-10-22T00:16:33Z

This PR adds support for HPAs scaling DeploymentConfigs. A new "HybridClient" struct is introduced to support Namespacers that need to point at resources or subresources that could be in either the Origin API or the Kube API (like Scale).

This commit also imports the Scale kind into the Origin v1 API (but it's still "backed" in the unversioned API by the underlying Kube resource).

Finally, this commit contains a patch to Kube that converts the HPA controller from using a single Client type to using separate Namespacers for its various needs.

TODOs

cherry-pick Fix GetRequestInfo subresource parsing for proxy/redirect verbs kubernetes/kubernetes#16570 and update policy.
cherry pick HorizontalPodAutoscaler and Scale subresource APIs graduated to beta. kubernetes/kubernetes#15706 from upstream into this PR (underneath your changes
open issue for " fix the metrics client to take pod names, rather than requiring cluster-wide pod-listing privileges" https://github.com/openshift/origin/pull/5310/files#r43427148 (DONE: HPA Metrics Client should take a list of pod names instead of a selector kubernetes/kubernetes#16675)
rename UPSTREAM commits to match UPSTREAM: PR or UPSTREAM: <carry> format
remove WIP from commit name
open issue for " fix the client to no longer need root proxy access" https://github.com/openshift/origin/pull/5310/files#r43427078
add end-to-end or extended test in origin to prove that this works without default (and comparatively restrictive) policy.
add a comment to Update to document the order in which the deploymentconfig controller does the updates to resources as it creates new RCs and changes its RC's replicas. That information is required to reason about race potential.
resolve issue with cross-namespace HPA requests (prevent it and add a test ensuring it is prevented) UPSTREAM: 16668: Fix hpa escalation #5579

DirectXMan12 · 2015-10-22T00:16:50Z

cc @ncdc @deads2k

DirectXMan12 · 2015-10-22T00:21:31Z

Once #5286 lands, I'll rebase on top of that.

@ncdc @smarterclayton this PR currently follows the upstream Kube in that it updates the replicas field on the actual deployment controller. It occurs to me that this does not appear to do much in Origin (since future deployments seem to be based on the Scale of the previous deployment). Should I update so that it affects the latest deployment itself (like the scale command in Origin)?

ncdc · 2015-10-22T00:38:51Z

@ironcladlou

ncdc · 2015-10-22T00:40:00Z

@DirectXMan12 please use separate commits for Godeps modifications, and make sure the commit message starts with UPSTREAM. Thanks!

smarterclayton · 2015-10-22T02:36:52Z

Answer is yes.

On Wed, Oct 21, 2015 at 8:40 PM, Andy Goldstein [email protected]
wrote:

@DirectXMan12 https://github.com/DirectXMan12 please use separate
commits for Godeps modifications, and make sure the commit message starts
with UPSTREAM. Thanks!

—
Reply to this email directly or view it on GitHub
#5310 (comment).

liggitt · 2015-10-22T02:46:22Z

pkg/client/client.go

@@ -297,3 +297,16 @@ func DefaultOpenShiftUserAgent() string {
 	version = seg[0]
 	return fmt.Sprintf("%s/%s (%s/%s) openshift/%s", path.Base(os.Args[0]), version, runtime.GOOS, runtime.GOARCH, commit)
 }
+
+type HybridClient struct {


add var _ = unversioned.ScaleNamespacer(&HybridClient{}) to clarify and enforce the intent

Nix this. Its been rejected on principle mulitple times.

@deads2k joint client?

@deads2k joint client?

Yes. HybridClient should not exist. You should specify what you want in your API.

Ok, I think that our Client should include the ExtensionsClient and then internally mix the clients like we already do for scale and delete.

See how the Kubernetes client is currently implemented: https://github.com/kubernetes/kubernetes/blob/071d21257fdfd439a9286fffbd5972c85643cd49/pkg/client/unversioned/client.go#L128-l134

We should do the same for our client

See how the Kubernetes client is currently implemented: https://github.com/kubernetes/kubernetes/blob/071d21257fdfd439a9286fffbd5972c85643cd49/pkg/client/unversioned/client.go#L128-l134

We should do the same for our client

This is what they do

// Client is the implementation of a Kubernetes client. type Client struct { *RESTClient *ExtensionsClient // TODO: remove this when we re-structure pkg/client. *DiscoveryClient }

I don't think that this is a good thing. Having a single interface like that encourages non-specificity and it does not scale well as you add different clients. I think that a reasonable division is on the APIGroup boundary. Using different clients for different APIGroups will help keep the dependency structure clear, prevents a mega-client problem, and allows a clear expectation as APIGroups are enabled and disabled.

@smarterclayton as I recall, you were not a fan combined Clients.

I am not, we should be providing an implementation of ScaleNamespacer and
ScaleInterface that multiplexes to clients based on known kinds.

On Thu, Oct 22, 2015 at 1:35 PM, David Eads [email protected]
wrote:

In pkg/client/client.go
#5310 (comment):

@@ -297,3 +297,16 @@ func DefaultOpenShiftUserAgent() string {
version = seg[0]
return fmt.Sprintf("%s/%s (%s/%s) openshift/%s", path.Base(os.Args[0]), version, runtime.GOOS, runtime.GOARCH, commit)
}
+
+type HybridClient struct {

See how the Kubernetes client is currently implemented:
https://github.com/kubernetes/kubernetes/blob/071d21257fdfd439a9286fffbd5972c85643cd49/pkg/client/unversioned/client.go#L128-l134

We should do the same for our client

This is what they do

// Client is the implementation of a Kubernetes client.type Client struct {
*RESTClient
*ExtensionsClient
// TODO: remove this when we re-structure pkg/client.
*DiscoveryClient
}

I don't think that this is a good thing. Having a single interface like
that encourages non-specificity and it does not scale well as you add
different clients. I think that a reasonable division is on the APIGroup
boundary. Using different clients for different APIGroups will help keep
the dependency structure clear, prevents a mega-client problem, and allows
a clear expectation as APIGroups are enabled and disabled.

@smarterclayton https://github.com/smarterclayton as I recall, you were
not a fan combined Clients.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5310/files#r42779351.

Or even better, uses "IsOriginResource" to select the origin kinds, and
defaults all other to kube.

On Thu, Oct 22, 2015 at 2:13 PM, Clayton Coleman [email protected]
wrote:

I am not, we should be providing an implementation of ScaleNamespacer and
ScaleInterface that multiplexes to clients based on known kinds.

On Thu, Oct 22, 2015 at 1:35 PM, David Eads [email protected]
wrote:

In pkg/client/client.go
#5310 (comment):

@@ -297,3 +297,16 @@ func DefaultOpenShiftUserAgent() string {
version = seg[0]
return fmt.Sprintf("%s/%s (%s/%s) openshift/%s", path.Base(os.Args[0]), version, runtime.GOOS, runtime.GOARCH, commit)
}
+
+type HybridClient struct {

See how the Kubernetes client is currently implemented:
https://github.com/kubernetes/kubernetes/blob/071d21257fdfd439a9286fffbd5972c85643cd49/pkg/client/unversioned/client.go#L128-l134

We should do the same for our client

This is what they do

// Client is the implementation of a Kubernetes client.type Client struct {
*RESTClient
*ExtensionsClient
// TODO: remove this when we re-structure pkg/client.
*DiscoveryClient
}

I don't think that this is a good thing. Having a single interface like
that encourages non-specificity and it does not scale well as you add
different clients. I think that a reasonable division is on the APIGroup
boundary. Using different clients for different APIGroups will help keep
the dependency structure clear, prevents a mega-client problem, and allows
a clear expectation as APIGroups are enabled and disabled.

@smarterclayton https://github.com/smarterclayton as I recall, you
were not a fan combined Clients.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5310/files#r42779351.

0xmichalis · 2015-10-22T13:19:42Z

Can you rebase on top of the latest master?

0xmichalis · 2015-10-22T14:41:29Z

pkg/deploy/api/v1/register.go

 	)
 }

 func (*DeploymentConfig) IsAnAPIObject()         {}
 func (*DeploymentConfigList) IsAnAPIObject()     {}
 func (*DeploymentConfigRollback) IsAnAPIObject() {}
+func (*Scale) IsAnAPIObject() {}


Since this is going to be a dc subresource I would go with DeploymentConfigScale.

Upstream didn't do that. They just call it scale, and it's a subresource under both ReplicationController and Deployment.

DirectXMan12 · 2015-10-22T19:13:14Z

I rebased and made the changes WRT the scale client. The change to have it affect the current deployment should be up shortly

DirectXMan12 · 2015-10-22T20:07:06Z

Ok, scale client affects current deployment now.

ncdc · 2015-10-22T20:09:35Z

pkg/deploy/registry/deployconfig/etcd/etcd.go

+	Scale            *ScaleREST
+}
+
+func NewStorage(s storage.Interface, rcNamespacer kclient.ReplicationControllersNamespacer) DeploymentConfigStorage {


DirectXMan12 · 2015-10-22T20:47:47Z

@deads2k @liggitt this look good for the new way of accessing the scale subresources?

DirectXMan12 · 2015-10-23T00:08:16Z

It also occurs to me that currently, the HPA expects heapster to be at "kube-system/heapster". Where should/does it live for OpenShift (and should I modify the kube code so that it's not in a constant, or do we just want to carry a patch to the constant)?

liggitt · 2015-10-23T00:09:45Z

That's hard coded? I would expect it to be configurable.

DirectXMan12 · 2015-10-23T00:55:09Z

@liggitt behold: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/podautoscaler/metrics/metrics_client.go#L37

smarterclayton · 2015-10-23T01:11:24Z

It's ok to upstream patch this first in this PR and get the follow up in
later.

On Oct 22, 2015, at 8:55 PM, Solly [email protected] wrote:

@liggitt https://github.com/liggitt behold:
https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/podautoscaler/metrics/metrics_client.go#L37

—
Reply to this email directly or view it on GitHub
#5310 (comment).

deads2k · 2015-11-02T18:56:39Z

pkg/deploy/registry/deployconfig/etcd/etcd.go

+
+	oldReplicas := controller.Spec.Replicas
+	controller.Spec.Replicas = scale.Spec.Replicas
+	if _, err = r.rcNamespacer.ReplicationControllers(deploymentConfig.Namespace).Update(controller); err != nil {


Add TODO to handle scale down by scaling down old deployment RCs in preference to new ones. If we merge like this, we should make an issue to fix it.

We shouldn't be scaling at all unless there's only one RC in business, correct?

Since this is going to race against the controller, you cannot get into a state where the new RC is being created but the old RC has been completely scaled down.

We shouldn't be scaling at all unless there's only one RC in business, correct?

I'm reasonably certain that its possible for HPA read a DC on thread A, DCController starts a new RC and scales down the old one on thread B, HPA reads old RC on threadA, HPA scales old RC on threadA.

Once that happens, then the Get will indicate that you have too many replicas running, the Update is called again and you scale down the new RC

Ah, I see what you're saying. The next cycle through should see that we have two RCs running, though, so it won't succeed on the second scale:

[HPA] get dc/scale --> need to scale up

HPA calls update, races with DC controller due to out of date info between [get dc] and [get rc], scales old RC

HPA waits for next cycle

Either we're done deploying and HPA scales up the new RC or we're not done scaling, HPA does a get, calculates, and then does an update, which bounces since we're not done deploying.

deads2k · 2015-11-02T19:44:05Z

lgetm

smarterclayton · 2015-11-02T19:49:49Z

can you link the follow up issue for races here and set it to p1?

openshift-bot · 2015-11-02T19:56:56Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6693/) (Image: devenv-rhel7_2637)

openshift-bot · 2015-11-02T19:56:57Z

[Test]ing while waiting on the merge queue

deads2k · 2015-11-02T20:00:39Z

can you link the follow up issue for races here and set it to p1?

created #5597

deads2k · 2015-11-02T20:42:49Z

gofmt

hack/verify-gofmt.sh
!!! gofmt needs to be run on the following files: 
./pkg/client/scale.go
./pkg/client/testclient/fake_deploymentconfigs.go
./pkg/cmd/server/kubernetes/master.go
./pkg/deploy/registry/deployconfig/etcd/etcd.go
Try running 'gofmt -s -d [path]'

liggitt · 2015-11-02T21:21:51Z

please run all hack/verify-*.sh scripts locally (as well as hack/test-{go,cmd,integration}.sh) to help the poor merge queue

This commit introduces the Scale subresource for DeploymentConfigs. The "Spec" field reflects the state of the most recent deployment, or, if none is present, the state of the DeploymentConfig template. The "Status" field reflects the state of all deployments for the given DeploymentConfig. This roughtly equivalent to how the Scale resource for upstream Deployments work.

…irect verbs

This commit makes the HorizontalPodAutoscaler controller use the DelegatingScaleNamespacer so that it can reach both Kubernetes objects with Scale subresources, as well as Origin DeploymentConfigs.

liggitt · 2015-11-03T05:37:03Z

[test]

openshift-bot · 2015-11-03T06:33:00Z

Evaluated for origin test up to 27cc70a

openshift-bot · 2015-11-03T07:34:34Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6693/)

deads2k · 2015-11-03T12:56:03Z

removed merge. Is it actually too late?

deads2k · 2015-11-03T14:49:10Z

got ok from @smarterclayton [merge]

openshift-bot · 2015-11-03T15:29:12Z

Evaluated for origin merge up to 27cc70a

Merged by openshift-bot

liggitt reviewed Oct 22, 2015
View reviewed changes

0xmichalis reviewed Oct 22, 2015
View reviewed changes

DirectXMan12 force-pushed the feature/hpa-deploymentconfig branch from 9ee049b to 5c3f06e Compare October 22, 2015 19:11

DirectXMan12 force-pushed the feature/hpa-deploymentconfig branch from 5c3f06e to ff4ef9c Compare October 22, 2015 20:06

ncdc reviewed Oct 22, 2015
View reviewed changes

DirectXMan12 force-pushed the feature/hpa-deploymentconfig branch 3 times, most recently from edcd5ed to af348ec Compare October 22, 2015 20:44

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2015

openshift-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 23, 2015

UPSTREAM: 16677: Add Validator for Scale Objects

603c7d3

DirectXMan12 force-pushed the feature/hpa-deploymentconfig branch from 8ff972b to d0860f3 Compare November 2, 2015 18:56

deads2k reviewed Nov 2, 2015
View reviewed changes

deads2k changed the title ~~[WIP] Enable HorizontalPodAutoscaler Support for DeploymentConfigs~~ Enable HorizontalPodAutoscaler Support for DeploymentConfigs Nov 2, 2015

smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2015

This was referenced Nov 2, 2015

Need extended test to show HPA controller works with default permissions #5595

Closed

Resolve HPA DC scaling races #5597

Closed

DirectXMan12 force-pushed the feature/hpa-deploymentconfig branch 2 times, most recently from cca8151 to e0e5c29 Compare November 2, 2015 22:46

DirectXMan12 added 4 commits November 2, 2015 19:19

UPSTREAM: 16671: Customize HPA Heapster service namespace/name

c6f3898

UPSTREAM: 16570: Fix GetRequestInfo subresource parsing for proxy/red…

27cc70a

…irect verbs

Add HPA support for DeploymentConfig

663f1d9

This commit makes the HorizontalPodAutoscaler controller use the DelegatingScaleNamespacer so that it can reach both Kubernetes objects with Scale subresources, as well as Origin DeploymentConfigs.

DirectXMan12 force-pushed the feature/hpa-deploymentconfig branch from e0e5c29 to 663f1d9 Compare November 3, 2015 00:19

openshift-bot pushed a commit that referenced this pull request Nov 3, 2015

Merge pull request #5310 from DirectXMan12/feature/hpa-deploymentconfig

b12d9f1

Merged by openshift-bot

openshift-bot merged commit b12d9f1 into openshift:master Nov 3, 2015

DirectXMan12 deleted the feature/hpa-deploymentconfig branch November 3, 2015 15:32

Enable HorizontalPodAutoscaler Support for DeploymentConfigs #5310

Enable HorizontalPodAutoscaler Support for DeploymentConfigs #5310

Conversation

DirectXMan12 commented Oct 22, 2015

DirectXMan12 commented Oct 22, 2015

DirectXMan12 commented Oct 22, 2015

ncdc commented Oct 22, 2015

ncdc commented Oct 22, 2015

smarterclayton commented Oct 22, 2015

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

0xmichalis commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Oct 22, 2015

DirectXMan12 commented Oct 22, 2015

Choose a reason for hiding this comment

DirectXMan12 commented Oct 22, 2015

DirectXMan12 commented Oct 23, 2015

liggitt commented Oct 23, 2015

DirectXMan12 commented Oct 23, 2015

smarterclayton commented Oct 23, 2015

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

deads2k commented Nov 2, 2015

smarterclayton commented Nov 2, 2015

openshift-bot commented Nov 2, 2015

openshift-bot commented Nov 2, 2015

deads2k commented Nov 2, 2015

deads2k commented Nov 2, 2015

liggitt commented Nov 2, 2015

liggitt commented Nov 3, 2015

openshift-bot commented Nov 3, 2015

openshift-bot commented Nov 3, 2015

deads2k commented Nov 3, 2015

deads2k commented Nov 3, 2015

openshift-bot commented Nov 3, 2015