Polymorphic Scale Client #53743

DirectXMan12 · 2017-10-11T19:01:44Z

This PR introduces a polymorphic scale client based on discovery information that's able to scale scalable resources in arbitrary group-versions, as long as they present the scale subresource in their discovery information.

Currently, it supports extensions/v1beta1.Scale and autoscaling/v1.Scale, but supporting other versions of scale if/when we produce them should be fairly trivial.

It also updates the HPA to use this client, meaning the HPA will now work on any scalable resource, not just things in the extensions/v1beta1 API group.

Release note:

Introduces a polymorphic scale client, allowing HorizontalPodAutoscalers to properly function on scalable resources in any API group.

Unblocks #29698
Unblocks #38756
Unblocks #49504
Fixes #38810

DirectXMan12 · 2017-10-11T19:04:46Z

cc @deads2k @Kargakis @soltysh at long last ;-)

DirectXMan12 · 2017-10-11T19:05:18Z

P.S. I think I got most of the open issues around this, but I'm sure there are more. Let me know if I missed one.

deads2k · 2017-10-11T19:06:26Z

@sttts

deads2k · 2017-10-11T19:07:06Z

cmd/kube-controller-manager/app/autoscaling.go

 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/discovery"
+	discocache "k8s.io/client-go/discovery/cached" // Saturday Night Fever


if the rest of the this pull earns it, I'll let you have it :)

whoops, didn't mean to leave that in :-P

deads2k · 2017-10-11T19:13:40Z

staging/src/k8s.io/client-go/scale/util.go

+	r.mu.RLock()
+	gv, exists := r.subresMap[resource]
+	if !exists {
+		// retry on misses in case we have an update


Can you build a non-caching version, then build a caching version on top of it? This is a little hard for me to read and the complexity can be built up in another pull.

sure, can-do

hmm... this is a little weirder than I initially thought, since a non-cached version would probably use different code paths entirely (i.e. it wouldn't use the "get-all-at-once" API).

hmm... this is a little weirder than I initially thought, since a non-cached version would probably use different code paths entirely (i.e. it wouldn't use the "get-all-at-once" API).

Yeah, that's the bit I don't like.

deads2k · 2017-10-11T19:13:57Z

staging/src/k8s.io/client-go/scale/util.go

+	if !exists {
+		// retry on misses in case we have an update
+		r.mu.RUnlock()
+		r.generateKindMap()


ignoring an error doesn't look right.

deads2k · 2017-10-11T19:16:05Z

staging/src/k8s.io/client-go/scale/util.go

+}
+
+func NewScaleConverter() *ScaleConverter {
+	scheme := runtime.NewScheme()


This makes me very happy.

deads2k · 2017-10-11T19:16:47Z

staging/src/k8s.io/client-go/scale/util.go

+		scheme: scheme,
+		internalVersioner: runtime.NewMultiGroupVersioner(
+			scalescheme.SchemeGroupVersion,
+			schema.GroupKind{Group: scaleext.GroupName, Kind: "Scale"},


This ordering controls default preference, right? Make autoscaling come first.

nope, the order shouldn't matter -- this says "if any of the objects passed in match autoscaling/*.Scale or extensions/*.Scale, return autoscaling/__scale_internal.Scale as the target GVK"

deads2k · 2017-10-11T19:18:25Z

staging/src/k8s.io/client-go/scale/scheme/types.go

+)
+
+// This file contains our own "internal" version of scale that we use for conversions,
+// since we can't use the main Kubernetes internal versions.


This makes me very sad, but so be it.

IIUC with the "alternative API representations", client can request the server to do the conversion. So the internal types and the entire conversion machinery can be removed in the future. Am I too optimistic?

IIUC with the "alternative API representations", client can request the server to do the conversion. So the internal types and the entire conversion machinery can be removed in the future. Am I too optimistic?

Too optimistic. This code will still be correct even once we get there. I would guess we're multiple releases away from re-loosening the server to support this.

deads2k · 2017-10-11T19:21:33Z

staging/src/k8s.io/client-go/scale/scheme/register.go

+limitations under the License.
+*/
+
+package scheme


@caesarxuchao have a look at the package location. It looks ok to me.

The location lgtm, too.

sttts · 2017-10-11T19:23:02Z

/cc @nikhita

k8s-ci-robot · 2017-10-11T19:23:03Z

@sttts: GitHub didn't allow me to request PR reviews from the following users: nikhita.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @nikhita

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

deads2k · 2017-10-11T19:23:29Z

staging/src/k8s.io/client-go/scale/scheme/extensionsv1beta1/register.go

+)
+
+// GroupName is the group name use in this package
+const GroupName = "extensions"


these have to match exactly and you already have a package dependency. Assign them to the extensionsapiv1beta1 values I think.

ah, good point, will do.

deads2k · 2017-10-11T19:24:40Z

staging/src/k8s.io/client-go/scale/scheme/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// Scale represents a scaling request for a resource.
+type Scale struct {


need fuzzing tests on this.

deads2k · 2017-10-11T19:25:57Z

staging/src/k8s.io/client-go/scale/interfaces.go

+	Get(kind schema.GroupKind, name string) (*autoscalingapi.Scale, error)
+
+	// Update updates the scale of the the given scalable resource.
+	Update(ind schema.GroupKind, scale *autoscalingapi.Scale) (*autoscalingapi.Scale, error)


Why aren't you using the hub you defined? If you want to use autoscaling/v1 as your hub, why define a new internal to convert to?

I considered it, but I figured we didn't want to ever expose internal versions in clients. It does make some sense to me to use it, though (although it also make some sense to use internal versions elsewhere, so /me shrugs).

On the flip side, even though we don't expose it, having an internal version makes it a bit easier to reason about the conversions (IMO), especially since it's following the normal Kubernetes hub-and-spoke pattern. AFAIK, there's not a technical reason why we couldn't use external as the hub, though.

On the flip side, even though we don't expose it, having an internal version makes it a bit easier to reason about the conversions (IMO), especially since it's following the normal Kubernetes hub-and-spoke pattern. AFAIK, there's not a technical reason why we couldn't use external as the hub, though.

I think this demonstrates the inherent problem of not exposing internal types. If you want to keep your own hub, I can live with it. You need fuzzers though.

deads2k · 2017-10-11T19:31:25Z

staging/src/k8s.io/client-go/scale/client.go

+		Resource(mapping.Resource).
+		Name(name).
+		SubResource("scale").
+		Do().


If you switched to use a DoRaw, then used your custom scheme for decoding, wouldn't you be able to use any given rest.Interface without constructing a new client from config? I like supporting NewForConfig, I'm just not seeing why I need to have multiple clients and not supporting New.

We still need a rest.Interface for each group-version, no?

We still need a rest.Interface for each group-version, no?

Is it because you need to point to different endpoints? You can construct a path and call AbsPath(). We can expose the function the restclient uses to build the path so that we don't have to hard code here.

Actually making defaultServerURLFor public seems reasonable, but just circumventing the request with AbsPath seems hacky. I think if we're going to change this, we should have a type that provides clients for a given GroupVersion only, and use it in both the scale client and dynamic client (since the dynamic client currently uses the same approach that I have here).

For example:

type Provider interface { GroupVersion(gv schema.GroupVersion) rest.Interface } provider.GroupVersion(schema.GroupVersion{"apps", "v1beta2"}).Get() .Resource("deployments") .Name("foo") .SubResource("scale") .Do()

That looks a bit clunky with the GroupVersion before the Get, but provides a rest.Interface out of it, so it fits with more existing code. An alternative might be:

type MultiVersionInterface interface { // the same methods as rest.Interface, but they return a GroupVersionProvider below } type GroupVersionProvider interface { GroupVersion(schema.GroupVersion) rest.Request } type client MultiVersionInterface = ... client.Get().GroupVersion(schema.GroupVersion{"apps", "v1beta2"}) .Resouce("deployments") .Name("foo") .Do()

But since that doesn't return a rest.Interface anywhere, it's mostly useful only here and in the dynamic client.

WDYT?

But since that doesn't return a rest.Interface anywhere, it's mostly useful only here and in the dynamic client.

WDYT?

I think the dynamic client interface needs some work, but I would definitely accept an .AbsPath here to make the externally facing bits of this API easier to work with.

Let's use .AbsPath here and refactor it with the dynamic client in another PR.

Also, with the first approach, it looks like the provider still returns different restclient for different group versions. That sounds unnecessary.

Before taking the second approach, could we try to add a method to the rest.Request to allow us change group/version after its construction?

deads2k · 2017-10-11T19:35:53Z

staging/src/k8s.io/client-go/scale/interfaces.go

+// the scale subresource.
+type ScaleInterface interface {
+	// Get fetches the scale of the given scalable resource.
+	Get(kind schema.GroupKind, name string) (*autoscalingapi.Scale, error)


This should take a GroupResource. GroupKind was a mistake. The responsibility of determining which resource to use should lie with the caller.

deads2k · 2017-10-11T19:36:28Z

pkg/kubectl/cmd/annotate_test.go

@@ -451,7 +451,7 @@ func TestAnnotateObject(t *testing.T) {
 	f, tf, codec, _ := cmdtesting.NewAPIFactory()
 	tf.Printer = &testPrinter{}
 	tf.UnstructuredClient = &fake.RESTClient{
-		APIRegistry:          api.Registry,
+		GroupVersion:         api.Registry.GroupOrDie(api.GroupName).GroupVersion,


Is this fix in a separate commit. You've got lots of these lines and this update looks good. We could split it out and merge it.

yeah, it's in its own commit (8f9abfd)

deads2k · 2017-10-11T19:37:13Z

pkg/controller/podautoscaler/horizontal.go

+		a.updateStatusIfNeeded(hpaStatusOriginal, hpa)
+		return fmt.Errorf("invalid API version in scale target reference: %v", err)
+	}
+	targetGK := targetGV.WithKind(hpa.Spec.ScaleTargetRef.Kind).GroupKind()


You need to resolve a GroupResource here. You client should deal in the correct types. It helps make it clear that the HPA Kind make a mistake in their API.

ack, makes sense to me

caesarxuchao · 2017-10-11T22:39:55Z

Will kubectl use this client? cc @foxish

foxish · 2017-10-11T22:56:39Z

@caesarxuchao Yeah, kubectl scale should then be able to use this polymorphic client instead.

caesarxuchao · 2017-10-12T01:42:52Z

@DirectXMan12 could you describe what an ideal world will be, supposing we have the alternative API representation, have deprecated extensions/v1beta1.Autoscale?
I think in that ideal world, the scale client will have the same interface, but it won't need to do any conversion or discovery.

deads2k · 2017-10-12T12:28:36Z

@DirectXMan12 could you describe what an ideal world will be, supposing we have the alternative API representation, have deprecated extensions/v1beta1.Autoscale?
I think in that ideal world, the scale client will have the same interface, but it won't need to do any conversion or discovery.

I think you're correct, but I think that is a long way off.

sttts · 2017-10-12T12:45:12Z

federation/pkg/kubefed/unjoin_test.go

@@ -184,9 +184,9 @@ func testUnjoinFederationFactory(name, server, secret string) cmdutil.Factory {
 	tf.ClientConfig = kubefedtesting.DefaultClientConfig()
 	ns := serializer.NegotiatedSerializerWrapper(runtime.SerializerInfo{Serializer: runtime.NewCodec(f.JSONEncoder(), api.Codecs.UniversalDecoder(fedv1beta1.SchemeGroupVersion))})
 	tf.Client = &fake.RESTClient{
-		APIRegistry:          api.Registry,
+		GroupVersion:         api.Registry.GroupOrDie("federation").GroupVersion,
+		InternalGroupName:    api.Registry.GroupOrDie("federation").GroupVersion.Group,


do we have cases with distinct internal and external group names? I think we always register the internal types into all relevant internal group versions.

I don't think so, but figured I'd let them be explicitly separate here since they were previously controlled separately. I can change that if you want.

It simplifies the interface. So why not.

sttts · 2017-10-18T18:20:16Z

"openshift isn't kube, but we can use kube types like Scale for our resources"

But kube wouldn't use OpenShift types. It's not a symmetric relation.

Also note that we don't have the internal type. So we have to build our own. So in the kube-apiserver binary we will have two autoscaling groups, depending on which scheme you look.

deads2k · 2017-10-19T14:12:32Z

@DirectXMan12 failures look real

DirectXMan12 · 2017-10-19T14:15:24Z

yep, I think something broke when I had to rebase.

Previously, the fake RESTClient in client-go required a Registry. It used the Registry to fetch the GroupVersion for the fake client. However, the way it did so was dubious in some cases (it hard-coded the default API group in places), and not strictly necssary. This updates the fake client to just recieve the GroupVersion and internal group name directly, instead of requiring a Registry, so that it can be consumed in unit tests where a Registry isn't necessarily readily available (e.g. elsewhere in client-go).

The fake discovery client currently returns `nil, nil` for several methods. Among them is the `ServerGroups` method, which is used by the discovery REST mapper implementations. This updates the fake discovery client to actually return server groups so that the discovery REST mapper can be used in tests.

This introduces a polymorphic scale client capable of operating against scale subresources which return different group-versions of Scale. The scale subresources may be in group-versions different than the scale itself, so that we no longer need a copy of every scalable resource in the extensions API group. To discovery which Scale group-versions go to which subresources, discovery is used. The scale client maintains its own internal versions and conversions to several external versions, with a "hub" version that's a copy of the autoscaling internal version. It currently supports the following group-versions for Scale subresources: - extensions/v1beta1.Scale - autoscaling/v1.Scale

Previously, we did not have custom code for fuzzing label selectors. Anything that used a label selector (like Scale) had to manually bypass fuzzing the selector, or write its own fuzzer. This introduces a fuzzer for label selectors which generates random correct selectors with random keys and values.

This removes the override on the extensions fuzzer so that extensions.Scale will use the full label selector fuzzer.

k8s-ci-robot · 2017-10-19T16:09:03Z

@DirectXMan12: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	53c0e18b3f63bc5dd2ccb625df1749a690795f72	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-etcd3	53c0e18b3f63bc5dd2ccb625df1749a690795f72	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

This updates the HPA controller to use the polymorphic scale client from client-go. This should enable HPAs to work with arbitrary scalable resources, instead of just those in the extensions API group (meaning we can deprecate the copy of ReplicationController in extensions/v1beta1). It also means that the HPA controller now pays attention to the APIVersion field in `scaleTargetRef` (more specifically, the group part of it). Note that currently, discovery information on which resources are available where is only fetched once (the first time that it's requested). In the future, we may want a refreshing discovery REST mapper.

The output of `go test` is passed throug a grep filter that's used when producing junit output. Unfortunately, when testing round-trip conversions with random strings, grep can become convinced that the test output is binary, and will truncate a failed test before any output is displayed, showing "binary file (standard input) matches". This forces grep to consider test output to be text, so that we always see test results.

This adds a new fake scale client (for use in testing) to match the new polymorphic scale client.

DirectXMan12 · 2017-10-19T17:21:14Z

ok, rebase issues should be solved

deads2k · 2017-10-23T18:25:31Z

/lgtm

k8s-github-robot · 2017-10-23T18:26:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, DirectXMan12, smarterclayton

Associated issue: 29698

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-controller-manager/OWNERS~~ [deads2k,smarterclayton]
~~federation/OWNERS~~ [smarterclayton]
~~hack/OWNERS~~ [deads2k,smarterclayton]
~~pkg/api/OWNERS~~ [smarterclayton]
~~pkg/controller/podautoscaler/OWNERS~~ [DirectXMan12,deads2k,smarterclayton]
~~pkg/kubectl/OWNERS~~ [deads2k,smarterclayton]
~~staging/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-23T20:13:17Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-23T20:39:07Z

Automatic merge from submit-queue (batch tested with PRs 53743, 53564). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 11, 2017

k8s-github-robot assigned luxas and liggitt Oct 11, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2017

deads2k reviewed Oct 11, 2017

View reviewed changes

sttts reviewed Oct 12, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2017

DirectXMan12 force-pushed the feature/polymorphic-scale-client branch 2 times, most recently from e016fbd to 576c1a2 Compare October 18, 2017 20:04

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2017

DirectXMan12 force-pushed the feature/polymorphic-scale-client branch from 576c1a2 to 472edea Compare October 18, 2017 21:31

DirectXMan12 added 5 commits October 19, 2017 11:01

Update extensions fuzzer to use selector fuzzer

3f51bab

This removes the override on the extensions fuzzer so that extensions.Scale will use the full label selector fuzzer.

DirectXMan12 force-pushed the feature/polymorphic-scale-client branch from 472edea to 4abac06 Compare October 19, 2017 15:07

DirectXMan12 added 3 commits October 19, 2017 13:21

[client-go] Add fake scale client

f22bfcd

This adds a new fake scale client (for use in testing) to match the new polymorphic scale client.

DirectXMan12 force-pushed the feature/polymorphic-scale-client branch from 4abac06 to f22bfcd Compare October 19, 2017 17:21

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2017

k8s-github-robot merged commit ca8d97d into kubernetes:master Oct 23, 2017

DirectXMan12 deleted the feature/polymorphic-scale-client branch October 24, 2017 18:36

MaciekPytel mentioned this pull request Oct 25, 2017

[e2e failure] [sig-autoscaling] [HPA] Horizontal pod autoscaling (scale resource: CPU) [sig-autoscaling] [Serial] [Slow] ReplicaSet Should scale ... #54574

Closed

dixudx mentioned this pull request Nov 10, 2017

Automated cherry pick of #54046 #55364

Merged

nikhita mentioned this pull request Jan 11, 2018

kubectl scale should use the scale subresource #29698

Closed

Polymorphic Scale Client #53743

Polymorphic Scale Client #53743

Conversation

DirectXMan12 commented Oct 11, 2017 • edited Loading

DirectXMan12 commented Oct 11, 2017

DirectXMan12 commented Oct 11, 2017

deads2k commented Oct 11, 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

DirectXMan12 Oct 11, 2017 • edited Loading

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

sttts commented Oct 11, 2017

k8s-ci-robot commented Oct 11, 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

DirectXMan12 Oct 12, 2017 • edited Loading

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

DirectXMan12 Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

caesarxuchao commented Oct 11, 2017

foxish commented Oct 11, 2017

caesarxuchao commented Oct 12, 2017

deads2k commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Oct 18, 2017

deads2k commented Oct 19, 2017

DirectXMan12 commented Oct 19, 2017

k8s-ci-robot commented Oct 19, 2017 • edited Loading

DirectXMan12 commented Oct 19, 2017

deads2k commented Oct 23, 2017

k8s-github-robot commented Oct 23, 2017

k8s-github-robot commented Oct 23, 2017

k8s-github-robot commented Oct 23, 2017

DirectXMan12 commented Oct 11, 2017 •

edited

Loading

DirectXMan12 Oct 11, 2017 •

edited

Loading

DirectXMan12 Oct 12, 2017 •

edited

Loading

DirectXMan12 Oct 11, 2017 •

edited

Loading

k8s-ci-robot commented Oct 19, 2017 •

edited

Loading