client-gen: independent scheme for clientsets #41486

sttts · 2017-02-15T16:45:30Z

This PR adds a clientset internal scheme instead of using pkg/api.Scheme. The clientset API stays the same.

In detail:

introduce a scheme for each clientset, i.e. do not use pkg/api.Scheme+Registry+Codec+ParameterCodecs.

This makes it easier to compose client-go's clientset (which is rewritten in staging/copy.sh and therefore hardcoded to use k8s.io/client-go/pkg/api.Scheme+Registry+Codecs+ParameterCodecs) with third-party clientsets (kube-aggregator, openshift, federation) which are not rewritten using copy.sh as all of them are self-contained and therefore relocatable.

This fixes https://github.com/kubernetes/kubernetes/pull/41403/files#diff-76edfb07dee54ff7ddeda25c33c10d29R81 and prepares client-gen for use in OpenShift.
register types into the clientset scheme via AddToScheme for versioned clientsets. This decouples the client-go clients from announce+registration (internal clients continue using announce+registry and apigroup installers).

This reduces complexity for client-go, possibly remove the necessity for the announce+register machinery for many use-cases, maybe even to delete it mid-term.
port federation and testgroup install/install.go to announced.GroupMetaFactory in order to have a proper Install.Install(...) func for registration.

With the first change it's easy to add the types of one clientset to the scheme of the other using the clientset/scheme.AddToScheme method. This allows to use cross-clientset runtime.RawExtensions:

import (
     "k8s.io/client-go/kubernetes"
     clientsetscheme "k8s.io/client-go/kuberentes/scheme"
     aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme"
)

kclientset, _ := kubernetes.NewForConfig(c)
aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme)

Kubernetes types with a RawExtension can en/decode aggregator types after this.

TODO:

fix fake clientsets
get the *Options types registered correctly for core, compare DO-NOT-MERGE commit.
get prefered version right in internal client. Do we need all versions registered in the internal client to support negotiation?
run staging/copy.sh and run tests: TEST: update client-go with clientset specific scheme #41744
~~[ ] fixup usage through-out the code-base~~
Follow-up: move import_known_versions.go files somewhere such that import of the api.Scheme package automatically installs the apigroups. It looks like we depended on the import fo the clientset for this purpose.

k8s-reviewable · 2017-02-15T16:46:38Z

This change is

deads2k · 2017-02-15T17:48:25Z

staging/src/k8s.io/client-go/kubernetes/clientset.go

+		configShallowCopy.ParameterCodec = parameterCodec
+	}
+	if configShallowCopy.NegotiatedSerializer == nil {
+		configShallowCopy.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: codecs}


I've missed where codecs is coming from.

register.go in the same package.

sttts · 2017-02-15T18:14:26Z

staging/src/k8s.io/client-go/kubernetes/register.go

+
+var groupFactoryRegistry = make(announced.APIGroupFactoryRegistry)
+var registry = registered.NewOrDie(os.Getenv("KUBE_API_VERSIONS"))
+var scheme = runtime.NewScheme()


@deads2k your comment from irc was that runtime.RawExtension needs all types that should be embedded. E.g. in the PodTemplate case this is the pod.

We could make scheme public of course, or add some way to compose apigroups and clients explicitly.

It needs to be public or openshift will break downstream.

deads2k · 2017-02-15T18:39:29Z

staging/src/k8s.io/client-go/kubernetes/register.go

+	"k8s.io/apimachinery/pkg/apimachinery/registered"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
+	"k8s.io/client-go/pkg/apis/batch/install"


your plan is to generate this and import and install all the ones we need?

With a bit more thought we should be able to handle the clientsets as a big module and combine them easily. Something around these lines:

kubeclientset.AddToScheme(originclientset.Scheme)

With a bit more thought we should be able to handle the clientsets as a big module and combine them easily. Something around these lines:

I agree, but I don't know know if we get there in 1.6. Think you can tie off generators for a separate scheme in 1.6?

deads2k · 2017-02-16T15:39:01Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_clientset.go

@@ -60,9 +60,9 @@ func (g *genClientset) Imports(c *generator.Context) (imports []string) {
 	for _, group := range g.groups {
 		for _, version := range group.Versions {
 			typedClientPath := filepath.Join(g.typedClientPath, group.Group.NonEmpty(), version.NonEmpty())
-			imports = append(imports, strings.ToLower(fmt.Sprintf("%s%s \"%s\"", version.NonEmpty(), group.Group.NonEmpty(), typedClientPath)))
+			imports = append(imports, strings.ToLower(fmt.Sprintf("%s%s \"%s\"", group.Group.NonEmpty(), version.NonEmpty(), typedClientPath)))


wow, really? This is how its being done in client-go? I was reasonably sure you could use it from the universe and it would just end up imported.

This is not clean for sure. Maybe somebody didn't like the autogenerated aliases.

deads2k · 2017-02-16T15:39:44Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_clientset.go

@@ -102,6 +112,17 @@ func (g *genClientset) GenerateType(c *generator.Context, t *types.Type, w io.Wr
 	return sw.Error()
 }

+var globalsTemplate = `
+var Scheme = $.runtimeNewScheme|raw$()
+var codecs = $.serializerNewCodecFactory|raw$(scheme)


Capital Scheme?

fake clientsets have some issues left.

Will do this tomorrow.

deads2k · 2017-02-16T15:44:35Z

cmd/libs/go2idl/client-gen/generators/generator_for_group.go

@@ -195,26 +191,20 @@ func (c *$.GroupVersion$Client) RESTClient() $.RESTClientInterface|raw$ {

 var newClientForRESTClientTemplate = `
 // New creates a new $.GroupVersion$Client for the given RESTClient.
-func New(c $.RESTClientInterface|raw$) *$.GroupVersion$Client {
-	return &$.GroupVersion$Client{c}
+func New(c $.RESTClientInterface|raw$, parameterCodec $.runtimeParameterCodec|raw$) *$.GroupVersion$Client {


This is going to be a very unpopular change. You should be able to keep the old API with a generated and working default param codec (which we'll use 99% of the time) and if you must, a separate method that can take it as a parameter.

deads2k · 2017-02-16T19:13:20Z

federation/client/clientset_generated/federation_clientset/scheme/register.go

+var Codecs = serializer.NewCodecFactory(Scheme)
+var ParameterCodec = runtime.NewParameterCodec(Scheme)
+
+func init() {


This matches my expectations.

deads2k · 2017-02-16T19:14:30Z

...n/client/clientset_generated/federation_clientset/typed/autoscaling/v1/autoscaling_client.go

@@ -63,26 +63,22 @@ func NewForConfigOrDie(c *rest.Config) *AutoscalingV1Client {

 // New creates a new AutoscalingV1Client for the given RESTClient.
 func New(c rest.Interface) *AutoscalingV1Client {
-	return &AutoscalingV1Client{c}
+	return &AutoscalingV1Client{c, scheme.ParameterCodec}


Looks good.

deads2k · 2017-02-16T19:14:47Z

...n/client/clientset_generated/federation_clientset/typed/autoscaling/v1/autoscaling_client.go

-	gv, err := schema.ParseGroupVersion("autoscaling/v1")
-	if err != nil {
-		return err
+	gv := schema.GroupVersion{Group: "autoscaling", Version: "v1"}


much nicer.

deads2k · 2017-02-16T19:15:51Z

...n/client/clientset_generated/federation_clientset/typed/autoscaling/v1/autoscaling_client.go

-	// if autoscaling/v1 is not enabled, return an error
-	if !api.Registry.IsEnabledVersion(gv) {
-		return fmt.Errorf("autoscaling/v1 is not enabled")
+	if config.ParameterCodec == nil {


Would it be correct enough to auto-assign the generated one? If this doesn't cause many problems, I don't really object, but I also don't expect many people to mess with it.

deads2k · 2017-02-16T19:16:22Z

...n/client/clientset_generated/federation_clientset/typed/autoscaling/v1/autoscaling_client.go

-		return err
+	gv := schema.GroupVersion{Group: "autoscaling", Version: "v1"}
+	if config.NegotiatedSerializer == nil {
+		return fmt.Errorf("expected non-nil NegotiatedSerializer for %v client", gv)


Why don't we choose our generated one by default?

Before we had the circular dependency which is gone now. Fixing.

deads2k · 2017-02-16T19:16:47Z

Some comments on defaulting, but I like the output a lot better.

deads2k · 2017-02-17T13:20:47Z

lgtm. Looks like you have a few rough edges to work out.

deads2k · 2017-02-22T14:57:05Z

@k8s-bot unit test this: issue #41885

sttts · 2017-02-22T15:27:54Z

@k8s-bot gce etcd3 e2e test this issue #41889

sttts · 2017-02-22T15:37:23Z

@k8s-bot gci gce e2e test this #41889

deads2k · 2017-02-22T17:43:50Z

@sttts check the staging godeps on this.

k8s-github-robot · 2017-02-22T18:04:27Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: smarterclayton, sttts

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

~~OWNERS~~ [smarterclayton]
~~cmd/libs/go2idl/client-gen/OWNERS~~ [smarterclayton]
~~federation/OWNERS~~ [smarterclayton]
~~pkg/client/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/kube-aggregator/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-ci-robot · 2017-02-22T19:16:46Z

@sttts: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins CRI GCE Node e2e	`c421b89`	link	`@k8s-bot cri node e2e test this`

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.

deads2k · 2017-02-22T19:40:17Z

@k8s-bot verify test this

deads2k · 2017-02-22T19:40:31Z

@k8s-bot gce etcd3 e2e test this

sttts · 2017-02-22T20:51:46Z

@k8s-bot verify test this

k8s-github-robot · 2017-02-23T05:09:32Z

Automatic merge from submit-queue (batch tested with PRs 41146, 41486, 41482, 41538, 41784)

sttts assigned deads2k Feb 15, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Feb 15, 2017

deads2k reviewed Feb 15, 2017

View reviewed changes

sttts commented Feb 15, 2017

View reviewed changes

deads2k reviewed Feb 15, 2017

View reviewed changes

sttts force-pushed the sttts-clientset-scheme branch from 119d47c to dd47c2b Compare February 16, 2017 15:36

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2017

sttts changed the title ~~[PoC] client-go: independent scheme for clientset~~ [WIP] client-go: independent scheme for clientset Feb 16, 2017

deads2k reviewed Feb 16, 2017

View reviewed changes

sttts force-pushed the sttts-clientset-scheme branch from dd47c2b to c421b89 Compare February 16, 2017 17:48

deads2k reviewed Feb 16, 2017

View reviewed changes

sttts force-pushed the sttts-clientset-scheme branch from c421b89 to 3f43ace Compare February 17, 2017 07:47

sttts changed the title ~~[WIP] client-go: independent scheme for clientset~~ client-go: independent scheme for clientset Feb 17, 2017

sttts force-pushed the sttts-clientset-scheme branch from 3f43ace to f284cea Compare February 17, 2017 16:26

sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 17, 2017

sttts force-pushed the sttts-clientset-scheme branch from f284cea to 4a27690 Compare February 17, 2017 17:00

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2017

sttts added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 22, 2017

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

sttts force-pushed the sttts-clientset-scheme branch from 732a8ec to d48f390 Compare February 22, 2017 16:22

sttts added 4 commits February 22, 2017 19:03

Switch to GroupMetaFactory in federation and clientset testgroup

0834ff7

client-gen: general cleanup

49c536b

client-set: use clientset internal scheme

92c36be

Update generated files

595f4ed

sttts force-pushed the sttts-clientset-scheme branch from d48f390 to 595f4ed Compare February 22, 2017 18:03

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017

sttts added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2017

k8s-github-robot merged commit e49f44d into kubernetes:master Feb 23, 2017

sttts deleted the sttts-clientset-scheme branch February 23, 2017 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client-gen: independent scheme for clientsets #41486

client-gen: independent scheme for clientsets #41486

sttts commented Feb 15, 2017 •

edited

k8s-reviewable commented Feb 15, 2017

deads2k Feb 15, 2017

sttts Feb 15, 2017

sttts Feb 15, 2017

deads2k Feb 15, 2017

deads2k Feb 15, 2017

sttts Feb 16, 2017

sttts Feb 16, 2017 •

edited

deads2k Feb 16, 2017

deads2k Feb 16, 2017

sttts Feb 16, 2017

deads2k Feb 16, 2017

sttts Feb 16, 2017

sttts Feb 16, 2017

deads2k Feb 16, 2017

sttts Feb 16, 2017

deads2k Feb 16, 2017

deads2k Feb 16, 2017

deads2k Feb 16, 2017

deads2k Feb 16, 2017

deads2k Feb 16, 2017

sttts Feb 17, 2017

deads2k commented Feb 16, 2017

deads2k commented Feb 17, 2017

deads2k commented Feb 22, 2017

sttts commented Feb 22, 2017

sttts commented Feb 22, 2017

deads2k commented Feb 22, 2017

k8s-github-robot commented Feb 22, 2017

k8s-ci-robot commented Feb 22, 2017 •

edited

deads2k commented Feb 22, 2017

deads2k commented Feb 22, 2017

sttts commented Feb 22, 2017

k8s-github-robot commented Feb 23, 2017

client-gen: independent scheme for clientsets #41486

client-gen: independent scheme for clientsets #41486

Conversation

sttts commented Feb 15, 2017 • edited

k8s-reviewable commented Feb 15, 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

sttts Feb 16, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 16, 2017

deads2k commented Feb 17, 2017

deads2k commented Feb 22, 2017

sttts commented Feb 22, 2017

sttts commented Feb 22, 2017

deads2k commented Feb 22, 2017

k8s-github-robot commented Feb 22, 2017

k8s-ci-robot commented Feb 22, 2017 • edited

deads2k commented Feb 22, 2017

deads2k commented Feb 22, 2017

sttts commented Feb 22, 2017

k8s-github-robot commented Feb 23, 2017

sttts commented Feb 15, 2017 •

edited

sttts Feb 16, 2017 •

edited

k8s-ci-robot commented Feb 22, 2017 •

edited