From 42f80367be3d5944982d7c04ab042bc3eb95e657 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Thu, 19 Dec 2019 10:15:37 -0600 Subject: [PATCH] Restore a few more service-kind index updates so blocking in ServiceDump works in more cases (#6948) Restore a few more service-kind index updates so blocking in ServiceDump works in more cases Namely one omission was that check updates for dumped services were not unblocking. Also adds a ServiceDump state store test and also fix a watch bug with the normal dump. Follow-on from #6916 --- agent/consul/state/catalog.go | 11 +- agent/consul/state/catalog_oss.go | 7 + agent/consul/state/catalog_test.go | 297 +++++++++++++++++++++++++++++ 3 files changed, 314 insertions(+), 1 deletion(-) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index 0140cb383..5d066221a 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -552,6 +552,9 @@ func (s *Store) deleteNodeTxn(tx *memdb.Txn, idx uint64, nodeName string) error if err := s.catalogUpdateServiceIndexes(tx, svc.ServiceName, idx, &svc.EnterpriseMeta); err != nil { return err } + if err := s.catalogUpdateServiceKindIndexes(tx, svc.ServiceKind, idx, &svc.EnterpriseMeta); err != nil { + return err + } } // Do the delete in a separate loop so we don't trash the iterator. @@ -1334,6 +1337,9 @@ func (s *Store) updateAllServiceIndexesOfNode(tx *memdb.Txn, idx uint64, nodeID if err := s.catalogUpdateServiceIndexes(tx, svc.ServiceName, idx, &svc.EnterpriseMeta); err != nil { return err } + if err := s.catalogUpdateServiceKindIndexes(tx, svc.ServiceKind, idx, &svc.EnterpriseMeta); err != nil { + return err + } } return nil } @@ -1423,6 +1429,9 @@ func (s *Store) ensureCheckTxn(tx *memdb.Txn, idx uint64, hc *structs.HealthChec if err = s.catalogUpdateServiceIndexes(tx, svc.ServiceName, idx, &svc.EnterpriseMeta); err != nil { return err } + if err := s.catalogUpdateServiceKindIndexes(tx, svc.ServiceKind, idx, &svc.EnterpriseMeta); err != nil { + return err + } } } else { if existing != nil && existing.(*structs.HealthCheck).IsSame(hc) { @@ -2033,7 +2042,7 @@ func (s *Store) ServiceDump(ws memdb.WatchSet, kind structs.ServiceKind, useKind func (s *Store) serviceDumpAllTxn(tx *memdb.Txn, ws memdb.WatchSet, entMeta *structs.EnterpriseMeta) (uint64, structs.CheckServiceNodes, error) { // Get the table index - idx := s.catalogMaxIndex(tx, entMeta, true) + idx := s.catalogMaxIndexWatch(tx, ws, entMeta, true) services, err := s.catalogServiceList(tx, entMeta, true) if err != nil { diff --git a/agent/consul/state/catalog_oss.go b/agent/consul/state/catalog_oss.go index 97b249184..8c89238bf 100644 --- a/agent/consul/state/catalog_oss.go +++ b/agent/consul/state/catalog_oss.go @@ -262,6 +262,13 @@ func (s *Store) catalogMaxIndex(tx *memdb.Txn, _ *structs.EnterpriseMeta, checks return maxIndexTxn(tx, "nodes", "services") } +func (s *Store) catalogMaxIndexWatch(tx *memdb.Txn, ws memdb.WatchSet, _ *structs.EnterpriseMeta, checks bool) uint64 { + if checks { + return maxIndexWatchTxn(tx, ws, "nodes", "services", "checks") + } + return maxIndexWatchTxn(tx, ws, "nodes", "services") +} + func (s *Store) catalogUpdateCheckIndexes(tx *memdb.Txn, idx uint64, _ *structs.EnterpriseMeta) error { // update the universal index entry if err := tx.Insert("index", &IndexEntry{"checks", idx}); err != nil { diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index a40382643..e72cd3dc9 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -3631,6 +3631,303 @@ func TestStateStore_Check_Snapshot(t *testing.T) { } } +func TestStateStore_ServiceDump(t *testing.T) { + s := testStateStore(t) + + type operation struct { + name string + modFn func(*testing.T) + allFired bool + kindFired bool + checkAll func(*testing.T, structs.CheckServiceNodes) + checkKind func(*testing.T, structs.CheckServiceNodes) + } + + sortDump := func(dump structs.CheckServiceNodes) { + sort.Slice(dump, func(i, j int) bool { + if dump[i].Node.Node < dump[j].Node.Node { + return true + } else if dump[i].Node.Node > dump[j].Node.Node { + return false + } + + if dump[i].Service.Service < dump[j].Service.Service { + return true + } else if dump[i].Service.Service > dump[j].Service.Service { + return false + } + + return false + }) + + for i := 0; i < len(dump); i++ { + sort.Slice(dump[i].Checks, func(m, n int) bool { + return dump[i].Checks[m].CheckID < dump[i].Checks[n].CheckID + }) + } + } + + operations := []operation{ + { + name: "register some nodes", + modFn: func(t *testing.T) { + testRegisterNode(t, s, 0, "node1") + testRegisterNode(t, s, 1, "node2") + }, + allFired: true, // fires due to "index" + kindFired: true, // fires due to "index" + checkAll: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 0) + }, + checkKind: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 0) + }, + }, + { + name: "register services against them", + modFn: func(t *testing.T) { + testRegisterService(t, s, 2, "node1", "service1") + testRegisterSidecarProxy(t, s, 3, "node1", "service1") + testRegisterService(t, s, 4, "node2", "service1") + testRegisterSidecarProxy(t, s, 5, "node2", "service1") + }, + allFired: true, // fires due to "index" + kindFired: true, // fires due to "index" + checkAll: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 4) + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node1", dump[1].Node.Node) + require.Equal(t, "node2", dump[2].Node.Node) + require.Equal(t, "node2", dump[3].Node.Node) + + require.Equal(t, "service1", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + require.Equal(t, "service1", dump[2].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[3].Service.Service) + + require.Len(t, dump[0].Checks, 0) + require.Len(t, dump[1].Checks, 0) + require.Len(t, dump[2].Checks, 0) + require.Len(t, dump[3].Checks, 0) + }, + checkKind: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 2) + + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node2", dump[1].Node.Node) + + require.Equal(t, "service1-sidecar-proxy", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + + require.Len(t, dump[0].Checks, 0) + require.Len(t, dump[1].Checks, 0) + }, + }, + { + name: "register service-level checks", + modFn: func(t *testing.T) { + testRegisterCheck(t, s, 6, "node1", "service1", "check1", api.HealthCritical) + testRegisterCheck(t, s, 7, "node2", "service1-sidecar-proxy", "check1", api.HealthCritical) + }, + allFired: true, // fires due to "index" + kindFired: true, // fires due to "index" + checkAll: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 4) + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node1", dump[1].Node.Node) + require.Equal(t, "node2", dump[2].Node.Node) + require.Equal(t, "node2", dump[3].Node.Node) + + require.Equal(t, "service1", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + require.Equal(t, "service1", dump[2].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[3].Service.Service) + + require.Len(t, dump[0].Checks, 1) + require.Len(t, dump[1].Checks, 0) + require.Len(t, dump[2].Checks, 0) + require.Len(t, dump[3].Checks, 1) + + require.Equal(t, api.HealthCritical, dump[0].Checks[0].Status) + require.Equal(t, api.HealthCritical, dump[3].Checks[0].Status) + }, + checkKind: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 2) + + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node2", dump[1].Node.Node) + + require.Equal(t, "service1-sidecar-proxy", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + + require.Len(t, dump[0].Checks, 0) + require.Len(t, dump[1].Checks, 1) + + require.Equal(t, api.HealthCritical, dump[1].Checks[0].Status) + }, + }, + { + name: "register node-level checks", + modFn: func(t *testing.T) { + testRegisterCheck(t, s, 8, "node1", "", "check2", api.HealthPassing) + testRegisterCheck(t, s, 9, "node2", "", "check2", api.HealthPassing) + }, + allFired: true, // fires due to "index" + kindFired: true, // fires due to "index" + checkAll: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 4) + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node1", dump[1].Node.Node) + require.Equal(t, "node2", dump[2].Node.Node) + require.Equal(t, "node2", dump[3].Node.Node) + + require.Equal(t, "service1", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + require.Equal(t, "service1", dump[2].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[3].Service.Service) + + require.Len(t, dump[0].Checks, 2) + require.Len(t, dump[1].Checks, 1) + require.Len(t, dump[2].Checks, 1) + require.Len(t, dump[3].Checks, 2) + + require.Equal(t, api.HealthCritical, dump[0].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[0].Checks[1].Status) + require.Equal(t, api.HealthPassing, dump[1].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[2].Checks[0].Status) + require.Equal(t, api.HealthCritical, dump[3].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[3].Checks[1].Status) + }, + checkKind: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 2) + + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node2", dump[1].Node.Node) + + require.Equal(t, "service1-sidecar-proxy", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + + require.Len(t, dump[0].Checks, 1) + require.Len(t, dump[1].Checks, 2) + + require.Equal(t, api.HealthPassing, dump[0].Checks[0].Status) + require.Equal(t, api.HealthCritical, dump[1].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[1].Checks[1].Status) + }, + }, + { + name: "pass a previously failing check", + modFn: func(t *testing.T) { + testRegisterCheck(t, s, 10, "node1", "service1", "check1", api.HealthPassing) + testRegisterCheck(t, s, 11, "node2", "service1-sidecar-proxy", "check1", api.HealthPassing) + }, + allFired: true, // fires due to "index" + kindFired: true, // fires due to "index" + checkAll: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 4) + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node1", dump[1].Node.Node) + require.Equal(t, "node2", dump[2].Node.Node) + require.Equal(t, "node2", dump[3].Node.Node) + + require.Equal(t, "service1", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + require.Equal(t, "service1", dump[2].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[3].Service.Service) + + require.Len(t, dump[0].Checks, 2) + require.Len(t, dump[1].Checks, 1) + require.Len(t, dump[2].Checks, 1) + require.Len(t, dump[3].Checks, 2) + + require.Equal(t, api.HealthPassing, dump[0].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[0].Checks[1].Status) + require.Equal(t, api.HealthPassing, dump[1].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[2].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[3].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[3].Checks[1].Status) + }, + checkKind: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 2) + + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node2", dump[1].Node.Node) + + require.Equal(t, "service1-sidecar-proxy", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + + require.Len(t, dump[0].Checks, 1) + require.Len(t, dump[1].Checks, 2) + + require.Equal(t, api.HealthPassing, dump[0].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[1].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[1].Checks[1].Status) + }, + }, + { + name: "delete a node", + modFn: func(t *testing.T) { + s.DeleteNode(12, "node2") + }, + allFired: true, // fires due to "index" + kindFired: true, // fires due to "index" + checkAll: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 2) + require.Equal(t, "node1", dump[0].Node.Node) + require.Equal(t, "node1", dump[1].Node.Node) + + require.Equal(t, "service1", dump[0].Service.Service) + require.Equal(t, "service1-sidecar-proxy", dump[1].Service.Service) + + require.Len(t, dump[0].Checks, 2) + require.Len(t, dump[1].Checks, 1) + + require.Equal(t, api.HealthPassing, dump[0].Checks[0].Status) + require.Equal(t, api.HealthPassing, dump[0].Checks[1].Status) + require.Equal(t, api.HealthPassing, dump[1].Checks[0].Status) + }, + checkKind: func(t *testing.T, dump structs.CheckServiceNodes) { + require.Len(t, dump, 1) + + require.Equal(t, "node1", dump[0].Node.Node) + + require.Equal(t, "service1-sidecar-proxy", dump[0].Service.Service) + + require.Len(t, dump[0].Checks, 1) + + require.Equal(t, api.HealthPassing, dump[0].Checks[0].Status) + }, + }, + } + for _, op := range operations { + op := op + require.True(t, t.Run(op.name, func(t *testing.T) { + wsAll := memdb.NewWatchSet() + _, _, err := s.ServiceDump(wsAll, "", false, nil) + require.NoError(t, err) + + wsKind := memdb.NewWatchSet() + _, _, err = s.ServiceDump(wsKind, structs.ServiceKindConnectProxy, true, nil) + require.NoError(t, err) + + op.modFn(t) + + require.Equal(t, op.allFired, watchFired(wsAll), "all dump watch firing busted") + require.Equal(t, op.kindFired, watchFired(wsKind), "kind dump watch firing busted") + + _, dump, err := s.ServiceDump(nil, "", false, nil) + require.NoError(t, err) + sortDump(dump) + op.checkAll(t, dump) + + _, dump, err = s.ServiceDump(nil, structs.ServiceKindConnectProxy, true, nil) + require.NoError(t, err) + sortDump(dump) + op.checkKind(t, dump) + })) + } +} + func TestStateStore_NodeInfo_NodeDump(t *testing.T) { s := testStateStore(t)