Don't deregister services and checks which are not managed by Nomad #568
Conversation
| @@ -31,7 +31,7 @@ func testAllocRunner(restarts bool) (*MockAllocStateUpdater, *AllocRunner) { | |||
| conf.AllocDir = os.TempDir() | |||
| upd := &MockAllocStateUpdater{} | |||
| alloc := mock.Alloc() | |||
| consulClient, _ := NewConsulService(logger, "127.0.0.1:8500", "", "", false, false) | |||
| consulClient, _ := NewConsulService(logger, "127.0.0.1:8500", "", "", false, false, &structs.Node{}) | |||
There was a problem hiding this comment.
It is worth looking into creating a ConsulConfig struct since the parameters of this constructor keep growing up.
There was a problem hiding this comment.
@c4milo Yeah makes sense.
|
Looks good to me overall @diptanu! Thanks for addressing this so quickly. |
| } | ||
|
|
||
| nomadServices := c.filterConsulServices(srvs) | ||
|
|
There was a problem hiding this comment.
Nitpick: extra line here
|
LGTM! |
| Address: "10.10.1.11", | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
The best practice is to create a new expected map and then use reflect.DeepEquals:
exp := map[string]*consul.AgentService{
"nomad-2121212": {
ID: "nomad-2121212",
Service: "identity-service",
Tags: []string{"global"},
Port: 8080,
Address: "10.10.1.11",
},
act := c.filterConsulServices(serves)
if !reflect.DeepEquals(act, exp) {
t.Fatalf(...)
}
}
diptanu
force-pushed
the
f-precise-consul-dereg
branch
from
e155f94 to
57ff1a3
Compare
Don't deregister services and checks which are not managed by Nomad
|
@diptanu, did this fix made it into 0.2.2? I'm still seeing the issue in 0.2.2 |
|
@c4milo yeah I had merged the PR, and you shouldn't see this in 0.2.2 anymore |
|
@c4milo Do you see the issue with master too? |
|
@c4milo I just used the 0.2.2 binary, added a service by running the command |
|
hm, I just tested using master branch and a service definition for a Nomad server and I'm still able to reproduce the issue. |
|
ah I think it is because I'm not assigning any ID to my service so Consul uses |
|
@c4milo Yeah we are using the nomad prefix to determine if Nomad manages the services and their corresponding checks. Please use something else for the ID. We should probably document this behavior. |
|
@diptanu, why not use an ID less likely to clash with users's IDs, such as a static random ID instead? |
|
@c4milo Sure we can. This just needs to be the same between Nomad client restarts. |
|
right, that's what I meant with static. On Sat, Dec 12, 2015 at 2:20 PM Diptanu Choudhury [email protected]
|
|
@diptanu it seems that the current behaviour is not documented. It should be mentioned that the |
|
@antoineco To make sure your issue is not lost since this PR is merged, please open a new issue. You can create a link to this thread for context. Thanks! |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #560 and #525