use new dynamic client #80

crimsonfaith91 · 2018-08-23T00:59:48Z

This PR addresses issue #47. It changes the codebase to use new dynamic client.

Fixes #47
Fixes #79

crimsonfaith91 · 2018-08-23T01:30:13Z

dynamic/informer/informer.go

+type dynamicResourceClientListFunc func(metav1.ListOptions) (*unstructured.UnstructuredList, error)
+
+func castToCacheListFunc(fn dynamicResourceClientListFunc) cache.ListFunc {
+	options := metav1.ListOptions{}


Is an empty ListOptions acceptable?

In practice, I've only set the LabelSelector of a ListOption before. It might be good hygiene to set timeout but that will likely cause more problems on slow clusters that it would prevent.

So, I think an empty LabelSelector is alright.

This is meant to just be an adapter function, so you should pass along the ListOptions from the caller. See my other comment below, which should make this clearer.

rlguarino · 2018-08-23T15:22:47Z

Gopkg.toml


 [[override]]
  name = "github.com/json-iterator/go"
-  version = "1.1.4" # as used by apimachinery@kubernetes-1.9.9
+  version = "1.1.5" # as used by apimachinery@kubernetes-1.11.0

 [prune]


I take it this is working for you? I only say that because I was running into all sorts of grouble getting code generation to work. See #79. I was able to get it to work adding the following to my GoPkg.toml:

[prune] go-tests = true unused-packages = true [[prune.project]] name = "k8s.io/code-generator" unused-packages = false

I was getting errors that the generator couldn't find the boilerplate.go.txt that comes with the code-generator repository.

Me too - There is no boilerplate.go.txt within the vendor code-generator repository, so I added it manually (which is wrong) to unblock myself. I forgot to point this out, thanks for the reminder!

Let me sync with @enisoc, and get back to you later. I believe your way of getting around this is better.

@crimsonfaith91 I agree with the solution @rlguarino suggested. Can you try adding that?

@rlguarino Thanks for tracking that down!

@enisoc Sure! I will include the [prune.project] section.

rlguarino · 2018-08-23T15:29:14Z

dynamic/informer/informer.go

+type dynamicResourceClientListFunc func(metav1.ListOptions) (*unstructured.UnstructuredList, error)
+
+func castToCacheListFunc(fn dynamicResourceClientListFunc) cache.ListFunc {
+	options := metav1.ListOptions{}


In practice, I've only set the LabelSelector of a ListOption before. It might be good hygiene to set timeout but that will likely cause more problems on slow clusters that it would prevent.

So, I think an empty LabelSelector is alright.

rlguarino · 2018-08-23T15:31:10Z

controller/composite/controller.go

@@ -471,7 +471,7 @@ func (pc *parentController) makeSelector(parent *unstructured.Unstructured, extr
 }

 func (pc *parentController) canAdoptFunc(parent *unstructured.Unstructured) func() error {
-	nsParentClient := pc.parentClient.WithNamespace(parent.GetNamespace())
+	nsParentClient := pc.parentClient


Do we actually need to reassign this variable here? Could we just directly call ps.parentClient.Get(...) below?

I agree too. In addition, make sure pc.parentClient is a namespace-agnostic client since a single parentController instance manages objects in multiple namespaces. In other words, the line below should ultimately read pc.parentClient.Namespace(parent.GetNamespace()).Get(parent.GetName(), ...).

Noted, thanks for pointing this out. :)

rlguarino · 2018-08-23T15:32:10Z

controller/composite/controller.go

@@ -536,7 +536,7 @@ func (pc *parentController) updateParentStatus(parent *unstructured.Unstructured
 	// We can't use Patch() because we need to ensure that the UID matches.
 	// TODO(enisoc): Use /status subresource when that exists.
 	// TODO(enisoc): Update status.observedGeneration when spec.generation starts working.
-	nsParentClient := pc.parentClient.WithNamespace(parent.GetNamespace())
+	nsParentClient := pc.parentClient


Simmarly to above about calling pc.parentClient.UpdateWIthRetries()... directly instead of defining a new variable. We could use:

return pc.parentClient.UpdateWithRetries(...

+1, you are right. :)

Also like above, this may need to change to account for pc.parentClient becoming namespace-agnostic. To make something like pc.parentClient.Namespace(...).UpdateWithRetries(...) work, we'll need to restructure how we add extra methods to ResourceInterface in our dynamic/clientset wrapper.

Thanks for the detailed explanation. Did you mean ResourceClient? NamespaceableResourceInterface is a vendor API.

The original purpose of ResourceClient was to extend ResourceInterface with extra methods that are useful for Metacontroller, which works with many different resources from many different angles.

Now that the new dynamic client has been split into ResourceInterface and NamespaceableResourceInterface, we need to restructure ResourceClient to correspond to NamespaceableResourceInterface, and add another level of wrapper to extend ResourceInterface as well.

enisoc · 2018-08-23T19:37:42Z

Gopkg.toml

@@ -30,19 +30,19 @@ required = [

 [[constraint]]
  name = "k8s.io/client-go"
-  version = "~6.0"
+  revision = "kubernetes-1.11.0"


I think we should stick to version and use 8.0.0 (the current recommended tag from the client-go README). The client-go versions are meant to be somewhat independent of Kubernetes versions.

Good point. I thought the requirement is to use the client-go version of 1.11 kubernetes version (misunderstood issue description).

enisoc · 2018-08-23T20:00:37Z

Gopkg.toml


 [[override]]
  name = "github.com/json-iterator/go"
-  version = "1.1.4" # as used by apimachinery@kubernetes-1.9.9
+  version = "1.1.5" # as used by apimachinery@kubernetes-1.11.0


How did you determine that apimachinery uses 1.1.5 at the kubernetes-1.11.0 tag? From the Godeps file (below) it looks like it still uses a commit that's contained in 1.1.4:

https://github.com/kubernetes/apimachinery/blob/103fd098999dc9c0c88536f5c9ad2e5da39373ae/Godeps/Godeps.json#L94-L95

That said, I'm fine updating to 1.1.5 if it works. We should just reword the comment to say # same minor version track as used by apimachinery@kubernetes-1.11.0

@enisoc How do you get the info that the commit is 1.1.4? The relevant PR can be known via this page, but tag 1.1.5 (not 1.1.4) at the top of the page is in bold.

I'm not sure what the bolding means, but I believe that list is all the tags that contain this commit in their histories. If we wanted to find the closest release tag at or after that commit, we should take the earliest tag that contains it. That's how I got 1.1.4 before.

However, as mentioned above, I think we should update to 1.1.5 as long as it's compatible, since it contains additional bug fixes.

I see - we should take the earliest (not latest) tag. Thank you!
I verify that 1.1.5 works.

enisoc · 2018-08-23T20:08:05Z

Gopkg.toml


 [[override]]
  name = "github.com/json-iterator/go"
-  version = "1.1.4" # as used by apimachinery@kubernetes-1.9.9
+  version = "1.1.5" # as used by apimachinery@kubernetes-1.11.0

 [prune]


@crimsonfaith91 I agree with the solution @rlguarino suggested. Can you try adding that?

@rlguarino Thanks for tracking that down!

enisoc · 2018-08-23T20:12:57Z

controller/composite/controller.go

@@ -471,7 +471,7 @@ func (pc *parentController) makeSelector(parent *unstructured.Unstructured, extr
 }

 func (pc *parentController) canAdoptFunc(parent *unstructured.Unstructured) func() error {
-	nsParentClient := pc.parentClient.WithNamespace(parent.GetNamespace())
+	nsParentClient := pc.parentClient


I agree too. In addition, make sure pc.parentClient is a namespace-agnostic client since a single parentController instance manages objects in multiple namespaces. In other words, the line below should ultimately read pc.parentClient.Namespace(parent.GetNamespace()).Get(parent.GetName(), ...).

enisoc · 2018-08-23T20:17:11Z

controller/composite/controller.go

@@ -536,7 +536,7 @@ func (pc *parentController) updateParentStatus(parent *unstructured.Unstructured
 	// We can't use Patch() because we need to ensure that the UID matches.
 	// TODO(enisoc): Use /status subresource when that exists.
 	// TODO(enisoc): Update status.observedGeneration when spec.generation starts working.
-	nsParentClient := pc.parentClient.WithNamespace(parent.GetNamespace())
+	nsParentClient := pc.parentClient


Also like above, this may need to change to account for pc.parentClient becoming namespace-agnostic. To make something like pc.parentClient.Namespace(...).UpdateWithRetries(...) work, we'll need to restructure how we add extra methods to ResourceInterface in our dynamic/clientset wrapper.

enisoc · 2018-08-23T20:35:05Z

dynamic/clientset/dynamic_clientset.go

+		Resource: resource.Name,
+	}
+}
+
 func (cs *Clientset) resource(apiResource *dynamicdiscovery.APIResource, namespace string) (*ResourceClient, error) {


One of the improvements in the new dynamic client is that it decouples specification of the namespace from the creation of the client. We should mirror that by removing the namespace argument here and in Clientset.Resource().

And also Clientset.Kind()?

Oh yeah, that one too. Good catch. :)

enisoc · 2018-08-23T20:38:27Z

dynamic/clientset/dynamic_clientset.go


-	dc       *dynamic.Client
+	dc       dynamic.Interface


We might not need to store dc here anymore, since we no longer need to make a new client just to change the namespace.

Removal of dc means we have to find another way of initializing ResourceClient.NamespaceableResourceInterface. I will see what is the possible workaround. This will take me some time because I am basically have zero knowledge on API machinery.

enisoc · 2018-08-23T20:41:08Z

dynamic/clientset/dynamic_clientset.go

 	gv       schema.GroupVersion
 	resource *dynamicdiscovery.APIResource
 }

-func (rc *ResourceClient) WithNamespace(namespace string) *ResourceClient {


In order to add UpdateWithRetries() for namespaced clients, we probably need to override Namespace() here and make it return a new wrapper struct that anonymously embeds a dynamic.ResourceInterface as well as implementing UpdateWithRetries().

Hasn't UpdateWithRetries() already implemented here?

We need to implement UpdateWithRetries() twice, although they can both just call the same private function that does the real work. My goal was to match the pattern used in the new dynamic client for handling the fact that some API calls are namespaced and others are not.

Notice that NamespaceableResourceInterface contains both a Namespace() method and a full embedding of ResourceInterface:

type NamespaceableResourceInterface interface { Namespace(string) ResourceInterface ResourceInterface }

That means, for example, you can do both dc.Resource().List() and dc.Resource().Namespace().List(). Which one you use depends on whether you want to send a namespaced request or a cluster-scoped request. For example, you can list objects for one namespace, or for all namespaces.

To match this for the extra functions we add like UpdateWithRetries(), we should make sure both rc.UpdateWithRetries() and rc.Namespace().UpdateWithRetries() work, because we want to be able to update objects that are either cluster-scoped or namespaced.

Oh, i see! I have to support rc.UpdateWithRetries() too for cluster-scoped operations. I thought we need to support only namespaced operations.

enisoc · 2018-08-23T20:46:55Z

dynamic/informer/informer.go

+type dynamicResourceClientListFunc func(metav1.ListOptions) (*unstructured.UnstructuredList, error)
+
+func castToCacheListFunc(fn dynamicResourceClientListFunc) cache.ListFunc {
+	options := metav1.ListOptions{}


This is meant to just be an adapter function, so you should pass along the ListOptions from the caller. See my other comment below, which should make this clearer.

enisoc · 2018-08-23T20:56:17Z

dynamic/informer/informer.go

 func newSharedResourceInformer(client *dynamicclientset.ResourceClient, defaultResyncPeriod time.Duration, close func()) *sharedResourceInformer {
 	informer := cache.NewSharedIndexInformer(
 		&cache.ListWatch{
-			ListFunc:  client.List,
+			ListFunc:  castToCacheListFunc(client.List),


Since we only need to do this once, I don't think it warrants all the stuff broken out above. It should be possible to write this concisely as something like this:

ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) { return client.List(opts) }

Note that you don't need to cast the return value because a type that implements an interface is already assignable to a variable of that interface type.

This is a much better way to do it! Thanks for specifying that the casting is not necessary.

crimsonfaith91 · 2018-08-24T02:18:11Z

dynamic/clientset/dynamic_clientset.go

 }

-func New(config *rest.Config, resources *dynamicdiscovery.ResourceMap) *Clientset {
+func New(config *rest.Config, resources *dynamicdiscovery.ResourceMap) (*Clientset, error) {
+	// TODO: find a way to obtain APIResource for getting GroupVersion.


@enisoc I shifted dc field from ResourceClient to Clientset. Not sure whether this is what you suggested. Feel free to sync by person if I get you wrong.

When calling NewForConfig within New, I couldn't find any way to obtain apiVersion and resource info, so I couldn't call resources.Get(apiVersion, resource) to obtain the GroupVersion for populating config.

I most probably at the wrong track, but decided to show my current progress for facilitating discussion (the codes compiled, but the smoke tests did not pass).

My hope was that the new dynamic client is designed to let us use one dc for many different GroupVersions and Resources. If that's so, we should be able to make a new dc here without filling in any GroupVersion or Resource, since we will provide that later when calling dc.Resource().

This code looks like what I would expect to work if that were the case. However, if the tests don't pass it might be that we still need to make a new dc for different GroupVersions. Are you seeing errors in the Metacontroller logs?

Thanks for verifying that I am on the right track. I will post log errors later if I cannot resolve it. :)

Yes, the new dynamic client should allow us to use one dc across multiple GVRs.

crimsonfaith91 · 2018-08-24T02:22:41Z

dynamic/clientset/dynamic_clientset.go

 	return &ResourceClient{
-		NamespaceableResourceInterface: dc.Resource(resourceGVR),
-		dc:                             dc,
+		NamespaceableResourceInterface: cs.dc.Resource(gvr),


Embedding dynamic interface dc into the clientset cs itself for initializing NamespaceableResourceInterface.

I could not find another way to initialize NamespaceableResourceInterface other than this.

crimsonfaith91 · 2018-08-27T21:16:22Z

controller/common/manage_children.go

@@ -111,7 +111,7 @@ func deleteChildren(client *dynamicclientset.ResourceClient, parent *unstructure
 			// This observed object wasn't listed as desired.
 			glog.Infof("%v %v/%v: deleting %v %v", parent.GetKind(), parent.GetNamespace(), parent.GetName(), obj.GetKind(), obj.GetName())
 			uid := obj.GetUID()
-			err := client.Delete(name, &metav1.DeleteOptions{
+			err := client.Namespace(parent.GetNamespace()).Delete(name, &metav1.DeleteOptions{


We can either set Namespace here, or earlier in the sync call tree via dynClient.Kind(apiVersion, kind).Namespace(parent.GetNamespace()). I prefer existing implementation because we may need to work with cluster-scoped client too.

crimsonfaith91 · 2018-08-27T21:17:39Z

dynamic/clientset/dynamic_clientset.go

+	}
+}
+
+func (nrc *NamespacedResourceClient) UpdateWithRetries(orig *unstructured.Unstructured, update func(obj *unstructured.Unstructured) bool) (result *unstructured.Unstructured, err error) {


Should we implement other functions (in another file) for NamespacedResourceClient?

For now, let's implement only UpdateWithRetries on NamespacedResourceClient. The others like GroupResource shouldn't be necessary if we pass around ResourceClient and only call .Namespace() as needed.

Regarding UpdateWithRetries, it should be possible to extract this into a helper method that accepts a dynamic.ResourceInterface to avoid duplicating code. Both ResourceClient and NamespacedResourceClient satisfy that interface.

Sound good to me, thanks!

enisoc · 2018-08-31T22:55:33Z

dynamic/clientset/dynamic_clientset.go

+	}
+}
+
+func (nrc *NamespacedResourceClient) UpdateWithRetries(orig *unstructured.Unstructured, update func(obj *unstructured.Unstructured) bool) (result *unstructured.Unstructured, err error) {


For now, let's implement only UpdateWithRetries on NamespacedResourceClient. The others like GroupResource shouldn't be necessary if we pass around ResourceClient and only call .Namespace() as needed.

Regarding UpdateWithRetries, it should be possible to extract this into a helper method that accepts a dynamic.ResourceInterface to avoid duplicating code. Both ResourceClient and NamespacedResourceClient satisfy that interface.

enisoc · 2018-08-31T23:03:04Z

dynamic/clientset/dynamic_clientset.go

-		resource:          apiResource,
-	}, nil
+		NamespaceableResourceInterface: cs.dc.Resource(apiResource.GroupVersionResource()),
+		gv:                             apiResource.GroupVersion(),


I don't remember why I originally stored gv in addition to resource, since you can get one from the other. Can you try removing gv from the struct and see if that simplifies things without adding some other cost?

Noted, will try removing gv.

crimsonfaith91 · 2018-09-05T01:31:22Z

dynamic/clientset/dynamic_clientset.go

@@ -105,74 +99,57 @@ func (rc *ResourceClient) Kind() string {
 }

 func (rc *ResourceClient) GroupVersion() schema.GroupVersion {
-	return rc.gv
+	return rc.resource.GroupVersion()
 }

 func (rc *ResourceClient) GroupResource() schema.GroupResource {


GroupResource() is still necessary for ResourceClient because it is called here.

enisoc

I noticed one more problem (see below). Did the smoke tests already pass? I guess this shows we need more tests.

Everything else looks good to me after that's fixed.

enisoc · 2018-09-05T22:17:22Z

controller/composite/controller.go

@@ -499,7 +498,7 @@ func (pc *parentController) claimChildren(parent *unstructured.Unstructured) (co
 	childMap := make(common.ChildMap)
 	for _, child := range pc.cc.Spec.ChildResources {
 		// List all objects of the child kind in the parent object's namespace.
-		childClient, err := pc.dynClient.Resource(child.APIVersion, child.Resource, namespace)
+		childClient, err := pc.dynClient.Resource(child.APIVersion, child.Resource)


Sorry for not noticing this earlier: I think since we no longer set the namespace here, we need to fix UnstructuredManager in the dynamic/controllerref package to call Namespace() when needed (if the child object has a namespace set).

@enisoc Yes, they passed. I ran all the tests for every commit now with image containing my changes via skaffold run after changing skaffold.yaml and image.yaml to my docker ID.

Noted, I will change dynamic/controllerref package accordingly.

@enisoc Do we also need to fix ControllerRevisionManager? ControllerRevision are namespaced objects.

That's a good point, I hadn't thought of that. I just checked though and it looks like ControllerRevisionManager is unaffected by your changes because we use a generated client for that rather than the dynamic client.

I see. I will push a commit soon. Thanks!

crimsonfaith91 · 2018-09-07T17:56:36Z

@enisoc Should we combine these two if-blocks into one if-else-block?

enisoc · 2018-09-07T18:08:08Z

Style-wise, I prefer them as separate blocks. The first block follows the pattern of short-circuiting control flow (return/break/continue) if an error occurs. The second block is not part of error handling, but is actually part of the normal flow -- it's just that the normal flow happens to consist of an if block.

enisoc · 2018-09-07T18:22:09Z

dynamic/controllerref/unstructured.go

-		obj.SetOwnerReferences(ownerRefs)
-		return true
-	})
+	var err error


This logic (check if it has a namespace and call UpdateWithRetries one way or the other) can be wrapped in a helper. If you do that, you can keep the code here looking simple (the only change will be calling your helper instead of directly calling UpdateWithRetries).

Noted, will move the logic to a helper function.

enisoc

LGTM

crimsonfaith91 changed the title ~~Client~~ Use new dynamic client Aug 23, 2018

crimsonfaith91 changed the title ~~Use new dynamic client~~ Switch to new dynamic client Aug 23, 2018

crimsonfaith91 requested a review from enisoc August 23, 2018 01:01

crimsonfaith91 commented Aug 23, 2018

View reviewed changes

rlguarino reviewed Aug 23, 2018

View reviewed changes

enisoc mentioned this pull request Aug 23, 2018

Unable to run make deepcopy #79

Closed

enisoc reviewed Aug 23, 2018

View reviewed changes

crimsonfaith91 changed the title ~~Switch to new dynamic client~~ use new dynamic client Aug 23, 2018

crimsonfaith91 commented Aug 24, 2018

View reviewed changes

crimsonfaith91 force-pushed the client branch from 9ad249c to 85527fe Compare August 24, 2018 23:27

crimsonfaith91 added 2 commits August 27, 2018 13:59

Autogen

f419ae8

update to new dynamic client

81dccf3

crimsonfaith91 force-pushed the client branch from 85527fe to 963f9e0 Compare August 27, 2018 21:13

crimsonfaith91 commented Aug 27, 2018

View reviewed changes

crimsonfaith91 force-pushed the client branch from 963f9e0 to f419ae8 Compare August 27, 2018 21:18

enisoc reviewed Aug 31, 2018

View reviewed changes

address feedback

a2ea56e

crimsonfaith91 commented Sep 5, 2018

View reviewed changes

enisoc reviewed Sep 5, 2018

View reviewed changes

change UnstructuredManager

3a62059

enisoc reviewed Sep 7, 2018

View reviewed changes

refactor to helper function

d13c8a8

enisoc approved these changes Sep 7, 2018

View reviewed changes

enisoc merged commit 8b53f27 into GoogleCloudPlatform:master Sep 7, 2018

enisoc mentioned this pull request Sep 13, 2018

Support for cluster scoped parent resources #71

Merged

crimsonfaith91 deleted the client branch September 18, 2018 00:52

use new dynamic client #80

use new dynamic client #80

Conversation

crimsonfaith91 commented Aug 23, 2018 • edited by enisoc 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

crimsonfaith91 Aug 23, 2018 • 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

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

crimsonfaith91 Aug 23, 2018 • 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

crimsonfaith91 Aug 23, 2018 • 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

crimsonfaith91 Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crimsonfaith91 Aug 24, 2018 • 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

crimsonfaith91 Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crimsonfaith91 Aug 24, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enisoc 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

crimsonfaith91 commented Sep 7, 2018

enisoc commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enisoc left a comment

Choose a reason for hiding this comment

crimsonfaith91 commented Aug 23, 2018 •

edited by enisoc

Loading

crimsonfaith91 Aug 23, 2018 •

edited

Loading

crimsonfaith91 Aug 23, 2018 •

edited

Loading

crimsonfaith91 Aug 23, 2018 •

edited

Loading

crimsonfaith91 Aug 24, 2018 •

edited

Loading

crimsonfaith91 Aug 24, 2018 •

edited

Loading

crimsonfaith91 Aug 24, 2018 •

edited

Loading

crimsonfaith91 Aug 24, 2018 •

edited

Loading