This hoists the K8s Service reconciliation and makes it level-based. by mattmoor · Pull Request #1328 · knative/serving

mattmoor · 2018-06-22T22:02:34Z

This is the next phase of our level-based migration of the Revision controller. It includes the K8s Service for the user Deployment. This is patterned in much the same way as the Deployment refactor in #1322. The most notable difference in this change is that we establish readiness from the Endpoints resource instead of the Service resource directly.

In addition, I spent a bit of time cleaning up some of the revision_test.go code to more automatically update the informers as resources are reconciled. In particular, after controller.Reconcile we need to make sure all of the updates to the underlying objects are reflected in our informers, or a subsequent reconcile may act on stale state. To facilitate this, I added a helper that updates the informers with the Revision/Deployment/Service to be used after a controller.Reconcile.

mattmoor · 2018-06-23T04:39:51Z

$ go test -tags=e2e -count=10 ./test/conformance/...
ok  	github.com/knative/serving/test/conformance	299.609s
?   	github.com/knative/serving/test/conformance/test_images/pizzaplanetv1	[no test files]
?   	github.com/knative/serving/test/conformance/test_images/pizzaplanetv2	[no test files]

🎉

This is the next phase of our level-based migration of the Revision controller. It includes the K8s Service for the user Deployment. This is patterned in much the same way as the Deployment refactor in #1322. The most notable difference in this change is that we establish readiness from the Endpoints resource instead of the Service resource directly. In addition, I spent a bit of time cleaning up some of the `revision_test.go` code to more automatically update the informers as resources are reconciled. In particular, after `controller.Reconcile` we need to make sure all of the updates to the underlying objects are reflected in our informers, or a subsequent reconcile may act on stale state. To facilitate this, I added a helper that updates the informers with the Revision/Deployment/Service to be used after a `controller.Reconcile`.

knative-metrics-robot · 2018-06-25T22:03:15Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/revision/revision.go	78.1%	78.8%	0.8

*TestCoverage feature is being tested, do not rely on any info here yet

vaikas · 2018-06-25T22:07:10Z

 		configuration.NewController(opt, configurationInformer, revisionInformer, cfg),
 		revision.NewController(opt, vpaClient, revisionInformer, buildInformer, configMapInformer,
-			deploymentInformer, endpointsInformer, vpaInformer, cfg, &revControllerConfig),
+			deploymentInformer, coreServiceInformer, endpointsInformer, vpaInformer,


The more Informers I see being added, the more I like passing in the map of them so each and every NewController method doesn't need to change when one of them needs something ;)

benefit of this style

type safety

plus it makes dependencies explicit

vaikas · 2018-06-25T22:11:20Z

+	return c.KubeClientSet.CoreV1().Services(ns).Create(service)
+}
+
+func (c *Controller) checkAndUpdateService(ctx context.Context, rev *v1alpha1.Revision, service *corev1.Service) (*corev1.Service, bool, error) {


nit, but it might be good to document what the bool here is. I think it means if it an update was attempted or not?

Use an enum - self documenting

vaikas · 2018-06-25T22:19:01Z

/lgtm
/approve

google-prow-robot · 2018-06-25T22:19:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/controller/OWNERS~~ [mattmoor,vaikas-google]
~~pkg/controller/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

dprotaso · 2018-06-26T14:58:04Z

 		configuration.NewController(opt, configurationInformer, revisionInformer, cfg),
 		revision.NewController(opt, vpaClient, revisionInformer, buildInformer, configMapInformer,
-			deploymentInformer, endpointsInformer, vpaInformer, cfg, &revControllerConfig),
+			deploymentInformer, coreServiceInformer, endpointsInformer, vpaInformer,


benefit of this style

type safety

plus it makes dependencies explicit

dprotaso · 2018-06-26T14:59:33Z

@@ -218,12 +224,8 @@ func NewController(
 	})

 	endpointsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{


I'm guessing the owner of the endpoints is the service - hence no filtering on the revision as an owner?

Does the Filter support label keys?

The filter is just a function, so we could potentially generalize this to something like our Controller kind filter that takes a label key. WDYT?

dprotaso · 2018-06-26T15:03:36Z

+			// should not allow, or if our expectations of how the service should look
+			// changes (e.g. we update our controller with new sidecars).
+			var updated bool
+			service, updated, err = c.checkAndUpdateService(ctx, rev, service)


I'm guessing there are is no defaulting involved with the service hence why it's safe to perform checkAndUpdateService?

I haven't seen the dueling controllers that I saw with Deployment, but if this becomes a problem we can relax it as well (until we find a better way of achieving this that works for both).

dprotaso · 2018-06-26T15:04:34Z

+	return c.KubeClientSet.CoreV1().Services(ns).Create(service)
+}
+
+func (c *Controller) checkAndUpdateService(ctx context.Context, rev *v1alpha1.Revision, service *corev1.Service) (*corev1.Service, bool, error) {


Use an enum - self documenting

mattmoor force-pushed the service-time branch 2 times, most recently from 2403d28 to 5fda32c Compare June 23, 2018 04:29

mattmoor force-pushed the service-time branch from 5fda32c to ef889b5 Compare June 23, 2018 14:30

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 Jun 23, 2018

mattmoor mentioned this pull request Jun 24, 2018

Complete the migration of the Revision controller to level-based. #1334

Merged

mattmoor force-pushed the service-time branch from ef889b5 to 17825bf Compare June 25, 2018 21:59

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

mattmoor changed the title ~~[WIP] This hoists the K8s Service reconciliation and makes it level-based.~~ This hoists the K8s Service reconciliation and makes it level-based. Jun 25, 2018

google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2018

mattmoor assigned vaikas and dprotaso Jun 25, 2018

vaikas reviewed Jun 25, 2018

View reviewed changes

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

google-prow-robot merged commit 60cfc29 into knative:master Jun 25, 2018

dprotaso reviewed Jun 26, 2018

View reviewed changes

		@@ -218,12 +224,8 @@ func NewController(
		})

		endpointsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{

Conversation

mattmoor commented Jun 22, 2018

Uh oh!

mattmoor commented Jun 23, 2018

Uh oh!

knative-metrics-robot commented Jun 25, 2018

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!

vaikas commented Jun 25, 2018

Uh oh!

google-prow-robot commented Jun 25, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants