refactor and cleanup

* add RPC api to check connectivity
* start PeerCheck directly from RPCHooks
* let CheckCompletionHandler do just completion
* allocate PeerCheck and CheckCompletionHandler inside parent request
* revert last changes in Cmd and Manager

13 files changed, 61 insertions, 54 deletions

diff --git a/configd/src/apps/sentinel/check-completion-handler.cpp b/configd/src/apps/sentinel/check-completion-handler.cpp
index f854a85d297..3ccda17e617 100644
--- a/configd/src/apps/sentinel/check-completion-handler.cpp
+++ b/configd/src/apps/sentinel/check-completion-handler.cpp
@@ -1,31 +1,20 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "check-completion-handler.h"
-#include <vespa/log/log.h>
-LOG_SETUP(".check-completion-handler");
 
 namespace config::sentinel {
 
-void CheckCompletionHandler::create(Cmd::UP request, FRT_Supervisor &orb) {
-    LOG_ASSERT(request->type() == Cmd::CHECK_CONNECTIVITY);
-    // will delete itself on completion:
-    new CheckCompletionHandler(std::move(request), orb);
-}
-
-CheckCompletionHandler::CheckCompletionHandler(Cmd::UP request, FRT_Supervisor &orb)
-  : _parentRequest(std::move(request))
+CheckCompletionHandler::CheckCompletionHandler(FRT_RPCRequest *parent)
+  : _parentRequest(parent)
 {
-    const std::string & host = _parentRequest->name();
-    int port = _parentRequest->portNumber();
-    _peerCheck = std::make_unique<PeerCheck>(*this, host, port, orb);
-    
 }
 
 CheckCompletionHandler::~CheckCompletionHandler() = default;
 
 void CheckCompletionHandler::returnStatus(bool ok) {
-    _parentRequest->retValue(ok ? "ok" : "bad");
-    delete this;
+    FRT_Values *dst = _parentRequest->GetReturn();
+    dst->AddString(ok ? "ok" : "bad");
+    _parentRequest->Return();
 }
 
 }
diff --git a/configd/src/apps/sentinel/check-completion-handler.h b/configd/src/apps/sentinel/check-completion-handler.h
index ae43799dd54..008e456f6d7 100644
--- a/configd/src/apps/sentinel/check-completion-handler.h
+++ b/configd/src/apps/sentinel/check-completion-handler.h
@@ -4,24 +4,21 @@
 
 #include "status-callback.h"
 #include "peer-check.h"
-#include "cmdq.h"
 
 namespace config::sentinel {
 
 /**
  * Handles a checkConnectivity request by making an outgoing
  * ping request.  When the ping finishes, fills an answer
- * into the parent request and deletes itself.
+ * into the parent request and send the answer back.
  **/
 class CheckCompletionHandler : public StatusCallback {
+private:
+    FRT_RPCRequest *_parentRequest;
 public:
-    static void create(Cmd::UP request, FRT_Supervisor &orb);
+    CheckCompletionHandler(FRT_RPCRequest *parentRequest);
     virtual ~CheckCompletionHandler();
     void returnStatus(bool ok) override;
-private:
-    CheckCompletionHandler(Cmd::UP request, FRT_Supervisor &orb);
-    Cmd::UP _parentRequest;
-    std::unique_ptr<PeerCheck> _peerCheck;
 };
 
 }
diff --git a/configd/src/apps/sentinel/cmdq.h b/configd/src/apps/sentinel/cmdq.h
index 38c95967d70..105d5f29b60 100644
--- a/configd/src/apps/sentinel/cmdq.h
+++ b/configd/src/apps/sentinel/cmdq.h
@@ -5,7 +5,6 @@
 #include <memory>
 #include <mutex>
 #include <vector>
-#include <string>
 
 class FRT_RPCRequest;
 
@@ -14,16 +13,14 @@ namespace config::sentinel {
 class Cmd {
 public:
     using UP = std::unique_ptr<Cmd>;
-    enum CmdType { LIST, RESTART, START, STOP, CHECK_CONNECTIVITY };
+    enum CmdType { LIST, RESTART, START, STOP };
 
-    Cmd(FRT_RPCRequest *req, CmdType cmdType, std::string name = "", int portnum = 0)
-      : _req(req), _cmdType(cmdType), _name(name), _port(portnum)
+    Cmd(FRT_RPCRequest *req, CmdType cmdType, const char *service = "")
+      : _req(req), _cmdType(cmdType), _serviceName(service)
     {}
 
     CmdType type() const { return _cmdType; }
-    // the service name or host name:
-    const std::string & name() const { return _name; }
-    int portNumber() const { return _port; }
+    const char *serviceName() const { return _serviceName; }
 
     void retError(const char *errorString) const;
     void retValue(const char *valueString) const;
@@ -32,8 +29,7 @@ public:
 private:
     FRT_RPCRequest *_req;
     CmdType _cmdType;
-    std::string _name;
-    int _port;
+    const char *_serviceName;
 };
 
 class CommandQueue
diff --git a/configd/src/apps/sentinel/env.cpp b/configd/src/apps/sentinel/env.cpp
index 11ba20a18d5..745fa8cef1f 100644
--- a/configd/src/apps/sentinel/env.cpp
+++ b/configd/src/apps/sentinel/env.cpp
@@ -1,6 +1,7 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "env.h"
+#include "check-completion-handler.h"
 #include <vespa/log/log.h>
 #include <vespa/config/common/exceptions.h>
 #include <vespa/vespalib/util/exceptions.h>
@@ -75,4 +76,8 @@ void Env::notifyConfigUpdated() {
 
 }
 
+void Env::handleCmd(Cmd::UP cmd) {
+    cmd->retError("still booting, not ready for all RPC commands");
+}
+
 }
diff --git a/configd/src/apps/sentinel/env.h b/configd/src/apps/sentinel/env.h
index da04a328c41..0213fd09460 100644
--- a/configd/src/apps/sentinel/env.h
+++ b/configd/src/apps/sentinel/env.h
@@ -30,7 +30,7 @@ public:
     void statePort(int portnum);
 
     void notifyConfigUpdated();
-
+    void handleCmd(Cmd::UP cmd);
 private:
     ConfigOwner _cfgOwner;
     CommandQueue _rpcCommandQueue;
diff --git a/configd/src/apps/sentinel/manager.cpp b/configd/src/apps/sentinel/manager.cpp
index 6f8bf69cb83..80dd2a3fda8 100644
--- a/configd/src/apps/sentinel/manager.cpp
+++ b/configd/src/apps/sentinel/manager.cpp
@@ -271,7 +271,7 @@ Manager::handleCmd(const Cmd& cmd)
         break;
     case Cmd::RESTART:
         {
-            Service *service = serviceByName(cmd.name());
+            Service *service = serviceByName(cmd.serviceName());
             if (service == nullptr) {
                 cmd.retError("Cannot find named service");
                 return;
@@ -287,7 +287,7 @@ Manager::handleCmd(const Cmd& cmd)
         break;
     case Cmd::START:
         {
-            Service *service = serviceByName(cmd.name());
+            Service *service = serviceByName(cmd.serviceName());
             if (service == nullptr) {
                 cmd.retError("Cannot find named service");
                 return;
@@ -301,7 +301,7 @@ Manager::handleCmd(const Cmd& cmd)
         break;
     case Cmd::STOP:
         {
-            Service *service = serviceByName(cmd.name());
+            Service *service = serviceByName(cmd.serviceName());
             if (service == nullptr) {
                 cmd.retError("Cannot find named service");
                 return;
@@ -312,10 +312,6 @@ Manager::handleCmd(const Cmd& cmd)
             }
         }
         break;
-    case Cmd::CHECK_CONNECTIVITY:
-        // XXX placeholder:
-        cmd.retValue("unknown");
-        break;
     }
 }
 
diff --git a/configd/src/apps/sentinel/peer-check.cpp b/configd/src/apps/sentinel/peer-check.cpp
index 6aef1443ef2..024f928c994 100644
--- a/configd/src/apps/sentinel/peer-check.cpp
+++ b/configd/src/apps/sentinel/peer-check.cpp
@@ -25,9 +25,8 @@ PeerCheck::PeerCheck(StatusCallback &callback, const std::string &host, int port
 }
 
 PeerCheck::~PeerCheck() {
-    if (_req != nullptr) {
-        _req->Abort();
-    }
+    LOG_ASSERT(_req == nullptr);
+    LOG_ASSERT(_target == nullptr);
 }
 
 void PeerCheck::RequestDone(FRT_RPCRequest *req) {
diff --git a/configd/src/apps/sentinel/rpchooks.cpp b/configd/src/apps/sentinel/rpchooks.cpp
index aef58b8a1dc..d364e74154c 100644
--- a/configd/src/apps/sentinel/rpchooks.cpp
+++ b/configd/src/apps/sentinel/rpchooks.cpp
@@ -2,6 +2,8 @@
 
 #include "rpchooks.h"
 #include "cmdq.h"
+#include "check-completion-handler.h"
+#include "peer-check.h"
 #include <vespa/fnet/frt/supervisor.h>
 #include <vespa/fnet/frt/rpcrequest.h>
 
@@ -10,6 +12,13 @@ LOG_SETUP(".rpchooks");
 
 namespace config::sentinel {
 
+RPCHooks::RPCHooks(CommandQueue &commands, FRT_Supervisor &supervisor)
+  : _commands(commands),
+    _orb(supervisor)
+{
+    initRPC(&_orb);
+}
+
 RPCHooks::~RPCHooks() = default;
 
 
@@ -36,6 +45,13 @@ RPCHooks::initRPC(FRT_Supervisor *supervisor)
                     FRT_METHOD(RPCHooks::rpc_startService), this);
     rb.MethodDesc("start a service");
     //-------------------------------------------------------------------------
+    rb.DefineMethod("sentinel.check.connectivity", "si", "s",
+                    FRT_METHOD(RPCHooks::rpc_checkConnectivity), this);
+    rb.MethodDesc("check connectivity for peer sentinel");
+    rb.ParamDesc("name", "Hostname of peer sentinel");
+    rb.ParamDesc("port", "Port number of peer sentinel");
+    rb.ReturnDesc("status", "Status (ok, bad, or unknown) for peer");
+    //-------------------------------------------------------------------------
 }
 
 void
@@ -76,4 +92,16 @@ RPCHooks::rpc_startService(FRT_RPCRequest *req)
     _commands.enqueue(std::make_unique<Cmd>(req, Cmd::START, srvNM));
 }
 
+void
+RPCHooks::rpc_checkConnectivity(FRT_RPCRequest *req)
+{
+    FRT_Values &args  = *req->GetParams();
+    const char *hostname = args[0]._string._str;
+    uint32_t portnum = args[1]._intval32;
+    LOG(debug, "got checkConnectivity %s [port %d]", hostname, portnum);
+    req->Detach();
+    auto & completionHandler = req->getStash().create<CheckCompletionHandler>(req);
+    req->getStash().create<PeerCheck>(completionHandler, hostname, portnum, _orb);
+}
+
 } // namespace slobrok
diff --git a/configd/src/apps/sentinel/rpchooks.h b/configd/src/apps/sentinel/rpchooks.h
index 05070830491..67f5804dcf7 100644
--- a/configd/src/apps/sentinel/rpchooks.h
+++ b/configd/src/apps/sentinel/rpchooks.h
@@ -24,17 +24,18 @@ class RPCHooks : public FRT_Invokable
 {
 private:
     CommandQueue &_commands;
-
+    FRT_Supervisor &_orb;
 public:
-    RPCHooks(CommandQueue &commands) : _commands(commands) {}
+    RPCHooks(CommandQueue &commands, FRT_Supervisor &supervisor);
     ~RPCHooks() override;
-
-    void initRPC(FRT_Supervisor *supervisor);
 private:
+    void initRPC(FRT_Supervisor *supervisor);
+
     void rpc_listServices(FRT_RPCRequest *req);
     void rpc_restartService(FRT_RPCRequest *req);
     void rpc_stopService(FRT_RPCRequest *req);
     void rpc_startService(FRT_RPCRequest *req);
+    void rpc_checkConnectivity(FRT_RPCRequest *req);
 };
 
 } // namespace config::sentinel
diff --git a/configd/src/apps/sentinel/rpcserver.cpp b/configd/src/apps/sentinel/rpcserver.cpp
index a49cba50e4d..80c3c81c826 100644
--- a/configd/src/apps/sentinel/rpcserver.cpp
+++ b/configd/src/apps/sentinel/rpcserver.cpp
@@ -9,10 +9,9 @@ namespace config::sentinel {
 
 RpcServer::RpcServer(int portNumber, CommandQueue &cmdQ)
     : _server(),
-      _rpcHooks(cmdQ),
+      _rpcHooks(cmdQ, _server.supervisor()),
       _port(portNumber)
 {
-    _rpcHooks.initRPC(&_server.supervisor());
     if (_server.supervisor().Listen(portNumber)) {
         LOG(config, "listening on port %d", portNumber);
     } else {
diff --git a/configd/src/apps/sentinel/rpcserver.h b/configd/src/apps/sentinel/rpcserver.h
index c1e7e6d0e0a..ef4b394fdca 100644
--- a/configd/src/apps/sentinel/rpcserver.h
+++ b/configd/src/apps/sentinel/rpcserver.h
@@ -22,8 +22,6 @@ public:
     ~RpcServer();
 
     int getPort() const { return _port; }
-
-    FRT_Supervisor & orb() { return _server.supervisor(); }
 };
 
 } // namespace config::sentinel
diff --git a/configd/src/apps/sentinel/status-callback.cpp b/configd/src/apps/sentinel/status-callback.cpp
index 9bd820c9f39..0bf11050b8e 100644
--- a/configd/src/apps/sentinel/status-callback.cpp
+++ b/configd/src/apps/sentinel/status-callback.cpp
@@ -3,6 +3,4 @@
 #include "status-callback.h"
 namespace config::sentinel {
 
-StatusCallback::~StatusCallback() = default;
-
 }
diff --git a/configd/src/apps/sentinel/status-callback.h b/configd/src/apps/sentinel/status-callback.h
index 53a3185623b..54100277fdc 100644
--- a/configd/src/apps/sentinel/status-callback.h
+++ b/configd/src/apps/sentinel/status-callback.h
@@ -7,7 +7,8 @@ namespace config::sentinel {
 /** very simple callback API with "ok" or "not ok" status only */
 struct StatusCallback {
     virtual void returnStatus(bool ok) = 0;
-    virtual ~StatusCallback();
+protected:
+    ~StatusCallback() = default;
 };
 
 }