Implement workqueue #63

asymmetric · 2017-08-18T14:38:20Z

This PR adds a ServiceGroup workqueue.

Following the guidelines, all event handlers (SG and Pod for now) do is retrieve the appropriate SG key and add a job to the queue.

The worker then consumes these jobs, and performs the necessary operations.

Closes #26, #51 and obsoletes #56.

asymmetric · 2017-08-18T14:40:35Z

@lilic are your E2E test going to cover the functionality of the peer IP handling, e.g. that we correctly replace the IP of a dead pod?

lilic · 2017-08-18T14:42:43Z

@asymmetric My idea was we just need to test that a CM was updated after our Pod died, not sure we need to care about if it was the correct IP. Think that might be more unit tests, wdyt?

lilic

Before we can merge this I would say we should take care of:

Watching Deployments to avoid unnecessary calls.
Save store/cache of all the resources we do a Get/List call to. Think that only needs to be SG, Pods and Deployments for now.

lilic · 2017-08-21T07:41:47Z

pkg/habitat/controller/controller.go

+
+	queue workqueue.RateLimitingInterface
+
+	store cache.Store


Maybe we can describe this better and call it something like SGStore?

For now it's the only store though :)

Maybe, but it still stores just SG cache so we can still rename it to describe what it stores. Its up to you I guess.

I think I'll leave it like this, if you don't mind. I'll add some field comments to explain what each field does instead.

lilic · 2017-08-21T07:43:16Z

pkg/habitat/controller/controller.go

-			UpdateFunc: hc.onUpdate,
-			DeleteFunc: hc.onDelete,
+			AddFunc: hc.enqueueSG,
+			UpdateFunc: func(old, new interface{}) {


I would suggest we move this to a separate func?

lilic · 2017-08-21T07:46:12Z

pkg/habitat/controller/controller.go

-			DeleteFunc: hc.onDelete,
+			AddFunc: hc.enqueueSG,
+			UpdateFunc: func(old, new interface{}) {
+				oldSG, ok1 := old.(*crv1.ServiceGroup)


If we separate to a func, we can then also return early here and log the error. So:

if !ok1 { ... }

We can do that even without extracting to a named function though. I quite like this small anonymous function (and I stole it from here ;))

lilic · 2017-08-21T07:48:51Z

pkg/habitat/controller/controller.go

+	return true
+}
+
+func (hc *HabitatController) syncServiceGroup(key string) error {


Maybe we can just call this sync? As you mention in the comment we react on pod events as well.

This follows the example of other controllers, and I guess the idea is that this function syncs the resource this controller cares about, including all depdencies.

lilic · 2017-08-21T07:50:00Z

pkg/habitat/controller/controller.go

+}
+
+func (hc *HabitatController) syncServiceGroup(key string) error {
+	// we got woken up, so either:


We can shorten this comment to something like: (Feel free to adjust the comment.)
// We react on every watched resource event.

That slipped in, wasnt' planning on having it committed :)

But I guess if it's reworded it can be useful, as you say.

lilic · 2017-08-21T07:50:44Z

pkg/habitat/controller/controller.go

+		return err
+	}
+	if !exists {
+		// The SG was deleted.


Wouldn't this be true if Pod was deleted as well?

Ignore the comment, the store naming confused me.

lilic · 2017-08-21T07:51:57Z

pkg/habitat/controller/controller.go

+		return hc.handleServiceGroupDeletion(key)
+	}
+
+	// it was either created or updated


Maybe something like:

// Resource was either created or updated.

lilic · 2017-08-21T07:58:32Z

pkg/habitat/controller/controller.go

-		level.Error(hc.logger).Log("msg", err)
-		return
+	// Create Deployment, if it doesn't already exist.
+	d, dErr := hc.config.KubernetesClientset.AppsV1beta1Client.Deployments(apiv1.NamespaceDefault).Create(deployment)


We should introduce watching for deployments as well. After that we can just do a Get here to our cache/store instead of a create to the API. This way we save calls to the API and use our cache better.

As right now from what I can see it will do a Create call is being made every time we update/create our Pod or SG.

lilic · 2017-08-21T08:00:34Z

pkg/habitat/controller/controller.go

+		FieldSelector: fs.String(),
+	}
+
+	// TODO We can use the store for this query.


I would suggest we do this now. From what I can see we already have access to pod store/cache, we just need to save it? Or is there anything else stopping us?

I was thinking this and other things (like increasing the number of workers or using watchers for other types of resources) can be done as additional PRs later on, as there is already enough going on in this PR.

I see those changes as basically performance optimizations, and I think we can split them up from this PR, which is about core functionality.

I am fine with that (although I would prefer we do it in this PR), but then we shouldn't close the issue it relates to, but rather maybe add a list of AC to the issue. Otherwise we can forget about it.

Something like:

Introduce WorkQueue

Add Deployment watching

Store Pod cache

etc.
This was we can just pick up those tasks and keep track of them.

If you mean #51, this PR does close that issue AFAICT. The features you mentioned are additional nice-to-haves IMO.

Or did you mean another issue?

Issue https://github.com/kinvolk/habitat-operator/issues/26.

I'll add separate issues for the items you mentioned above.

lilic · 2017-08-21T08:08:59Z

pkg/habitat/controller/controller.go

+		return
+	}
+
+	new, ok2 := newObj.(*apiv1.Pod)


new maybe not a reserved name, but I would still like to avoid using that. WDYT?

Didn't know about this.

lilic · 2017-08-21T09:56:18Z

I tested this branch and it seems like writing IP is not working the same, compared to master.

When I create two SG one after another, the ConfigMaps contain the same IP.

lilic · 2017-08-21T10:03:39Z

pkg/habitat/controller/controller.go

+				oldSG, ok1 := oldObj.(*crv1.ServiceGroup)
+				newSG, ok2 := newObj.(*crv1.ServiceGroup)
+
+				if !ok1 {


Maybe move it before newSG, ok2? That way we can return early and not create newSG.

asymmetric · 2017-08-21T10:55:39Z

Good catch! PTALA.

lilic

~~Tested again and it didn't seem to fix the problem I was having earlier. When creating two SG, example1 and example2, both CM contain IP from same SG (in my case example1).~~

Ignore me, forgot to pull in the latest changes. :)

lilic · 2017-08-21T14:32:31Z

pkg/habitat/controller/controller.go

+
+	newCM := newConfigMap(sg.Name, deploymentUID, leaderIP)
+
+	var cm *apiv1.ConfigMap


Why is this declaration needed?

lilic · 2017-08-21T14:34:51Z

pkg/habitat/controller/controller.go

+	}))
+
+	running := metav1.ListOptions{
+		FieldSelector: fs.String(),


I would just create fs and ls inline, as it more readable.

I find this style more readable, and unless there are guidelines going against it, I'd like to keep it.

It's also how we do it elsewhere.

lilic · 2017-08-21T14:41:40Z

pkg/habitat/controller/controller.go

-		return
+	// Create Deployment, if it doesn't already exist.
+	d, dErr := hc.config.KubernetesClientset.AppsV1beta1Client.Deployments(apiv1.NamespaceDefault).Create(deployment)
+	if dErr != nil {


Why not the standard naming of error?

lilic · 2017-08-21T14:42:52Z

pkg/habitat/controller/controller.go

+			return dErr
+		}
+
+		d, dErr = hc.config.KubernetesClientset.AppsV1beta1Client.Deployments(sg.Namespace).Get(deployment.Name, metav1.GetOptions{})


I am confused why we need to Get, as our Create already returns a deployment.

Because if we're here, the Create has failed, and the deployment already exists.

Hmm, I would suggest we restructure this, as right now from first glance its not really clear whats happening.

I would actually reverse those and start by first seeing if our deployment already exists, rather then trying to create it. If it doesn't then create it.

This way we save one call though. If the resource doesn't exist, we just create it, with no need to perform the check first.

I've seen the pattern in other controllers as well, so I'm more inclined on leaving this as is.

The performance argument will probably become irrelevant if/when we move to using a Deployments cache, so I think we can refactor this there.

But for now it seems like a good enough reason to me. I can add a comment to make it clearer though.

WDYT?

asymmetric · 2017-08-22T09:16:31Z

Are there any more blockers? If not, can this get a green-light?

lilic

LGTM. We just shouldn't forget about the followup issues to this PR. Other then that lets just clean up the commits and then 👍

asymmetric · 2017-08-22T09:51:57Z

I've created two issues: #65, #66.

asymmetric requested a review from lilic August 18, 2017 14:38

lilic mentioned this pull request Aug 18, 2017

Add log/level to Gopkg.lock #59

Merged

lilic suggested changes Aug 21, 2017

View reviewed changes

lilic reviewed Aug 21, 2017

View reviewed changes

asymmetric force-pushed the asymmetric/queue branch 2 times, most recently from e603102 to 6fc4f71 Compare August 21, 2017 10:08

This was referenced Aug 21, 2017

Use cache instead of making API calls #65

Closed

Add Deployment watcher #66

Closed

asymmetric force-pushed the asymmetric/queue branch from 8a437f0 to 84b08ac Compare August 21, 2017 11:01

lilic suggested changes Aug 21, 2017

View reviewed changes

lilic approved these changes Aug 22, 2017

View reviewed changes

Lorenzo Manacorda added 2 commits August 22, 2017 11:57

Introduce Workqueue

a327f14

dep ensure

9082f29

asymmetric force-pushed the asymmetric/queue branch from f9915f9 to 9082f29 Compare August 22, 2017 09:58

asymmetric merged commit 04cf885 into master Aug 22, 2017

asymmetric deleted the asymmetric/queue branch August 22, 2017 10:00

This was referenced Aug 22, 2017

ConfigMap not found error #51

Closed

Simplify Leader IP handling #48

Closed

Check dependencies are present in Pod handler #56

Closed


		newCM := newConfigMap(sg.Name, deploymentUID, leaderIP)

		var cm *apiv1.ConfigMap

Implement workqueue #63

Implement workqueue #63

Conversation

asymmetric commented Aug 18, 2017 • edited

asymmetric commented Aug 18, 2017

lilic commented Aug 18, 2017

lilic 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

asymmetric Aug 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asymmetric Aug 21, 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

asymmetric Aug 21, 2017 • edited

Choose a reason for hiding this comment

lilic Aug 21, 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

lilic commented Aug 21, 2017

Choose a reason for hiding this comment

asymmetric commented Aug 21, 2017

lilic left a comment • 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

asymmetric Aug 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asymmetric Aug 21, 2017 • edited

Choose a reason for hiding this comment

asymmetric Aug 21, 2017 • edited

Choose a reason for hiding this comment

asymmetric commented Aug 22, 2017

lilic left a comment

Choose a reason for hiding this comment

asymmetric commented Aug 22, 2017

asymmetric commented Aug 18, 2017 •

edited

asymmetric Aug 21, 2017 •

edited

asymmetric Aug 21, 2017 •

edited

asymmetric Aug 21, 2017 •

edited

lilic Aug 21, 2017 •

edited

lilic left a comment •

edited

asymmetric Aug 21, 2017 •

edited

asymmetric Aug 21, 2017 •

edited

asymmetric Aug 21, 2017 •

edited