diff --git a/.changelog/10865.txt b/.changelog/10865.txt new file mode 100644 index 000000000..ef3d73eb0 --- /dev/null +++ b/.changelog/10865.txt @@ -0,0 +1,3 @@ +```release-note:bug +consul: avoid extra sync operations when no action required +``` diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 5a32e4985..da4431966 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -348,6 +348,27 @@ type operations struct { deregChecks []string } +func (o *operations) empty() bool { + switch { + case o == nil: + return true + case len(o.regServices) > 0: + return false + case len(o.regChecks) > 0: + return false + case len(o.deregServices) > 0: + return false + case len(o.deregChecks) > 0: + return false + default: + return true + } +} + +func (o operations) String() string { + return fmt.Sprintf("<%d, %d, %d, %d>", len(o.regServices), len(o.regChecks), len(o.deregServices), len(o.deregChecks)) +} + // AllocRegistration holds the status of services registered for a particular // allocations by task. type AllocRegistration struct { @@ -560,11 +581,24 @@ func (c *ServiceClient) hasSeen() bool { type syncReason byte const ( - syncPeriodic = iota + syncPeriodic syncReason = iota syncShutdown syncNewOps ) +func (sr syncReason) String() string { + switch sr { + case syncPeriodic: + return "periodic" + case syncShutdown: + return "shutdown" + case syncNewOps: + return "operations" + default: + return "unexpected" + } +} + // Run the Consul main loop which retries operations against Consul. It should // be called exactly once. func (c *ServiceClient) Run() { @@ -680,6 +714,24 @@ INIT: // commit operations unless already shutting down. func (c *ServiceClient) commit(ops *operations) { + c.logger.Trace("commit sync operations", "ops", ops) + + // Ignore empty operations - ideally callers will optimize out syncs with + // nothing to do, but be defensive anyway. Sending an empty ops on the chan + // will trigger an unnecessary sync with Consul. + if ops.empty() { + return + } + + // Prioritize doing nothing if we are being signaled to shutdown. + select { + case <-c.shutdownCh: + return + default: + } + + // Send the ops down the ops chan, triggering a sync with Consul. Unless we + // receive a signal to shutdown. select { case c.opCh <- ops: case <-c.shutdownCh: @@ -713,6 +765,8 @@ func (c *ServiceClient) merge(ops *operations) { // sync enqueued operations. func (c *ServiceClient) sync(reason syncReason) error { + c.logger.Trace("execute sync", "reason", reason) + sreg, creg, sdereg, cdereg := 0, 0, 0, 0 var err error diff --git a/command/agent/consul/service_client_test.go b/command/agent/consul/service_client_test.go index 0b9544d29..5a72c4a0e 100644 --- a/command/agent/consul/service_client_test.go +++ b/command/agent/consul/service_client_test.go @@ -1,6 +1,7 @@ package consul import ( + "fmt" "testing" "time" @@ -599,3 +600,27 @@ func TestSyncLogic_proxyUpstreamsDifferent(t *testing.T) { } }) } + +func TestSyncReason_String(t *testing.T) { + t.Parallel() + + require.Equal(t, "periodic", fmt.Sprintf("%s", syncPeriodic)) + require.Equal(t, "shutdown", fmt.Sprintf("%s", syncShutdown)) + require.Equal(t, "operations", fmt.Sprintf("%s", syncNewOps)) + require.Equal(t, "unexpected", fmt.Sprintf("%s", syncReason(128))) +} + +func TestSyncOps_empty(t *testing.T) { + t.Parallel() + + try := func(ops *operations, exp bool) { + require.Equal(t, exp, ops.empty()) + } + + try(&operations{regServices: make([]*api.AgentServiceRegistration, 1)}, false) + try(&operations{regChecks: make([]*api.AgentCheckRegistration, 1)}, false) + try(&operations{deregServices: make([]string, 1)}, false) + try(&operations{deregChecks: make([]string, 1)}, false) + try(&operations{}, true) + try(nil, true) +}