On 3/30/22 16:07, Dumitru Ceara wrote:
> On 3/30/22 15:08, Ilya Maximets wrote:
>> On 3/21/22 11:45, Dumitru Ceara wrote:
>>> Hi Ilya.
>>>
>>> On 3/10/22 23:33, Ilya Maximets wrote:
>>>> Clustered OVSDB allows to use DNS names as addresses of raft members.
>>>> However, if DNS resolution fails during the initial database read,
>>>> this causes a fatal failure and exit of the ovsdb-server process.
>>>>
>>>> Also, if DNS name of a joining server is not resolvable for one of the
>>>> followers, this follower will reject append requests for a new server
>>>> to join until the name is successfully resolved. This makes a follower
>>>> effectively non-functional while DNS is unavailable.
>>>>
>>>> To fix the problem relaxing the address verification. Allowing
>>>
>>> s/relaxing/relax/
>>>
>>>> validation to pass if only name resolution failed and the address
>>>> is valid otherwise. This will allow addresses to be added to the
>>>> database, so connections could be established later when the DNS
>>>> is available.
>>>
>>> This sounds reasonable to me.
>>>
>>>>
>>>> Additionally fixing missed initialization of the dns-resolve module.
>>>> Wihtout it, DNS requests are blocking. This causes unexpected delays
>>>
>>> Typo: Wihtout
>>>
>>>> in runtime.
>>>>
>>>> Fixes: 771680d96fb6 ("DNS: Add basic support for asynchronous DNS
>>>> resolving")
>>>> Reported-at: https://bugzilla.redhat.com/2055097
>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>> ---
>>>> lib/dns-resolve.c | 2 +-
>>>> lib/socket-util.c | 39 ++++++++++++++++++++++++------------
>>>> lib/socket-util.h | 3 ++-
>>>> lib/stream.c | 2 +-
>>>> ofproto/ofproto-dpif-sflow.c | 3 ++-
>>>> ovsdb/ovsdb-server.c | 3 +++
>>>> ovsdb/raft-private.c | 5 ++++-
>>>> 7 files changed, 39 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/lib/dns-resolve.c b/lib/dns-resolve.c
>>>> index d34451434..8bcecb90c 100644
>>>> --- a/lib/dns-resolve.c
>>>> +++ b/lib/dns-resolve.c
>>>> @@ -265,7 +265,7 @@ resolve_callback__(void *req_, int err, struct
>>>> ub_result *result)
>>>> if (err != 0 || (result->qtype == ns_t_aaaa && !result->havedata)) {
>>>> ub_resolve_free(result);
>>>> req->state = RESOLVE_ERROR;
>>>> - VLOG_ERR_RL(&rl, "%s: failed to resolve", req->name);
>>>> + VLOG_WARN_RL(&rl, "%s: failed to resolve", req->name);
>>>> return;
>>>> }
>>>>
>>>> diff --git a/lib/socket-util.c b/lib/socket-util.c
>>>> index 4f1ffecf5..38705cc51 100644
>>>> --- a/lib/socket-util.c
>>>> +++ b/lib/socket-util.c
>>>> @@ -62,7 +62,8 @@ static bool parse_sockaddr_components(struct
>>>> sockaddr_storage *ss,
>>>> const char *port_s,
>>>> uint16_t default_port,
>>>> const char *s,
>>>> - bool resolve_host);
>>>> + bool resolve_host,
>>>> + bool *dns_failure);
>>>>
>>>> /* Sets 'fd' to non-blocking mode. Returns 0 if successful, otherwise a
>>>> * positive errno value. */
>>>> @@ -438,7 +439,7 @@ parse_sockaddr_components_dns(struct sockaddr_storage
>>>> *ss OVS_UNUSED,
>>>> dns_resolve(host_s, &tmp_host_s);
>>>> if (tmp_host_s != NULL) {
>>>> parse_sockaddr_components(ss, tmp_host_s, port_s,
>>>> - default_port, s, false);
>>>> + default_port, s, false, NULL);
>>>> free(tmp_host_s);
>>>> return true;
>>>> }
>>>> @@ -450,11 +451,15 @@ parse_sockaddr_components(struct sockaddr_storage
>>>> *ss,
>>>> char *host_s,
>>>> const char *port_s, uint16_t default_port,
>>>> const char *s,
>>>> - bool resolve_host)
>>>> + bool resolve_host, bool *dns_failure)
>>>> {
>>>> struct sockaddr_in *sin = sin_cast(sa_cast(ss));
>>>> int port;
>>>>
>>>> + if (dns_failure) {
>>>> + *dns_failure = false;
>>>> + }
>>>> +
>>>> if (port_s && port_s[0]) {
>>>> if (!str_to_int(port_s, 10, &port) || port < 0 || port > 65535) {
>>>> VLOG_ERR("%s: bad port number \"%s\"", s, port_s);
>>>> @@ -501,10 +506,15 @@ parse_sockaddr_components(struct sockaddr_storage
>>>> *ss,
>>>> return true;
>>>>
>>>> resolve:
>>>> - if (resolve_host && parse_sockaddr_components_dns(ss, host_s, port_s,
>>>> - default_port, s)) {
>>>> - return true;
>>>> - } else if (!resolve_host) {
>>>> + if (resolve_host) {
>>>> + if (parse_sockaddr_components_dns(ss, host_s, port_s,
>>>> + default_port, s)) {
>>>> + return true;
>>>> + }
>>>> + if (dns_failure) {
>>>> + *dns_failure = true;
>>>> + }
>>>> + } else {
>>>> VLOG_ERR("%s: bad IP address \"%s\"", s, host_s);
>>>> }
>>>> exit:
>>>> @@ -521,10 +531,12 @@ exit:
>>>> * It resolves the host if 'resolve_host' is true.
>>>> *
>>>> * On success, returns true and stores the parsed remote address into
>>>> '*ss'.
>>>> - * On failure, logs an error, stores zeros into '*ss', and returns false.
>>>> */
>>>> + * On failure, logs an error, stores zeros into '*ss', and returns false,
>>>> + * '*dns_failure' indicates if the host resolution failed. */
>>>> bool
>>>> inet_parse_active(const char *target_, int default_port,
>>>> - struct sockaddr_storage *ss, bool resolve_host)
>>>> + struct sockaddr_storage *ss,
>>>> + bool resolve_host, bool *dns_failure)
>>>
>>> It's not really a fair ask I guess, but would it be possible to avoid
>>> changing this signature?
>>>
>>> OVN uses it in a few places. I know there's no real contract for
>>> functions in lib/*.h but OVN does use them directly.
>>>
>>> If you don't see a simple way of avoiding the signature change, it's not
>>> a big deal I guess, we can always just bump the OVS submodule version in
>>> OVN.
>>
>> Yeah, I'm not sure we can actually keep the signature intact.
>> In general, DNS resolve is a bit hacked into the process and
>> it's hard to manage it in a good way. I think, this API should
>> be split in parts or changed to make error reporting more
>> clear, e.g. by returning a enum instead of bool. But none of
>> these options will actually preserve the signature.
>>
>> Does that make sense?
>>
>> Also, the next submodule update in OVN will likely require
>> code changes due to UB fixes for loops, so it doesn't seem
>> important to keep update seamless for this part.
>>
>
> Good point. Let's keep it like in your original patch.
>
>>>
>>>> {
>>>> char *target = xstrdup(target_);
>>>> char *port, *host;
>>>> @@ -539,7 +551,7 @@ inet_parse_active(const char *target_, int
>>>> default_port,
>>>> ok = false;
>>>> } else {
>>>> ok = parse_sockaddr_components(ss, host, port, default_port,
>>>> - target_, resolve_host);
>>>> + target_, resolve_host,
>>>> dns_failure);
>>>> }
>>>> if (!ok) {
>>>> memset(ss, 0, sizeof *ss);
>>>> @@ -576,7 +588,7 @@ inet_open_active(int style, const char *target, int
>>>> default_port,
>>>> int error;
>>>>
>>>> /* Parse. */
>>>> - if (!inet_parse_active(target, default_port, &ss, true)) {
>>>> + if (!inet_parse_active(target, default_port, &ss, true, NULL)) {
>>>> error = EAFNOSUPPORT;
>>>> goto exit;
>>>> }
>>>> @@ -660,7 +672,7 @@ inet_parse_passive(const char *target_, int
>>>> default_port,
>>>> ok = false;
>>>> } else {
>>>> ok = parse_sockaddr_components(ss, host, port, default_port,
>>>> - target_, true);
>>>> + target_, true, NULL);
>>>> }
>>>> if (!ok) {
>>>> memset(ss, 0, sizeof *ss);
>>>> @@ -783,7 +795,8 @@ inet_parse_address(const char *target_, struct
>>>> sockaddr_storage *ss)
>>>> {
>>>> char *target = xstrdup(target_);
>>>> char *host = unbracket(target);
>>>> - bool ok = parse_sockaddr_components(ss, host, NULL, 0, target_,
>>>> false);
>>>> + bool ok = parse_sockaddr_components(ss, host, NULL, 0,
>>>> + target_, false, NULL);
>>>> if (!ok) {
>>>> memset(ss, 0, sizeof *ss);
>>>> }
>>>> diff --git a/lib/socket-util.h b/lib/socket-util.h
>>>> index 9ccb7d4cc..bf66393df 100644
>>>> --- a/lib/socket-util.h
>>>> +++ b/lib/socket-util.h
>>>> @@ -49,7 +49,8 @@ ovs_be32 guess_netmask(ovs_be32 ip);
>>>> void inet_parse_host_port_tokens(char *s, char **hostp, char **portp);
>>>> void inet_parse_port_host_tokens(char *s, char **portp, char **hostp);
>>>> bool inet_parse_active(const char *target, int default_port,
>>>> - struct sockaddr_storage *ssp, bool resolve_host);
>>>> + struct sockaddr_storage *ssp,
>>>> + bool resolve_host, bool *dns_failure);
>>>> int inet_open_active(int style, const char *target, int default_port,
>>>> struct sockaddr_storage *ssp, int *fdp, uint8_t
>>>> dscp);
>>>>
>>>> diff --git a/lib/stream.c b/lib/stream.c
>>>> index fcaddf10a..71039e24f 100644
>>>> --- a/lib/stream.c
>>>> +++ b/lib/stream.c
>>>> @@ -788,7 +788,7 @@ stream_parse_target_with_default_port(const char
>>>> *target, int default_port,
>>>> struct sockaddr_storage *ss)
>>>> {
>>>> return ((!strncmp(target, "tcp:", 4) || !strncmp(target, "ssl:", 4))
>>>> - && inet_parse_active(target + 4, default_port, ss, true));
>>>> + && inet_parse_active(target + 4, default_port, ss, true,
>>>> NULL));
>>>> }
>>>>
>>>> /* Attempts to guess the content type of a stream whose first few bytes
>>>> were
>>>> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
>>>> index 30e7caf54..36b5c1768 100644
>>>> --- a/ofproto/ofproto-dpif-sflow.c
>>>> +++ b/ofproto/ofproto-dpif-sflow.c
>>>> @@ -468,7 +468,8 @@ sflow_choose_agent_address(const char *agent_device,
>>>> const char *target;
>>>> SSET_FOR_EACH (target, targets) {
>>>> struct sockaddr_storage ss;
>>>> - if (inet_parse_active(target, SFL_DEFAULT_COLLECTOR_PORT, &ss,
>>>> true)) {
>>>> + if (inet_parse_active(target, SFL_DEFAULT_COLLECTOR_PORT,
>>>> + &ss, true, NULL)) {
>>>> /* sFlow only supports target in default routing table with
>>>> * packet mark zero.
>>>> */
>>>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>>>> index b975c17fc..a4f6bca73 100644
>>>> --- a/ovsdb/ovsdb-server.c
>>>> +++ b/ovsdb/ovsdb-server.c
>>>> @@ -26,6 +26,7 @@
>>>> #include "command-line.h"
>>>> #include "daemon.h"
>>>> #include "dirs.h"
>>>> +#include "dns-resolve.h"
>>>> #include "openvswitch/dynamic-string.h"
>>>> #include "fatal-signal.h"
>>>> #include "file.h"
>>>> @@ -329,6 +330,7 @@ main(int argc, char *argv[])
>>>> service_start(&argc, &argv);
>>>> fatal_ignore_sigpipe();
>>>> process_init();
>>>> + dns_resolve_init(true);
>>>>
>>>> bool active = false;
>>>> parse_options(argc, argv, &db_filenames, &remotes, &unixctl_path,
>>>> @@ -511,6 +513,7 @@ main(int argc, char *argv[])
>>>> run_command, process_status_msg(status));
>>>> }
>>>> }
>>>> + dns_resolve_destroy();
>>>> perf_counters_destroy();
>>>> service_stop();
>>>> return 0;
>>>> diff --git a/ovsdb/raft-private.c b/ovsdb/raft-private.c
>>>> index 30760233e..970cf3b54 100644
>>>> --- a/ovsdb/raft-private.c
>>>> +++ b/ovsdb/raft-private.c
>>>> @@ -36,7 +36,10 @@ raft_address_validate(const char *address)
>>>> return NULL;
>>>> } else if (!strncmp(address, "ssl:", 4) || !strncmp(address, "tcp:",
>>>> 4)) {
>>>> struct sockaddr_storage ss;
>>>> - if (!inet_parse_active(address + 4, -1, &ss, true)) {
>>>> + bool dns_failure;
>>>
>>> Not really an issue, but just to make it more clear, could we explicitly
>>> initialize dns_failure here?
>>
>> ok.
>>
>>>
>>>> +
>>>> + if (!inet_parse_active(address + 4, -1, &ss, true, &dns_failure)
>>>> + && !dns_failure) {
>>>> return ovsdb_error(NULL, "%s: syntax error in address",
>>>> address);
>>>> }
>>>> return NULL;
>>>
>>> Not really mandatory for this patch but we might as well update some
>>> comments in raft.c to make it clear that it's allowed to refer to
>>> servers by name, e.g., the comment for raft_join_cluster() mentions:
>>>
>>> * The new server is located at 'local_address', which must take one of the
>>> * forms "tcp:IP:PORT" or "ssl:IP:PORT", where IP is an IPv4 address or a
>>> * square bracket enclosed IPv6 address and PORT is a TCP port number.
>>>
>>> The same thing applies to raft_create_cluster().
>>
>> As I said, DNS resolve is a bit hacked into the logic here, so there
>> are lots of fairly confusing comments in both raft and socket-util
>> modules. When I worked on the patch I started re-writing and clarifying
>> all of them but stopped in the middle. It's just a lot of comment
>> changes and it's not really relevant to the current patch, so deserves
>> a separate one. What do you think?
>>
>
> Sounds good to me, therefore:
>
> Acked-by: Dumitru Ceara <[email protected]>
Thanks! I addressed all other minor comments and applied the fix.
Also backported down to 2.13.
Best regards, Ilya Maximets.
>
>>>
>>> Most of the minor comments above can be addressed at apply time so
>>> there's no need for a v2 but I'd like to see if there's a way to avoid
>>> changing the signature of inet_parse_active() before ACKing the patch.
>>>
>>> Regards,
>>> Dumitru
>>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
