From 7c44a9b6c932e292f17ebaf0c5df6e02c9eb2ca0 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 28 Oct 2015 12:40:47 -0700 Subject: [PATCH 1/2] Fixes a bad error message. --- consul/state/state_store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consul/state/state_store.go b/consul/state/state_store.go index 36a61ec65..c3393c68a 100644 --- a/consul/state/state_store.go +++ b/consul/state/state_store.go @@ -1079,7 +1079,7 @@ func (s *StateStore) ensureCheckTxn(tx *memdb.Txn, idx uint64, watches *DumbWatc // Persist the check registration in the db. if err := tx.Insert("checks", hc); err != nil { - return fmt.Errorf("failed inserting service: %s", err) + return fmt.Errorf("failed inserting check: %s", err) } if err := tx.Insert("index", &IndexEntry{"checks", idx}); err != nil { return fmt.Errorf("failed updating index: %s", err) From eb4bfa3411ae2d783c64e5698810d47cca51848c Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 28 Oct 2015 14:32:00 -0700 Subject: [PATCH 2/2] Prevents agents from considering Raft information when doing sync checks. --- command/agent/local.go | 7 +- consul/structs/structs.go | 38 +++++++++++ consul/structs/structs_test.go | 114 +++++++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 4 deletions(-) diff --git a/command/agent/local.go b/command/agent/local.go index 1bd86395a..706cd20e0 100644 --- a/command/agent/local.go +++ b/command/agent/local.go @@ -3,7 +3,6 @@ package agent import ( "fmt" "log" - "reflect" "strings" "sync" "sync/atomic" @@ -385,7 +384,7 @@ func (l *localState) setSyncState() error { if existing.EnableTagOverride { existing.Tags = service.Tags } - equal := reflect.DeepEqual(existing, service) + equal := existing.IsSame(service) l.serviceStatus[id] = syncStatus{inSync: equal} } @@ -419,13 +418,13 @@ func (l *localState) setSyncState() error { // If our definition is different, we need to update it var equal bool if l.config.CheckUpdateInterval == 0 { - equal = reflect.DeepEqual(existing, check) + equal = existing.IsSame(check) } else { eCopy := new(structs.HealthCheck) *eCopy = *existing eCopy.Output = "" check.Output = "" - equal = reflect.DeepEqual(eCopy, check) + equal = eCopy.IsSame(check) } // Update the status diff --git a/consul/structs/structs.go b/consul/structs/structs.go index a3a9a8315..72ecee00b 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -3,6 +3,7 @@ package structs import ( "bytes" "fmt" + "reflect" "time" "github.com/hashicorp/consul/acl" @@ -318,6 +319,23 @@ type NodeService struct { RaftIndex } +// IsSame checks if one NodeService is the same as another, without looking +// at the Raft information (that's why we didn't call it IsEqual). This is +// useful for seeing if an update would be idempotent for all the functional +// parts of the structure. +func (s *NodeService) IsSame(other *NodeService) bool { + if s.ID != other.ID || + s.Service != other.Service || + !reflect.DeepEqual(s.Tags, other.Tags) || + s.Address != other.Address || + s.Port != other.Port || + s.EnableTagOverride != other.EnableTagOverride { + return false + } + + return true +} + // ToServiceNode converts the given node service to a service node. func (s *NodeService) ToServiceNode(node, address string) *ServiceNode { return &ServiceNode{ @@ -354,6 +372,26 @@ type HealthCheck struct { RaftIndex } + +// IsSame checks if one HealthCheck is the same as another, without looking +// at the Raft information (that's why we didn't call it IsEqual). This is +// useful for seeing if an update would be idempotent for all the functional +// parts of the structure. +func (c *HealthCheck) IsSame(other *HealthCheck) bool { + if c.Node != other.Node || + c.CheckID != other.CheckID || + c.Name != other.Name || + c.Status != other.Status || + c.Notes != other.Notes || + c.Output != other.Output || + c.ServiceID != other.ServiceID || + c.ServiceName != other.ServiceName { + return false + } + + return true +} + type HealthChecks []*HealthCheck // CheckServiceNode is used to provide the node, its service diff --git a/consul/structs/structs_test.go b/consul/structs/structs_test.go index a89123efd..30c1e8983 100644 --- a/consul/structs/structs_test.go +++ b/consul/structs/structs_test.go @@ -95,6 +95,120 @@ func TestStructs_ServiceNode_Conversions(t *testing.T) { } } +func TestStructs_NodeService_IsSame(t *testing.T) { + ns := &NodeService{ + ID: "node1", + Service: "theservice", + Tags: []string{"foo", "bar"}, + Address: "127.0.0.1", + Port: 1234, + EnableTagOverride: true, + } + if !ns.IsSame(ns) { + t.Fatalf("should be equal to itself") + } + + other := &NodeService{ + ID: "node1", + Service: "theservice", + Tags: []string{"foo", "bar"}, + Address: "127.0.0.1", + Port: 1234, + EnableTagOverride: true, + RaftIndex: RaftIndex{ + CreateIndex: 1, + ModifyIndex: 2, + }, + } + if !ns.IsSame(other) || !other.IsSame(ns) { + t.Fatalf("should not care about Raft fields") + } + + check := func(twiddle, restore func()) { + if !ns.IsSame(other) || !other.IsSame(ns) { + t.Fatalf("should be the same") + } + + twiddle() + if ns.IsSame(other) || other.IsSame(ns) { + t.Fatalf("should not be the same") + } + + restore() + if !ns.IsSame(other) || !other.IsSame(ns) { + t.Fatalf("should be the same") + } + } + + check(func() { other.ID = "XXX" }, func() { other.ID = "node1" }) + check(func() { other.Service = "XXX" }, func() { other.Service = "theservice" }) + check(func() { other.Tags = nil }, func() { other.Tags = []string{"foo", "bar"} }) + check(func() { other.Tags = []string{"foo"} }, func() { other.Tags = []string{"foo", "bar"} }) + check(func() { other.Address = "XXX" }, func() { other.Address = "127.0.0.1" }) + check(func() { other.Port = 9999 }, func() { other.Port = 1234 }) + check(func() { other.EnableTagOverride = false }, func() { other.EnableTagOverride = true }) +} + +func TestStructs_HealthCheck_IsSame(t *testing.T) { + hc := &HealthCheck{ + Node: "node1", + CheckID: "check1", + Name: "thecheck", + Status: HealthPassing, + Notes: "it's all good", + Output: "lgtm", + ServiceID: "service1", + ServiceName: "theservice", + } + if !hc.IsSame(hc) { + t.Fatalf("should be equal to itself") + } + + other := &HealthCheck{ + Node: "node1", + CheckID: "check1", + Name: "thecheck", + Status: HealthPassing, + Notes: "it's all good", + Output: "lgtm", + ServiceID: "service1", + ServiceName: "theservice", + RaftIndex: RaftIndex{ + CreateIndex: 1, + ModifyIndex: 2, + }, + } + if !hc.IsSame(other) || !other.IsSame(hc) { + t.Fatalf("should not care about Raft fields") + } + + check := 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") + } + } + + check(&other.Node) + check(&other.CheckID) + check(&other.Name) + check(&other.Status) + check(&other.Notes) + check(&other.Output) + check(&other.ServiceID) + check(&other.ServiceName) +} + func TestStructs_DirEntry_Clone(t *testing.T) { e := &DirEntry{ LockIndex: 5,