From fcdb5b3494b8a8a2b4b9a6794ec2dfb86a2245b1 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Tue, 8 Jan 2019 10:13:49 +0000 Subject: [PATCH] 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 | 6 ++++++ agent/agent_endpoint_test.go | 13 ++++++++++++ agent/agent_test.go | 38 +++++++++++++++++++++++++++--------- api/agent_test.go | 10 +++++++--- 4 files changed, 55 insertions(+), 12 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 9e161b3f0..6daa16810 100644 --- a/agent/agent.go +++ b/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 "+ diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 4bfd413b7..e2ecc5ed3 100644 --- a/agent/agent_endpoint_test.go +++ b/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", diff --git a/agent/agent_test.go b/agent/agent_test.go index 7db415343..107203ccd 100644 --- a/agent/agent_test.go +++ b/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"}, - Port: 8200, + Weights: &structs.Weights{ + 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 - verify.Values(t, "", got, want) + got := a.State.Services()[tt.srv.ID] + // 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 - verify.Values(t, k, got, want) + got := a.State.Checks()[types.CheckID(k)] + 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) { - t.Fatalf("bad: %#v", result) - } + require.Equal(t, svc, result) } func TestAgent_PurgeService(t *testing.T) { diff --git a/api/agent_test.go b/api/agent_test.go index ba58541d1..221c7d413 100644 --- a/api/agent_test.go +++ b/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()