From e328136911990f9545a71560e3c236f9d15f8925 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Wed, 9 Nov 2016 15:35:05 -0500 Subject: [PATCH 1/2] Log a better message on trying to deregister a nonexistent service --- command/agent/local.go | 12 ++++++++---- command/agent/local_test.go | 4 ++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/command/agent/local.go b/command/agent/local.go index c3ad782f10be..1a6d4b1a89ac 100644 --- a/command/agent/local.go +++ b/command/agent/local.go @@ -174,10 +174,14 @@ func (l *localState) RemoveService(serviceID string) { l.Lock() defer l.Unlock() - delete(l.services, serviceID) - delete(l.serviceTokens, serviceID) - l.serviceStatus[serviceID] = syncStatus{inSync: false} - l.changeMade() + if _, ok := l.services[serviceID]; ok { + delete(l.services, serviceID) + delete(l.serviceTokens, serviceID) + l.serviceStatus[serviceID] = syncStatus{inSync: false} + l.changeMade() + } else { + l.logger.Printf("[WARN] agent: Tried to deregister nonexistent service '%s'", serviceID) + } } // Services returns the locally registered services that the diff --git a/command/agent/local_test.go b/command/agent/local_test.go index 5989df388ccd..0892442875c9 100644 --- a/command/agent/local_test.go +++ b/command/agent/local_test.go @@ -1074,6 +1074,10 @@ func TestAgent_serviceTokens(t *testing.T) { l := new(localState) l.Init(config, nil) + l.AddService(&structs.NodeService{ + ID: "redis", + }, "") + // Returns default when no token is set if token := l.ServiceToken("redis"); token != "default" { t.Fatalf("bad: %s", token) From bd23a2cf9c82562ccdcb4d582256bd6039348ec6 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Wed, 9 Nov 2016 16:45:30 -0500 Subject: [PATCH 2/2] Log the warning in agent.RemoveService instead --- command/agent/agent.go | 9 ++++++++- command/agent/agent_test.go | 5 +++++ command/agent/local.go | 6 ++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 2b7bfb93c9df..23137f275e7f 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -906,7 +906,14 @@ func (a *Agent) RemoveService(serviceID string, persist bool) error { } // Remove service immediately - a.state.RemoveService(serviceID) + err := a.state.RemoveService(serviceID) + + // TODO: Return the error instead of just logging here in Consul 0.8 + // For now, keep the current idempotent behavior on deleting a nonexistent service + if err != nil { + a.logger.Printf("[WARN] agent: Failed to deregister service %q: %s", serviceID, err) + return nil + } // Remove the service from the data dir if persist { diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 1375d460fa56..92740f352099 100644 --- a/command/agent/agent_test.go +++ b/command/agent/agent_test.go @@ -935,6 +935,11 @@ func TestAgent_PurgeService(t *testing.T) { t.Fatalf("err: %s", err) } + // Re-add the service + if err := agent.AddService(svc, nil, true, ""); err != nil { + t.Fatalf("err: %v", err) + } + // Removed if err := agent.RemoveService(svc.ID, true); err != nil { t.Fatalf("err: %s", err) diff --git a/command/agent/local.go b/command/agent/local.go index 1a6d4b1a89ac..475fe56d9cd6 100644 --- a/command/agent/local.go +++ b/command/agent/local.go @@ -170,7 +170,7 @@ func (l *localState) AddService(service *structs.NodeService, token string) { // RemoveService is used to remove a service entry from the local state. // The agent will make a best effort to ensure it is deregistered -func (l *localState) RemoveService(serviceID string) { +func (l *localState) RemoveService(serviceID string) error { l.Lock() defer l.Unlock() @@ -180,8 +180,10 @@ func (l *localState) RemoveService(serviceID string) { l.serviceStatus[serviceID] = syncStatus{inSync: false} l.changeMade() } else { - l.logger.Printf("[WARN] agent: Tried to deregister nonexistent service '%s'", serviceID) + return fmt.Errorf("Service does not exist") } + + return nil } // Services returns the locally registered services that the