Introduce a new interface to encapsulate Ingress sync and controller implementation of the sync #428

rramkumar1 · 2018-08-08T19:30:33Z

This PR introduces one high level sync interface and an interface for controllers to implement the sync.
The benefit of this PR is that it enforces a simple contract for controllers to follow and makes creating controller mocks much easier. For example, the existing controller in pkg/controller/controller.go would be modified to implement the Controller interface.

Note that this PR works at a layer above #424. A controller would potentially implement SyncBackends using the new interfaces defined in pkg/backends.

Also note that this abstraction will make it easier to parallelize syncs although the current implementation would need to be rewired a bit.

The second commit attempts to use the new interfaces in pkg/controller the way the code is currently structured.

/assign @bowei

bowei · 2018-08-21T17:21:50Z

pkg/controller/controller.go

+// of garbage collecting GCLB resources during the sync of an Ingress.
+type GarbageCollectionState struct {
+	lbNames     []string
+	allSvcPorts []utils.ServicePort


just call this svcPorts?

bowei · 2018-08-21T17:22:33Z

pkg/controller/controller.go

-		glog.V(2).Infof("Ingress %q no longer exists, triggering GC", key)
-		// GC will find GCE resources that were used for this ingress and delete them.
-		return lbc.gc(lbNames, gceSvcPorts)
+// Implements Controller.


fix these to be the right format (all other instances as well)

bowei · 2018-08-21T17:27:08Z

pkg/controller/controller.go

-	// Get ingress and DeepCopy for assurance that we don't pollute other goroutines with changes.
-	ing, ok := obj.(*extensions.Ingress)
+// Implements Controller.
+func (lbc *LoadBalancerController) SyncBackends(ing *extensions.Ingress, state interface{}) error {


Can we make this take in a utils.GCEURLMap? Do the conversion as close to the place where we get the interface{}

bowei · 2018-08-21T17:27:31Z

pkg/controller/controller.go

-	// free up enough quota for the next sync to pass.
-	if gcErr := lbc.gc(lbNames, gceSvcPorts); gcErr != nil {
-		retErr = fmt.Errorf("error during sync %v, error during GC %v", retErr, gcErr)
+	ports := []int64{}


can we make this a function by itself

bowei · 2018-08-21T17:31:44Z

pkg/sync/sync.go

+
+// ShortCircuit is an error that can be returned by a Controller to
+// short-circuit the remaining sync processes.
+var ShortCircuit = errors.New("")


ErrSkipBackendSync = errors.New("ingress skip backend sync")

If someone accidentally uses, we should know when it gets printed out.

Errors should be named Errxxx

bowei · 2018-08-21T17:34:01Z

pkg/controller/controller.go

+}
+
+// Implements Controller.
+func (lbc *LoadBalancerController) GCBackends(state interface{}) error {


again, would like to not have to take in interface{}

My thinking was that if its an interface, we aren't constrained to use one specific type to hold pre-processing data.

bowei · 2018-08-21T17:34:22Z

pkg/controller/controller.go

+	// We expect state to be a GarbageCollectionState
+	gcState, ok := state.(*GarbageCollectionState)
+	if !ok {
+		return fmt.Errorf("Expected state type to be GarbageCollectionState, type was %T", state)


don't capitalize errors

bowei · 2018-08-21T17:35:28Z

pkg/controller/controller.go


+// Implements Controller.
+func (lbc *LoadBalancerController) PostProcess(ing *extensions.Ingress) error {


call this UpdateStatus?

I feel like PostProcess is a little more generic. We don't have to necessarily just update the status in the post process routine, we could potentially do other things like remove a finalizer

bowei · 2018-08-21T17:35:51Z

pkg/controller/controller.go

-	l7, err := lbc.l7Pool.Get(lb.Name)
+	k, err := utils.KeyFunc(ing)
+	if err != nil {
+		return fmt.Errorf("cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err)


... Ingress %v/%v: %q

%q for an err?

ah misread ignore

bowei · 2018-08-21T17:36:35Z

pkg/sync/interfaces.go

@@ -0,0 +1,38 @@
+package sync


I think it's kind of an anti-pattern to put interfaces into their own .go file. We can keep it this way for now.

…implementations of it

rramkumar1 · 2018-08-22T23:41:14Z

/retest

bowei · 2018-08-23T20:09:03Z

/lgtm

k8s-ci-robot · 2018-08-23T20:09:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, rramkumar1

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~~ [bowei,rramkumar1]

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

k8s-ci-robot assigned bowei Aug 8, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2018

rramkumar1 force-pushed the syncer-interface branch from 6cbb7cb to a75d694 Compare August 8, 2018 19:34

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2018

rramkumar1 force-pushed the syncer-interface branch 6 times, most recently from 9fce965 to a8be678 Compare August 14, 2018 00:12

bowei reviewed Aug 21, 2018

View reviewed changes

rramkumar1 force-pushed the syncer-interface branch from a8be678 to db69511 Compare August 21, 2018 21:52

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2018

rramkumar1 force-pushed the syncer-interface branch 3 times, most recently from 502f229 to ac0c8ce Compare August 22, 2018 00:18

rramkumar1 added 2 commits August 22, 2018 16:25

Introduce a new interface to encapsulate Ingress sync and controller …

f5fb3ee

…implementations of it

Integrate with pkg/controller

6f8e6d6

rramkumar1 force-pushed the syncer-interface branch from ac0c8ce to 6f8e6d6 Compare August 22, 2018 23:35

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2018

k8s-ci-robot merged commit 7adac05 into kubernetes:master Aug 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a new interface to encapsulate Ingress sync and controller implementation of the sync #428

Introduce a new interface to encapsulate Ingress sync and controller implementation of the sync #428

rramkumar1 commented Aug 8, 2018 •

edited

Loading

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018

bowei Aug 21, 2018

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018

bowei Aug 21, 2018

bowei Aug 22, 2018

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018 •

edited

Loading

bowei Aug 21, 2018

rramkumar1 Aug 21, 2018

bowei Aug 22, 2018

bowei Aug 21, 2018

rramkumar1 commented Aug 22, 2018

bowei commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018


		// Implements Controller.
		func (lbc LoadBalancerController) PostProcess(ing extensions.Ingress) error {

Introduce a new interface to encapsulate Ingress sync and controller implementation of the sync #428

Introduce a new interface to encapsulate Ingress sync and controller implementation of the sync #428

Conversation

rramkumar1 commented Aug 8, 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

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

rramkumar1 commented Aug 22, 2018

bowei commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018

rramkumar1 commented Aug 8, 2018 •

edited

Loading

rramkumar1 Aug 21, 2018 •

edited

Loading