Safe node removal (#4008) (#4077)

.daily_canary

@@ -1 +1 @@
-It's a new dawn, it's a new canary.
+...

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## Unreleased
 
+### Added
+
+- New `GET /node/network/removable_nodes` and `DELETE /node/network/nodes/{node_id}` exposed to allow operator to decide which nodes can be safely shut down after retirement, and clear their state from the Key-Value Store.
+
 ### Dependencies
 
 - Upgraded Open Enclave to 0.18.1 (#4023).

doc/architecture/consensus/1tx-reconfig.rst

@@ -87,7 +87,9 @@ The following sample illustrates replacing the node in a one-node network:
 
 At this point, Node 0 is aware that its retirement has been committed. It therefore stops replicating and issuing heartbeats. **However**, it does not immediately stop responding to voting requests and also does not stop propagating its own view of the global commit index. In the single node example above, the old leader Node 0 could remove itself from the network without consequences upon realizing that its retirement has been committed. For larger networks however, the leader could not do that as it would lead to situations where other nodes would not know of the global commit of the reconfiguration as the leader immediately left the network upon observing this change. In that case, followers of the old configuration may trigger timeouts that are unnecessary and potentially dangerous for the liveness of the system if they each leave the network upon noticing that the new configuration is globally committed.
 
-Instead, upon retiring from a network, retired leaders still respond to requests from followers in a way that helps to propagate the current global commit index to all other nodes and will also vote in the next election to help one of the nodes in the new configuration become elected. The leader in the old configuration will not however accept any new entries into the log or send any more heartbeats. It effectively stepped down as leader and will not replicate new messages but will stay available for queries of the latest state that it was responsible for. The old leader can leave the network or be taken offline from the network once the new configuration makes progress in its global commit (i.e., once the newly elected leader sees its global commit index increase beyond the index that included the reconfiguration itself).
+Instead, upon retiring from a network, retired leaders still respond to requests from followers in a way that helps to propagate the current commit index to all other nodes and will also vote in the next election to help one of the nodes in the new configuration become elected. The leader in the old configuration will not however accept any new entries into the log or send any more heartbeats. It effectively stepped down as leader and will not replicate new messages but will stay available for queries of the latest state that it was responsible for.
+
+The old leader can leave the network or be taken offline from the network once the new configuration makes progress in its commit (i.e., once the newly elected leader sees its commit index increase beyond the index that included the reconfiguration itself). As a convenience to the operator, the :http:GET:`/node/network/removable_nodes` exposes a list of nodes whose retirement is complete and who are no longer useful to consensus.
 
 For crash fault tolerance, this means the following: Before the reconfiguration the network could suffer f_C0 failures. After the reconfiguration, the network can suffer f_C1 failures. During the reconfiguration, the network can only suffer a maximum of f_C0 failures in the old **and** f_C1 failures in the new configuration as a failure in either configuration is unacceptable. This transitive period where the system relies on both configurations ends once the new configuration's leader's global commit index surpasses the commit that included the reconfiguration as described above.
 

doc/governance/common_member_operations.rst

@@ -15,9 +15,11 @@ Once the proposal successfully completes, the new node automatically becomes par
 Removing an Existing Node
 -------------------------
 
-A node that is already part of the service can safely be retired using the ``remove_node`` proposal.
+A node that is already part of the service can be retired using the ``remove_node`` proposal.
 
-.. note:: If the now-retired node was the primary node, once the proposal successfully completes, a new primary node will have to be elected.
+The operator can establish if it is safe to remove a node by calling :http:GET:`/node/network/removable_nodes`. Nodes that have been shut down must be cleaned up from the store by calling :http:DELETE:`/node/network/nodes/{node_id}`.
+
+.. note:: If the now-retired node was the primary node, once the proposal successfully completes, an election will take place to elect a new primary.
 
 
 Updating Code Version

doc/operations/code_upgrade.rst

@@ -156,7 +156,7 @@ Procedure
             end
         end
 
-4. Once all old nodes ``0``, ``1`` and ``2`` have been retired (and their retirement committed, as per :ref:`use_apps/verify_tx:Checking for Commit`), operators can safely stop them:
+4. Once all old nodes ``0``, ``1`` and ``2`` have been retired, and they are listed under :http:GET:`/node/network/removable_nodes`, operators can safely stop them and delete them from the state (:http:DELETE:`/node/network/nodes/{node_id}`):
 
 .. mermaid::
 

doc/schemas/node_openapi.json

@@ -813,7 +813,7 @@
   "info": {
     "description": "This API provides public, uncredentialed access to service and node state.",
     "title": "CCF Public Node API",
-    "version": "2.24.0"
+    "version": "2.28.0"
   },
   "openapi": "3.0.0",
   "paths": {
@@ -1125,6 +1125,23 @@
       }
     },
     "/node/network/nodes/{node_id}": {
+      "delete": {
+        "responses": {
+          "200": {
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/boolean"
+                }
+              }
+            },
+            "description": "Default response description"
+          }
+        },
+        "x-ccf-forwarding": {
+          "$ref": "#/components/x-ccf-forwarding/always"
+        }
+      },
       "get": {
         "responses": {
           "200": {
@@ -1153,6 +1170,25 @@
         }
       ]
     },
+    "/node/network/removable_nodes": {
+      "get": {
+        "responses": {
+          "200": {
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/GetNodes__Out"
+                }
+              }
+            },
+            "description": "Default response description"
+          }
+        },
+        "x-ccf-forwarding": {
+          "$ref": "#/components/x-ccf-forwarding/sometimes"
+        }
+      }
+    },
     "/node/primary": {
       "head": {
         "responses": {

include/ccf/ds/openapi.h

@@ -421,6 +421,11 @@ namespace ds
       const nlohmann::json& param)
     {
       auto& params = parameters(path(document, uri));
+      for (auto& p : params)
+      {
+        if (p["name"] == param["name"])
+          return;
+      }
       params.push_back(param);
     }
 

include/ccf/odata_error.h

@@ -76,6 +76,7 @@ namespace ccf
     ERROR(TransactionInvalid)
     ERROR(PrimaryNotFound)
     ERROR(RequestAlreadyForwarded)
+    ERROR(NodeNotRetiredCommitted)
 
     // node-to-node (/join and /create):
     ERROR(ConsensusTypeMismatch)

include/ccf/service/node_info.h

@@ -69,6 +69,13 @@ namespace ccf
      * node identity in `public_key` field. Service-endorsed certificate is
      * recorded in "public:ccf.nodes.endorsed_certificates" table */
     std::optional<crypto::Pem> cert = std::nullopt;
+
+    /** Commit state for Retired state
+     *
+     * Introduced during 2.x (2.0.5), and so optional for backward
+     * compatibility.
+     */
+    bool retired_committed = false;
   };
   DECLARE_JSON_TYPE_WITH_BASE_AND_OPTIONAL_FIELDS(NodeInfo, NodeInfoNetwork);
   DECLARE_JSON_REQUIRED_FIELDS(
@@ -80,7 +87,8 @@ namespace ccf
     code_digest,
     certificate_signing_request,
     public_key,
-    node_data);
+    node_data,
+    retired_committed);
 }
 
 FMT_BEGIN_NAMESPACE

python/ccf/ledger.py

@@ -367,7 +367,7 @@ class LedgerValidator:
 
     accept_deprecated_entry_types: bool = True
     node_certificates: Dict[str, str] = {}
-    node_activity_status: Dict[str, Tuple[str, int]] = {}
+    node_activity_status: Dict[str, Tuple[str, int, bool]] = {}
     signature_count: int = 0
 
     def __init__(self, accept_deprecated_entry_types: bool = True):
@@ -425,6 +425,7 @@ class LedgerValidator:
                 self.node_activity_status[node_id] = (
                     node_info["status"],
                     transaction_public_domain.get_seqno(),
+                    node_info.get("retired_committed", False),
                 )
 
         if ENDORSED_NODE_CERTIFICATES_TABLE_NAME in tables:
@@ -517,12 +518,13 @@ class LedgerValidator:
         """Verify item 1, The merkle root is signed by a valid node in the given network"""
         # Note: A retired primary will still issue signature transactions until
         # its retirement is committed
-        node_status = NodeStatus(tx_info.node_activity[tx_info.signing_node][0])
+        node_info = tx_info.node_activity[tx_info.signing_node]
+        node_status = NodeStatus(node_info[0])
         if node_status not in (
             NodeStatus.TRUSTED,
             NodeStatus.RETIRING,
             NodeStatus.RETIRED,
-        ):
+        ) or (node_status == NodeStatus.RETIRED and node_info[2]):
             raise UntrustedNodeException(
                 f"The signing node {tx_info.signing_node} has unexpected status {node_status.value}"
             )

src/node/retired_nodes_cleanup.h

@@ -22,8 +22,8 @@ namespace ccf
         fmt::format(
           "/{}/{}",
           ccf::get_actor_prefix(ccf::ActorsType::nodes),
-          "network/nodes/retired"),
+          "network/nodes/set_retired_committed"),
-        HTTP_DELETE);
+        HTTP_POST);
       request.set_header(http::headers::CONTENT_LENGTH, fmt::format("{}", 0));
 
       node_client->make_request(request);

src/node/rpc/node_frontend.h

@@ -370,7 +370,7 @@ namespace ccf
       openapi_info.description =
         "This API provides public, uncredentialed access to service and node "
         "state.";
-      openapi_info.document_version = "2.24.0";
+      openapi_info.document_version = "2.28.0";
     }
 
     void init_handlers() override
@@ -619,7 +619,7 @@ namespace ccf
         .set_openapi_hidden(true)
         .install();
 
-      auto remove_retired_nodes = [this](auto& ctx, nlohmann::json&&) {
+      auto set_retired_committed = [this](auto& ctx, nlohmann::json&&) {
         // This endpoint should only be called internally once it is certain
         // that all nodes recorded as Retired will no longer issue transactions.
         auto nodes = ctx.tx.rw(network.nodes);
@@ -631,10 +631,17 @@ namespace ccf
             node_info.status == ccf::NodeStatus::RETIRED &&
             node_id != this->context.get_node_id())
           {
-            nodes->remove(node_id);
+            // Set retired_committed on nodes for which RETIRED status
-            node_endorsed_certificates->remove(node_id);
+            // has been committed. This endpoint is only triggered for a
+            // a given node once their retirement has been committed.
+            auto node = nodes->get(node_id);
+            if (node.has_value())
+            {
+              node->retired_committed = true;
+              nodes->put(node_id, node.value());
+            }
 
-            LOG_DEBUG_FMT("Removing retired node {}", node_id);
+            LOG_DEBUG_FMT("Setting retired_committed on node {}", node_id);
           }
           return true;
         });
@@ -642,9 +649,9 @@ namespace ccf
         return make_success();
       };
       make_endpoint(
-        "network/nodes/retired",
+        "network/nodes/set_retired_committed",
-        HTTP_DELETE,
+        HTTP_POST,
-        json_adapter(remove_retired_nodes),
+        json_adapter(set_retired_committed),
         {std::make_shared<NodeCertAuthnPolicy>()})
         .set_openapi_hidden(true)
         .install();
@@ -957,6 +964,111 @@ namespace ccf
           "status", ccf::endpoints::OptionalParameter)
         .install();
 
+      auto get_removable_nodes = [this](auto& args, nlohmann::json&&) {
+        GetNodes::Out out;
+
+        auto nodes = args.tx.ro(this->network.nodes);
+        nodes->foreach(
+          [this, &out, nodes](const NodeId& node_id, const NodeInfo& ni) {
+            // Only nodes whose retire_committed status is committed can be
+            // safely removed, because any primary elected from here on would
+            // consider them retired, and would consequently not need their
+            // input in any quorum. We must therefore read the KV at its
+            // globally committed watermark, for the purpose of this RPC. Since
+            // this transaction does not perform a write, it is safe to do this.
+            auto node = nodes->get_globally_committed(node_id);
+            if (
+              node.has_value() && node->status == ccf::NodeStatus::RETIRED &&
+              node->retired_committed)
+            {
+              out.nodes.push_back(
+                {node_id,
+                 node->status,
+                 false /* is_primary */,
+                 node->rpc_interfaces,
+                 node->node_data,
+                 nodes->get_version_of_previous_write(node_id).value_or(0)});
+            }
+            return true;
+          });
+
+        return make_success(out);
+      };
+
+      make_read_only_endpoint(
+        "/network/removable_nodes",
+        HTTP_GET,
+        json_read_only_adapter(get_removable_nodes),
+        no_auth_required)
+        .set_auto_schema<void, GetNodes::Out>()
+        .install();
+
+      auto delete_retired_committed_node =
+        [this](auto& args, nlohmann::json&&) {
+          GetNodes::Out out;
+
+          std::string node_id;
+          std::string error;
+          if (!get_path_param(
+                args.rpc_ctx->get_request_path_params(),
+                "node_id",
+                node_id,
+                error))
+          {
+            return make_error(
+              HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidResourceName, error);
+          }
+
+          auto nodes = args.tx.rw(this->network.nodes);
+          if (!nodes->has(node_id))
+          {
+            return make_error(
+              HTTP_STATUS_NOT_FOUND,
+              ccf::errors::ResourceNotFound,
+              "No such node");
+          }
+
+          auto node_endorsed_certificates =
+            args.tx.rw(network.node_endorsed_certificates);
+
+          // A node's retirement is only complete when the
+          // transition of retired_committed is itself committed,
+          // i.e. when the next eligible primary is guaranteed to
+          // be aware the retirement is committed.
+          // As a result, the handler must check node info at the
+          // current committed level, rather than at the end of the
+          // local suffix.
+          // While this transaction does execute a write, it specifically
+          // deletes the value it reads from. It is therefore safe to
+          // execute on the basis of a potentially stale read-set,
+          // which get_globally_committed() typically produces.
+          auto node = nodes->get_globally_committed(node_id);
+          if (
+            node.has_value() && node->status == ccf::NodeStatus::RETIRED &&
+            node->retired_committed)
+          {
+            nodes->remove(node_id);
+            node_endorsed_certificates->remove(node_id);
+          }
+          else
+          {
+            return make_error(
+              HTTP_STATUS_BAD_REQUEST,
+              ccf::errors::NodeNotRetiredCommitted,
+              "Node is not completely retired");
+          }
+
+          return make_success(true);
+        };
+
+      make_endpoint(
+        "/network/nodes/{node_id}",
+        HTTP_DELETE,
+        json_adapter(delete_retired_committed_node),
+        no_auth_required)
+        .set_auto_schema<void, bool>()
+        .install();
+
       auto get_self_signed_certificate = [this](auto& args, nlohmann::json&&) {
         return SelfSignedNodeCertificateInfo{
           this->node_operation.get_self_signed_node_certificate()};

tests/infra/consortium.py

@@ -325,6 +325,10 @@ class Consortium:
             return r.body.json()
 
     def retire_node(self, remote_node, node_to_retire, timeout=10):
+        pending = False
+        with remote_node.client(connection_timeout=timeout) as c:
+            r = c.get(f"/node/network/nodes/{node_to_retire.node_id}")
+            pending = r.body.json()["status"] == infra.node.State.PENDING.value
         LOG.info(f"Retiring node {node_to_retire.local_node_id}")
         proposal_body, careful_vote = self.make_proposal(
             "remove_node",
@@ -332,6 +336,7 @@ class Consortium:
         )
         proposal = self.get_any_active_member().propose(remote_node, proposal_body)
         self.vote_using_majority(remote_node, proposal, careful_vote)
+        return pending
 
     def trust_node(
         self, remote_node, node_id, valid_from, validity_period_days=None, timeout=3

tests/infra/network.py

@@ -792,11 +792,35 @@ class Network:
             self.wait_for_all_nodes_to_commit(primary=primary)
 
     def retire_node(self, remote_node, node_to_retire, timeout=10):
-        self.consortium.retire_node(
+        pending = self.consortium.retire_node(
-            remote_node,
+            remote_node, node_to_retire, timeout=timeout
-            node_to_retire,
-            timeout=timeout,
         )
+        if remote_node == node_to_retire:
+            remote_node, _ = self.wait_for_new_primary(remote_node)
+        if remote_node.version_after("ccf-2.0.4") and not pending:
+            end_time = time.time() + timeout
+            r = None
+            while time.time() < end_time:
+                try:
+                    with remote_node.client(connection_timeout=timeout) as c:
+                        r = c.get("/node/network/removable_nodes").body.json()
+                        if node_to_retire.node_id in {n["node_id"] for n in r["nodes"]}:
+                            check_commit = infra.checker.Checker(c)
+                            r = c.delete(
+                                f"/node/network/nodes/{node_to_retire.node_id}"
+                            )
+                            check_commit(r)
+                            break
+                        else:
+                            r = c.get(
+                                f"/node/network/nodes/{node_to_retire.node_id}"
+                            ).body.json()
+                except ConnectionRefusedError:
+                    pass
+                time.sleep(0.1)
+            else:
+                raise TimeoutError(f"Timed out waiting for node to become removed: {r}")
+
         self.nodes.remove(node_to_retire)
 
     def create_user(self, local_user_id, curve, record=True):

tests/lts_compatibility.py

@@ -134,14 +134,6 @@ def test_new_service(
     test_all_nodes_cert_renewal(network, args, valid_from=valid_from)
     test_service_cert_renewal(network, args, valid_from=valid_from)
 
-    LOG.info("Waiting for retired nodes to be automatically removed")
-    for node in all_nodes:
-        network.wait_for_node_in_store(
-            primary,
-            node.node_id,
-            node_status=ccf.ledger.NodeStatus.TRUSTED if node.is_joined() else None,
-        )
-
     if args.check_2tx_reconfig_migration:
         test_migration_2tx_reconfiguration(
             network,

tests/reconfiguration.py

@@ -298,12 +298,6 @@ def test_retire_backup(network, args):
     primary, _ = network.find_primary()
     backup_to_retire = network.find_any_backup()
     network.retire_node(primary, backup_to_retire)
-    network.wait_for_node_in_store(
-        primary,
-        backup_to_retire.node_id,
-        node_status=None,
-        timeout=3,
-    )
     backup_to_retire.stop()
     check_can_progress(primary)
     wait_for_reconfiguration_to_complete(network)
@@ -323,14 +317,6 @@ def test_retire_primary(network, args):
     new_primary, _ = network.wait_for_new_primary(primary, nodes=[backup])
     # See https://github.com/microsoft/CCF/issues/1713
     check_can_progress(new_primary)
-    # The old primary should automatically be removed from the store
-    # once a new primary is elected
-    network.wait_for_node_in_store(
-        new_primary,
-        primary.node_id,
-        node_status=None,
-        timeout=3,
-    )
     check_can_progress(backup)
     post_count = count_nodes(node_configs(network), network)
     assert pre_count == post_count + 1