Issue 3452 #3500
Issue 3452 #3500
Conversation
|
Still needs unit test |
…g of catalog nodes that have been removed from serf
preetapan
force-pushed
the
issue_3452
branch
from
18cdaee to
2a9e25d
Compare
agent/consul/leader.go
Outdated
| for _, service := range services.Services { | ||
| if service.ID == structs.ConsulServiceID { | ||
| serverPort = service.Port | ||
| _, node, err := state.GetNode(check.Node) | ||
| if err != nil { | ||
| s.logger.Printf("[ERR] Unable to look up node with name %v, got error:%v", check.Node, err) |
There was a problem hiding this comment.
Should be [ERR] consul: for the prefix, and we usually just do a colon before the error, so [ERR] consul: Unable to look up node with name %q: %v"
agent/consul/leader.go
Outdated
| for _, service := range services.Services { | ||
| if service.ID == structs.ConsulServiceID { | ||
| serverPort = service.Port | ||
| _, node, err := state.GetNode(check.Node) | ||
| if err != nil { | ||
| s.logger.Printf("[ERR] Unable to look up node with name %v, got error:%v", check.Node, err) |
There was a problem hiding this comment.
If we can't get an address we should probably just bail rather than let the net.ParseIP return something bogus below. I'd add a label and do a continue CHECKS here for the error.
agent/consul/leader_test.go
Outdated
| joinLAN(t, s1, s2) | ||
| joinLAN(t, s1, s3) | ||
|
|
||
| state := s1.fsm.State() |
There was a problem hiding this comment.
I'd move this down before the first retry.Run just since it seems out of place here.
agent/consul/leader_test.go
Outdated
| Status: api.HealthCritical, | ||
| }) | ||
|
|
||
| // call reconcileReaped with a map that does not contain s2 |
agent/consul/leader_test.go
Outdated
| } | ||
| }) | ||
|
|
||
| // Make a failed healthcheck for s2 |
There was a problem hiding this comment.
I don't think we need this health check to be registered.
agent/consul/leader_test.go
Outdated
| // Should be deregistered; we have to poll quickly here because | ||
| // anti-entropy will put it back. | ||
| reaped := false | ||
| for start := time.Now(); time.Since(start) < 5*time.Second; { |
There was a problem hiding this comment.
This might be a little flaky, but since AE is fairly slow this should be ok. I'd use retry.Run here though and have it fail with an error if the node is still registered, that'll simplify the test.
agent/consul/leader.go
Outdated
| for _, service := range services.Services { | ||
| if service.ID == structs.ConsulServiceID { | ||
| _, node, err := state.GetNode(check.Node) | ||
| if err != nil { | ||
| s.logger.Printf("[ERR] consul: Unable to look up node with name %q:%v", check.Node, err) |
There was a problem hiding this comment.
I'd add a space after the ":"
agent/consul/leader.go
Outdated
| if err != nil { | ||
| s.logger.Printf("[ERR] consul: Unable to look up node with name %q:%v", check.Node, err) | ||
| continue checks | ||
| } else { |
There was a problem hiding this comment.
You can drop the else :-)
Fixes #3452.