From b1760b223e7ac67eec94f64c6d7879b2a8ef7c71 Mon Sep 17 00:00:00 2001
From: Kyle Havlovitz <kylehav@gmail.com>
Date: Wed, 9 Nov 2016 16:56:54 -0500
Subject: [PATCH] Improve logging when deregistering a nonexistent service
 (#2492)

Log a warning instead of a success message when attempting to deregister a nonexistent service. In Consul 0.8 this can be changed to giving an error outright, but for now we can keep the idempotent delete behavior.
---
 command/agent/agent.go      |  9 ++++++++-
 command/agent/agent_test.go |  5 +++++
 command/agent/local.go      | 16 +++++++++++-----
 command/agent/local_test.go |  4 ++++
 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/command/agent/agent.go b/command/agent/agent.go
index 2b7bfb93c9..23137f275e 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 1375d460fa..92740f3520 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 c3ad782f10..475fe56d9c 100644
--- a/command/agent/local.go
+++ b/command/agent/local.go
@@ -170,14 +170,20 @@ 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()
 
-	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 {
+		return fmt.Errorf("Service does not exist")
+	}
+
+	return nil
 }
 
 // Services returns the locally registered services that the
diff --git a/command/agent/local_test.go b/command/agent/local_test.go
index 5989df388c..0892442875 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)