Merge pull request #18132 from vespa-engine/arnej/actually-wait-for-connectivity

Arnej/actually wait for connectivity

5 files changed, 97 insertions, 112 deletions

diff --git a/configd/src/apps/sentinel/connectivity.cpp b/configd/src/apps/sentinel/connectivity.cpp
index 9cced1d3475..132b57fc884 100644
--- a/configd/src/apps/sentinel/connectivity.cpp
+++ b/configd/src/apps/sentinel/connectivity.cpp
@@ -1,5 +1,6 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
+#include "config-owner.h"
 #include "connectivity.h"
 #include "outward-check.h"
 #include <vespa/defaults.h>
@@ -16,19 +17,14 @@ using namespace std::chrono_literals;
 
 namespace config::sentinel {
 
-Connectivity::Connectivity(const SentinelConfig::Connectivity & config, RpcServer &rpcServer)
-  : _config(config),
-    _rpcServer(rpcServer)
-{
-    LOG(config, "connectivity.maxBadReverseCount = %d", _config.maxBadReverseCount);
-    LOG(config, "connectivity.maxBadOutPercent = %d", _config.maxBadOutPercent);
-}
+constexpr std::chrono::milliseconds MODEL_TIMEOUT_MS = 60s;
 
+Connectivity::Connectivity() = default;
 Connectivity::~Connectivity() = default;
 
 namespace {
 
-const char *toString(CcResult value) {
+std::string toString(CcResult value) {
     switch (value) {
         case CcResult::UNKNOWN: return "BAD: missing result"; // very very bad
         case CcResult::REVERSE_FAIL: return "connect OK, but reverse check FAILED"; // very bad
@@ -65,16 +61,28 @@ std::map<std::string, std::string> specsFrom(const ModelConfig &model) {
 
 }
 
-Connectivity::CheckResult
-Connectivity::checkConnectivity(const ModelConfig &model) {
-    const auto checkSpecs = specsFrom(model);
-    size_t clusterSize = checkSpecs.size();
+void Connectivity::configure(const SentinelConfig::Connectivity &config) {
+    _config = config;
+    LOG(config, "connectivity.maxBadReverseCount = %d", _config.maxBadReverseCount);
+    LOG(config, "connectivity.maxBadOutPercent = %d", _config.maxBadOutPercent);
+    if (auto up = ConfigOwner::fetchModelConfig(MODEL_TIMEOUT_MS)) {
+        _checkSpecs = specsFrom(*up);
+    }
+}
+
+bool
+Connectivity::checkConnectivity(RpcServer &rpcServer) {
+    size_t clusterSize = _checkSpecs.size();
+    if (clusterSize == 0) {
+        LOG(warning, "could not get model config, skipping connectivity checks");
+        return true;
+    }
     OutwardCheckContext checkContext(clusterSize,
                                      vespa::Defaults::vespaHostname(),
-                                     _rpcServer.getPort(),
-                                     _rpcServer.orb());
+                                     rpcServer.getPort(),
+                                     rpcServer.orb());
     std::map<std::string, OutwardCheck> connectivityMap;
-    for (const auto & [ hn, spec ] : checkSpecs) {
+    for (const auto & [ hn, spec ] : _checkSpecs) {
         connectivityMap.try_emplace(hn, spec, checkContext);
     }
     checkContext.latch.await();
@@ -82,6 +90,12 @@ Connectivity::checkConnectivity(const ModelConfig &model) {
     size_t numFailedReverse = 0;
     bool allChecksOk = true;
     for (const auto & [hostname, check] : connectivityMap) {
+        std::string detail = toString(check.result());
+        std::string prev = _detailsPerHost[hostname];
+        if (prev != detail) {
+            LOG(info, "Connectivity check details: %s -> %s", hostname.c_str(), detail.c_str());
+        }
+        _detailsPerHost[hostname] = detail;
         LOG_ASSERT(check.result() != CcResult::UNKNOWN);
         if (check.result() == CcResult::CONN_FAIL) ++numFailedConns;
         if (check.result() == CcResult::REVERSE_FAIL) ++numFailedReverse;
@@ -97,16 +111,12 @@ Connectivity::checkConnectivity(const ModelConfig &model) {
             numFailedConns, clusterSize, pct, _config.maxBadOutPercent);
         allChecksOk = false;
     }
-    std::vector<std::string> details;
-    for (const auto & [hostname, check] : connectivityMap) {
-        std::string detail = fmt("%s -> %s", hostname.c_str(), toString(check.result()));
-        details.push_back(detail);
+    if (allChecksOk && (numFailedConns == 0) && (numFailedReverse == 0)) {
+        LOG(info, "All connectivity checks OK, proceeding with service startup");
+    } else if (allChecksOk) {
+        LOG(info, "Enough connectivity checks OK, proceeding with service startup");
     }
-    CheckResult result{false, false, {}};
-    result.enoughOk = allChecksOk;
-    result.allOk = (numFailedConns == 0) && (numFailedReverse == 0);
-    result.details = std::move(details);
-    return result;
+    return allChecksOk;
 }
 
 }
diff --git a/configd/src/apps/sentinel/connectivity.h b/configd/src/apps/sentinel/connectivity.h
index 0e32b5243e0..1c7ee8ddc57 100644
--- a/configd/src/apps/sentinel/connectivity.h
+++ b/configd/src/apps/sentinel/connectivity.h
@@ -6,7 +6,7 @@
 #include <vespa/config-sentinel.h>
 #include <vespa/config-model.h>
 #include <string>
-#include <vector>
+#include <map>
 
 using cloud::config::SentinelConfig;
 using cloud::config::ModelConfig;
@@ -18,19 +18,14 @@ namespace config::sentinel {
  **/
 class Connectivity {
 public:
-    Connectivity(const SentinelConfig::Connectivity & config, RpcServer &rpcServer);
+    Connectivity();
     ~Connectivity();
-
-    struct CheckResult {
-        bool enoughOk;
-        bool allOk;
-        std::vector<std::string> details;
-    };
-
-    CheckResult checkConnectivity(const ModelConfig &model);
+    void configure(const SentinelConfig::Connectivity &config);
+    bool checkConnectivity(RpcServer &rpcServer);
 private:
-    const SentinelConfig::Connectivity _config;
-    RpcServer &_rpcServer;
+    SentinelConfig::Connectivity _config;
+    std::map<std::string, std::string> _checkSpecs;
+    std::map<std::string, std::string> _detailsPerHost;
 };
 
 }
diff --git a/configd/src/apps/sentinel/env.cpp b/configd/src/apps/sentinel/env.cpp
index e4174ee450d..5bbbfd8f0bd 100644
--- a/configd/src/apps/sentinel/env.cpp
+++ b/configd/src/apps/sentinel/env.cpp
@@ -2,11 +2,12 @@
 
 #include "env.h"
 #include "check-completion-handler.h"
-#include "outward-check.h"
+#include "connectivity.h"
 #include <vespa/defaults.h>
 #include <vespa/log/log.h>
 #include <vespa/config/common/exceptions.h>
 #include <vespa/vespalib/util/exceptions.h>
+#include <vespa/vespalib/util/signalhandler.h>
 #include <vespa/vespalib/util/stringfmt.h>
 #include <thread>
 #include <chrono>
@@ -18,8 +19,20 @@ using namespace std::chrono_literals;
 
 namespace config::sentinel {
 
+namespace {
+
+void maybeStopNow() {
+    if (vespalib::SignalHandler::INT.check() ||
+        vespalib::SignalHandler::TERM.check())
+    {
+        throw vespalib::FatalException("got signal during boot()");
+    }
+}
+
 constexpr std::chrono::milliseconds CONFIG_TIMEOUT_MS = 3min;
-constexpr std::chrono::milliseconds MODEL_TIMEOUT_MS = 1500ms;
+constexpr int maxConnectivityRetries = 100;
+
+} // namespace <unnamed>
 
 Env::Env()
   : _cfgOwner(),
@@ -31,6 +44,7 @@ Env::Env()
     _statePort(0)
 {
     _startMetrics.startedTime = vespalib::steady_clock::now();
+    _stateApi.myHealth.setFailed("initializing...");
 }
 
 Env::~Env() = default;
@@ -38,17 +52,36 @@ Env::~Env() = default;
 void Env::boot(const std::string &configId) {
     LOG(debug, "Reading configuration for ID: %s", configId.c_str());
     _cfgOwner.subscribe(configId, CONFIG_TIMEOUT_MS);
-    bool ok = _cfgOwner.checkForConfigUpdate();
     // subscribe() should throw if something is not OK
-    LOG_ASSERT(ok && _cfgOwner.hasConfig());
-    const auto & cfg = _cfgOwner.getConfig();
-    LOG(config, "Booting sentinel '%s' with [stateserver port %d] and [rpc port %d]",
-        configId.c_str(), cfg.port.telnet, cfg.port.rpc);
-    rpcPort(cfg.port.rpc);
-    statePort(cfg.port.telnet);
-    if (auto up = ConfigOwner::fetchModelConfig(MODEL_TIMEOUT_MS)) {
-        waitForConnectivity(*up);
+    Connectivity checker;
+    for (int retry = 0; retry < maxConnectivityRetries; ++retry) {
+        bool changed = _cfgOwner.checkForConfigUpdate();
+        LOG_ASSERT(changed || retry > 0);
+        if (changed) {
+            LOG_ASSERT(_cfgOwner.hasConfig());
+            const auto & cfg = _cfgOwner.getConfig();
+            LOG(config, "Booting sentinel '%s' with [stateserver port %d] and [rpc port %d]",
+                configId.c_str(), cfg.port.telnet, cfg.port.rpc);
+            rpcPort(cfg.port.rpc);
+            statePort(cfg.port.telnet);
+            checker.configure(cfg.connectivity);
+        }
+        if (checker.checkConnectivity(*_rpcServer)) {
+            _stateApi.myHealth.setOk();
+            return;
+        } else {
+            _stateApi.myHealth.setFailed("FAILED connectivity check");
+            if ((retry % 10) == 0) {
+                LOG(warning, "Bad network connectivity (try %d)", 1+retry);
+            }
+            for (int i = 0; i < 5; ++i) {
+                respondAsEmpty();
+                maybeStopNow();
+                std::this_thread::sleep_for(600ms);
+            }
+        }
     }
+    throw vespalib::FatalException("Giving up - too many connectivity check failures");
 }
 
 void Env::rpcPort(int port) {
@@ -93,60 +126,4 @@ void Env::respondAsEmpty() {
     }
 }
 
-namespace {
-
-const char *toString(CcResult value) {
-    switch (value) {
-        case CcResult::UNKNOWN: return "unknown";
-        case CcResult::CONN_FAIL: return "failed to connect";
-        case CcResult::REVERSE_FAIL: return "connect OK, but reverse check FAILED";
-        case CcResult::REVERSE_UNAVAIL: return "connect OK, but reverse check unavailable";
-        case CcResult::ALL_OK: return "both ways connectivity OK";
-    }
-    LOG(error, "Unknown CcResult enum value: %d", (int)value);
-    LOG_ABORT("Unknown CcResult enum value");
-}
-
-std::map<std::string, std::string> specsFrom(const ModelConfig &model) {
-    std::map<std::string, std::string> checkSpecs;
-    for (const auto & h : model.hosts) {
-        bool foundSentinelPort = false;
-        for (const auto & s : h.services) {
-            if (s.name == "config-sentinel") {
-                for (const auto & p : s.ports) {
-                    if (p.tags.find("rpc") != p.tags.npos) {
-                        auto spec = fmt("tcp/%s:%d", h.name.c_str(), p.number);
-                        checkSpecs[h.name] = spec;
-                        foundSentinelPort = true;
-                    }
-                }
-            }
-        }
-        if (! foundSentinelPort) {
-            LOG(warning, "Did not find 'config-sentinel' RPC port in model for host %s [%zd services]",
-                h.name.c_str(), h.services.size());
-        }
-    }
-    return checkSpecs;
-}
-
-}
-
-void Env::waitForConnectivity(const ModelConfig &model) {
-    auto checkSpecs = specsFrom(model);
-    OutwardCheckContext checkContext(checkSpecs.size(),
-                                     vespa::Defaults::vespaHostname(),
-                                     _rpcServer->getPort(),
-                                     _rpcServer->orb());
-    std::map<std::string, OutwardCheck> connectivityMap;
-    for (const auto & [ hn, spec ] : checkSpecs) {
-        connectivityMap.try_emplace(hn, spec, checkContext);
-    }
-    checkContext.latch.await();
-    for (const auto & [hostname, check] : connectivityMap) {
-        LOG(info, "outward check status for host %s is: %s",
-            hostname.c_str(), toString(check.result()));
-    }
-}
-
 }
diff --git a/configd/src/apps/sentinel/env.h b/configd/src/apps/sentinel/env.h
index f117854f006..f71fb537068 100644
--- a/configd/src/apps/sentinel/env.h
+++ b/configd/src/apps/sentinel/env.h
@@ -32,7 +32,6 @@ public:
     void notifyConfigUpdated();
 private:
     void respondAsEmpty();
-    void waitForConnectivity(const ModelConfig &model);
     ConfigOwner _cfgOwner;
     CommandQueue _rpcCommandQueue;
     std::unique_ptr<RpcServer> _rpcServer;
diff --git a/configd/src/apps/sentinel/sentinel.cpp b/configd/src/apps/sentinel/sentinel.cpp
index 7f3ddcc5882..ebed1549106 100644
--- a/configd/src/apps/sentinel/sentinel.cpp
+++ b/configd/src/apps/sentinel/sentinel.cpp
@@ -63,16 +63,20 @@ main(int argc, char **argv)
     LOG(debug, "Reading configuration");
     try {
         environment.boot(configId);
+    } catch (vespalib::FatalException& ex) {
+        LOG(error, "Stopping before boot complete: %s", ex.message());
+        EV_STOPPING("config-sentinel", ex.message());
+        return EXIT_FAILURE;
     } catch (ConfigTimeoutException & ex) {
-        LOG(warning, "Timeout getting config, please check your setup. Will exit and restart: %s", ex.getMessage().c_str());
-        EV_STOPPING("config-sentinel", ex.what());
+        LOG(warning, "Timeout getting config, please check your setup. Will exit and restart: %s", ex.message());
+        EV_STOPPING("config-sentinel", ex.message());
         return EXIT_FAILURE;
     } catch (InvalidConfigException& ex) {
-        LOG(error, "Fatal: Invalid configuration, please check your setup: %s", ex.getMessage().c_str());
-        EV_STOPPING("config-sentinel", ex.what());
+        LOG(error, "Fatal: Invalid configuration, please check your setup: %s", ex.message());
+        EV_STOPPING("config-sentinel", ex.message());
         return EXIT_FAILURE;
     } catch (ConfigRuntimeException& ex) {
-        LOG(error, "Fatal: Could not get config, please check your setup: %s", ex.getMessage().c_str());
+        LOG(error, "Fatal: Could not get config, please check your setup: %s", ex.message());
         EV_STOPPING("config-sentinel", ex.what());
         return EXIT_FAILURE;
     }
@@ -84,13 +88,13 @@ main(int argc, char **argv)
             vespalib::SignalHandler::CHLD.clear();
             manager.doWork();       // Check for child procs & commands
         } catch (InvalidConfigException& ex) {
-            LOG(warning, "Configuration problem: (ignoring): %s", ex.what());
+            LOG(warning, "Configuration problem: (ignoring): %s", ex.message());
         } catch (vespalib::PortListenException& ex) {
-            LOG(error, "Fatal: %s", ex.getMessage().c_str());
+            LOG(error, "Fatal: %s", ex.message());
             EV_STOPPING("config-sentinel", ex.what());
             return EXIT_FAILURE;
         } catch (vespalib::FatalException& ex) {
-            LOG(error, "Fatal: %s", ex.getMessage().c_str());
+            LOG(error, "Fatal: %s", ex.message());
             EV_STOPPING("config-sentinel", ex.what());
             return EXIT_FAILURE;
         }