diff --git a/libuavcan/include/uavcan/protocol/dynamic_node_id_server/allocation_request_manager.hpp b/libuavcan/include/uavcan/protocol/dynamic_node_id_server/allocation_request_manager.hpp index 65cd62a0fb..30fc0915e4 100644 --- a/libuavcan/include/uavcan/protocol/dynamic_node_id_server/allocation_request_manager.hpp +++ b/libuavcan/include/uavcan/protocol/dynamic_node_id_server/allocation_request_manager.hpp @@ -24,6 +24,14 @@ namespace dynamic_node_id_server class IAllocationRequestHandler { public: + /** + * Allocation request manager uses this method to detect if it is allowed to publish follow-up responses. + */ + virtual bool canPublishFollowupAllocationResponse() const = 0; + + /** + * This method will be invoked when a new allocation request is received. + */ virtual void handleAllocationRequest(const UniqueID& unique_id, NodeID preferred_node_id) = 0; virtual ~IAllocationRequestHandler() { } @@ -42,7 +50,6 @@ class AllocationRequestManager const MonotonicDuration stage_timeout_; - bool active_; MonotonicTime last_message_timestamp_; protocol::dynamic_node_id::Allocation::FieldTypes::unique_id current_unique_id_; @@ -95,10 +102,8 @@ class AllocationRequestManager return InvalidStage; } - void broadcastIntermediateAllocationResponse() + void publishFollowupAllocationResponse() { - UAVCAN_ASSERT(active_); - protocol::dynamic_node_id::Allocation msg; msg.unique_id = current_unique_id_; UAVCAN_ASSERT(msg.unique_id.size() < msg.unique_id.capacity()); @@ -120,11 +125,6 @@ class AllocationRequestManager return; // This is a response from another allocator, ignore } - if (!active_) - { - return; // The local node is not the leader, ignore - } - /* * Reset the expected stage on timeout */ @@ -171,6 +171,7 @@ class AllocationRequestManager /* * Proceeding with allocation if possible + * Note that single-frame CAN FD allocation requests will be delivered to the server even if it's not leader. */ if (current_unique_id_.size() == current_unique_id_.capacity()) { @@ -185,7 +186,14 @@ class AllocationRequestManager } else { - broadcastIntermediateAllocationResponse(); + if (handler_.canPublishFollowupAllocationResponse()) + { + publishFollowupAllocationResponse(); + } + else + { + current_unique_id_.clear(); + } } /* @@ -197,7 +205,6 @@ class AllocationRequestManager public: AllocationRequestManager(INode& node, IAllocationRequestHandler& handler) : stage_timeout_(MonotonicDuration::fromMSec(Allocation::FOLLOWUP_TIMEOUT_MS)) - , active_(false) , handler_(handler) , allocation_sub_(node) , allocation_pub_(node) @@ -222,25 +229,8 @@ public: return 0; } - void setActive(bool x) - { - active_ = x; - if (!active_) - { - current_unique_id_.clear(); - } - } - - bool isActive() const { return active_; } - int broadcastAllocationResponse(const UniqueID& unique_id, NodeID allocated_node_id) { - if (!active_) - { - UAVCAN_ASSERT(0); - return -ErrLogic; - } - protocol::dynamic_node_id::Allocation msg; msg.unique_id.resize(msg.unique_id.capacity()); diff --git a/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/raft_core.hpp b/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/raft_core.hpp index cd79f352c4..b83f9621ee 100644 --- a/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/raft_core.hpp +++ b/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/raft_core.hpp @@ -36,11 +36,6 @@ public: */ virtual void handleLogCommitOnLeader(const Entry& committed_entry) = 0; - /** - * Assume false by default. - */ - virtual void handleLocalLeadershipChange(bool local_node_is_leader) = 0; - virtual ~IRaftLeaderMonitor() { } }; @@ -281,12 +276,6 @@ class RaftCore : private TimerBase int(server_state_), int(new_state)); trace(TraceRaftStateSwitch, new_state); - if ((ServerStateLeader == server_state_) || - (ServerStateLeader == new_state)) - { - leader_monitor_.handleLocalLeadershipChange(ServerStateLeader == new_state); - } - server_state_ = new_state; cluster_.resetAllServerIndices(); @@ -782,6 +771,11 @@ public: */ Log::Index getCommitIndex() const { return commit_index_; } + /** + * This essentially indicates whether the server could replicate log since last allocation. + */ + bool areAllLogEntriesCommitted() const { return commit_index_ == persistent_state_.getLog().getLastIndex(); } + /** * Only the leader can call @ref appendLog(). */ diff --git a/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/server.hpp b/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/server.hpp index a3f1e225e9..cf28299cec 100644 --- a/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/server.hpp +++ b/libuavcan/include/uavcan/protocol/dynamic_node_id_server/distributed/server.hpp @@ -62,7 +62,85 @@ class Server : private IAllocationRequestHandler AllocationRequestManager allocation_request_manager_; /* - * Methods + * Methods of IAllocationRequestHandler + */ + virtual bool canPublishFollowupAllocationResponse() const + { + /* + * The server is allowed to publish follow-up allocation responses only if both conditions are met: + * - The server is leader. + * - The last allocation request has been completed successfully. + * + * Why second condition? Imagine a case when there's two Raft nodes that don't hear each other - A and B, + * both of them are leaders (but only A can commit to the log, B is in a minor partition); then there's a + * client X that can exchange with both leaders, and a client Y that can exchange only with A. Such a + * situation can occur in case of a very unlikely failure of redundant interfaces. + * + * Both clients X and Y initially send a first-stage Allocation request; A responds to Y with a first-stage + * response, whereas B responds to X. Both X and Y will issue a follow-up second-stage requests, which may + * cause A to mix second-stage Allocation requests from different nodes, leading to reception of an invalid + * unique ID. When both leaders receive full unique IDs (A will receive an invalid one, B will receive a valid + * unique ID of X), only A will be able to make a commit, because B is in a minority. Since both clients were + * unable to receive node ID values in this round, they will try again later. + * + * Now, in order to prevent B from disrupting client-server communication second time around, we introduce this + * second restriction: the server cannot exchange with clients as long as its log contains uncommitted entries. + * + * Note that this restriction does not apply to allocation requests sent via CAN FD frames, as in this case + * no follow-up responses are necessary. So only CAN FD can offer reliable Allocation exchange. + */ + return raft_core_.isLeader() && raft_core_.areAllLogEntriesCommitted(); + } + + virtual void handleAllocationRequest(const UniqueID& unique_id, const NodeID preferred_node_id) + { + /* + * Note that it is possible that the local node is not leader. We will still perform the log search + * and try to find the node that requested allocation. If the node is found, response will be sent; + * otherwise the request will be ignored because only leader can add new allocations. + */ + const LazyConstructor result = + raft_core_.traverseLogFromEndUntil(UniqueIDLogPredicate(unique_id)); + + if (result.isConstructed()) + { + if (result->committed) + { + tryPublishAllocationResult(result->entry); + UAVCAN_TRACE("dynamic_node_id_server::distributed::Server", + "Allocation request served with existing allocation; node ID %d", + int(result->entry.node_id)); + } + else + { + UAVCAN_TRACE("dynamic_node_id_server::distributed::Server", + "Allocation request ignored - allocation exists but not committed yet; node ID %d", + int(result->entry.node_id)); + } + } + else + { + // TODO: new allocations can be added only if the list of unidentified nodes is not empty + if (raft_core_.isLeader()) + { + allocateNewNode(unique_id, preferred_node_id); + } + } + } + + /* + * Methods of IRaftLeaderMonitor + */ + virtual void handleLogCommitOnLeader(const protocol::dynamic_node_id::server::Entry& entry) + { + /* + * Maybe this node did not request allocation at all, we don't care, we publish anyway. + */ + tryPublishAllocationResult(entry); + } + + /* + * Private methods */ bool isNodeIDTaken(const NodeID node_id) const { @@ -86,50 +164,10 @@ class Server : private IAllocationRequestHandler const int res = raft_core_.appendLog(unique_id, allocated_node_id); if (res < 0) { - node_.registerInternalFailure("Raft log append"); + node_.registerInternalFailure("Raft log append new allocation"); } } - virtual void handleAllocationRequest(const UniqueID& unique_id, const NodeID preferred_node_id) - { - // TODO: allocation requests must not be served if the list of unidentified nodes is not empty - - const LazyConstructor result = - raft_core_.traverseLogFromEndUntil(UniqueIDLogPredicate(unique_id)); - - if (result.isConstructed()) - { - if (result->committed) - { - tryPublishAllocationResult(result->entry); - UAVCAN_TRACE("dynamic_node_id_server::distributed::Server", - "Allocation request served with existing allocation; node ID %d", - int(result->entry.node_id)); - } - else - { - UAVCAN_TRACE("dynamic_node_id_server::distributed::Server", - "Allocation request ignored - allocation exists but not committed yet; node ID %d", - int(result->entry.node_id)); - } - } - else - { - allocateNewNode(unique_id, preferred_node_id); - } - } - - virtual void handleLogCommitOnLeader(const protocol::dynamic_node_id::server::Entry& entry) - { - tryPublishAllocationResult(entry); - } - - virtual void handleLocalLeadershipChange(bool local_node_is_leader) - { - UAVCAN_TRACE("dynamic_node_id_server::distributed::Server", "I am leader: %d", int(local_node_is_leader)); - allocation_request_manager_.setActive(local_node_is_leader); - } - void tryPublishAllocationResult(const protocol::dynamic_node_id::server::Entry& entry) { const int res = allocation_request_manager_.broadcastAllocationResponse(entry.unique_id, entry.node_id); diff --git a/libuavcan/test/protocol/dynamic_node_id_server/allocation_request_manager.cpp b/libuavcan/test/protocol/dynamic_node_id_server/allocation_request_manager.cpp index a11969b088..edf82906e4 100644 --- a/libuavcan/test/protocol/dynamic_node_id_server/allocation_request_manager.cpp +++ b/libuavcan/test/protocol/dynamic_node_id_server/allocation_request_manager.cpp @@ -15,11 +15,20 @@ class AllocationRequestHandler : public uavcan::dynamic_node_id_server::IAllocat std::vector > requests_; public: + bool can_followup; + + AllocationRequestHandler() : can_followup(false) { } + virtual void handleAllocationRequest(const UniqueID& unique_id, uavcan::NodeID preferred_node_id) { requests_.push_back(std::pair(unique_id, preferred_node_id)); } + virtual bool canPublishFollowupAllocationResponse() const + { + return can_followup; + } + bool matchAndPopLastRequest(const UniqueID& unique_id, uavcan::NodeID preferred_node_id) { if (requests_.empty()) @@ -80,15 +89,12 @@ TEST(dynamic_node_id_server_AllocationRequestManager, Basic) * Request manager initialization */ AllocationRequestHandler handler; + handler.can_followup = true; AllocationRequestManager manager(nodes.a, handler); ASSERT_LE(0, manager.init()); - ASSERT_FALSE(manager.isActive()); - manager.setActive(true); - ASSERT_TRUE(manager.isActive()); - /* * Allocation */