Skip to content
This repository has been archived by the owner on Mar 4, 2024. It is now read-only.

Add support for hostnames instead of static IP addresses #298

Closed
NorbertHeusser opened this issue Aug 16, 2022 · 10 comments · Fixed by #309
Closed

Add support for hostnames instead of static IP addresses #298

NorbertHeusser opened this issue Aug 16, 2022 · 10 comments · Fixed by #309

Comments

@NorbertHeusser
Copy link
Contributor

We would like to use the raft library in the context of a docker swarm runtime environment. In contrast to traditional docker runtime it's not possible to assign internal static IP addresses to the individual cluster members. Static IP assignment not working seems to be an open issue in docker swarm now for several year so I don't expect it to be get fixed soon.

I tried to implement the DNS lookup in our application layer and keep the raft library config on IP addresses. But this has several caveats:

  • The name service in a docker swarm environment for an instance is added once the instance has started. So performing the name resolution to early will fail.
  • If the instance gets moved to a different worker node the dynamic IP address of this instance may changed and I cannot change it in the raft config.

I guess in general it will make sense to support hostnames in the raft config instead of static IPs. And I already start to add the necessary code extension to provide a pull request. But I would need some design guidance how you would prefer the solution to be later on accepted into you code base.

The base problem about adding host name resolving is the DNS request will add an additional and potentially blocking call to the TCP connection handling. The libuv used in your integration implementation provides an async API to perform the name resolving asynchronously. In general I currently see to code paths, where a name resolve would need to happen:

  1. In the outgoing TCP connection to the other cluster members. This code path is already implemented asynchronous and I would need to add an additional asynchronous step before the connect itself.
    2.Before the bind of the listen port for the address (or the bind_address). The current code path does not include any asynchronous steps for the bind/list calls. So in this case I would implement the name resolving in a traditional potential timeout blocking getaddrinfo base call. Adding the name resolving as an asynchronous step would need some general design changes in the bind/listen handling.

The design questions, which are currently open to me are:

  • Are you fine adding a synchronous and potentially timeout blocking name resolving to the bind/listen socket implementation code path ?
  • As the name resolving will add an addition asynchronous step to the outgoing tcp connection I may encapsulate it totally insite the uv_tcp_listen.c implementation. This way I would need to add an additional internal state flag telling me wether the connection is still in name resolving or already trying to connect. Or I would need to add a new queue into the uv_tcp.h/.c (e.g. name_resolving) and a single tcp-connection would be added to this queue first and moved to the connecting queue once the name resolving is finished. I personally would prefer the first way encapsulating it inside the uv_tcp_connect, but would respect your decision, if you prefer an additional queue to be more transparent.
@freeekanayaka
Copy link
Contributor

Thanks for looking at this! It'd be a nice feature to have.

Regarding implementation:

  1. For the listening part, I think it's fine to implement it in a blocking way, because that part of code is run during startup and actually the UV loop shouldn't be running at all yet. The sequence of event of a node start-up is the one you can see in example/server.c, where first raft_start() is called (which internally ends up calling UvTcpListen) and then uv_run(). Whatever happens in raft_start() is supposed to be blocking and not use the event loop, so you won't be able to use the DNS utilities from libuv unless you start a separate event loop in a separate thread and then use uv_async_t to have a blocking call from UvTcpListen() that waits for the job to be done. This approach has the advantage of being cross-platform, but maybe you'll want to just use a non-portable gethostbyname() call for simplicity, since we aren't really cross-platform anyway yet.

  2. For the connecting part, I believe no extra queue or state flag is needed. Basically the "state" you are in is encoded in the callback sequence itself. What you have to do is that when the resolve callback is fired you need to check the t->closing flag (which basically means that the raft engine is shutting down after a raft_close() call) and the status return value of the async libuv request you've done, in both those cases you'll want to "bail out". See uvTcpConnectUvConnectCb and uvTcpConnectUvWriteCb and just copy the code flow you find there and you should be good.

PS: don't forget to sign the contribution agreement before submitting a PR.

@NorbertHeusser
Copy link
Contributor Author

NorbertHeusser commented Aug 29, 2022

Created a first pull request now, which adds just the required additional async getaddrinfo for the outgoing connections.
#301 Right now restricted to static IP address as before. Next steps will follow soon.
But this additional step required some changes to the connect abort implementation as the addrinfo is totally unrelated to the tcp handle. Therefore I created an individual pull request to relieve the review effort :-)

@NorbertHeusser
Copy link
Contributor Author

NorbertHeusser commented Aug 29, 2022

Question on the required contribution agreement: Is this agreement the right one and whom do I need to add as the Please add the Canonical Project Manager or contact (it's a required field) ?

@MathieuBordere
Copy link
Contributor

MathieuBordere commented Aug 29, 2022

@NorbertHeusser That agreement looks like the right one, you could add "Stéphane Graber" @stgraber in that field.

@stgraber
Copy link
Contributor

Yep, that's the correct one and it's fine to put me as the contact.

@freeekanayaka
Copy link
Contributor

freeekanayaka commented Aug 29, 2022

@NorbertHeusser one thing I forgot to ask: are you using this raft library as part of dqlite, or for an entirely different project on your own that itself builds on this raft implementation?

@NorbertHeusser
Copy link
Contributor Author

NorbertHeusser commented Aug 30, 2022

I work at the Cedalo GmbH with @ralight at our Mosquitto Premium MQTT broker. We use the raft library for HA of the broker right now.

@freeekanayaka
Copy link
Contributor

I work at the Cedalo GmbH with @ralight at our Mosquitto Premium MQTT broker. We use the raft library for HA of the broker right now.

Oh I see, interesting. And out of curiosity, is it a single-threaded even-driven application or are you running this raft engine in its own thread and interacting with it from another thread?

@NorbertHeusser
Copy link
Contributor Author

We have a dedicated thread running the raft engine. And different thread(s) running the broker itself. Communication between our thread and the raft engine thread is performed using small in-memory-queues to avoid race conditions.

@MathieuBordere
Copy link
Contributor

@NorbertHeusser can this be closed?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants