search

mantl / mesos-consul

Mesos to Consul bridge for service discovery
Apache License 2.0
338 stars 95 forks source link

Configurable consul endpoint address #60

Closed sashkachan closed 3 years ago

sashkachan commented 8 years ago

Adds address option to consul.

michaeljs1990 commented 8 years ago

Why is it not configurable to change the consul endpoint? Possibly i'm missing something in how this is suppose to work but it seems the way it runs right now every single docker contianer needs to run consul which seems needlessly excessive.

mattkanwisher commented 8 years ago

I was confused about this also? Is the assumption the Mesos server itself is running consul? Seems pretty safe to add this patch

ChrisAubuchon commented 8 years ago

Due to the design on Consul, every mesos agent needs to run consul. mesos-consul connects to a consul instance that is running on the same node as the service is running on. Specifying a central consul endpoint will result in services being registered on that endpoint instead of on the mesos agent as they should be.

mattkanwisher commented 8 years ago

So I installed consul agent on all my mesos nodes, however it appears this code tries to access consul agent on a public interface not on 127.0.0.1, so it won't find it. As we dont want that exposed on the network.

evilezh commented 8 years ago

+1

ernestas-poskus commented 8 years ago

:+1:

thomas-maurice commented 7 years ago

+1

kopinions commented 7 years ago

when it will be merged?

Theaxiom commented 7 years ago

@sjkyspa When the build completes successfully. 👍

Theaxiom commented 7 years ago

It also needs to be rebased to master first.

ChrisAubuchon commented 7 years ago

If you specify a consul agent, then all of the registered services will be assigned to that agent. So if you have a process running on say, mesos-worker-1, and register the service on consul-master-1, if consul-master-1 becomes unavailable then the service running on mesos-worker-1 goes critical. Which is clearly incorrect. I have specifically not merged this for that reason.