fed: Provide updater timeout to instance rather than to Update()

federation/pkg/federation-controller/util/deletionhelper/deletion_helper.go

@@ -22,7 +22,6 @@ package deletionhelper
 import (
 	"fmt"
 	"strings"
-	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -50,18 +49,16 @@ type ObjNameFunc func(runtime.Object) string
 type DeletionHelper struct {
 	updateObjFunc UpdateObjFunc
 	objNameFunc   ObjNameFunc
-	updateTimeout time.Duration
 	informer      util.FederatedInformer
 	updater       util.FederatedUpdater
 }
 
 func NewDeletionHelper(
-	updateObjFunc UpdateObjFunc, objNameFunc ObjNameFunc, updateTimeout time.Duration,
+	updateObjFunc UpdateObjFunc, objNameFunc ObjNameFunc,
 	informer util.FederatedInformer, updater util.FederatedUpdater) *DeletionHelper {
 	return &DeletionHelper{
 		updateObjFunc: updateObjFunc,
 		objNameFunc:   objNameFunc,
-		updateTimeout: updateTimeout,
 		informer:      informer,
 		updater:       updater,
 	}
@@ -155,7 +152,7 @@ func (dh *DeletionHelper) HandleObjectInUnderlyingClusters(obj runtime.Object) (
 			Key:         objName,
 		})
 	}
-	err = dh.updater.Update(operations, dh.updateTimeout)
+	err = dh.updater.Update(operations)
 	if err != nil {
 		return nil, fmt.Errorf("failed to execute updates for obj %s: %v", objName, err)
 	}

federation/pkg/federation-controller/util/federated_updater.go

@@ -48,11 +48,8 @@ type FederatedOperation struct {
 
 // A helper that executes the given set of updates on federation, in parallel.
 type FederatedUpdater interface {
-	// Executes the given set of operations within the specified timeout.
-	// Timeout is best-effort. There is no guarantee that the underlying operations are
-	// stopped when it is reached. However the function will return after the timeout
-	// with a non-nil error.
-	Update([]FederatedOperation, time.Duration) error
+	// Executes the given set of operations.
+	Update([]FederatedOperation) error
 }
 
 // A function that executes some operation using the passed client and object.
@@ -63,6 +60,8 @@ type federatedUpdaterImpl struct {
 
 	kind string
 
+	timeout time.Duration
+
 	eventRecorder record.EventRecorder
 
 	addFunction    FederatedOperationHandler
@@ -70,10 +69,11 @@ type federatedUpdaterImpl struct {
 	deleteFunction FederatedOperationHandler
 }
 
-func NewFederatedUpdater(federation FederationView, kind string, recorder record.EventRecorder, add, update, del FederatedOperationHandler) FederatedUpdater {
+func NewFederatedUpdater(federation FederationView, kind string, timeout time.Duration, recorder record.EventRecorder, add, update, del FederatedOperationHandler) FederatedUpdater {
 	return &federatedUpdaterImpl{
 		federation:     federation,
 		kind:           kind,
+		timeout:        timeout,
 		eventRecorder:  recorder,
 		addFunction:    add,
 		updateFunction: update,
@@ -86,7 +86,11 @@ func (fu *federatedUpdaterImpl) recordEvent(obj runtime.Object, eventType, event
 	fu.eventRecorder.Eventf(obj, api.EventTypeNormal, eventType, messageFmt, args...)
 }
 
-func (fu *federatedUpdaterImpl) Update(ops []FederatedOperation, timeout time.Duration) error {
+// Update executes the given set of operations within the timeout specified for
+// the instance. Timeout is best-effort. There is no guarantee that the
+// underlying operations are stopped when it is reached. However the function
+// will return after the timeout with a non-nil error.
+func (fu *federatedUpdaterImpl) Update(ops []FederatedOperation) error {
 	done := make(chan error, len(ops))
 	for _, op := range ops {
 		go func(op FederatedOperation) {
@@ -136,16 +140,16 @@ func (fu *federatedUpdaterImpl) Update(ops []FederatedOperation, timeout time.Du
 	start := time.Now()
 	for i := 0; i < len(ops); i++ {
 		now := time.Now()
-		if !now.Before(start.Add(timeout)) {
-			return fmt.Errorf("failed to finish all operations in %v", timeout)
+		if !now.Before(start.Add(fu.timeout)) {
+			return fmt.Errorf("failed to finish all operations in %v", fu.timeout)
 		}
 		select {
 		case err := <-done:
 			if err != nil {
 				return err
 			}
-		case <-time.After(start.Add(timeout).Sub(now)):
-			return fmt.Errorf("failed to finish all operations in %v", timeout)
+		case <-time.After(start.Add(fu.timeout).Sub(now)):
+			return fmt.Errorf("failed to finish all operations in %v", fu.timeout)
 		}
 	}
 	// All operations finished in time.

federation/pkg/federation-controller/util/federated_updater_test.go

@@ -70,7 +70,7 @@ func TestFederatedUpdaterOK(t *testing.T) {
 	addChan := make(chan string, 5)
 	updateChan := make(chan string, 5)
 
-	updater := NewFederatedUpdater(&fakeFederationView{}, "foo", &fakeEventRecorder{},
+	updater := NewFederatedUpdater(&fakeFederationView{}, "foo", time.Minute, &fakeEventRecorder{},
 		func(_ kubeclientset.Interface, obj pkgruntime.Object) error {
 			service := obj.(*apiv1.Service)
 			addChan <- service.Name
@@ -92,7 +92,7 @@ func TestFederatedUpdaterOK(t *testing.T) {
 			Type: OperationTypeUpdate,
 			Obj:  makeService("B", "s2"),
 		},
-	}, time.Minute)
+	})
 	assert.NoError(t, err)
 	add := <-addChan
 	update := <-updateChan
@@ -101,7 +101,7 @@ func TestFederatedUpdaterOK(t *testing.T) {
 }
 
 func TestFederatedUpdaterError(t *testing.T) {
-	updater := NewFederatedUpdater(&fakeFederationView{}, "foo", &fakeEventRecorder{},
+	updater := NewFederatedUpdater(&fakeFederationView{}, "foo", time.Minute, &fakeEventRecorder{},
 		func(_ kubeclientset.Interface, obj pkgruntime.Object) error {
 			return fmt.Errorf("boom")
 		}, noop, noop)
@@ -115,13 +115,13 @@ func TestFederatedUpdaterError(t *testing.T) {
 			Type: OperationTypeUpdate,
 			Obj:  makeService("B", "s1"),
 		},
-	}, time.Minute)
+	})
 	assert.Error(t, err)
 }
 
 func TestFederatedUpdaterTimeout(t *testing.T) {
 	start := time.Now()
-	updater := NewFederatedUpdater(&fakeFederationView{}, "foo", &fakeEventRecorder{},
+	updater := NewFederatedUpdater(&fakeFederationView{}, "foo", time.Second, &fakeEventRecorder{},
 		func(_ kubeclientset.Interface, obj pkgruntime.Object) error {
 			time.Sleep(time.Minute)
 			return nil
@@ -137,7 +137,7 @@ func TestFederatedUpdaterTimeout(t *testing.T) {
 			Type: OperationTypeUpdate,
 			Obj:  makeService("B", "s1"),
 		},
-	}, time.Second)
+	})
 	end := time.Now()
 	assert.Error(t, err)
 	assert.True(t, start.Add(10*time.Second).After(end))