Adding ability for clients, cache and watcher to work with unstructured #101

shawn-hurley · 2018-07-31T19:37:16Z

Attempt to fix #17 and I think it also addresses #33

/cc @DirectXMan12 @pwittrock can you PTAL and let me know if I am even on the right track?

DirectXMan12

a few comments inline, but generally on the right track.
Needs tests around unstructured as well.

DirectXMan12 · 2018-07-31T19:43:45Z

pkg/cache/internal/cache_reader.go

@@ -73,6 +75,21 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out runtime.O
 	// TODO(directxman12): revisit the decision to always deepcopy
 	obj = obj.(runtime.Object).DeepCopyObject()

+	// If out is supposed to be unstructured handle correctly.
+	if o, ok := out.(*unstructured.Unstructured); ok {


I'm not sure I'm following the logic behind the marshalling and unmarshalling... shouldn't the object already be an unstructured? We should just be able to do the reflection hack...

DirectXMan12 · 2018-07-31T19:44:55Z

pkg/client/apiutil/apimachinery.go

+	}
+	jsonInfo.Serializer = unstructured.UnstructuredJSONScheme
+
+	cfg.NegotiatedSerializer = serializer.NegotiatedSerializerWrapper(jsonInfo)


the only thing that changes here is the negotiated serializer setup, right? Refactor the common bits out into a private function so we're not copying and pasting code around.

Also, do you have to care about status, like the old dynamic client did?

DirectXMan12 · 2018-07-31T19:46:11Z

pkg/client/client_cache.go

@@ -46,7 +47,10 @@ type clientCache struct {

 	// resourceByType caches type metadata
 	resourceByType map[reflect.Type]*resourceMeta
-	mu             sync.RWMutex
+	// resourceByGVK caches type metadat for unstructured


typo: "metadata"

also, call this unstructuredResourceByGVK, so it's clear why it's separate

DirectXMan12 · 2018-07-31T19:47:27Z

pkg/client/client_cache.go

-// getResource returns the resource meta information for the given type of object.
-// If the object is a list, the resource represents the item's type instead.
-func (c *clientCache) getResource(obj runtime.Object) (*resourceMeta, error) {
+func (c *clientCache) handleResourceByGVK(obj runtime.Object) (*resourceMeta, error) {


leave these as get -- it's a bit clearer, IMO

DirectXMan12 · 2018-08-01T15:33:03Z

So, I've been thinking about this a bit more, and I want to make sure we're not introducing a foot-gun with (specifically) the unstructured cache part. Client I'm totally fine with.

I'm curious as to the use case for unstructured cache. The normal case for unstructured client is something like "I want to access a bunch of object regardless of the kind". But if you do that with the caching client, you'll get a new watch that you might not want (you might accidentally end up caching the entire cluster, for instance, if you're running something like GC). You might even get a duplicate cache (one for the concrete object, one for the unstructured).

We should careful here...

shawn-hurley · 2018-08-03T15:34:38Z

@DirectXMan12 and I discussed offline the problems that were identified. I will be re-working this based on the suggestions and help.

DirectXMan12 · 2018-08-09T17:19:44Z

pkg/client/client_test.go

+				By("creating the object a second time")
+				err = cl.Create(context.TODO(), u)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("already exists"))


you should be able to use kapierrors.IsNotFound here.

shawn-hurley · 2018-08-10T15:06:45Z

@DirectXMan12 @droot @pwittrock PTAL I think this is ready for review

DirectXMan12

looks a lot better. couple nits inline. Also want to try something this afternoon around code structure (abstracting over the details of structured vs unstructured to make code flow more straightforward). I'll let you know if it seems reasonable -- have to play with it a bit.b

DirectXMan12 · 2018-08-14T15:57:36Z

pkg/cache/internal/informers_map.go

@@ -73,6 +75,10 @@ type InformersMap struct {
 	// informersByGVK is the cache of informers keyed by groupVersionKind
 	informersByGVK map[schema.GroupVersionKind]*MapEntry

+	// unstructuredInformerByGVK is a cache of informers for unstructured types
+	// keyed by groupVersionKind


add a note as to why they're kept separately -- we don't want to request a structured object but get an unstructured informer and vice versa

DirectXMan12 · 2018-08-14T15:59:27Z

pkg/cache/internal/informers_map.go

+	return i, ok
+
+}
+
 // Get will create a new Informer and add it to the map of InformersMap if none exists.  Returns
 // the Informer from the map.


we need a note that structured and unstructured are treated distinctly

DirectXMan12 · 2018-08-14T16:02:22Z

pkg/client/apiutil/apimachinery.go

+//Copied from deprecated-dynamic/bad_debt.go
+// dynamicCodec is a codec that wraps the standard unstructured codec
+// with special handling for Status objects.
+// Deprecated only used by test code and its wrong


we should have a TODO to figure out why this is bad...

droot · 2018-08-14T17:25:35Z

@shawn-hurley will take a look today. Thanks

droot

Made a first pass, have a few minor comments. Learnt a few new things :). Will take another look, have a few doubts about unstructured, so want to read a bit more.

droot · 2018-08-17T21:56:02Z

pkg/cache/informer_cache.go

-	if err != nil {
-		return nil
-	}
-
 	gvk, err := apiutil.GVKForObject(out, ip.Scheme)


I am assuming GVKForObject has special handling for unstructured.

I don't think that it does. unstructured does conform to the runtime.Object interface and has the methods to retrieve the GVK from that interface at least that is my understanding.

Is there an edge case that I am not considering?

Got it. No, was just curious.

droot · 2018-08-17T21:59:55Z

pkg/cache/internal/deleg_map.go

+	return nil
+}
+
+// WaitForCacheSync waits until all the caches have been synced


nit: . at the end of comment :)

droot · 2018-08-17T22:05:16Z

pkg/cache/internal/informers_map.go

@@ -228,3 +237,38 @@ func (ip *InformersMap) newListWatch(gvk schema.GroupVersionKind) (*cache.ListWa
 		},
 	}, nil
 }
+
+// createUnstructuredClient is a ClientCreatorFunc for use with structured


s/createUnstructuredClient/createStructuredClient

droot · 2018-08-17T22:11:05Z

pkg/client/apiutil/apimachinery.go

+			break
+		}
+	}
+	jsonInfo.Serializer = dynamicCodec{}


comment below about dynamicCodec doesn't boost my confidence. I thought initially it will be used in test code or so, but I see it is being used here in real code.

@droot This code has been used for multiple releases of client-go including 7.0. please see here: https://github.com/kubernetes/client-go/blob/release-7.0/dynamic/client.go#L300

I don't know why it is considered wrong, but if I were to use the rest mapper and create a client-pool and get that client, it would be using the same codec.

I am interested in using the new dynamic client when everything moves to 3.11.

Any ideas on what may be a good workaround if this is unacceptable?

droot · 2018-08-17T22:11:55Z

pkg/client/apiutil/apimachinery.go

+		return nil, nil, err
+	}
+
+	if _, ok := obj.(*metav1.Status); !ok && strings.ToLower(gvk.Kind) == "status" {


interesting we have Status resource as well ?

https://godoc.org/k8s.io/apimachinery/pkg/apis/meta/v1#Status

I believe this is used for errors from the API servers but I don't know for a fact.

correct, the Status Kind is used to convey API errors. It doesn't belong to any particular resource, IIRC.

droot · 2018-08-17T22:14:23Z

pkg/client/client_cache.go

+	// resourceByGVK caches type metadata for unstructured
+	unstructuredResourceByGVK map[schema.GroupVersionKind]*resourceMeta
+	muByType                  sync.RWMutex
+	muByGVK                   sync.RWMutex


consider moving these above the fields they guard.

shawn-hurley · 2018-08-20T19:35:12Z

@DirectXMan12 and I had an offline discussion about the dynamic client.

In the changes, we discussed using the new dynamic client for the cache's list watcher setup. We also talked about a separate client for the unstructured client. This new unstructured client would use the new dynamic client.

@droot Wondering if the above would help with the concerns about the client?

droot

@droot Wondering if the above would help with the concerns about the client?

Sounds good to me.

Can we please add tests ?

droot · 2018-08-22T20:11:29Z

pkg/client/apiutil/apimachinery.go

+		return nil, nil, err
+	}
+
+	if _, ok := obj.(*metav1.Status); !ok && strings.ToLower(gvk.Kind) == "status" {


droot · 2018-08-22T22:01:21Z

pkg/cache/informer_cache.go

-	if err != nil {
-		return nil
-	}
-
 	gvk, err := apiutil.GVKForObject(out, ip.Scheme)


Got it. No, was just curious.

droot · 2018-08-22T22:04:07Z

pkg/client/apiutil/apimachinery.go

+			break
+		}
+	}
+	jsonInfo.Serializer = dynamicCodec{}


droot · 2018-08-22T22:19:34Z

@shawn-hurley ignore my comment about the tests :) I completely missed them (they were probably collapsed)

shawn-hurley · 2018-08-31T19:32:23Z

Updated to the new dynamic client for the unstructured clients.

PTAL @DirectXMan12 @droot

shawn-hurley · 2018-09-05T18:24:59Z

@DirectXMan12 @droot PTAL I think this should be ready to go now

DirectXMan12 · 2018-09-06T20:29:08Z

For what this does, I think it came out pretty sane and readable. Please squash down the commits into logical chunks, and then I think it looks good from my end. We'll want equivalent docs in the gitbook about unstructured as well.

shawn-hurley · 2018-09-10T13:26:20Z

@DirectXMan12 I tried to make it more clear for each commit. PTAL

DirectXMan12 · 2018-09-10T15:43:07Z

better -- generally, commits should not show review or development history -- so for instance, the "refactor structure out of..." commit should probably be squashed into the first commit. We can merge it like this, but I'd prefer to fix that kind of thing up first.

DirectXMan12 · 2018-09-10T18:55:38Z

Want to give @droot one more chance to look this over, but I think it looks good from my end.

pwittrock · 2018-09-14T01:04:18Z

@droot will be back Monday to TAL

* remove cache reader marshal/unmarshal * adding seperation between types and unstructured types in the cache

* By default the client should not create informers for get and list requsts of unstructured types.

droot

Looks good except one comment.

droot · 2018-09-19T16:59:42Z

pkg/client/client.go

@@ -173,7 +154,7 @@ var _ StatusWriter = &statusWriter{}

 // Update implements client.StatusWriter
 func (sw *statusWriter) Update(_ context.Context, obj runtime.Object) error {
-	o, err := sw.client.cache.getObjMeta(obj)
+	o, err := sw.client.typedClient.cache.getObjMeta(obj)


Looks like we forgot to add support for unstructured type here ? Is it intentional ? I think dynamic client support CRUD on "subresources".

shawn-hurley · 2018-09-19T19:22:47Z

@droot PTAL when you have a second

k8s-ci-robot · 2018-09-20T20:25:11Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: shawn-hurley

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

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Adding ability for clients, cache and watcher to work with unstructured

Comment fixes for code generation

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2018

DirectXMan12 suggested changes Jul 31, 2018

View reviewed changes

shawn-hurley force-pushed the unstructured branch from 07b9138 to 6d5cca3 Compare August 1, 2018 14:57

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2018

DirectXMan12 reviewed Aug 9, 2018

View reviewed changes

shawn-hurley changed the title ~~[WIP] - Adding ability for clients, cache and watcher to work with unstructured~~ Adding ability for clients, cache and watcher to work with unstructured Aug 10, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2018

DirectXMan12 suggested changes Aug 14, 2018

View reviewed changes

shawn-hurley force-pushed the unstructured branch 2 times, most recently from 934f564 to 4a4996f Compare August 15, 2018 13:27

droot reviewed Aug 17, 2018

View reviewed changes

droot reviewed Aug 22, 2018

View reviewed changes

shawn-hurley force-pushed the unstructured branch 2 times, most recently from 3ce1536 to 0d3d74b Compare August 31, 2018 19:28

shawn-hurley force-pushed the unstructured branch from 27a3c52 to 9f8892f Compare September 4, 2018 18:28

shawn-hurley force-pushed the unstructured branch from 9f8892f to 44e39f9 Compare September 10, 2018 13:21

shawn-hurley force-pushed the unstructured branch from 1f04ff6 to 15e1a2e Compare September 10, 2018 18:51

DirectXMan12 approved these changes Sep 10, 2018

View reviewed changes

shawn-hurley force-pushed the unstructured branch from 15e1a2e to b6b3cfe Compare September 17, 2018 14:31

Shawn Hurley and others added 7 commits September 18, 2018 09:06

Adding ability for clients, cache and watcher to work with unstructured

f06aa3e

* remove cache reader marshal/unmarshal * adding seperation between types and unstructured types in the cache

Adding delegating reader for get and list requests for unstructured objs

dbc79c9

* By default the client should not create informers for get and list requsts of unstructured types.

Adding test cases for getting and listing unstructured types.

07715f6

Adding unstructured clients testing.

247e85a

Adding informer cache tests.

7744200

Adding ability to use dynamic list for unstructured list watcher.

10ed9c2

Adding two types of client for unstructured and typed

00c5c79

shawn-hurley force-pushed the unstructured branch from b6b3cfe to 00c5c79 Compare September 18, 2018 13:46

shawn-hurley mentioned this pull request Sep 19, 2018

Gopkg.*,pkg/generator/templates.go: update to kubernetes 1.11.2 operator-framework/operator-sdk#391

Closed

droot reviewed Sep 19, 2018

View reviewed changes

Status Writer working with unstructured as well as typed objects

7471369

droot approved these changes Sep 20, 2018

View reviewed changes

droot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 20, 2018

k8s-ci-robot merged commit df7c11e into kubernetes-sigs:master Sep 20, 2018

shawn-hurley mentioned this pull request Sep 20, 2018

Support pkg/client operations for Unstructured objects #33

Closed

hasbro17 mentioned this pull request Sep 20, 2018

Option to restrict informer cache to a namespace #136

Merged

grantr mentioned this pull request Sep 21, 2018

Subscription Controller basics. knative/eventing#437

Merged

hasbro17 mentioned this pull request Oct 3, 2018

Update helm-app-operator-kit to use controller-runtime operator-framework/helm-app-operator-kit#42

Closed

grantr mentioned this pull request Dec 3, 2018

Use CR unstructured client for dynamic objects knative/eventing-contrib#137

Merged

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#101 from shawn-hurley/unstructured

29f003c

Adding ability for clients, cache and watcher to work with unstructured

shawn-hurley mentioned this pull request Mar 26, 2019

REQUEST: New membership for @shawn-hurley kubernetes/org#655

Closed

6 tasks

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #101 from pmorie/comment-fixes

4987c4d

Comment fixes for code generation

Adding ability for clients, cache and watcher to work with unstructured #101

Adding ability for clients, cache and watcher to work with unstructured #101

Conversation

shawn-hurley commented Jul 31, 2018

DirectXMan12 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

DirectXMan12 commented Aug 1, 2018

shawn-hurley commented Aug 3, 2018

Choose a reason for hiding this comment

shawn-hurley commented Aug 10, 2018

DirectXMan12 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

droot commented Aug 14, 2018

droot 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

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

shawn-hurley commented Aug 20, 2018

droot 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

droot commented Aug 22, 2018

shawn-hurley commented Aug 31, 2018

shawn-hurley commented Sep 5, 2018

DirectXMan12 commented Sep 6, 2018

shawn-hurley commented Sep 10, 2018

DirectXMan12 commented Sep 10, 2018

DirectXMan12 commented Sep 10, 2018

pwittrock commented Sep 14, 2018

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley commented Sep 19, 2018

k8s-ci-robot commented Sep 20, 2018