Do not manage events on non-consul apps #234
Conversation
events/event_handler.go
Outdated
| @@ -160,6 +161,18 @@ func (fh *eventHandler) handleStatusEvent(body []byte) error { | |||
| "TaskStatus": task.TaskStatus, | |||
| }).Info("Got StatusEvent") | |||
|
|
|||
| app, err := fh.marathon.App(task.AppID) | |||
There was a problem hiding this comment.
This operation is quite expensive and could fail. Why you want to cut non consul app on this level?
There was a problem hiding this comment.
Right now, when a "Killing" or "Failed" event happen, it try to deregister application even if it's not consul.
We have a lot of level=error msg="There was a problem deregistering task" Id={APP}.e41a91e6-3be7-11e7-bcb0-c4346bb6b3b0 error="Couldn't find any service matching task id {APP}.e41a91e6-3be7-11e7-bcb0-c4346bb6b3b0"
There was a problem hiding this comment.
I need to test it, this could double the traffic to Marathon for a deployment. Becouse new tasks will need to be registered and old will check if should be deregistered.
Instead of asking marathon I'll prefer to ask consul if it has specific task and then deregister it. Third solution is to check error and log if it's not error that means there is no such task. If we want to deregister task and it's already gone we are done and this is not an error.
There was a problem hiding this comment.
I checked the code and we already did it. IMO best to change error and check if this is real error. Again if we want to deregister a service that is not registered this is not a problem. The problem was before, why we haven't registered it if we should?
There was a problem hiding this comment.
problem is that we have task managed by tool mesos-consul when tag consul isn't present. and tasks with tag consul managed by marathon-consul (as a transition step).
We really don't want to see marathon-consul deregistering apps managed by mesos-consul.
There was a problem hiding this comment.
Marathon-consul sets different service ID then mesos-consul so it won't delete apps registered by mesos-consul.
There was a problem hiding this comment.
you're right :)
Maybe a change on log is a good trade-off.
Best will be to create an internal "cached" list of applications that we managed (sync periodically).
There was a problem hiding this comment.
I'm not sure if I understand. Caching applications/tasks that are manages could help with performance (less calls to Marathon and Consul) and can be first step to fix #170
This fix a lot of issues when dealing with apps not managed by marathon-consul
|
@janisz I dig really into code and don't find why tests are failing... |
|
I'm closing this PR because as seen in comments it's not the right way to do it. |
This fix a lot of issues when dealing with apps not managed by marathon-consul