router: Fix detection of initial sync

 Previously, if both resource queues were populated on initial sync,
 commit would be called before handleFirstSync could set the sync
 state and the initial reload might not bind ports if bindPortsOnSync
 was true.  This change relies on the first call to Commit setting the
 sync state instead, relegating the handleFirstSync method to ensuring
 a commit on sync if either or both of the resource queues are empty
 after the initial sync.

Author: marun

pkg/router/controller/controller.go

@@ -79,12 +79,6 @@ func (c *RouterController) handleFirstSync() bool {
 		return false
 	}
 
-	err := c.Plugin.SetSyncedAtLeastOnce()
-	if err != nil {
-		utilruntime.HandleError(err)
-		return false
-	}
-
 	// If either of the event queues were empty after the initial
 	// List, the tracking listConsumed variable's default value of
 	// 'false' may prevent the router from committing the readiness

pkg/router/controller/controller_test.go

@@ -32,10 +32,6 @@ func (p *fakeRouterPlugin) Commit() error {
 	return nil
 }
 
-func (p *fakeRouterPlugin) SetSyncedAtLeastOnce() error {
-	return nil
-}
-
 type fakeNamespaceLister struct {
 }
 

pkg/router/controller/extended_validator.go

@@ -82,7 +82,3 @@ func (p *ExtendedValidator) HandleNamespaces(namespaces sets.String) error {
 func (p *ExtendedValidator) Commit() error {
 	return p.plugin.Commit()
 }
-
-func (p *ExtendedValidator) SetSyncedAtLeastOnce() error {
-	return p.plugin.SetSyncedAtLeastOnce()
-}

pkg/router/controller/host_admitter.go

@@ -152,10 +152,6 @@ func (p *HostAdmitter) Commit() error {
 	return p.plugin.Commit()
 }
 
-func (p *HostAdmitter) SetSyncedAtLeastOnce() error {
-	return p.plugin.SetSyncedAtLeastOnce()
-}
-
 // addRoute admits routes based on subdomain ownership - returns errors if the route is not admitted.
 func (p *HostAdmitter) addRoute(route *routeapi.Route) error {
 	// Find displaced routes (or error if an existing route displaces us)

pkg/router/controller/status.go

@@ -315,7 +315,3 @@ func (a *StatusAdmitter) HandleNamespaces(namespaces sets.String) error {
 func (a *StatusAdmitter) Commit() error {
 	return a.plugin.Commit()
 }
-
-func (a *StatusAdmitter) SetSyncedAtLeastOnce() error {
-	return a.plugin.SetSyncedAtLeastOnce()
-}

pkg/router/controller/status_test.go

@@ -43,10 +43,6 @@ func (p *fakePlugin) Commit() error {
 	return fmt.Errorf("not expected")
 }
 
-func (p *fakePlugin) SetSyncedAtLeastOnce() error {
-	return fmt.Errorf("not expected")
-}
-
 func TestStatusNoOp(t *testing.T) {
 	now := nowFn()
 	touched := unversioned.Time{Time: now.Add(-time.Minute)}

pkg/router/controller/unique_host.go

@@ -259,10 +259,6 @@ func (p *UniqueHost) Commit() error {
 	return p.plugin.Commit()
 }
 
-func (p *UniqueHost) SetSyncedAtLeastOnce() error {
-	return p.plugin.SetSyncedAtLeastOnce()
-}
-
 // routeKeys returns the internal router key to use for the given Route.
 func routeKeys(route *routeapi.Route) []string {
 	keys := make([]string, 1+len(route.Spec.AlternateBackends))

pkg/router/f5/plugin.go

@@ -633,8 +633,3 @@ func (p *F5Plugin) HandleRoute(eventType watch.EventType,
 func (p *F5Plugin) Commit() error {
 	return nil
 }
-
-// No-op since f5 has its own concept of what 'ready' means
-func (p *F5Plugin) SetSyncedAtLeastOnce() error {
-	return nil
-}

pkg/router/interfaces.go

@@ -17,5 +17,4 @@ type Plugin interface {
 	HandleNamespaces(namespaces sets.String) error
 	HandleNode(watch.EventType, *kapi.Node) error
 	Commit() error
-	SetSyncedAtLeastOnce() error
 }

pkg/router/template/plugin.go

@@ -83,9 +83,6 @@ type routerInterface interface {
 	// Commit applies the changes in the background. It kicks off a rate-limited
 	// commit (persist router state + refresh the backend) that coalesces multiple changes.
 	Commit()
-
-	// SetSyncedAtLeastOnce indicates to the router that state has been read from the api at least once
-	SetSyncedAtLeastOnce()
 }
 
 func env(name, defaultValue string) string {
@@ -227,11 +224,6 @@ func (p *TemplatePlugin) Commit() error {
 	return nil
 }
 
-func (p *TemplatePlugin) SetSyncedAtLeastOnce() error {
-	p.Router.SetSyncedAtLeastOnce()
-	return nil
-}
-
 // routeKeys returns the internal router keys to use for the given Route.
 // A route can have several services that it can point to, now
 func routeKeys(route *routeapi.Route) ([]string, []int32) {

pkg/router/template/plugin_test.go

@@ -235,9 +235,6 @@ func (r *TestRouter) Commit() {
 	// No op
 }
 
-func (r *TestRouter) SetSyncedAtLeastOnce() {
-}
-
 // TestHandleEndpoints test endpoint watch events
 func TestHandleEndpoints(t *testing.T) {
 	testCases := []struct {

pkg/router/template/router.go

@@ -87,7 +87,7 @@ type templateRouter struct {
 	// If true, haproxy should only bind ports when it has route and endpoint state
 	bindPortsAfterSync bool
 	// whether the router state has been read from the api at least once
-	syncedAtLeastOnce bool
+	synced bool
 	// whether a state change has occurred
 	stateChanged bool
 }
@@ -351,6 +351,12 @@ func (r *templateRouter) Commit() {
 	r.lock.Lock()
 	defer r.lock.Unlock()
 
+	if !r.synced {
+		glog.V(4).Infof("Router state synchronized for the first time")
+		r.synced = true
+		r.stateChanged = true
+	}
+
 	if r.stateChanged {
 		r.rateLimitedCommitFunction.Invoke(r.rateLimitedCommitFunction)
 		r.stateChanged = false
@@ -418,7 +424,7 @@ func (r *templateRouter) writeConfig() error {
 			StatsUser:          r.statsUser,
 			StatsPassword:      r.statsPassword,
 			StatsPort:          r.statsPort,
-			BindPorts:          !r.bindPortsAfterSync || r.syncedAtLeastOnce,
+			BindPorts:          !r.bindPortsAfterSync || r.synced,
 		}
 		if err := template.Execute(file, data); err != nil {
 			file.Close()
@@ -750,17 +756,6 @@ func (r *templateRouter) shouldWriteCerts(cfg *ServiceAliasConfig) bool {
 	return false
 }
 
-// SetSyncedAtLeastOnce indicates to the router that state has been
-// read from the api.
-func (r *templateRouter) SetSyncedAtLeastOnce() {
-	r.lock.Lock()
-	defer r.lock.Unlock()
-
-	glog.V(4).Infof("Router state synchronized for the first time")
-	r.syncedAtLeastOnce = true
-	r.stateChanged = true
-}
-
 // HasRoute indicates whether the given route is known to this router.
 func (r *templateRouter) HasRoute(route *routeapi.Route) bool {
 	r.lock.Lock()

test/integration/router_stress_test.go

@@ -288,13 +288,6 @@ func (p *DelayPlugin) Commit() error {
 	return p.plugin.Commit()
 }
 
-func (p *DelayPlugin) SetSyncedAtLeastOnce() error {
-	if p.committed {
-		return nil
-	}
-	return p.plugin.SetSyncedAtLeastOnce()
-}
-
 // launchRouter launches a template router that communicates with the
 // api via the provided clients.
 func launchRouter(oc osclient.Interface, kc kclientset.Interface, maxDelay int32, name string, reloadInterval int, reloadedMap map[string]bool) (templatePlugin *templateplugin.TemplatePlugin) {