Never remove unknown agent services

Fixes #2827

This is a tradeoff. The pro is that you can run separate client and
server agents on the same node and advertise both. The con is that if a
Nomad agent crashes and isn't restarted on that node in the same mode
its entry will not be cleaned up.

That con scenario seems far less likely to occur than the scenario on
the pro side, and even if we do leak an agent entry the checks will be
failing so nothing should attempt to use it.

command/agent/consul/client.go

@@ -779,7 +779,9 @@ func createCheckReg(serviceID, checkID string, check *structs.ServiceCheck, host
 }
 
 // isNomadService returns true if the ID matches the pattern of a Nomad managed
-// service.
+// service. Agent services return false as independent client and server agents
+// may be running on the same machine. #2827
 func isNomadService(id string) bool {
-	return strings.HasPrefix(id, nomadServicePrefix)
+	const prefix = nomadServicePrefix + "-executor"
+	return strings.HasPrefix(id, prefix)
 }

command/agent/consul/unit_test.go

@@ -1231,3 +1231,27 @@ func TestConsul_DriverNetwork_Change(t *testing.T) {
 
 	syncAndAssertPort(net.PortMap["x"])
 }
+
+// TestIsNomadService asserts the isNomadService helper returns true for Nomad
+// task IDs and false for unknown IDs and Nomad agent IDs (see #2827).
+func TestIsNomadService(t *testing.T) {
+	tests := []struct {
+		id     string
+		result bool
+	}{
+		{"_nomad-client-nomad-client-http", false},
+		{"_nomad-server-nomad-serf", false},
+		{"_nomad-executor-abc", true},
+		{"_nomad-executor", true},
+		{"not-nomad", false},
+	}
+
+	for _, test := range tests {
+		t.Run(test.id, func(t *testing.T) {
+			actual := isNomadService(test.id)
+			if actual != test.result {
+				t.Error("%q should be %t but found %t", test.id, test.result, actual)
+			}
+		})
+	}
+}