Improvements to Consul nameresolver #2594
Conversation
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
ItalyPaleAle
changed the title
|
Moving this back to draft as I realized there's a better way to do this: https://developer.hashicorp.com/consul/api-docs/agent/check#deregistercriticalserviceafter The risk is that if we delete a service, we may delete multiple instances when we just need to delete one. I'll also test in a Load balanced scenario |
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
|
After further investigation, looks like the initial approach of unregistering the service won't work and can't be made to work. I'm leaving this PR with only the "ancillary" improvements to the component. |
| if err := r.client.Agent().ServiceRegister(r.config.Registration); err != nil { | ||
| agent := r.client.Agent() | ||
|
|
||
| err = agent.ServiceRegister(r.config.Registration) |
There was a problem hiding this comment.
I saw your comment about using deregistercriticalserviceafter - but that fix not working.
Should we rather deregister before registering?
Documentation for ServiceDeregister(serviceID string) error (https://developer.hashicorp.com/consul/commands/services/deregister) also recommends that it is meant to be paired with services register.
It may not completely solve the issue but would certainly be a good practice most probably and may also help in solving issues like #2489
There was a problem hiding this comment.
That was actually what I first tried doing, and it didn't work because a service can have multiple instances if the app is scaled horizontally. Calling "ServiceDeregister" removed the entire service and so un-registered other instances of the app too.
I spent 2 hours on this yesterday and doesn't really look like Consul has any way to remove only one instance :(
ItalyPaleAle
changed the title
Signed-off-by: ItalyPaleAle <[email protected]>
Description
This PR contains improvements to the Consul nameresolver, especially the
ResolveIDmethod which was more complex than it needed to be. In particular, the algorithm for picking a random entry from the list of results could be replaced with a much simpler one (complexity: O(n) to O(1), and removes N allocations on the heap), plus other small tweaks.Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: