dns-resolve: Do not treat never accessed responses as expired.

If a daemon has multiple remotes to connect to and it already reached
pretty high backoff interval on those connections, it is possible that
it will never be able to connect, if the DNS TTL value is too low.

For example, if ovn-controller has 3 remotes in the ovn-remote
configuration, where each is presented as a host name, the following
is happening when it reaches default max backoff of 8 seconds:

1. Tries to connect to the first remote - sends async DNS request.
2. Since jsonrpc/reconnect modules are not really aware of this, they
   just treat connection as temporarily failed - 8 second backoff.
3. After the backoff - switching to a new remote - sending DNS request.
4. Temporarily failing - 8 second backoff.
5. Switching to the third remote - sending DNS request.
6. Temporarily failing - 8 second backoff.
7. Finally trying the first remote again - checking DNS.
8. There is a processed response, but it is already 24 seconds old.
9. If DNS TTL is lower than 24 seconds - consider expired - send
   a new DNS request.
10. Go to step 2.

With that, if DNS TTL is lower than 3x of the backoff interval, the
process will never be able to connect without some external help to
break the loop.

A proper solution for this should include:

1. Making jsonrpc and reconnect and all the users of these modules
   aware of the DNS request being made.  This means introduction of
   a new RECONNECT state for DNS request and not switching to a new
   target while we're still in this state.

2. Making the poll loop state machine properly react to DNS responses
   by waiting on the file descriptor provided by the unbound library.

However, such solution will be very invasive to the code structure
and all the involved libraries, so it may not be something that we
would want to backport as a bug fix to stable branches.

Instead, making a much simpler change to allow use of never previously
accessed DNS replies for a short period of time, so the loop can be
broken.  It's not caching if we just requested the value, but didn't
use it yet, it's a "transaction in progress" situation in which we can
use the response even if TTL is zero.  Without a proper solution though
we can't be sure that the process will ever look at the result of
asynchronous request, so we need to have an upper limit for such
"transactions in progress".  Limiting them to a fairly arbitrary, but
big enough, value of 5 minutes.  In the worst case where the address
actually goes stale in between our request and the first access, we'll
try to use the stale value once and then re-request right away on
failure to connect.

This solution seems reasonable and simple enough to backport to stable
branches while working on the proper solution on main.

Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2025-June/053738.html
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Mike Pattrick <mkp@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

diff --git a/lib/dns-resolve.c b/lib/dns-resolve.c
index 1afcc65adbddb6aaa924fcb6e2e63382e09f28b2..1212c587e5baf8bc026d20c9e9ea2fb5486c7a20 100644 (file)
--- a/lib/dns-resolve.c
+++ b/lib/dns-resolve.c
@@ -28,6 +28,7 @@
 #include "openvswitch/hmap.h"
 #include "openvswitch/vlog.h"
 #include "timeval.h"
+#include "util.h"
 
 VLOG_DEFINE_THIS_MODULE(dns_resolve);
 
@@ -43,6 +44,7 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
 enum resolve_state {
     RESOLVE_INVALID,
     RESOLVE_PENDING,
+    RESOLVE_GOOD_UNUSED,
     RESOLVE_GOOD,
     RESOLVE_ERROR
 };
@@ -220,11 +222,29 @@ resolve_find_or_new__(const char *name)
     return req;
 }
 
+#define RESOLVE_UNUSED_TIMEOUT 300
+
 static bool
 resolve_check_expire__(struct resolve_request *req)
     OVS_REQUIRES(dns_mutex__)
 {
-    return time_now() > req->time + req->ub_result->ttl;
+    int ttl = req->ub_result->ttl;
+
+    /* When we just sent a request, but didn't look at the response yet, it's
+     * not caching, but a "transaction in progress" situation, so we can use
+     * the response even with TTL of 0 and more than 1 second passed.
+     * Allow such values to be accessed for at least RESOLVE_UNUSED_TIMEOUT
+     * seconds without considering them stale.  This is necessary in case of
+     * large backoff intervals on connections or if the process is doing some
+     * other work not looking at the response for longer than TTL. */
+    if (req->state == RESOLVE_GOOD_UNUSED) {
+        ttl = MAX(ttl, RESOLVE_UNUSED_TIMEOUT);
+        /* Not a "transaction in progress" anymore, normal caching rules should
+         * apply from this point forward. */
+        req->state = RESOLVE_GOOD;
+    }
+
+    return time_now() > req->time + ttl;
 }
 
 static bool
@@ -232,7 +252,7 @@ resolve_check_valid__(struct resolve_request *req)
     OVS_REQUIRES(dns_mutex__)
 {
     return (req != NULL
-        && req->state == RESOLVE_GOOD
+        && (req->state == RESOLVE_GOOD || req->state == RESOLVE_GOOD_UNUSED)
         && !resolve_check_expire__(req));
 }
 
@@ -289,7 +309,7 @@ resolve_callback__(void *req_, int err, struct ub_result *result)
 
     req->ub_result = result;
     req->addr = addr;
-    req->state = RESOLVE_GOOD;
+    req->state = RESOLVE_GOOD_UNUSED;
     req->time = time_now();
 }