rickfast / consul-client

Java Client for Consul HTTP API
Other
571 stars 240 forks source link

`AgentClient.pass()` always throws `com.orbitz.consul.NotRegisteredException` #201

Open guss77 opened 7 years ago

guss77 commented 7 years ago

I've implemented the service registration and check-in exactly as per the examples:

consul.agentClient().register(8080, 300, "test-client", id);
consul.agentClient().pass(id);

But I keep getting a com.orbitz.consul.NotRegisteredException exception.

tracing through the code, I can see that the error from Consul is Consul request failed with status [500]: CheckID "service:id-RAtstC2HnE5DOg" does not have associated TTL.

Looking at the Consul UI, I can see that there is a "test-client" service with a check, it looks like this:

spectacle t15103

I tried an alternative registration with a TTL (actually, just with a TTL) using a Registration object that I construct like this:

ImmutableRegistration.builder()
            .name("test-client")
            .id(id)
            .addChecks(RegCheck.ttl(300))
            .build();

And that results in exactly the same result.

Is it possible I'm missing something obvious?

alugowski commented 7 years ago

I'm experiencing the same problem.

I think it has to do with having multiple checks. pass() constructs the check ID to be "service:" + serviceId, but if you have multiple checks then Consul also appends :1 or :2 to the check. Therefore pass() ends up checking in to a check that doesn't exist.

I have no workaround other than registering only the dead man switch, and even then I've had Consul sometimes append a :1 and other times not.

alugowski commented 7 years ago

I found a workaround.

RegCheck, the class used to bundle check registration with service registration, doesn't allow you to specify IDs, and for whatever reason sometimes it auto selects an unworkable ID. If you include your dead man switch registration here then you're at the mercy of Consul choosing the ID that pass() expects.

Check is a different but nearly identical class that you use to register checks after service registration. This class does let you specify id() and name().

The workaround is to not register any checks along with service registration, and instead register the TTL check manually, specifying id to be "service": + serviceid to match what pass() will use.

// register service
Registration registration = ImmutableRegistration.builder()
        .port(myPort)
        .name(myServerType)
        .id(myServerId)
        .build();
agentClient.register(registration);

// register dead man switch
String deadManId = "service:" + myServerId;
agentClient.registerCheck(deadManId, deadManId, 60, "Dead Man Switch");

Now agentClient.pass(myServerId) will work.

You can also register your other checks using Check, and since you can control the IDs you can make sure they don't clash.

Note you may wish to clear all checks before registering new ones. This way if you abandon a check it won't keep hanging around requiring manual intervention:

agentClient.getChecks().forEach((checkId, check) -> agentClient.deregisterCheck(checkId));
alugowski commented 7 years ago

@rickfast and Orbitz team:

I think it makes sense for the check specified in registration to have a forced id of "service:" + serviceId to make sure pass() works properly. checks can be left alone.

ltamrazov commented 7 years ago

+1 on this issue

rickfast commented 7 years ago

@alugowski there is no more orbitz team. i maintain this when i get time. unfortunately, i don't have time this week to look into this. if you make a PR i will be happy to merge it and cut a release for you

alugowski commented 7 years ago

@rickfast I see, no worries. This fix would only be for future users. Registering checks separately works better for my use case anyway, so I'm not blocked.

I'm not sure where the best place to implement the check id would be. RegCheck doesn't have an Id field at all. I guess that field would have to be added, then right before registration if the Id is null it's set to "service:" + serviceId? I'm not sure I can work on this either, though I may have other contributions if I can get them to work.

SuppieRK commented 6 years ago

I would like to raise this issue up by adding the fact that the type of the request now is different, instead of GET now there is POST: http: Request GET /v1/agent/check/pass/, error: method GET not allowed

alugowski commented 6 years ago

@SuppieRK that is a separate issue that warrants its own ticket. This ticket is about a name mismatch only.

nh2 commented 6 years ago

instead of GET now there is POST

@SuppieRK I don't think that's accurate: It's a PUT.