Register service in consul #802

Isonami · 2018-09-03T15:35:02Z

Implementation of #771, but a bit different, register service 'scope_name' with tag 'master' or 'replica'

example with scope 'pgsql-pgpi'

Using domain server:
Name: 127.0.0.1
Address: 127.0.0.1#53
Aliases:

pgsql-pgpi.service.consul has SRV record 1 1 5432 pgpi1.node.dc.consul.
pgsql-pgpi.service.consul has SRV record 1 1 5432 pgpi2.node.dc.consul.
[root@pgpi1 ~]# host -t SRV master.pgsql-pgpi.service.consul. 127.0.0.1
Using domain server:
Name: 127.0.0.1
Address: 127.0.0.1#53
Aliases:

master.pgsql-pgpi.service.consul has SRV record 1 1 5432 pgpi2.node.dc.consul.
[root@pgpi1 ~]# host -t SRV replica.pgsql-pgpi.service.consul. 127.0.0.1
Using domain server:
Name: 127.0.0.1
Address: 127.0.0.1#53
Aliases:

replica.pgsql-pgpi.service.consul has SRV record 1 1 5432 pgpi1.node.dc.consul.```

CyberDem0n

It looks like you forgot to take care about deregistering services. What about specifying the check with DeregisterCriticalServiceAfter?
And in case of demote I think it should explicitly call service.deregister

CyberDem0n · 2018-09-03T17:11:07Z

patroni/dcs/consul.py

+            'tags': [role]
+        }
+
+        if role in ['master', 'replica']:


I have couple of remarks here:

[WIP] Standby cluster implementation #679 introduces a new role, standby_leader, should we register is as a usual replica or create a special tag for that?

Does it make sense to avoid registering a replica which is not in the state "running"?

I think the best way is to create a special tag for this and make a configuration option to tag this nodes as replica additionally.

Yes, it make sense. Especially with DeregisterCriticalServiceAfter it can be deregister by consul and we won't register it again when state change to "running".

CyberDem0n · 2018-09-03T17:47:05Z

patroni/dcs/consul.py

+    @catch_consul_errors
+    def register_service(self, service_name, **kwargs):
+        logger.info('Register service {}, params {}'.format(service_name, kwargs))
+        return self.retry(self._client.agent.service.register, service_name, **kwargs)


One should be really careful with introducing new retries and timeouts, it is very important to make sure that one cycle of HA on the primary doesn't exceed ttl. So far Patroni does only two calls with such long retries: when it loads the cluster state (_load_cluster`) and when it refreshes the leader key (_update_leader).

In some cases it might become the 3rd place, but maybe it is fine.

if we would fail registration silently then we will never retry to register again before one of ['role', 'api_url', 'conn_url'] is changed. Otherwise is it ok to fail entire touch_member if registration is failed?

CyberDem0n · 2018-09-03T17:57:12Z

patroni/dcs/consul.py

            self._my_member_data = data
            return True
        except Exception:
            logger.exception('touch_member')
        return False

+    @catch_consul_errors
+    def register_service(self, service_name, **kwargs):
+        logger.info('Register service {}, params {}'.format(service_name, kwargs))


logging.info supports nice printf-like string formatting, please don't mix it with string.format.

Changed it in af74036

patroni/dcs/consul.py

Isonami · 2018-09-04T10:43:58Z

It looks like you forgot to take care about deregistering services. What about specifying the check with DeregisterCriticalServiceAfter?

Good point, but what will be reasonable time for such deregistration?

And in case of demote I think it should explicitly call service.deregister

Is delete_leader good place to put call for service.deregister? And i don't know code very deep, is there a possible race condition with register call from touch_member?

CyberDem0n · 2018-09-04T14:11:38Z

what will be reasonable time for such deregistration?

ttl * 5 should be good enough I think.

Is delete_leader good place to put call for service.deregister? And i don't know code very deep, is there a possible race condition with register call from touch_member?

I think it is better to stick with touch_member. All calls are serialized and you can also rely on start == 'stopped'

CyberDem0n · 2018-09-04T14:39:26Z

patroni/dcs/consul.py

+        api_parts = api_parts._replace(path='/{}'.format(role))
+        conn_parts = urlparse(data['conn_url'])
+        params = {
+            'address': conn_parts.hostname,


We should also add 'service_id': service_name_from_scope_name(self._name), to params, otherwise it is using service_name as a service_id, what makes impossible to run master and replica with the same agent.

i've added deregistration and service_id is now based on scope_name and name

register service only if state is running force service registration if `create_member` is True

CyberDem0n · 2018-09-06T06:51:21Z

patroni/dcs/consul.py

@@ -346,8 +346,7 @@ def touch_member(self, data, ttl=None, permanent=False):
            args = {} if permanent else {'acquire': self._session}
            self._client.kv.put(self.member_path, json.dumps(data, separators=(',', ':')), **args)
            if self._register_service:
-                self.update_service(self._my_member_data, data, force=create_member)
-            self._my_member_data = data
+                self.update_service(not create_member and member and member.data or {}, data)


I had to resolve the conflict after the merge of #805, because it made _my_member_data not available.
The force argument in the update_service is still available. I think it should be set to True during the next call if the last attempt to call register_service has failed.

update_service now save last result and set force to True if last result was False
And because of this I've removed retry from register_service

CyberDem0n · 2018-09-06T06:58:43Z

patroni/dcs/consul.py

        conn_parts = urlparse(data['conn_url'])
        check = base.Check.http(api_parts.geturl(), self._service_check_interval, deregister=self._client.http.ttl * 10)
        params = {
-            'service_id': '{}_{}'.format(self._scope, self._name),
+            'service_id': '{0}/{1}'.format(self._scope, self._name),


I've deliberately changed the delimiter from '_' to '/', because usage of underscore may lead to collisions in service_id's across different clusters.

keep last result and set force=True if last result was False removed retry in register_service

CyberDem0n · 2018-09-07T13:20:44Z

Merged, thank you!

Mention Consul related parameters introduced by #802

* fix some warnings when running unit-tests * allow python-kubernetes up to 10.0.1 * python-consul>=0.7.1 is required due to #802

register/update service in consul after updating kv values for node.

ae6e1db

CyberDem0n reviewed Sep 3, 2018

View reviewed changes

use logger string formating

af74036

CyberDem0n reviewed Sep 4, 2018

View reviewed changes

Isonami and others added 2 commits September 5, 2018 15:17

service deregistration

a280288

register service only if state is running force service registration if `create_member` is True

Merge branch 'master' into master

3916cb7

CyberDem0n reviewed Sep 6, 2018

View reviewed changes

return False only if consul request failed

606701e

keep last result and set force=True if last result was False removed retry in register_service

CyberDem0n requested a review from erthalion September 7, 2018 06:42

CyberDem0n approved these changes Sep 7, 2018

View reviewed changes

CyberDem0n merged commit 2e9cb41 into zalando:master Sep 7, 2018

CyberDem0n mentioned this pull request Nov 15, 2018

Patroni not register service to consul ? #865

Closed

kostiantyn-nemchenko mentioned this pull request Nov 17, 2018

Describe Consul registration parameters #870

Merged

CyberDem0n pushed a commit that referenced this pull request Nov 21, 2018

Describe Consul registration parameters (#870)

ce9c7bd

Mention Consul related parameters introduced by #802

CyberDem0n pushed a commit that referenced this pull request Oct 11, 2019

A few small fixes

93f793f

* fix some warnings when running unit-tests * allow python-kubernetes up to 10.0.1 * python-consul>=0.7.1 is required due to #802

CyberDem0n mentioned this pull request Oct 11, 2019

A few small fixes #1221

Merged

CyberDem0n added a commit that referenced this pull request Oct 11, 2019

A few small fixes (#1221)

21ed8e2

* fix some warnings when running unit-tests * allow python-kubernetes up to 10.0.1 * python-consul>=0.7.1 is required due to #802

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register service in consul #802

Register service in consul #802

Isonami commented Sep 3, 2018 •

edited

CyberDem0n left a comment

CyberDem0n Sep 3, 2018

Isonami Sep 4, 2018

CyberDem0n Sep 3, 2018

Isonami Sep 4, 2018

CyberDem0n Sep 3, 2018

Isonami Sep 4, 2018

Isonami commented Sep 4, 2018

CyberDem0n commented Sep 4, 2018

CyberDem0n Sep 4, 2018

Isonami Sep 5, 2018

CyberDem0n Sep 6, 2018

Isonami Sep 6, 2018

CyberDem0n Sep 6, 2018

CyberDem0n commented Sep 7, 2018

Register service in consul #802

Register service in consul #802

Conversation

Isonami commented Sep 3, 2018 • edited

CyberDem0n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isonami commented Sep 4, 2018

CyberDem0n commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyberDem0n commented Sep 7, 2018

Isonami commented Sep 3, 2018 •

edited