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