patchpanel: Generate neighbor failed/recovered events
Currently, we have a "connected" state for each neighbor monitored by
NeighborLinkMonitor, which is determined by whether the NUD state is in
NUD_VALID or not, and send D-Bus signals when that state changes. This
logic has the following issues:
- We don't really need a "connected" state changed event, what we need
is the event of link failed and recovered. One difference between them
is that when the link is going down, it will be disconnected but there
may not be a failure. We don't need to emit metrics or performing the
WiFi reattach in that case.
- The current logic for this state could be problematic. NUD_VALID means
the entry in the kernel neighbor table has a layer 2 address, but not
bidirectionally reachable. For example, when the device receives a ARP
request from an unknown neighbor, it will create an entry with state
of NUD_STALE, it's valid but actually the reachability is not
confirmed. We should only rely on NUD_REACHABLE and NUD_FAILED to
defer its reachability state.
So this patch does the following changes:
- Change the signal name from "NeighborConnectedStateChanged" to
"NeighborReachablilityEvent", which contains an event type of FAILED
or RECOVERED;
- Change the logic for generating events to rely on NUD_REACHABLE and
NUD_FAILED, instead of NUD_VALID.
BUG=b:162194516
TEST=Manually set the NUD state of gateway to NUD_FAILED, observed the
FAILED log on the shill side, and another RECOVERED log after some
time.
Change-Id: I580639917ed4bcf7ffe36e5128566cde4282ae8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2532026
Reviewed-by: Hugo Benichi <hugobenichi@google.com>
Tested-by: Jie Jiang <jiejiang@chromium.org>
Commit-Queue: Jie Jiang <jiejiang@chromium.org>
diff --git a/patchpanel/dbus/client.cc b/patchpanel/dbus/client.cc
index 53fda20..ff7d85e 100644
--- a/patchpanel/dbus/client.cc
+++ b/patchpanel/dbus/client.cc
@@ -65,11 +65,11 @@
{response.counters().begin(), response.counters().end()});
}
-void OnNeighborConnectedStateChangedSignal(
- const Client::NeighborConnectedStateChangedHandler& handler,
+void OnNeighborReachabilityEventSignal(
+ const Client::NeighborReachabilityEventHandler& handler,
dbus::Signal* signal) {
dbus::MessageReader reader(signal);
- NeighborConnectedStateChangedSignal proto;
+ NeighborReachabilityEventSignal proto;
if (!reader.PopArrayOfBytesAsProto(&proto)) {
LOG(ERROR) << "Failed to parse NeighborConnectedStateChangedSignal proto";
return;
@@ -133,8 +133,8 @@
const std::string& dst_ip,
uint32_t dst_port) override;
- void RegisterNeighborConnectedStateChangedHandler(
- NeighborConnectedStateChangedHandler handler) override;
+ void RegisterNeighborReachabilityEventHandler(
+ NeighborReachabilityEventHandler handler) override;
private:
scoped_refptr<dbus::Bus> bus_;
@@ -587,11 +587,11 @@
return true;
}
-void ClientImpl::RegisterNeighborConnectedStateChangedHandler(
- NeighborConnectedStateChangedHandler handler) {
+void ClientImpl::RegisterNeighborReachabilityEventHandler(
+ NeighborReachabilityEventHandler handler) {
proxy_->ConnectToSignal(
- kPatchPanelInterface, kNeighborConnectedStateChangedSignal,
- base::BindRepeating(OnNeighborConnectedStateChangedSignal, handler),
+ kPatchPanelInterface, kNeighborReachabilityEventSignal,
+ base::BindRepeating(OnNeighborReachabilityEventSignal, handler),
base::BindOnce(OnSignalConnectedCallback));
}
diff --git a/patchpanel/dbus/client.h b/patchpanel/dbus/client.h
index 2675065..c6662bf 100644
--- a/patchpanel/dbus/client.h
+++ b/patchpanel/dbus/client.h
@@ -28,8 +28,8 @@
public:
using GetTrafficCountersCallback =
base::OnceCallback<void(const std::vector<TrafficCounter>&)>;
- using NeighborConnectedStateChangedHandler =
- base::RepeatingCallback<void(const NeighborConnectedStateChangedSignal&)>;
+ using NeighborReachabilityEventHandler =
+ base::RepeatingCallback<void(const NeighborReachabilityEventSignal&)>;
static std::unique_ptr<Client> New();
@@ -97,12 +97,12 @@
const std::string& dst_ip,
uint32_t dst_port) = 0;
- // Registers a handler that will be called on receiving a signal of neighbor
- // connected state changed. Currently these events are generated only for WiFi
+ // Registers a handler that will be called on receiving a neighbor
+ // reachability event. Currently these events are generated only for WiFi
// devices. The handler is registered for as long as this patchpanel::Client
// instance is alive.
- virtual void RegisterNeighborConnectedStateChangedHandler(
- NeighborConnectedStateChangedHandler handler) = 0;
+ virtual void RegisterNeighborReachabilityEventHandler(
+ NeighborReachabilityEventHandler handler) = 0;
protected:
Client() = default;
diff --git a/patchpanel/dbus/client_test.cc b/patchpanel/dbus/client_test.cc
index c577fda..17811fd 100644
--- a/patchpanel/dbus/client_test.cc
+++ b/patchpanel/dbus/client_test.cc
@@ -79,10 +79,10 @@
}
TEST_F(ClientTest, RegisterNeighborEventHandler) {
- static NeighborConnectedStateChangedSignal actual_signal_proto;
+ static NeighborReachabilityEventSignal actual_signal_proto;
static int call_num = 0;
auto callback =
- base::BindRepeating([](const NeighborConnectedStateChangedSignal& sig) {
+ base::BindRepeating([](const NeighborReachabilityEventSignal& sig) {
call_num++;
actual_signal_proto = sig;
});
@@ -91,17 +91,16 @@
EXPECT_CALL(*proxy_,
DoConnectToSignal(kPatchPanelInterface,
- kNeighborConnectedStateChangedSignal, _, _))
+ kNeighborReachabilityEventSignal, _, _))
.WillOnce(SaveArg<2>(®istered_dbus_callback));
- client_->RegisterNeighborConnectedStateChangedHandler(callback);
+ client_->RegisterNeighborReachabilityEventHandler(callback);
- NeighborConnectedStateChangedSignal signal_proto;
+ NeighborReachabilityEventSignal signal_proto;
signal_proto.set_ifindex(1);
signal_proto.set_ip_addr("1.2.3.4");
- signal_proto.set_role(NeighborConnectedStateChangedSignal::GATEWAY);
- signal_proto.set_connected(false);
- dbus::Signal signal(kPatchPanelInterface,
- kNeighborConnectedStateChangedSignal);
+ signal_proto.set_role(NeighborReachabilityEventSignal::GATEWAY);
+ signal_proto.set_type(NeighborReachabilityEventSignal::FAILED);
+ dbus::Signal signal(kPatchPanelInterface, kNeighborReachabilityEventSignal);
dbus::MessageWriter writer(&signal);
writer.AppendProtoAsArrayOfBytes(signal_proto);
@@ -111,7 +110,7 @@
EXPECT_EQ(actual_signal_proto.ifindex(), signal_proto.ifindex());
EXPECT_EQ(actual_signal_proto.ip_addr(), signal_proto.ip_addr());
EXPECT_EQ(actual_signal_proto.role(), signal_proto.role());
- EXPECT_EQ(actual_signal_proto.connected(), signal_proto.connected());
+ EXPECT_EQ(actual_signal_proto.type(), signal_proto.type());
}
} // namespace
diff --git a/patchpanel/dbus/fake_client.cc b/patchpanel/dbus/fake_client.cc
index 8617558..03d1f1f 100644
--- a/patchpanel/dbus/fake_client.cc
+++ b/patchpanel/dbus/fake_client.cc
@@ -90,13 +90,13 @@
return true;
}
-void FakeClient::RegisterNeighborConnectedStateChangedHandler(
- NeighborConnectedStateChangedHandler handler) {
+void FakeClient::RegisterNeighborReachabilityEventHandler(
+ NeighborReachabilityEventHandler handler) {
neighbor_handlers_.push_back(handler);
}
-void FakeClient::TriggerNeighborConnectedStateChange(
- const NeighborConnectedStateChangedSignal& signal) {
+void FakeClient::TriggerNeighborReachabilityEvent(
+ const NeighborReachabilityEventSignal& signal) {
for (const auto& handler : neighbor_handlers_)
handler.Run(signal);
}
diff --git a/patchpanel/dbus/fake_client.h b/patchpanel/dbus/fake_client.h
index b23e3c4..e09936c 100644
--- a/patchpanel/dbus/fake_client.h
+++ b/patchpanel/dbus/fake_client.h
@@ -62,12 +62,12 @@
const std::string& dst_ip,
uint32_t dst_port) override;
- void RegisterNeighborConnectedStateChangedHandler(
- NeighborConnectedStateChangedHandler handler) override;
+ void RegisterNeighborReachabilityEventHandler(
+ NeighborReachabilityEventHandler handler) override;
- // Triggers registered handlers for NeighborConnectedStateChangedSignal.
- void TriggerNeighborConnectedStateChange(
- const NeighborConnectedStateChangedSignal& signal);
+ // Triggers registered handlers for NeighborReachabilityEventSignal.
+ void TriggerNeighborReachabilityEvent(
+ const NeighborReachabilityEventSignal& signal);
void set_stored_traffic_counters(
const std::vector<TrafficCounter>& counters) {
@@ -76,7 +76,7 @@
private:
std::vector<TrafficCounter> stored_traffic_counters_;
- std::vector<NeighborConnectedStateChangedHandler> neighbor_handlers_;
+ std::vector<NeighborReachabilityEventHandler> neighbor_handlers_;
};
} // namespace patchpanel
diff --git a/patchpanel/manager.cc b/patchpanel/manager.cc
index 8562316..ac5dc06 100644
--- a/patchpanel/manager.cc
+++ b/patchpanel/manager.cc
@@ -238,7 +238,7 @@
datapath_.get(), forwarder);
network_monitor_svc_ = std::make_unique<NetworkMonitorService>(
shill_client_.get(),
- base::BindRepeating(&Manager::OnNeighborConnectedStateChanged,
+ base::BindRepeating(&Manager::OnNeighborReachabilityEvent,
weak_factory_.GetWeakPtr()));
network_monitor_svc_->Start();
@@ -789,20 +789,21 @@
return dbus_response;
}
-void Manager::OnNeighborConnectedStateChanged(
+void Manager::OnNeighborReachabilityEvent(
int ifindex,
const shill::IPAddress& ip_addr,
NeighborLinkMonitor::NeighborRole role,
- bool connected) {
+ NeighborReachabilityEventSignal::EventType event_type) {
if (!ip_addr.IsValid()) {
LOG(DFATAL) << "ip_addr is not valid";
return;
}
- using SignalProto = NeighborConnectedStateChangedSignal;
+ using SignalProto = NeighborReachabilityEventSignal;
SignalProto proto;
proto.set_ifindex(ifindex);
proto.set_ip_addr(ip_addr.ToString());
+ proto.set_type(event_type);
switch (role) {
case NeighborLinkMonitor::NeighborRole::kGateway:
proto.set_role(SignalProto::GATEWAY);
@@ -817,11 +818,10 @@
NOTREACHED();
}
- dbus::Signal signal(kPatchPanelInterface,
- kNeighborConnectedStateChangedSignal);
+ dbus::Signal signal(kPatchPanelInterface, kNeighborReachabilityEventSignal);
dbus::MessageWriter writer(&signal);
if (!writer.AppendProtoAsArrayOfBytes(proto)) {
- LOG(ERROR) << "Failed to encode proto NeighborConnectedStateChangedSignal";
+ LOG(ERROR) << "Failed to encode proto NeighborReachabilityEventSignal";
return;
}
diff --git a/patchpanel/manager.h b/patchpanel/manager.h
index 6fd5e76..fdf36bd 100644
--- a/patchpanel/manager.h
+++ b/patchpanel/manager.h
@@ -159,11 +159,12 @@
std::unique_ptr<dbus::Response> OnModifyPortRule(
dbus::MethodCall* method_call);
- // Sends out DBus signal for notifying neighbor connected state changed.
- void OnNeighborConnectedStateChanged(int ifindex,
- const shill::IPAddress& ip_addr,
- NeighborLinkMonitor::NeighborRole role,
- bool connected);
+ // Sends out DBus signal for notifying neighbor reachability event.
+ void OnNeighborReachabilityEvent(
+ int ifindex,
+ const shill::IPAddress& ip_addr,
+ NeighborLinkMonitor::NeighborRole role,
+ NeighborReachabilityEventSignal::EventType event_type);
std::unique_ptr<patchpanel::ConnectNamespaceResponse> ConnectNamespace(
base::ScopedFD client_fd,
diff --git a/patchpanel/network_monitor_service.cc b/patchpanel/network_monitor_service.cc
index 9ccdbca..a0dac16 100644
--- a/patchpanel/network_monitor_service.cc
+++ b/patchpanel/network_monitor_service.cc
@@ -13,10 +13,8 @@
#include <base/strings/strcat.h>
#include <base/threading/sequenced_task_runner_handle.h>
#include <base/logging.h>
-#include <chromeos/dbus/service_constants.h>
-
-#include "shill/net/rtnl_handler.h"
-#include "shill/net/rtnl_listener.h"
+#include <shill/net/rtnl_handler.h>
+#include <shill/net/rtnl_listener.h>
namespace patchpanel {
@@ -71,13 +69,13 @@
} // namespace
constexpr base::TimeDelta NeighborLinkMonitor::kActiveProbeInterval;
-constexpr base::TimeDelta NeighborLinkMonitor::kBackToConnectedTimeout;
+constexpr base::TimeDelta NeighborLinkMonitor::kResetFailureStateTimeout;
NeighborLinkMonitor::NeighborLinkMonitor(
int ifindex,
const std::string& ifname,
shill::RTNLHandler* rtnl_handler,
- ConnectedStateChangedHandler* neighbor_event_handler)
+ NeighborReachabilityEventHandler* neighbor_event_handler)
: ifindex_(ifindex),
ifname_(ifname),
rtnl_handler_(rtnl_handler),
@@ -316,35 +314,46 @@
if (old_nud_state == NUD_NONE && NeedProbeForState(new_nud_state))
SendNeighborProbeRTNLMessage(it->second);
- // Triggers NeighborStateChanged logic if the NUD valid state changed. Note
- // that normally if |connected| is true then the NUD state should be valid,
- // but this is not true for a new added entry because we always assume a new
- // neighbor is connected, so we treat that case specially.
- bool old_nud_state_is_valid =
- (old_nud_state & kNUDStateValid) || it->second.connected;
+ // When the "valid" state (i.e., whether kernel knows the MAC address of a
+ // neighbor) changed from valid to invalid, it doesn't always mean a failure
+ // happens: e.g., an NUD_STALE entry could be removed if it's not been
+ // accessed for a while. But it's still an uncommon case here, because we're
+ // trying to make kernel probing the neighbor periodically. Thus we would
+ // expect the NUD state stays valid if the neighbor is reachable.
+ bool old_nud_state_is_valid = old_nud_state & kNUDStateValid;
bool new_nud_state_is_valid = new_nud_state & kNUDStateValid;
- if (old_nud_state_is_valid == new_nud_state_is_valid)
+ if (old_nud_state_is_valid != new_nud_state_is_valid) {
+ LOG(INFO) << "NUD state changed on " << ifname_ << " for "
+ << it->second.ToString()
+ << ", old_state=" << NUDStateToString(old_nud_state);
+ }
+
+ // NUD_REACHABLE indicates the bidirectional reachability has been confirmed.
+ if (new_nud_state == NUD_REACHABLE) {
+ if (!it->second.in_failure ||
+ it->second.reset_failure_state_timer.IsRunning())
+ return;
+
+ it->second.reset_failure_state_timer.Start(
+ FROM_HERE, kResetFailureStateTimeout,
+ base::BindOnce(&NeighborLinkMonitor::ChangeWatchingEntryInFailureState,
+ base::Unretained(this), addr, false /* in_failure */));
return;
+ }
- LOG(INFO) << "NUD state changed on " << ifname_ << " for "
- << it->second.ToString()
- << ", old_state=" << NUDStateToString(old_nud_state);
-
- if (new_nud_state_is_valid) {
- it->second.back_to_connected_timer.Start(
- FROM_HERE, kBackToConnectedTimeout,
- base::BindOnce(&NeighborLinkMonitor::ChangeWatchingEntryState,
- base::Unretained(this), addr, true /* connected */));
- } else {
- LOG(WARNING) << "Neighbor becomes invalid on " << ifname_ << " "
+ // NUD_FAILED indicates we have a reachability issue now.
+ if (new_nud_state == NUD_FAILED) {
+ LOG(WARNING) << "Neighbor becomes NUD_FAILED from "
+ << NUDStateToString(old_nud_state) << " on " << ifname_ << " "
<< it->second.ToString();
- it->second.back_to_connected_timer.Stop();
- ChangeWatchingEntryState(addr, false /* connected */);
+ it->second.reset_failure_state_timer.Stop();
+ if (!it->second.in_failure)
+ ChangeWatchingEntryInFailureState(addr, true /* in_failure */);
}
}
-void NeighborLinkMonitor::ChangeWatchingEntryState(const shill::IPAddress& addr,
- bool connected) {
+void NeighborLinkMonitor::ChangeWatchingEntryInFailureState(
+ const shill::IPAddress& addr, bool in_failure) {
// If this function is triggered by a timer, the WatchingEntry which contains
// that timer must exist.
const auto it = watching_entries_.find(addr);
@@ -353,17 +362,23 @@
return;
}
- if (it->second.connected == connected)
+ if (it->second.in_failure == in_failure) {
+ LOG(DFATAL) << __func__ << " is called with |in_failure|=" << in_failure
+ << ", while the entry is already in that state";
return;
+ }
- it->second.connected = connected;
+ it->second.in_failure = in_failure;
+ using SignalProto = NeighborReachabilityEventSignal;
+ SignalProto::EventType event_type =
+ in_failure ? SignalProto::FAILED : SignalProto::RECOVERED;
neighbor_event_handler_->Run(ifindex_, it->second.addr, it->second.role,
- connected);
+ event_type);
}
NetworkMonitorService::NetworkMonitorService(
ShillClient* shill_client,
- const NeighborLinkMonitor::ConnectedStateChangedHandler&
+ const NeighborLinkMonitor::NeighborReachabilityEventHandler&
neighbor_event_handler)
: neighbor_event_handler_(neighbor_event_handler),
shill_client_(shill_client),
diff --git a/patchpanel/network_monitor_service.h b/patchpanel/network_monitor_service.h
index ef75012..68f90ae 100644
--- a/patchpanel/network_monitor_service.h
+++ b/patchpanel/network_monitor_service.h
@@ -15,11 +15,12 @@
#include <base/memory/weak_ptr.h>
#include <base/timer/timer.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
+#include <patchpanel/proto_bindings/patchpanel_service.pb.h>
+#include <shill/net/ip_address.h>
+#include <shill/net/rtnl_listener.h>
+#include <shill/net/rtnl_message.h>
#include "patchpanel/shill_client.h"
-#include "shill/net/ip_address.h"
-#include "shill/net/rtnl_listener.h"
-#include "shill/net/rtnl_message.h"
namespace patchpanel {
@@ -60,26 +61,30 @@
// NUD_FAILED. Then we will do nothing for this address, until we heard about
// it again from kernel.
//
-// We use the following logic to determine L2 connectivity state of a neighbor,
-// and broadcast a signal when the state changed, based on the NUD state:
-// - If the NUD state is not in NUD_VALID, the neighbor is considered as
-// "disconnected".
-// - If the NUD state is kept in NUD_VALID for a while, the neighbor is
-// considered as "connected". That means we will not send out the signal
-// immediately after the NUD state back to NUD_VALID, but wait for some time
-// to make sure it will not become invalid again soon.
-// - A new neighbor will always be considered as "connected", before we know its
-// NUD state.
+// We will broadcast a signal when the bidirectional reachability of a monitored
+// neighbor changes, based on its NUD state, as follows:
+// - If the NUD state becomes NUD_FAILED, that is a clear signal that the
+// reachability has been lost. We will consider the neighbor is "in failure"
+// now and broadcast the "FAILED" signal if its previous state was not in
+// failure. (It's possible that the NUD state becomes NUD_FAILED for several
+// times before it comes to NUD_REACHABLE, because (something makes) the
+// kernel probing that entry. We only generate one signal for that case.)
+// - If the NUD state becomes NUD_REACHABLE when the neighbor is in failure,
+// that is a clear signal that the reachability has been recovered. To avoid
+// the unstable case, we will wait for some time to make sure it will not
+// become NUD_FAILURE again soon, and after that, we will reset the in failure
+// state, and broadcast a "RECOVERED" signal.
+// Also see the comments for |WatchingEntry::in_failure|.
class NeighborLinkMonitor {
public:
static constexpr base::TimeDelta kActiveProbeInterval =
base::TimeDelta::FromSeconds(60);
- // If a neighbor does not become invalid again in kBackToConnectedTimeout
- // after it comes back to NUD_VALID, we consider it as connected. Since
- // currently the "connected" signal is only used by shill for comparing link
- // monitors, we use a relatively longer value here.
- static constexpr base::TimeDelta kBackToConnectedTimeout =
+ // If a neighbor does not become NUD_FAILED again in kResetFailureStateTimeout
+ // after it comes back to NUD_REACHABLE, we consider it as recovered from the
+ // previous failure. Since currently the RECOVERED signal is only used by
+ // shill for comparing link monitors, we use a relatively longer value here.
+ static constexpr base::TimeDelta kResetFailureStateTimeout =
base::TimeDelta::FromMinutes(3);
// Possible neighbor roles in the ipconfig. Represents each individual role by
@@ -90,16 +95,16 @@
kGatewayAndDNSServer = 0x3,
};
- using ConnectedStateChangedHandler =
- base::RepeatingCallback<void(int ifindex,
- const shill::IPAddress& ip_addr,
- NeighborRole role,
- bool connected)>;
+ using NeighborReachabilityEventHandler = base::RepeatingCallback<void(
+ int ifindex,
+ const shill::IPAddress& ip_addr,
+ NeighborRole role,
+ NeighborReachabilityEventSignal::EventType event_type)>;
NeighborLinkMonitor(int ifindex,
const std::string& ifname,
shill::RTNLHandler* rtnl_handler,
- ConnectedStateChangedHandler* neighbor_event_handler);
+ NeighborReachabilityEventHandler* neighbor_event_handler);
~NeighborLinkMonitor() = default;
NeighborLinkMonitor(const NeighborLinkMonitor&) = delete;
@@ -140,14 +145,23 @@
// changing this struct into a class if it becomes more complicated.
uint16_t nud_state = NUD_NONE;
- // Indicates the L2 connectivity state of this neighbor. See the class
- // comment above for more details.
- bool connected = true;
+ // Indicates whether we have detected a failure and the layer 2 reachability
+ // has not been recovered from that. Specifically, this state will be set
+ // when the |nud_state| changes to NUD_FAILED, and be reset when the
+ // |nud_state| changes to NUD_REACHABLE once and hasn't become NUD_FAILED
+ // again in a given period (kResetFailureStateTimeout). Note that this state
+ // doesn't exactly mean whether the neighbor is reachable currently: for
+ // instance, when the link is going down, the kernel would remove this entry
+ // from the neighbor table and thus we will get a RTM_DELNEIGH message and
+ // change the |nud_state| to NUD_NONE. Although the neighbor may not be
+ // reachable at that time, we will not consider it as a failure case, unless
+ // we get a NUD_FAILED signal.
+ bool in_failure = false;
- // This timer is set when the NUD state of neighbor back to NUD_VALID to
- // broadcast the connected signal, and reset if the NUD state becomes
- // invalid again before triggered.
- base::OneShotTimer back_to_connected_timer;
+ // This timer is used to reset |in_failure| state. It will be set on the
+ // first time when the NUD state of neighbor back to NUD_REACHABLE, and
+ // will be reset if the NUD state becomes NUD_FAILED again before triggered.
+ base::OneShotTimer reset_failure_state_timer;
};
// ProbeAll() is invoked periodically by |probe_timer_|. It will scan the
@@ -170,10 +184,11 @@
// Creates a new entry if not exist or updates the role of an existing entry.
void UpdateWatchingEntry(const shill::IPAddress& addr, NeighborRole role);
- // Sets the connected state of the watching entry with |addr| to |connected|,
+ // Sets the failure state of the watching entry with |addr| to |in_failure|,
// and invokes |neighbor_event_handler_| to sent out a signal if the state
// changes.
- void ChangeWatchingEntryState(const shill::IPAddress& addr, bool connected);
+ void ChangeWatchingEntryInFailureState(const shill::IPAddress& addr,
+ bool in_failure);
void SendNeighborDumpRTNLMessage();
void SendNeighborProbeRTNLMessage(const WatchingEntry& entry);
@@ -190,15 +205,15 @@
// RTNLHandler is a singleton object. Stores it here for test purpose.
shill::RTNLHandler* rtnl_handler_;
- const ConnectedStateChangedHandler* neighbor_event_handler_;
+ const NeighborReachabilityEventHandler* neighbor_event_handler_;
};
class NetworkMonitorService {
public:
explicit NetworkMonitorService(
ShillClient* shill_client,
- const NeighborLinkMonitor::ConnectedStateChangedHandler&
- neighbor_handler);
+ const NeighborLinkMonitor::NeighborReachabilityEventHandler&
+ neighbor_event_handler);
~NetworkMonitorService() = default;
NetworkMonitorService(const NetworkMonitorService&) = delete;
@@ -215,7 +230,7 @@
// ifname => NeighborLinkMonitor.
std::map<std::string, std::unique_ptr<NeighborLinkMonitor>>
neighbor_link_monitors_;
- NeighborLinkMonitor::ConnectedStateChangedHandler neighbor_event_handler_;
+ NeighborLinkMonitor::NeighborReachabilityEventHandler neighbor_event_handler_;
ShillClient* shill_client_;
// RTNLHandler is a singleton object. Stores it here for test purpose.
shill::RTNLHandler* rtnl_handler_;
diff --git a/patchpanel/network_monitor_service_test.cc b/patchpanel/network_monitor_service_test.cc
index 2081ca9..188b623 100644
--- a/patchpanel/network_monitor_service_test.cc
+++ b/patchpanel/network_monitor_service_test.cc
@@ -12,9 +12,9 @@
#include <base/test/task_environment.h>
#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
+#include <shill/net/mock_rtnl_handler.h>
#include "patchpanel/fake_shill_client.h"
-#include "shill/net/mock_rtnl_handler.h"
namespace patchpanel {
@@ -49,9 +49,9 @@
// Helper class for testing. Similar to mock class but only allowed one
// expectation set at the same time.
-class FakeNeighborConnectedStateChangedHandler {
+class FakeNeighborReachabilityEventHandler {
public:
- ~FakeNeighborConnectedStateChangedHandler() {
+ ~FakeNeighborReachabilityEventHandler() {
if (!enabled_)
return;
@@ -71,7 +71,7 @@
void Expect(int ifindex,
const std::string& ip_addr,
NeighborLinkMonitor::NeighborRole role,
- bool connected) {
+ NeighborReachabilityEventSignal::EventType event_type) {
EXPECT_TRUE(enabled_);
EXPECT_FALSE(expectation_set_)
<< "Expected " << ExpectationToString() << ", but not called.";
@@ -79,41 +79,43 @@
expected_ifindex_ = ifindex;
expected_ip_addr_ = ip_addr;
expected_role_ = role;
- expected_connected_ = connected;
+ expected_event_type_ = event_type;
}
void Run(int ifindex,
const shill::IPAddress& ip_addr,
NeighborLinkMonitor::NeighborRole role,
- bool connected) {
+ NeighborReachabilityEventSignal::EventType event_type) {
if (!enabled_)
return;
const std::string callback_str =
- CallbackToString(ifindex, ip_addr.ToString(), role, connected);
+ CallbackToString(ifindex, ip_addr.ToString(), role, event_type);
EXPECT_TRUE(expectation_set_)
<< callback_str << " called, but not expected.";
expectation_set_ = false;
EXPECT_TRUE((expected_ifindex_ == ifindex) &&
(expected_ip_addr_ == ip_addr.ToString()) &&
- (expected_role_ == role) && (expected_connected_ == connected))
+ (expected_role_ == role) &&
+ (expected_event_type_ == event_type))
<< "Expected " << ExpectationToString() << ", but got " << callback_str;
}
private:
- static std::string CallbackToString(int ifindex,
- const std::string& ip_addr,
- NeighborLinkMonitor::NeighborRole role,
- bool connected) {
+ static std::string CallbackToString(
+ int ifindex,
+ const std::string& ip_addr,
+ NeighborLinkMonitor::NeighborRole role,
+ NeighborReachabilityEventSignal::EventType event_type) {
return base::StrCat(
{"{ ifindex: ", base::NumberToString(ifindex), ", ip_addr: ", ip_addr,
", role: ", NeighborLinkMonitor::NeighborRoleToString(role),
- ", connected: ", base::NumberToString(connected), " }"});
+ ", type: ", base::NumberToString(event_type), " }"});
}
std::string ExpectationToString() {
return CallbackToString(expected_ifindex_, expected_ip_addr_,
- expected_role_, expected_connected_);
+ expected_role_, expected_event_type_);
}
bool enabled_ = false;
@@ -122,7 +124,8 @@
std::string expected_ip_addr_;
NeighborLinkMonitor::NeighborRole expected_role_ =
NeighborLinkMonitor::NeighborRole::kGateway;
- bool expected_connected_ = false;
+ NeighborReachabilityEventSignal::EventType expected_event_type_ =
+ NeighborReachabilityEventSignal::INVALID_EVENT_TYPE;
};
} // namespace
@@ -132,7 +135,7 @@
void SetUp() override {
mock_rtnl_handler_ = std::make_unique<shill::MockRTNLHandler>();
callback_ =
- base::BindRepeating(&FakeNeighborConnectedStateChangedHandler::Run,
+ base::BindRepeating(&FakeNeighborReachabilityEventHandler::Run,
base::Unretained(&fake_neighbor_event_handler_));
link_monitor_ = std::make_unique<NeighborLinkMonitor>(
kTestInterfaceIndex, kTestInterfaceName, mock_rtnl_handler_.get(),
@@ -187,8 +190,8 @@
base::test::TaskEnvironment task_env_{
base::test::TaskEnvironment::TimeSource::MOCK_TIME
};
- FakeNeighborConnectedStateChangedHandler fake_neighbor_event_handler_;
- NeighborLinkMonitor::ConnectedStateChangedHandler callback_;
+ FakeNeighborReachabilityEventHandler fake_neighbor_event_handler_;
+ NeighborLinkMonitor::NeighborReachabilityEventHandler callback_;
std::unique_ptr<shill::MockRTNLHandler> mock_rtnl_handler_;
std::unique_ptr<NeighborLinkMonitor> link_monitor_;
shill::RTNLListener* registered_listener_ = nullptr;
@@ -316,7 +319,7 @@
link_monitor_->OnIPConfigChanged(ipconfig);
}
-TEST_F(NeighborLinkMonitorTest, NotifyNeighborStateChanged) {
+TEST_F(NeighborLinkMonitorTest, NotifyNeighborReachabilityEvent) {
ShillClient::IPConfig ipconfig;
ipconfig.ipv4_address = "1.2.3.4";
ipconfig.ipv4_gateway = "1.2.3.5";
@@ -328,14 +331,13 @@
link_monitor_->OnIPConfigChanged(ipconfig);
NotifyNUDStateChanged("1.2.3.5", NUD_PROBE);
NotifyNUDStateChanged("1.2.3.5", NUD_REACHABLE);
- task_env_.FastForwardBy(NeighborLinkMonitor::kBackToConnectedTimeout);
+ task_env_.FastForwardBy(NeighborLinkMonitor::kResetFailureStateTimeout);
- SCOPED_TRACE(
- "Messages with NUD state not in NUD_VALID should trigger the callback "
- "once.");
+ SCOPED_TRACE("Messages with NUD_FAILED should trigger the callback once.");
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.5",
- NeighborLinkMonitor::NeighborRole::kGateway, false /* connected */);
+ NeighborLinkMonitor::NeighborRole::kGateway,
+ NeighborReachabilityEventSignal::FAILED);
NotifyNUDStateChanged("1.2.3.5", NUD_FAILED);
NotifyNUDStateChanged("1.2.3.5", NUD_FAILED);
NotifyNeighborRemoved("1.2.3.5");
@@ -344,20 +346,22 @@
NotifyNUDStateChanged("1.2.3.5", NUD_INCOMPLETE);
NotifyNUDStateChanged("1.2.3.5", NUD_REACHABLE);
NotifyNUDStateChanged("1.2.3.5", NUD_FAILED);
- task_env_.FastForwardBy(NeighborLinkMonitor::kBackToConnectedTimeout);
+ task_env_.FastForwardBy(NeighborLinkMonitor::kResetFailureStateTimeout);
SCOPED_TRACE(
- "Callback should be called at kBackToConnectedTimeout seconds after NUD "
+ "Callback should be called at kResetFailureStateTimeout seconds after "
+ "NUD "
"state becomes NUD_VALID.");
NotifyNUDStateChanged("1.2.3.5", NUD_REACHABLE);
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.5",
- NeighborLinkMonitor::NeighborRole::kGateway, true /* connected */);
- task_env_.FastForwardBy(NeighborLinkMonitor::kBackToConnectedTimeout);
+ NeighborLinkMonitor::NeighborRole::kGateway,
+ NeighborReachabilityEventSignal::RECOVERED);
+ task_env_.FastForwardBy(NeighborLinkMonitor::kResetFailureStateTimeout);
SCOPED_TRACE("No callback should come when the neighbor keeps in NUD_VALID.");
NotifyNUDStateChanged("1.2.3.5", NUD_REACHABLE);
- task_env_.FastForwardBy(NeighborLinkMonitor::kBackToConnectedTimeout * 2);
+ task_env_.FastForwardBy(NeighborLinkMonitor::kResetFailureStateTimeout * 2);
}
TEST_F(NeighborLinkMonitorTest, NeighborRole) {
@@ -373,18 +377,20 @@
link_monitor_->OnIPConfigChanged(ipconfig);
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.5",
- NeighborLinkMonitor::NeighborRole::kGateway, false /* connected */);
+ NeighborLinkMonitor::NeighborRole::kGateway,
+ NeighborReachabilityEventSignal::FAILED);
NotifyNUDStateChanged("1.2.3.5", NUD_FAILED);
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.6",
- NeighborLinkMonitor::NeighborRole::kDNSServer, false /* connected */);
+ NeighborLinkMonitor::NeighborRole::kDNSServer,
+ NeighborReachabilityEventSignal::FAILED);
NotifyNUDStateChanged("1.2.3.6", NUD_FAILED);
SCOPED_TRACE("Neighbors back to normal.");
fake_neighbor_event_handler_.Disable();
NotifyNUDStateChanged("1.2.3.5", NUD_REACHABLE);
NotifyNUDStateChanged("1.2.3.6", NUD_REACHABLE);
- task_env_.FastForwardBy(NeighborLinkMonitor::kBackToConnectedTimeout);
+ task_env_.FastForwardBy(NeighborLinkMonitor::kResetFailureStateTimeout);
fake_neighbor_event_handler_.Enable();
SCOPED_TRACE("On neighbor as gateway and DNS server failed");
@@ -394,13 +400,13 @@
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.5",
NeighborLinkMonitor::NeighborRole::kGatewayAndDNSServer,
- false /* connected */);
+ NeighborReachabilityEventSignal::FAILED);
NotifyNUDStateChanged("1.2.3.5", NUD_FAILED);
SCOPED_TRACE("Neighbors back to normal.");
fake_neighbor_event_handler_.Disable();
NotifyNUDStateChanged("1.2.3.5", NUD_REACHABLE);
- task_env_.FastForwardBy(NeighborLinkMonitor::kBackToConnectedTimeout);
+ task_env_.FastForwardBy(NeighborLinkMonitor::kResetFailureStateTimeout);
fake_neighbor_event_handler_.Enable();
SCOPED_TRACE("Swaps the roles.");
@@ -409,11 +415,13 @@
link_monitor_->OnIPConfigChanged(ipconfig);
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.5",
- NeighborLinkMonitor::NeighborRole::kDNSServer, false /* connected */);
+ NeighborLinkMonitor::NeighborRole::kDNSServer,
+ NeighborReachabilityEventSignal::FAILED);
NotifyNUDStateChanged("1.2.3.5", NUD_FAILED);
fake_neighbor_event_handler_.Expect(
kTestInterfaceIndex, "1.2.3.6",
- NeighborLinkMonitor::NeighborRole::kGateway, false /* connected */);
+ NeighborLinkMonitor::NeighborRole::kGateway,
+ NeighborReachabilityEventSignal::FAILED);
NotifyNUDStateChanged("1.2.3.6", NUD_FAILED);
}
@@ -423,13 +431,13 @@
fake_shill_client_ = shill_helper_.FakeClient();
monitor_svc_ = std::make_unique<NetworkMonitorService>(
fake_shill_client_.get(),
- base::BindRepeating(&FakeNeighborConnectedStateChangedHandler::Run,
+ base::BindRepeating(&FakeNeighborReachabilityEventHandler::Run,
base::Unretained(&fake_neighbor_event_handler_)));
mock_rtnl_handler_ = std::make_unique<shill::MockRTNLHandler>();
}
FakeShillClientHelper shill_helper_;
- FakeNeighborConnectedStateChangedHandler fake_neighbor_event_handler_;
+ FakeNeighborReachabilityEventHandler fake_neighbor_event_handler_;
std::unique_ptr<FakeShillClient> fake_shill_client_;
std::unique_ptr<shill::MockRTNLHandler> mock_rtnl_handler_;
std::unique_ptr<NetworkMonitorService> monitor_svc_;