agent: add default weights to service in local state to prevent AE churn (#5126)

* Add default weights when adding a service with no weights to local state to prevent constant AE re-sync.

This fix was contributed by @42wim in https://github.com/hashicorp/consul/pull/5096 but was merged against the wrong base. This adds it to master and adds a test to cover the behaviour.

* Fix tests that broke due to comparing internal state which now has default weights

agent/agent.go

@@ -1880,6 +1880,12 @@ func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.Che
 		}
 	}
 
+	// Set default weights if not specified. This is important as it ensures AE
+	// doesn't consider the service different since it has nil weights.
+	if service.Weights == nil {
+		service.Weights = &structs.Weights{Passing: 1, Warning: 1}
+	}
+
 	// Warn if the service name is incompatible with DNS
 	if InvalidDnsRe.MatchString(service.Service) {
 		a.logger.Printf("[WARN] agent: Service name %q will not be discoverable "+

agent/agent_endpoint_test.go

@@ -2479,6 +2479,7 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) {
 		},
 		Port:                       8001,
 		EnableTagOverride:          true,
+		Weights:                    &structs.Weights{Passing: 1, Warning: 1},
 		LocallyRegisteredAsSidecar: true,
 		Proxy: structs.ConnectProxyConfig{
 			DestinationServiceName: "test",
@@ -2837,6 +2838,10 @@ func testDefaultSidecar(svc string, port int, fns ...func(*structs.NodeService))
 		Kind:    structs.ServiceKindConnectProxy,
 		Service: svc + "-sidecar-proxy",
 		Port:    2222,
+		Weights: &structs.Weights{
+			Passing: 1,
+			Warning: 1,
+		},
 		// Note that LocallyRegisteredAsSidecar should be true on the internal
 		// NodeService, but that we never want to see it in the HTTP response as
 		// it's internal only state. This is being compared directly to local state
@@ -3149,6 +3154,10 @@ func TestAgent_RegisterServiceDeregisterService_Sidecar(t *testing.T) {
 				ID:      "web-sidecar-proxy",
 				Service: "fake-sidecar",
 				Port:    9999,
+				Weights: &structs.Weights{
+					Passing: 1,
+					Warning: 1,
+				},
 			},
 			// After we deregister the web service above, the fake sidecar with
 			// clashing ID SHOULD NOT have been removed since it wasn't part of the
@@ -3184,6 +3193,10 @@ func TestAgent_RegisterServiceDeregisterService_Sidecar(t *testing.T) {
 				Service:                    "web-sidecar-proxy",
 				LocallyRegisteredAsSidecar: true,
 				Port:                       6666,
+				Weights: &structs.Weights{
+					Passing: 1,
+					Warning: 1,
+				},
 				Proxy: structs.ConnectProxyConfig{
 					DestinationServiceName: "web",
 					DestinationServiceID:   "web",

agent/agent_test.go

@@ -333,6 +333,7 @@ func TestAgent_AddService(t *testing.T) {
 	tests := []struct {
 		desc       string
 		srv        *structs.NodeService
+		wantSrv    func(ns *structs.NodeService)
 		chkTypes   []*structs.CheckType
 		healthChks map[string]*structs.HealthCheck
 	}{
@@ -342,8 +343,16 @@ func TestAgent_AddService(t *testing.T) {
 				ID:      "svcid1",
 				Service: "svcname1",
 				Tags:    []string{"tag1"},
+				Weights: nil, // nil weights...
 				Port:    8100,
 			},
+			// ... should be populated to avoid "IsSame" returning true during AE.
+			func(ns *structs.NodeService) {
+				ns.Weights = &structs.Weights{
+					Passing: 1,
+					Warning: 1,
+				}
+			},
 			[]*structs.CheckType{
 				&structs.CheckType{
 					CheckID: "check1",
@@ -370,9 +379,14 @@ func TestAgent_AddService(t *testing.T) {
 			&structs.NodeService{
 				ID:      "svcid2",
 				Service: "svcname2",
-				Tags:    []string{"tag2"},
+				Weights: &structs.Weights{
-				Port:    8200,
+					Passing: 2,
+					Warning: 1,
+				},
+				Tags: []string{"tag2"},
+				Port: 8200,
 			},
+			nil, // No change expected
 			[]*structs.CheckType{
 				&structs.CheckType{
 					CheckID: "check1",
@@ -443,15 +457,22 @@ func TestAgent_AddService(t *testing.T) {
 					t.Fatalf("err: %v", err)
 				}
 
-				got, want := a.State.Services()[tt.srv.ID], tt.srv
+				got := a.State.Services()[tt.srv.ID]
-				verify.Values(t, "", got, want)
+				// Make a copy since the tt.srv points to the one in memory in the local
+				// state still so changing it is a tautology!
+				want := *tt.srv
+				if tt.wantSrv != nil {
+					tt.wantSrv(&want)
+				}
+				require.Equal(t, &want, got)
+				require.True(t, got.IsSame(&want))
 			})
 
 			// check the health checks
 			for k, v := range tt.healthChks {
 				t.Run(k, func(t *testing.T) {
-					got, want := a.State.Checks()[types.CheckID(k)], v
+					got := a.State.Checks()[types.CheckID(k)]
-					verify.Values(t, k, got, want)
+					require.Equal(t, v, got)
 				})
 			}
 
@@ -1478,6 +1499,7 @@ func TestAgent_persistedService_compat(t *testing.T) {
 		Service: "redis",
 		Tags:    []string{"foo"},
 		Port:    8000,
+		Weights: &structs.Weights{Passing: 1, Warning: 1},
 	}
 
 	// Encode the NodeService directly. This is what previous versions
@@ -1507,9 +1529,7 @@ func TestAgent_persistedService_compat(t *testing.T) {
 	if !ok {
 		t.Fatalf("missing service")
 	}
-	if !reflect.DeepEqual(result, svc) {
+	require.Equal(t, svc, result)
-		t.Fatalf("bad: %#v", result)
-	}
 }
 
 func TestAgent_PurgeService(t *testing.T) {

api/agent_test.go

@@ -712,17 +712,21 @@ func TestAPI_AgentService(t *testing.T) {
 		ID:          "foo",
 		Service:     "foo",
 		Tags:        []string{"bar", "baz"},
-		ContentHash: "5d286f5494330b04",
+		ContentHash: "ad8c7a278470d1e8",
 		Port:        8000,
+		Weights: AgentWeights{
+			Passing: 1,
+			Warning: 1,
+		},
 	}
 	require.Equal(expect, got)
 	require.Equal(expect.ContentHash, qm.LastContentHash)
 
-	// Sanity check blocking behaviour - this is more thoroughly tested in the
+	// Sanity check blocking behavior - this is more thoroughly tested in the
 	// agent endpoint tests but this ensures that the API package is at least
 	// passing the hash param properly.
 	opts := QueryOptions{
-		WaitHash: "5d286f5494330b04",
+		WaitHash: qm.LastContentHash,
 		WaitTime: 100 * time.Millisecond, // Just long enough to be reliably measurable
 	}
 	start := time.Now()