From e9a2f5b40c79913d4a582d8a24651717a20d2cbb Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Tue, 7 Jun 2016 15:24:51 -0500 Subject: [PATCH] Chase casting types.CheckID to a string into the state_store. It turns out the indexer can only use strings as arguments when creating a query. Cast `types.CheckID` to a `string` before calling into `memdb`. Ideally the indexer would be smart enough to do this at compile-time, but I need to look into how to do this without reflection and the runtime package. For the time being statically cast `types.CheckID` to a `string` at the call sites. --- command/agent/agent_endpoint_test.go | 19 +++++++------ command/agent/local.go | 12 ++++---- command/agent/session_endpoint.go | 3 +- command/agent/session_endpoint_test.go | 5 ++-- command/agent/structs.go | 2 +- command/maint.go | 2 +- consul/fsm_test.go | 3 +- consul/leader.go | 15 +++++----- consul/state/state_store.go | 18 ++++++------ consul/state/state_store_test.go | 16 +++++------ consul/structs/structs.go | 2 +- consul/structs/structs_test.go | 38 +++++++++++++++++++------- 12 files changed, 80 insertions(+), 55 deletions(-) diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index d11842fc1a..9b8cdd4783 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/consul/testutil" + "github.com/hashicorp/consul/types" "github.com/hashicorp/serf/serf" ) @@ -61,7 +62,7 @@ func TestHTTPAgentChecks(t *testing.T) { if err != nil { t.Fatalf("Err: %v", err) } - val := obj.(map[string]*structs.HealthCheck) + val := obj.(map[types.CheckID]*structs.HealthCheck) if len(val) != 1 { t.Fatalf("bad checks: %v", obj) } @@ -294,21 +295,22 @@ func TestHTTPAgentRegisterCheck(t *testing.T) { } // Ensure we have a check mapping - if _, ok := srv.agent.state.Checks()["test"]; !ok { + checkID := types.CheckID("test") + if _, ok := srv.agent.state.Checks()[checkID]; !ok { t.Fatalf("missing test check") } - if _, ok := srv.agent.checkTTLs["test"]; !ok { + if _, ok := srv.agent.checkTTLs[checkID]; !ok { t.Fatalf("missing test check ttl") } // Ensure the token was configured - if token := srv.agent.state.CheckToken("test"); token == "" { + if token := srv.agent.state.CheckToken(checkID); token == "" { t.Fatalf("missing token") } // By default, checks start in critical state. - state := srv.agent.state.Checks()["test"] + state := srv.agent.state.Checks()[checkID] if state.Status != structs.HealthCritical { t.Fatalf("bad: %v", state) } @@ -343,15 +345,16 @@ func TestHTTPAgentRegisterCheckPassing(t *testing.T) { } // Ensure we have a check mapping - if _, ok := srv.agent.state.Checks()["test"]; !ok { + checkID := types.CheckID("test") + if _, ok := srv.agent.state.Checks()[checkID]; !ok { t.Fatalf("missing test check") } - if _, ok := srv.agent.checkTTLs["test"]; !ok { + if _, ok := srv.agent.checkTTLs[checkID]; !ok { t.Fatalf("missing test check ttl") } - state := srv.agent.state.Checks()["test"] + state := srv.agent.state.Checks()[checkID] if state.Status != structs.HealthPassing { t.Fatalf("bad: %v", state) } diff --git a/command/agent/local.go b/command/agent/local.go index 66a05721c2..6c6ae1362d 100644 --- a/command/agent/local.go +++ b/command/agent/local.go @@ -192,17 +192,17 @@ func (l *localState) Services() map[string]*structs.NodeService { return services } -// CheckToken is used to return the configured health check token, or -// if none is configured, the default agent ACL token. -func (l *localState) CheckToken(id types.CheckID) string { +// CheckToken is used to return the configured health check token for a +// Check, or if none is configured, the default agent ACL token. +func (l *localState) CheckToken(checkID types.CheckID) string { l.RLock() defer l.RUnlock() - return l.checkToken(id) + return l.checkToken(checkID) } // checkToken returns an ACL token associated with a check. -func (l *localState) checkToken(id types.CheckID) string { - token := l.checkTokens[id] +func (l *localState) checkToken(checkID types.CheckID) string { + token := l.checkTokens[checkID] if token == "" { token = l.config.ACLToken } diff --git a/command/agent/session_endpoint.go b/command/agent/session_endpoint.go index 0f2685465f..4049cf7d6b 100644 --- a/command/agent/session_endpoint.go +++ b/command/agent/session_endpoint.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/consul/consul" "github.com/hashicorp/consul/consul/structs" + "github.com/hashicorp/consul/types" ) const ( @@ -38,7 +39,7 @@ func (s *HTTPServer) SessionCreate(resp http.ResponseWriter, req *http.Request) Op: structs.SessionCreate, Session: structs.Session{ Node: s.agent.config.NodeName, - Checks: []string{consul.SerfCheckID}, + Checks: []types.CheckID{consul.SerfCheckID}, LockDelay: 15 * time.Second, Behavior: structs.SessionKeysRelease, TTL: "", diff --git a/command/agent/session_endpoint_test.go b/command/agent/session_endpoint_test.go index c7f744c84a..d64f6b41eb 100644 --- a/command/agent/session_endpoint_test.go +++ b/command/agent/session_endpoint_test.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/consul/consul" "github.com/hashicorp/consul/consul/structs" + "github.com/hashicorp/consul/types" ) func TestSessionCreate(t *testing.T) { @@ -38,7 +39,7 @@ func TestSessionCreate(t *testing.T) { raw := map[string]interface{}{ "Name": "my-cool-session", "Node": srv.agent.config.NodeName, - "Checks": []string{consul.SerfCheckID, "consul"}, + "Checks": []types.CheckID{consul.SerfCheckID, "consul"}, "LockDelay": "20s", } enc.Encode(raw) @@ -86,7 +87,7 @@ func TestSessionCreateDelete(t *testing.T) { raw := map[string]interface{}{ "Name": "my-cool-session", "Node": srv.agent.config.NodeName, - "Checks": []string{consul.SerfCheckID, "consul"}, + "Checks": []types.CheckID{consul.SerfCheckID, "consul"}, "LockDelay": "20s", "Behavior": structs.SessionKeysDelete, } diff --git a/command/agent/structs.go b/command/agent/structs.go index 442b4b2965..a0f4c6bcb2 100644 --- a/command/agent/structs.go +++ b/command/agent/structs.go @@ -43,7 +43,7 @@ func (s *ServiceDefinition) CheckTypes() (checks CheckTypes) { return } -// ChecKDefinition is used to JSON decode the Check definitions +// CheckDefinition is used to JSON decode the Check definitions type CheckDefinition struct { ID types.CheckID Name string diff --git a/command/maint.go b/command/maint.go index e1319db526..0151465650 100644 --- a/command/maint.go +++ b/command/maint.go @@ -117,7 +117,7 @@ func (c *MaintCommand) Run(args []string) int { c.Ui.Output(" Name: " + nodeName) c.Ui.Output(" Reason: " + check.Notes) c.Ui.Output("") - } else if strings.HasPrefix(check.CheckID, "_service_maintenance:") { + } else if strings.HasPrefix(string(check.CheckID), "_service_maintenance:") { c.Ui.Output("Service:") c.Ui.Output(" ID: " + check.ServiceID) c.Ui.Output(" Reason: " + check.Notes) diff --git a/consul/fsm_test.go b/consul/fsm_test.go index 44c85e43e2..2f63fd89ac 100644 --- a/consul/fsm_test.go +++ b/consul/fsm_test.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/consul/consul/state" "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/types" "github.com/hashicorp/go-uuid" "github.com/hashicorp/raft" ) @@ -860,7 +861,7 @@ func TestFSM_SessionCreate_Destroy(t *testing.T) { Session: structs.Session{ ID: generateUUID(), Node: "foo", - Checks: []string{"web"}, + Checks: []types.CheckID{"web"}, }, } buf, err := structs.Encode(structs.SessionRequestType, req) diff --git a/consul/leader.go b/consul/leader.go index 8aa1f19948..790c3c330a 100644 --- a/consul/leader.go +++ b/consul/leader.go @@ -10,18 +10,19 @@ import ( "github.com/armon/go-metrics" "github.com/hashicorp/consul/consul/agent" "github.com/hashicorp/consul/consul/structs" + "github.com/hashicorp/consul/types" "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" ) const ( - SerfCheckID = "serfHealth" - SerfCheckName = "Serf Health Status" - SerfCheckAliveOutput = "Agent alive and reachable" - SerfCheckFailedOutput = "Agent not live or unreachable" - ConsulServiceID = "consul" - ConsulServiceName = "consul" - newLeaderEvent = "consul:new-leader" + SerfCheckID types.CheckID = "serfHealth" + SerfCheckName = "Serf Health Status" + SerfCheckAliveOutput = "Agent alive and reachable" + SerfCheckFailedOutput = "Agent not live or unreachable" + ConsulServiceID = "consul" + ConsulServiceName = "consul" + newLeaderEvent = "consul:new-leader" ) // monitorLeadership is used to monitor if we acquire or lose our role diff --git a/consul/state/state_store.go b/consul/state/state_store.go index bf5334a327..7c48a1feee 100644 --- a/consul/state/state_store.go +++ b/consul/state/state_store.go @@ -83,7 +83,7 @@ type IndexEntry struct { // store and thus it is not exported. type sessionCheck struct { Node string - CheckID string + CheckID types.CheckID Session string } @@ -969,7 +969,7 @@ func (s *StateStore) EnsureCheck(idx uint64, hc *structs.HealthCheck) error { func (s *StateStore) ensureCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatchManager, hc *structs.HealthCheck) error { // Check if we have an existing health check - existing, err := tx.First("checks", "id", hc.Node, hc.CheckID) + existing, err := tx.First("checks", "id", hc.Node, string(hc.CheckID)) if err != nil { return fmt.Errorf("failed health check lookup: %s", err) } @@ -1014,7 +1014,7 @@ func (s *StateStore) ensureCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatc // Delete any sessions for this check if the health is critical. if hc.Status == structs.HealthCritical { - mappings, err := tx.Get("session_checks", "node_check", hc.Node, hc.CheckID) + mappings, err := tx.Get("session_checks", "node_check", hc.Node, string(hc.CheckID)) if err != nil { return fmt.Errorf("failed session checks lookup: %s", err) } @@ -1120,13 +1120,13 @@ func (s *StateStore) parseChecks(idx uint64, iter memdb.ResultIterator) (uint64, } // DeleteCheck is used to delete a health check registration. -func (s *StateStore) DeleteCheck(idx uint64, node string, id types.CheckID) error { +func (s *StateStore) DeleteCheck(idx uint64, node string, checkID types.CheckID) error { tx := s.db.Txn(true) defer tx.Abort() // Call the check deletion watches := NewDumbWatchManager(s.tableWatches) - if err := s.deleteCheckTxn(tx, idx, watches, node, id); err != nil { + if err := s.deleteCheckTxn(tx, idx, watches, node, checkID); err != nil { return err } @@ -1137,9 +1137,9 @@ func (s *StateStore) DeleteCheck(idx uint64, node string, id types.CheckID) erro // deleteCheckTxn is the inner method used to call a health // check deletion within an existing transaction. -func (s *StateStore) deleteCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatchManager, node string, id types.CheckID) error { +func (s *StateStore) deleteCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatchManager, node string, checkID types.CheckID) error { // Try to retrieve the existing health check. - hc, err := tx.First("checks", "id", node, id) + hc, err := tx.First("checks", "id", node, string(checkID)) if err != nil { return fmt.Errorf("check lookup failed: %s", err) } @@ -1156,7 +1156,7 @@ func (s *StateStore) deleteCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatc } // Delete any sessions for this check. - mappings, err := tx.Get("session_checks", "node_check", node, id) + mappings, err := tx.Get("session_checks", "node_check", node, string(checkID)) if err != nil { return fmt.Errorf("failed session checks lookup: %s", err) } @@ -1413,7 +1413,7 @@ func (s *StateStore) sessionCreateTxn(tx *memdb.Txn, idx uint64, sess *structs.S // Go over the session checks and ensure they exist. for _, checkID := range sess.Checks { - check, err := tx.First("checks", "id", sess.Node, checkID) + check, err := tx.First("checks", "id", sess.Node, string(checkID)) if err != nil { return fmt.Errorf("failed check lookup: %s", err) } diff --git a/consul/state/state_store_test.go b/consul/state/state_store_test.go index 99ee6c7b54..246dfbc80b 100644 --- a/consul/state/state_store_test.go +++ b/consul/state/state_store_test.go @@ -96,7 +96,7 @@ func testRegisterCheck(t *testing.T, s *StateStore, idx uint64, tx := s.db.Txn(false) defer tx.Abort() - c, err := tx.First("checks", "id", nodeID, checkID) + c, err := tx.First("checks", "id", nodeID, string(checkID)) if err != nil { t.Fatalf("err: %s", err) } @@ -2151,7 +2151,7 @@ func TestStateStore_SessionCreate_SessionGet(t *testing.T) { sess = &structs.Session{ ID: testUUID(), Node: "node1", - Checks: []string{"check1"}, + Checks: []types.CheckID{"check1"}, } err = s.SessionCreate(3, sess) if err == nil || !strings.Contains(err.Error(), "Missing check") { @@ -2176,7 +2176,7 @@ func TestStateStore_SessionCreate_SessionGet(t *testing.T) { sess2 := &structs.Session{ ID: testUUID(), Node: "node1", - Checks: []string{"check1", "check2"}, + Checks: []types.CheckID{"check1", "check2"}, } if err := s.SessionCreate(6, sess2); err != nil { t.Fatalf("err: %s", err) @@ -2210,7 +2210,7 @@ func TestStateStore_SessionCreate_SessionGet(t *testing.T) { for i, check := 0, checks.Next(); check != nil; i, check = i+1, checks.Next() { expectCheck := &sessionCheck{ Node: "node1", - CheckID: fmt.Sprintf("check%d", i+1), + CheckID: types.CheckID(fmt.Sprintf("check%d", i+1)), Session: sess2.ID, } if actual := check.(*sessionCheck); !reflect.DeepEqual(actual, expectCheck) { @@ -2408,7 +2408,7 @@ func TestStateStore_Session_Snapshot_Restore(t *testing.T) { ID: session1, Node: "node1", Behavior: structs.SessionKeysDelete, - Checks: []string{"check1"}, + Checks: []types.CheckID{"check1"}, }, &structs.Session{ ID: testUUID(), @@ -2625,7 +2625,7 @@ func TestStateStore_Session_Invalidate_DeleteService(t *testing.T) { session := &structs.Session{ ID: testUUID(), Node: "foo", - Checks: []string{"api"}, + Checks: []types.CheckID{"api"}, } if err := s.SessionCreate(14, session); err != nil { t.Fatalf("err: %v", err) @@ -2673,7 +2673,7 @@ func TestStateStore_Session_Invalidate_Critical_Check(t *testing.T) { session := &structs.Session{ ID: testUUID(), Node: "foo", - Checks: []string{"bar"}, + Checks: []types.CheckID{"bar"}, } if err := s.SessionCreate(14, session); err != nil { t.Fatalf("err: %v", err) @@ -2720,7 +2720,7 @@ func TestStateStore_Session_Invalidate_DeleteCheck(t *testing.T) { session := &structs.Session{ ID: testUUID(), Node: "foo", - Checks: []string{"bar"}, + Checks: []types.CheckID{"bar"}, } if err := s.SessionCreate(14, session); err != nil { t.Fatalf("err: %v", err) diff --git a/consul/structs/structs.go b/consul/structs/structs.go index c69db8ea26..4f99399b38 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -618,7 +618,7 @@ type Session struct { ID string Name string Node string - Checks []string + Checks []types.CheckID LockDelay time.Duration Behavior SessionBehavior // What to do when session is invalidated TTL string diff --git a/consul/structs/structs_test.go b/consul/structs/structs_test.go index 412b7f44b7..1aa84b3af3 100644 --- a/consul/structs/structs_test.go +++ b/consul/structs/structs_test.go @@ -5,6 +5,8 @@ import ( "reflect" "strings" "testing" + + "github.com/hashicorp/consul/types" ) func TestEncodeDecode(t *testing.T) { @@ -184,7 +186,7 @@ func TestStructs_HealthCheck_IsSame(t *testing.T) { t.Fatalf("should not care about Raft fields") } - check := func(field *string) { + checkCheckIDField := func(field *types.CheckID) { if !hc.IsSame(other) || !other.IsSame(hc) { t.Fatalf("should be the same") } @@ -201,15 +203,31 @@ func TestStructs_HealthCheck_IsSame(t *testing.T) { } } - check(&other.Node) - checkStr := string(other.CheckID) - check(&checkStr) - check(&other.Name) - check(&other.Status) - check(&other.Notes) - check(&other.Output) - check(&other.ServiceID) - check(&other.ServiceName) + checkStringField := func(field *string) { + if !hc.IsSame(other) || !other.IsSame(hc) { + t.Fatalf("should be the same") + } + + old := *field + *field = "XXX" + if hc.IsSame(other) || other.IsSame(hc) { + t.Fatalf("should not be the same") + } + *field = old + + if !hc.IsSame(other) || !other.IsSame(hc) { + t.Fatalf("should be the same") + } + } + + checkStringField(&other.Node) + checkCheckIDField(&other.CheckID) + checkStringField(&other.Name) + checkStringField(&other.Status) + checkStringField(&other.Notes) + checkStringField(&other.Output) + checkStringField(&other.ServiceID) + checkStringField(&other.ServiceName) } func TestStructs_HealthCheck_Clone(t *testing.T) {