From 60975f5ad53f0a16ac9be772d4ca9c8003c26733 Mon Sep 17 00:00:00 2001 From: Random-Liu Date: Tue, 17 Jan 2017 17:37:19 -0800 Subject: [PATCH] Update NPD to only do forcibly sync every 1 minutes. --- pkg/condition/manager.go | 58 +++++++++++++++++++++-------------- pkg/condition/manager_test.go | 33 +++++++++++++++++--- 2 files changed, 63 insertions(+), 28 deletions(-) diff --git a/pkg/condition/manager.go b/pkg/condition/manager.go index c04d4128..523be3e3 100644 --- a/pkg/condition/manager.go +++ b/pkg/condition/manager.go @@ -32,12 +32,12 @@ import ( ) const ( - // updatePeriod is the period which condition manager checks update. + // updatePeriod is the period at which condition manager checks update. updatePeriod = 1 * time.Second - // updateTimeout is the timeout of condition update operation. - updateTimeout = 10 * time.Second - // resyncPeriod is the period which condition manager does resync no matter whether these is any update. - resyncPeriod = 30 * time.Second + // resyncPeriod is the period at which condition manager does resync, only updates when needed. + resyncPeriod = 10 * time.Second + // heartbeatPeriod is the period at which condition manager does forcibly sync with apiserver. + heartbeatPeriod = 1 * time.Minute ) // ConditionManager synchronizes node conditions with the apiserver with problem client. @@ -52,17 +52,21 @@ const ( type ConditionManager interface { // Start starts the condition manager. Start() - // UpdateCondition update specific condition. + // UpdateCondition updates a specific condition. UpdateCondition(types.Condition) } type conditionManager struct { - sync.Mutex - clock clock.Clock - latest time.Time - client problemclient.Client - updates map[string]types.Condition - conditions map[string]types.Condition + clock clock.Clock + latestTry time.Time + resyncNeeded bool + client problemclient.Client + // updatesLock is the lock protecting updates. Only the field `updates` + // will be accessed by random caller and the sync routine, so only it + // needs to be protected. + updatesLock sync.Mutex + updates map[string]types.Condition + conditions map[string]types.Condition } // NewConditionManager creates a condition manager. @@ -80,8 +84,8 @@ func (c *conditionManager) Start() { } func (c *conditionManager) UpdateCondition(condition types.Condition) { - c.Lock() - defer c.Unlock() + c.updatesLock.Lock() + defer c.updatesLock.Unlock() // New node condition will override the old condition, because we only need the newest // condition for each condition type. c.updates[condition.Type] = condition @@ -92,17 +96,17 @@ func (c *conditionManager) syncLoop() { for { select { case <-updateCh: - if c.checkUpdates() || c.checkResync() { + if c.needUpdates() || c.needResync() || c.needHeartbeat() { c.sync() } } } } -// checkUpdates checks whether there are recent updates. -func (c *conditionManager) checkUpdates() bool { - c.Lock() - defer c.Unlock() +// needUpdates checks whether there are recent updates. +func (c *conditionManager) needUpdates() bool { + c.updatesLock.Lock() + defer c.updatesLock.Unlock() needUpdate := false for t, update := range c.updates { if !reflect.DeepEqual(c.conditions[t], update) { @@ -114,13 +118,21 @@ func (c *conditionManager) checkUpdates() bool { return needUpdate } -// checkResync checks whether a resync is needed. -func (c *conditionManager) checkResync() bool { - return c.clock.Now().Sub(c.latest) >= resyncPeriod +// needResync checks whether a resync is needed. +func (c *conditionManager) needResync() bool { + // Only update when resync is needed. + return c.clock.Now().Sub(c.latestTry) >= resyncPeriod && c.resyncNeeded +} + +// needHeartbeat checks whether a forcible heartbeat is needed. +func (c *conditionManager) needHeartbeat() bool { + return c.clock.Now().Sub(c.latestTry) >= heartbeatPeriod } // sync synchronizes node conditions with the apiserver. func (c *conditionManager) sync() { + c.latestTry = c.clock.Now() + c.resyncNeeded = false conditions := []api.NodeCondition{} for i := range c.conditions { conditions = append(conditions, problemutil.ConvertToAPICondition(c.conditions[i])) @@ -128,7 +140,7 @@ func (c *conditionManager) sync() { if err := c.client.SetConditions(conditions); err != nil { // The conditions will be updated again in future sync glog.Errorf("failed to update node conditions: %v", err) + c.resyncNeeded = true return } - c.latest = c.clock.Now() } diff --git a/pkg/condition/manager_test.go b/pkg/condition/manager_test.go index b9bfca21..1b565a8b 100644 --- a/pkg/condition/manager_test.go +++ b/pkg/condition/manager_test.go @@ -17,6 +17,7 @@ limitations under the License. package condition import ( + "fmt" "testing" "time" @@ -47,7 +48,7 @@ func newTestCondition(condition string) types.Condition { } } -func TestCheckUpdates(t *testing.T) { +func TestNeedUpdates(t *testing.T) { m, _, _ := newTestManager() var c types.Condition for desc, test := range map[string]struct { @@ -75,19 +76,41 @@ func TestCheckUpdates(t *testing.T) { c = newTestCondition(test.condition) } m.UpdateCondition(c) - assert.Equal(t, test.update, m.checkUpdates(), desc) + assert.Equal(t, test.update, m.needUpdates(), desc) assert.Equal(t, c, m.conditions[c.Type], desc) } } -func TestSync(t *testing.T) { +func TestResync(t *testing.T) { m, fakeClient, fakeClock := newTestManager() condition := newTestCondition("TestCondition") m.conditions = map[string]types.Condition{condition.Type: condition} m.sync() expected := []api.NodeCondition{problemutil.ConvertToAPICondition(condition)} assert.Nil(t, fakeClient.AssertConditions(expected), "Condition should be updated via client") - assert.False(t, m.checkResync(), "Should not resync before timeout exceeds") + + assert.False(t, m.needResync(), "Should not resync before resync period") fakeClock.Step(resyncPeriod) - assert.True(t, m.checkResync(), "Should resync after timeout exceeds") + assert.False(t, m.needResync(), "Should not resync after resync period without resync needed") + + fakeClient.InjectError("SetConditions", fmt.Errorf("injected error")) + m.sync() + + assert.False(t, m.needResync(), "Should not resync before resync period") + fakeClock.Step(resyncPeriod) + assert.True(t, m.needResync(), "Should resync after resync period and resync is needed") +} + +func TestHeartbeat(t *testing.T) { + m, fakeClient, fakeClock := newTestManager() + condition := newTestCondition("TestCondition") + m.conditions = map[string]types.Condition{condition.Type: condition} + m.sync() + expected := []api.NodeCondition{problemutil.ConvertToAPICondition(condition)} + assert.Nil(t, fakeClient.AssertConditions(expected), "Condition should be updated via client") + + assert.False(t, m.needHeartbeat(), "Should not heartbeat before heartbeat period") + + fakeClock.Step(heartbeatPeriod) + assert.True(t, m.needHeartbeat(), "Should heartbeat after heartbeat period") }