Controller v2 libs review #48

pwittrock · 2018-06-18T14:43:02Z

No description provided.

droot

I am reviewing each commit separately. So these comments are reviewing reconcile pkg.

droot · 2018-06-22T17:18:09Z

pkg/reconcile/reconcile.go

+For example if responding to a Pod Delete Event, the Request won't contain that a Pod was deleted,
+instead the reconcile function observes this when reading the cluster state and seeing the Pod as missing.
+*/
+type Reconcile interface {


Should this be called Reconciler because anyone who implements the verb Reconcile is a Reconciler ?

Like anyone implementing Read(...) is a io.Reader ?

agreed here -- the name is a bit confusing.

droot · 2018-06-22T17:27:11Z

pkg/reconcile/reconcile_test.go

+
+				return result, nil
+			})
+			actualResult, actualErr := instance(request)


may be invoke the reconcile using instance.Reconcile for clarity ?

droot · 2018-06-22T17:35:08Z

pkg/reconcile/example_test.go

+
+// This example declares a simple type that implements reconcile.
+func ExampleReconcile() {
+	type MyReconcileImplementation struct {


IMO, default example for demonstrating Reconcile should be struct based not adapter based because 99% use-cases involve a struct where an instance of client needs to be embedded etc. and speaking from my experience, I have found Adapter pattern a bit difficult to grok for newbies (experience from net/http)

droot · 2018-06-22T17:37:24Z

pkg/reconcile/reconcile.go

+// Result contains the result of a Reconcile invocation.
+type Result struct {
+	// Requeue tells the Controller to requeue the reconcile key.  Defaults to false.
+	Requeue bool


One question (not scope of reconcile pkg though). What is the semantics for handling the error returned by the Reconcile(). Is it going to be simply logging the error and ignoring it ? or using it for re-queuing ? If it is the former, then current controller writer might get surprised a bit (not saying it is a bad thing though).

the latter. will document

droot · 2018-06-22T17:44:58Z

pkg/event/event.go

+	"k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+


May be a few comments explaining where this pkg fit in the over-all picture. Who will be the producer/consumer of these objects etc ?

droot

comments from review of handler pkg.

droot · 2018-06-22T18:12:21Z

pkg/handler/enqueue_owner.go

+// InjectScheme is called by the Controller to provide a singleton scheme to the EnqueueOwner.
+func (e *EnqueueOwner) InjectScheme(s *runtime.Scheme) error {
+	return e.parseOwnerTypeGroupKind(s)
+}


would suggest moving InjectScheme related code at the bottom from code readability perspective, sort of coming in the way :)

droot · 2018-06-22T18:18:33Z

pkg/handler/enqueue_owner.go

+		return nil
+	}
+
+	// If filtered to Controller use all the OwnerReferences


not filtered

droot · 2018-06-22T18:23:21Z

pkg/handler/enqueue_owner.go

+				Namespace: object.GetNamespace(),
+				Name:      ref.Name,
+			}})
+		}


Its good that we are not performing lookup against the cluster to match the UUIDs for the owner because that would have made handling the cases of owner itself changing difficult.

droot · 2018-06-22T18:25:40Z

pkg/handler/eventhandler.go

+	// Generic is called in response to an event of an unknown type or a synthetic event triggered as a cron or
+	// external trigger request - e.g. reconcile Autoscaling, or a Webhook.
+	Generic(workqueue.RateLimitingInterface, event.GenericEvent)
+}


Consider switching ordering of the parameters. evt is the primary input to to the handler and q is sort of the output after the handling ?

droot · 2018-06-22T18:28:50Z

pkg/handler/eventhandler.go

+
+var _ EventHandler = Funcs{}
+
+// Funcs allows specifying a subset of EventHandler functions are fields.


This comment isn't very clear.

droot

reviewed predicates

droot · 2018-06-22T18:41:20Z

pkg/predicate/predicate.go

+		return p.GenericFunc(e)
+	}
+	return true
+}


looks like we don't have any default predicates implemented like one diffing resourceVersion field ?

droot

reviewed source pkg, looks good.

droot · 2018-06-22T18:55:32Z

pkg/source/source.go

+	}
+	i.AddEventHandler(internal.EventHandler{Queue: queue, EventHandler: handler, Predicates: prct})
+	return nil
+}


consider moving Kind source above Channel source from readability perspective. Kind source is much more simpler than channel source, so if someone is looking for a canonical example to implement a source, introducing Kind first is a good idea.

droot · 2018-06-22T18:57:11Z

Did a first pass today and reviewed source, handler, predicates, event and reconcile pkgs today. Will review controllers and manager in the second pass today or Monday.

pwittrock · 2018-06-25T04:41:57Z

Addressed in https://github.com/kubernetes-sigs/controller-runtime/pull/58/files

droot

Reviewed the manager pkg. Looks good, minor nits.

droot · 2018-06-25T19:18:45Z

pkg/manager/internal.go

+		// TODO(community): Check the return value and write a test
+		cm.cache.WaitForCacheSync(stop)
+
+		// Start the runnables after the promises


comment is outdated (mentions promises)

droot · 2018-06-25T19:20:55Z

pkg/manager/internal.go

+		cm.mu.Lock()
+		defer cm.mu.Unlock()
+
+		// Start the Cache.


nit: may be move the comment new the startCache block below.

droot · 2018-06-25T19:24:37Z

pkg/manager/manager.go

+	// Defaults to the kubernetes/client-go scheme.Scheme
+	Scheme *runtime.Scheme
+
+	// Mapper is the rest mapper used to map go types to Kubernetes APIs


Mapper --> MapperProvider provides the rest mapper......

droot · 2018-06-25T19:26:37Z

pkg/manager/manager.go

+	// Use the Kubernetes client-go scheme if none is specified
+	if cm.scheme == nil {
+		cm.scheme = scheme.Scheme
+	}


This should be moved to setOptionsDefaults

Done. Restructured this fn.

droot · 2018-06-25T19:30:05Z

pkg/manager/manager.go

+	}
+
+	// Allow newCache to be mocked
+	// TODO(directxman12): Figure out how to allow users to request a client without requesting a watch


Don't know this TODO is still valid ? I remember some discussion where it came as already done ?

droot

reviewed controller pkg. lgtm

droot · 2018-06-25T20:44:14Z

@pwittrock completed the entire review today. lgtm. Feel free to close the PR.

pwittrock · 2018-06-26T13:45:26Z

Comments addressed in #61

DirectXMan12

mostly didn't review tests -- focused on the main code.
Apologies if I commented on anything that was already fixed ;-)

DirectXMan12 · 2018-06-26T16:38:13Z

pkg/controller/controller.go

+	//
+	// Watch may be provided one or more Predicates to filter events before they are given to the EventHandler.
+	// Events will be passed to the EventHandler iff all provided Predicates evaluate to true.
+	Watch(src source.Source, evthdler handler.EventHandler, prct ...predicate.Predicate) error


don't abbreviate so much, especially in the interface definition, where people will be looking for documentation. eventHandler and predicates are fine.

DirectXMan12 · 2018-06-26T16:39:55Z

pkg/controller/controller.go

+
+// New returns a new Controller registered with the Manager.  The Manager will ensure that shared Caches have
+// been synced before the Controller is Started.
+func New(name string, mrg manager.Manager, options Options) (Controller, error) {


mrg --> mgr

DirectXMan12 · 2018-06-26T16:45:52Z

pkg/doc.go

+3 Pods exist in the system.  Reconcile creates 2 more Pods and sets their OwnerReference to point at the
+ReplicationController.
+
+* reconcile works on a single object type. - e.g. it will only reconcile ReplicaSets.


specify if you wish to trigger reconciles from other objects, you can provide a mapping (e.g. owner references) that maps the object that triggers the reconcile to the object being reconciled, perhaps?

DirectXMan12 · 2018-06-26T16:49:27Z

pkg/doc.go

+
+* Predicates are optional
+
+* Users SHOULD use the provided Predicate implementations, but MAY implement their own Predicates as needed.


give an example here (e.g. generation changed, label selectors, etc)

DirectXMan12 · 2018-06-26T16:50:16Z

pkg/doc.go

+The following example shows creating a new Controller program which Reconciles ReplicaSet objects in response
+to Pod or ReplicaSet events.  The Reconcile function simply adds a label to the ReplicaSet.
+
+See the example/main.go for a usage example.


if you name things right in the _test files they'll show up here as examples in the Godoc.

DirectXMan12 · 2018-06-26T18:20:22Z

pkg/source/source.go

+
+	dst := make(chan event.GenericEvent, cs.DestBufferSize)
+	go func() {
+		for evt := range dst {


shouldn't you check for a stop signal so you can kill this goroutine?

(oh, this'll end when dst closes, but you should add a note about that)

DirectXMan12 · 2018-06-26T19:00:12Z

pkg/handler/enqueue.go

+
+// Create implements EventHandler
+func (e *Enqueue) Create(q workqueue.RateLimitingInterface, evt event.CreateEvent) {
+	if evt.Meta == nil {


:-/ I think we want to Inject a logger if we can, so that we can track which enqueue is having issues.

DirectXMan12 · 2018-06-26T19:05:19Z

pkg/handler/enqueue_owner.go

+	}
+	// Expect only 1 kind.  If there is more than one kind this is probably an edge case such as ListOptions.
+	if len(kinds) != 1 {
+		err := fmt.Errorf("Expected exactly 1 kind for OwnerType")


add more info to this error (like kind is probably not a normal API type)

DirectXMan12 · 2018-06-26T19:05:42Z

pkg/handler/enqueue_owner.go

+		refGV, err := schema.ParseGroupVersion(ref.APIVersion)
+		if err != nil {
+			log.Error(err, "Could not parse OwnerReference GroupVersion",
+				"OwnerReference", ref.APIVersion)


ownerRef should be the full ownerRef

DirectXMan12 · 2018-06-26T19:09:10Z

pkg/handler/eventhandler.go

+//
+// * Use EnqueueMappendHandler to transform an event for an object to a reconcile of an object
+// of a different type - do this for events for types the Controller may be interested in, but doesn't create.
+// (e.g. If Foo responds to cluster size events, map Node events to Foo objects.)


maybe mention using indexers if you need to easily look things up?

kubernetes-sigs#48

Disallow pluralized Kinds when creating resources

✨ Rebase to v0.17.2

pwittrock added 8 commits June 18, 2018 07:41

Do not review. Already reviewed

ad0c1eb

Controller Event package

b9b5807

Controller handler package

08d6ea0

Controller Reconcile pkg

044e754

Controller source package

66f3905

Controller predicate package

0358f59

Controller manager package

973ead5

Controller controller package

fbd8207

pwittrock assigned DirectXMan12, droot, apelisse, Liujingfang1 and lichuqiang Jun 18, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 18, 2018

droot reviewed Jun 22, 2018

View reviewed changes

droot reviewed Jun 25, 2018

View reviewed changes

pwittrock closed this Jun 26, 2018

DirectXMan12 reviewed Jun 26, 2018

View reviewed changes

droot added a commit to droot/controller-runtime that referenced this pull request Jun 26, 2018

address feedback on PR 48

5a47857

kubernetes-sigs#48

droot mentioned this pull request Jun 26, 2018

address feedback on PR 48 #63

Merged

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

address feedback on PR 48

f4a207f

kubernetes-sigs#48

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

Merge pull request #48 from pwittrock/pluralkind

f6d0548

Disallow pluralized Kinds when creating resources

sttts pushed a commit to sttts/controller-runtime that referenced this pull request Mar 9, 2024

Merge pull request kubernetes-sigs#48 from sttts/sttts-kcp-0.17-rebase

e8b4ff6

✨ Rebase to v0.17.2


		var _ EventHandler = Funcs{}

		// Funcs allows specifying a subset of EventHandler functions are fields.


		* Predicates are optional

		* Users SHOULD use the provided Predicate implementations, but MAY implement their own Predicates as needed.

Controller v2 libs review #48

Controller v2 libs review #48

Uh oh!

Conversation

pwittrock commented Jun 18, 2018

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot commented Jun 22, 2018

Uh oh!

pwittrock commented Jun 25, 2018

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

droot Jun 22, 2018 •

edited

Loading