Patroni: Raft DCS does not work behind NAT

[aaliddell]

Describe the bug
The new Raft DCS does not support running in a network layout that contains NAT, such as Docker. This is because the self address needs to be the externally visible address, but this is not bindable within the container. The underlying PySyncObj library supports a separate bind address option to support this: https://github.com/bakwc/PySyncObj/blob/afae3288094fc4e4b7b1e1eb39e9fed07c995cef/pysyncobj/config.py#L66-L70

As an addition to this: there is no error in the logs when Patroni fails to bind the local port.

To Reproduce
Run with NAT between nodes when using Raft DCS.

Expected behavior

1. There should be a mechanism for specifying the bind address separately from the self address. e.g. PATRONI_RAFT_BIND_ADDR
2. If the provided self address is unbindable, an error should be logged and Patroni should exit.
3. patronictl show-config fails when in this state, see below.

Environment

• Patroni version: 2.0.0
• PostgreSQL version: 12
• DCS (and its version): Raft

patronictl show-config

Traceback (most recent call last):
  File "/usr/local/bin/patronictl", line 10, in <module>
    sys.exit(ctl())
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/usr/local/lib/python3.7/dist-packages/patroni/ctl.py", line 1270, in show_config
    cluster = get_dcs(obj, cluster_name).get_cluster()
  File "/usr/local/lib/python3.7/dist-packages/patroni/ctl.py", line 156, in get_dcs
    return _get_dcs(config)
  File "/usr/local/lib/python3.7/dist-packages/patroni/dcs/__init__.py", line 92, in get_dcs
    return item(config[name])
  File "/usr/local/lib/python3.7/dist-packages/patroni/dcs/raft.py", line 269, in __init__
    super(Raft, self).__init__(config)
  File "/usr/local/lib/python3.7/dist-packages/patroni/dcs/__init__.py", line 570, in __init__
    self._base_path = re.sub('/+', '/', '/'.join(['', config.get('namespace', 'service'), config['scope']]))
TypeError: sequence item 2: expected str instance, NoneType found

Have you checked Patroni logs?
Only message is this one repeated:

2020-09-18 22:08:01,287 INFO: waiting on raft

Have you checked PostgreSQL logs?
Postgres does not start due to waiting for DCS

Have you tried to use GitHub issue search?
Yes

[aaliddell]

Looking into the code in raft.py, there is a frankly frightening amount of monkey-patching of PySyncObj underscored properties/methods going on, to the point I'd be concerned about the guarantees the underlying consensus is actually still able to provide. Given that underscored methods are not intended to be used outside of their own class/library, this code is liable to break when PySyncObj make any changes to their internal code layout and has no guarantees that things won't subtly break prior to that. Since requirements.txt only specifies pysyncobj>=0.3.5, a version 0.4.0 or 1.0.0 that might make these such internal changes would be picked up by patroni users.

There are also a few locations that appear to send raw messages, which has additionally coupled this code to the PySyncObj internal message format/API, which again may shift beneath you at any time...

[CyberDem0n]

You are making some conclusions without analyzing all the code.

• First of all, interfaces of PySyncObj are quite far from ideal. For example, they don't provide a way to get values of the __raftLastApplied and __raftCurrentTerm.
• Second, there is a way to get the current topology with the help of syncobj_admin utility, I could have used the same protocol, but the server sends a response in a very cumbersome format, therefore it was much easier to override a few methods and implement my own utility message members.
• Implementing the second point requires access to the __connectedNodes and __otherNodes. This is not my fault that the library doesn't provide a sane interface for accessing them
• The PySyncObj still doesn't provide a way to automatically join a new node to the cluster, but it is very nice to have, therefore it required sending one more utility message (see below).
• There are only two "raw" message sent and they don't even interfere with RAFT protocol, because these are members and add utility messages.

The Patroni on RAFT was born nearly 5 years ago as a hackweek project. Nowdays I would have made it a bit differently (contribute some changes to PySyncObj), but back than I simply didn't have had enought time for it.
Actually, a couple of days from that week I spent on implementing python3 suppor in PySyncObj...

[alexeymyltsev]

@CyberDem0n Am I to understand that RAFT will be supported as stable DSC in future?
We just start to try patroni and would like to use RAFT as main DCS(not needed maintain consul/etcd/..). We understand that for this moment it is beta, but we can start work with this beta and provide feedback for improvement it. But would be pity spend a lot of time for improvement feature which not in plans.

[CyberDem0n]

@alexeymyltsev without intention to make it stable I would not have merged it to the master and released.

[aaliddell]

The Patroni on RAFT was born nearly 5 years ago as a hackweek project. Nowdays I would have made it a bit differently (contribute some changes to PySyncObj), but back than I simply didn't have had enought time for it.

Ok that is understandable, but the issues raised still stand that using internal APIs is setting yourself up for either subtle or major bugs in the future at the whims on the maintainers of PySyncObj. Either support for the necessary APIs should be upstreamed into PySyncObj or perhaps PySyncObj is not the best library for implementing this DCS?

I don't intend to come across as ungrateful, this really is a great addition for the reasons @alexeymyltsev raised and I'm keen to see it in-use. Just it'd be good to see it setup in a maintainable and stable way that doesn't use private APIs, whilst it is still in a beta state where major changes can be made.

[CyberDem0n]

Either support for the necessary APIs should be upstreamed into PySyncObj or perhaps PySyncObj is not the best library for implementing this DCS?

Do you know a better alternative? Others https://raft.github.io/#implementations seems either missing some important features (like dynamic membership change) or not actively maintained. I totally agree that the necessary API should be implemented in the PySyncObj, but right now I don't really have time for it.

[aaliddell]

Unfortunately the alternatives all indeed appear to be quite poorly maintained, so it would seem that upstreaming would be the most viable solution. Perhaps that intention to remove private API use should be tracked as an alternate issue, since this issue was originally about the NAT and port binding issues that are more immediately present.

[bakwc]

If you need any API that is currently missing in PySyncObj - feel free to create issue and describe what you need, if it's not something huge I can add it myself. I think it's a good idea to avoid accessing private methods and move to more robust integration.

[CyberDem0n]

Thanks @bakwc, I will create a few issues (or maybe even PRs) when will have time for it.