-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Allow to omit the RAFT port (#1759) #1774
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With raft port being deduced from the REST API port, the change of API port will implicitly change the pysyncobj identifier (host:port), what could create bring the cluster to an unrecoverable state, because on start Patroni checks whether this node is already known as a cluster member and if it is not - adds itself.
Example. We have a cluster from 10.0.0.1:18080,10.0.0.2:18080,10.0.0.3:18080
.
After the change of the REST API port on the first node to 8081, the raft address will become 10.0.0.1:18081
, and it will add itself to the cluster members. The list of members will become 10.0.0.1:18080,10.0.0.2:18080,10.0.0.3:18080,10.0.0.1:18081
. This configuration will stop working as soon as we stop the first node.
For me it looks like a footgun.
@@ -293,6 +296,19 @@ def __init__(self, config): | |||
self._sync_obj.forceLogCompaction() | |||
self.set_retry_timeout(int(config.get('retry_timeout') or 10)) | |||
|
|||
def _default_port(self, config): | |||
# Prepend '1' to the API port, i.e. use '18008' if the API port is '8008' | |||
port = '1' + config['restapi']['listen'].rsplit(':', 1)[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way it could go beyond 65535.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I made it return in that case in 89a6ae6. Do you think it should be more clever and start to remove leading digits in this?
port = '1' + config['restapi']['listen'].rsplit(':', 1)[1] | ||
n = 0 | ||
for addr in config['partner_addrs']: | ||
if ":" not in addr: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not work for IPv6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Will look at this later.
4a5df3a
to
89a6ae6
Compare
89a6ae6
to
3dea70b
Compare
If no ports are specified for self_addr, bind_addr and/or partner_addrs, then Patroni will self-assign the startup API port + 10000 as RAFT port.
3dea70b
to
355ca79
Compare
355ca79
to
98d8e34
Compare
If no ports are specified for self_addr, bind_addr and/or partner_addrs, then
Patroni will self-assign the startup API port + 10000 as RAFT port.