Steve controller #597

vaikas · 2018-04-05T22:36:14Z

Addresses Issue #412

Proposed Changes

Implement basic controller that reconciles service object into underlying route/configuration objects
Add sample/service to demonstrate both auto rollout as well as pinned

vaikas · 2018-04-05T22:36:52Z

/assign @evankanderson

vaikas · 2018-04-05T22:49:29Z

/retest

Apply Service label to route / configuration. add tests

stragglers

grantr

This was easier to review than I expected; thanks @vaikas-google for making it easy to understand. I have a few minor comments.

grantr · 2018-04-06T20:39:09Z

pkg/controller/service/service.go

+
+var (
+	controllerKind        = v1alpha1.SchemeGroupVersion.WithKind("Service")
+	routeProcessItemCount = prometheus.NewCounterVec(prometheus.CounterOpts{


Probably should be called serviceProcessItemCount

grantr · 2018-04-06T20:39:52Z

pkg/controller/service/service.go

+
+	glog.Infof("Service controller Init")
+
+	// obtain references to a shared index informer for the Services and


This comment appears to be incomplete (Services and...)

grantr · 2018-04-06T20:52:48Z

pkg/controller/service/service.go

+			return err
+		}
+	}
+	config.SetGeneration(existing.GetGeneration())


I'm not sure I understand why this line is necessary. Can you add a comment explaining it? In particular, is it still needed when the apiserver is setting Generation for CRDs?

grantr · 2018-04-06T20:53:02Z

pkg/controller/service/service.go

+			return err
+		}
+	}
+	route.SetGeneration(existing.GetGeneration())


Same comment as above could be here too.

It's not. For updates, I first had set the ResourceVersion (and just set the Generation as well). So, once I just switched to overwriting only the spec, I couldn't recall if the Generation was handled correctly forgot to go back and check.

loooong way to say, it's not necessary, yanked.

grantr · 2018-04-06T21:02:58Z

pkg/controller/service/service.go

+
+	revisionName := ""
+	if service.Spec.Pinned != nil {
+		revisionName = service.Spec.Pinned.RevisionName


Is service.Spec.Pinned.RevisionName allowed to be empty here (by the webhook)?

Nope. Pinned.RevisionName must not be empty.
https://github.com/elafros/elafros/blob/master/pkg/webhook/service.go#L60

grantr · 2018-04-06T21:10:56Z

pkg/controller/service/service_route.go

+	tt := v1alpha1.TrafficTarget{
+		Percent: 100,
+	}
+	if len(revisionName) != 0 {


It's a bit odd to me that MakeServiceConfiguration uses service.Spec.RunLatest to determine which configuration to use, while MakeServiceRoute uses the length of revisionName, relying on the implicit contract that an empty revision name means the service is in RunLatest mode. Is there a reason for that difference?

If MakeServiceRoute used the same method as MakeServiceConfiguration, maybe there wouldn't be a need for the revisionName parameter or the conditional at https://github.com/elafros/elafros/pull/597/files#diff-2f50e493d103b0bd9f2ac68800d40201R278.

WDYT @vaikas-google?

Great suggestion! Done.

Exposed a bug in the tests too btw :)

grantr · 2018-04-06T21:12:06Z

pkg/controller/service/service_test.go

+)
+
+func testNothing(t *testing.T) {
+	return


Intended to be filled in later? 😉

yep, added TODO as felt this was getting a bit large already.

grantr · 2018-04-06T21:13:18Z

pkg/queue/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")


I'd recommend removing changes to this file since it's not related to the PR and might cause a merge conflict if someone else already fixed it in master.

hm, odd. I assume this is from the update-codegen? At least I do not recall doing this.

It's from hack/update-deps.sh

Yeah, so when I take it out, I get this:
vaikas@vaikas-linux3:~/projects/go/src/github.com/elafros/elafros$ bazel test pkg/...
ERROR: /home/vaikas/projects/go/src/github.com/elafros/elafros/pkg/queue/BUILD.bazel:11:1: name 'go_test' is not defined (did you mean 'sh_test'?)
ERROR: package contains errors: pkg/queue
ERROR: error loading package 'pkg/queue': Package 'pkg/queue' contains errors
INFO: Elapsed time: 0.241s
FAILED: Build did NOT complete successfully (1 packages loaded)
ERROR: Couldn't start the build. Unable to run tests

Ah sorry, my comment was unclear. I meant remove this change from the PR, leaving it unstaged in your local checkout. Just don't stage this particular file when committing.

It's fine to ignore this advice, since merge conflicts are unlikely anyway (the file will have the same generated content in all recent branches). At this point it's probably more work to try to remove this change than keep it.

Turns out these were also fixed by @bobcatfish in #599

grantr · 2018-04-06T21:14:50Z

sample/service/README.md

+
+```
+NAME                                 HOSTS                     ADDRESS   PORTS     AGE
+route-example-ela-ingress   demo.myhost.net             80        14s


I think route-example should be replaced with service-example here and elsewhere in this file.

grantr · 2018-04-06T21:21:00Z

pkg/controller/service/service.go

+
+	copy := existing.DeepCopy()
+	copy.Spec = config.Spec
+	_, err = configClient.Update(copy)


Does this update the configuration if it hasn't changed? I think maybe the webhook makes the update a noop if there are no patches. IIUC that behavior is required here, because we need to be able to update a service without creating a new revision.

Yes, it doesn't diff yet, so will just update. webhook should cause the right thing to happen.
I added a few TODOs.

vaikas · 2018-04-06T22:14:58Z

Thanks @grantr I believe I addressed all the comments (and it exposed a test bug 👍 ), PTAL.

grantr

/lgtm

evankanderson

We really need a CLI so that the READMEs don't have so much crummy boilerplate (and automatic DNS setup for the same).

A bunch of medium-size comments, but no major red flags. Thanks for including the sample.

evankanderson · 2018-04-06T22:12:11Z

pkg/controller/service/service.go

+
+// NewController initializes the controller and is called by the generated code
+// Registers eventhandlers to enqueue events
+//TODO(vaikas): somewhat generic (generic behavior)


I don't understand your TODO here -- what needs to be done (in case someone else wanted to pick it up and do it)

These TODOs are copied from the other controllers. They identify functions that have no variance across the controller types and could be moved to the shared controller package. That was the original intent anyway - they might not be accurate anymore. In particular, some controllers now create multiple informers, so the NewController methods are no longer generic.

I'm fine with removing these. Anyone investigating how to generalize controller code will certainly do their own analysis.

leftover cruft, removed.

evankanderson · 2018-04-06T22:12:42Z

pkg/controller/service/service.go

+
+	glog.Infof("Service controller Init")
+
+	// obtain references to a shared index informer for the Services and


You lost the end of your sentence here.

evankanderson · 2018-04-06T22:16:39Z

pkg/controller/service/service.go

+// as syncing informer caches and starting workers. It will block until stopCh
+// is closed, at which point it will shutdown the workqueue and wait for
+// workers to finish processing their current work items.
+//TODO(grantr): generic


Again, TODO not clear.

leftover cruft from other skeleton controllers, removed.

evankanderson · 2018-04-06T22:17:48Z

pkg/controller/service/service.go

+	}
+
+	glog.Info("Starting workers")
+	// Launch two workers to process Service resources


This comment seems unnecessary and also wrong.

It's true in practice because threadiness is always two due to https://github.com/elafros/elafros/blob/master/cmd/ela-controller/main.go#L44. The controller isn't supposed to know that though, and this comment probably shouldn't either.

leftover cruft from other skeleton controllers, removed.

evankanderson · 2018-04-06T22:19:15Z

pkg/controller/service/service.go

+
+// processNextWorkItem will read a single work item off the workqueue and
+// attempt to process it, by calling the updateServiceEvent.
+//TODO(vaikas): generic


again, leftover cruft.

evankanderson · 2018-04-06T23:29:57Z

pkg/controller/service/service.go

+		if apierrs.IsNotFound(err) {
+			_, err := configClient.Create(config)
+			return err
+		}


Do you want to bail/return on other errors?

evankanderson · 2018-04-06T23:30:42Z

pkg/controller/service/service.go

+		if apierrs.IsNotFound(err) {
+			_, err := routeClient.Create(route)
+			return err
+		}


Ditto on returning err here if Get fails on the existing with something other than not found.

evankanderson · 2018-04-06T23:31:10Z

pkg/controller/service/service.go

+			return err
+		}
+	}
+	route.SetGeneration(existing.GetGeneration())


Ditto here as well on modifying the input route with some fields but not others.

evankanderson · 2018-04-06T23:35:01Z

pkg/controller/service/service_configuration_test.go

+	}
+}
+
+func createServiceWithRunLatest() *v1alpha1.Service {


It's slightly odd that this function is reused in service_route_test.go, though I don't have an immediate better suggestion.

Yeah, I could've created createServiceConfigurationTestWithRunLatest :) or something like that, you're ok though with this?

I am. I was wondering if a service_object_creators or the like for common test infra would make sense here, but I'm fine with this, too.

evankanderson · 2018-04-06T23:35:22Z

pkg/controller/service/service_test.go

+	"testing"
+)
+
+func testNothing(t *testing.T) {


Do you want a TODO here?

already did :) You must have looked at the older version?

vaikas · 2018-04-06T23:51:16Z

Thanks @evankanderson for the review, I removed bunch of carry-over cruft todos and fixed the error cases. PTAL.

evankanderson · 2018-04-07T00:00:47Z

It looks like I went through while you were addressing Grant's comments. I have a few more comments, but this generally LGTM:

#597 (comment)
#597 (comment)

vaikas · 2018-04-07T00:06:52Z

allrighty, made the changes and added a todo for type checking. PTAL.

mattmoor

This seems like a reasonable skeleton to me, but I think I'd like to see us deeply specify the semantics we have in mind before we get too deep into implementing the controller logic and its tests.

mattmoor · 2018-04-08T19:30:51Z

pkg/controller/service/service.go

+	glog.Infof("Running reconcile Service for %s\n%+v\n", service.Name, service)
+
+	config := MakeServiceConfiguration(service)
+	if err = c.reconcileConfiguration(config); err != nil {


it's already defined above in 249?

mattmoor · 2018-04-09T15:24:27Z

/lgtm

google-prow-robot · 2018-04-09T15:24:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, mattmoor, vaikas-google

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~~ [mattmoor,vaikas-google]

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

Ville Aikas added 4 commits April 5, 2018 14:37

Stub for service controller

2251653

checkpointing

8bd4ca0

checkpoint, reconcile route/configuration based on service

d0f946e

reconcile route/service, add samples

4ea2ab8

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 5, 2018

vaikas requested a review from grantr April 5, 2018 22:36

google-prow-robot assigned evankanderson Apr 5, 2018

vaikas mentioned this pull request Apr 5, 2018

Add Service CRD to conformance tests #591

Closed

Ville Aikas and others added 2 commits April 6, 2018 12:14

Apply Service label to route / configuration. add tests

07bf015

Merge pull request #1 from vaikas-google/steve-controller-tests

55d678e

Apply Service label to route / configuration. add tests

google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 6, 2018

Ville Aikas and others added 2 commits April 6, 2018 12:26

stragglers

2b3ea90

Merge pull request #2 from vaikas-google/steve-controller-tests

2543b52

stragglers

vaikas requested review from evankanderson and mattmoor as code owners April 6, 2018 19:29

grantr reviewed Apr 6, 2018

View reviewed changes

Ville Aikas added 3 commits April 6, 2018 15:00

address code review feedback

e0cf0a4

remove changes to pkg/queue/BUILD.bazel

e799cca

fix tests

b711ed1

google-prow-robot assigned grantr Apr 6, 2018

grantr approved these changes Apr 6, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018

evankanderson reviewed Apr 6, 2018

View reviewed changes

bobcatfish mentioned this pull request Apr 6, 2018

tide should merge elafros PRs automatically #543

Closed

address review comments

1c0d9b5

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018

more code review comments

0ee3c94

mattmoor approved these changes Apr 8, 2018

View reviewed changes

Address code review comment

3f14783

google-prow-robot assigned mattmoor Apr 9, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2018

google-prow-robot merged commit 9891c18 into knative:master Apr 9, 2018

evankanderson mentioned this pull request Apr 18, 2018

Service docs #685

Merged

6 tasks


		glog.Infof("Service controller Init")

		// obtain references to a shared index informer for the Services and

		@@ -1,4 +1,4 @@
		load("@io_bazel_rules_go//go:def.bzl", "go_library")
		load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

Steve controller #597

Steve controller #597

Conversation

vaikas commented Apr 5, 2018

Proposed Changes

vaikas commented Apr 5, 2018

vaikas commented Apr 5, 2018

grantr 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

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

vaikas commented Apr 6, 2018

grantr left a comment

Choose a reason for hiding this comment

evankanderson 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

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

vaikas commented Apr 6, 2018

evankanderson commented Apr 7, 2018

vaikas commented Apr 7, 2018

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Apr 9, 2018

google-prow-robot commented Apr 9, 2018