[agent]: Refactor handlers for AP_METRICS_RESPONSE_MES… by VladyslavTupikin · Pull Request #1412 · prplfoundation/prplMesh

VladyslavTupikin · 2020-06-09T12:37:09Z

Create new structure sExpectedApMetricsResponse for
saving mid, bssid vector, cmdutx message. Update
current implementation of handle_ap_metrics_query:
clear bssid vector, cmdutx message, save mid, change
sending message to the son_slave on simple forwarding.

Add new structure and member of structure, update
handle_ap_metrics_query method.

…SAGE Create new structure sExpectedApMetricsResponse for saving mid, bssid vector, cmdutx message. Update current implementation of handle_ap_metrics_query: clear bssid vector, cmdutx message, save mid, change sending message to the son_slave on simple forwarding. Add new structure and member of structure, update handle_ap_metrics_query method. Signed-off-by: Vladyslav Tupikin <v.tupikin@inango-systems.com>

According to the issue #1328 need to do refactoring for the send_slave_ap_metric_query_message method. This method should create 1 AP_METRICS_QUERY_MESSAGE with list of bssids instead 1 message per 1 bssid. Refactored send_slave_ap_metric_query_message, replaced UINT16_MAX to 0, removed useless commentary. Signed-off-by: Vladyslav Tupikin <v.tupikin@inango-systems.com>

According to the refactor issue, need to change the logic for preparing and sending AP_METRICS_RESPONSE_MESSAGE to the controller when it was triggered by query. Need add tlv payload to the response object from sExpectedApMetricsResponse structure, then after processing all expected_bssids send response to the controler. Update handle_slave_ap_metrics_response according to the logic described above. Signed-off-by: Vladyslav Tupikin <v.tupikin@inango-systems.com>

After refactoring need to remove next: Structures: sStaTrafficStats sStaLinkMetrics sApMetricsQuery sApMetrics sApMetricsResponse Objects: m_ap_metric_query m_ap_metric_response Also erase all places where that objects was used. MAP-4.7.4_ETH_FH24G:netgear-rax40 MAP-4.7.4_ETH_FH5GL:netgear-rax40 MAP-4.7.4_ETH_FH5GH:netgear-rax40 MAP-4.7.5_ETH_FH24G:netgear-rax40 MAP-4.7.5_ETH_FH5GL:netgear-rax40 MAP-4.7.5_ETH_FH5GH:netgear-rax40 MAP-4.7.6_ETH_FH24G:netgear-rax40 MAP-4.7.6_ETH_FH5GL:netgear-rax40 MAP-4.7.6_ETH_FH5GH:netgear-rax40 Signed-off-by: Vladyslav Tupikin <v.tupikin@inango-systems.com>

mariomaz

I don't fully agree with adopted design. Also some of the tests are failing. I'll want to re-review when they pass.

mariomaz · 2020-06-12T07:04:12Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    uint16_t length = message_com::get_uds_header(cmdu_rx)->length;
+    cmdu_rx.swap(); // swap back before forwarding
+    for (const auto &soc : slaves_sockets) {
+        if (!message_com::forward_cmdu_to_uds(soc->slave, cmdu_rx, length)) {


If CMDU fails to be forwarded to one slave, backhaul manager will never get the expected response from that slave.
It's pointless forwarding the CMDU to the rest of the slaves, therefore loop should break.

mariomaz · 2020-06-12T07:05:16Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    for (const auto &soc : slaves_sockets) {
+        if (!message_com::forward_cmdu_to_uds(soc->slave, cmdu_rx, length)) {
+            LOG(ERROR) << "Failed forwarding AP_METRICS_QUERY_MESSAGE message to slave: "
+                       << tlvf::mac_to_string(soc->radio_mac);


Suggested change

<< tlvf::mac_to_string(soc->radio_mac);

<< soc->radio_mac;

There's no need to convert MAC to string

mariomaz · 2020-06-12T07:10:45Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.h

                          const sLinkNeighbor &link_neighbor, const sLinkMetrics &link_metrics,
                          ieee1905_1::eLinkMetricsType link_metrics_type);

+    struct sExpectedApMetricsResponse {


Suggested change

struct sExpectedApMetricsResponse {

struct sApMetricsResponse {

I think we should reconsider the name for this struct, though I'm not happy with this name either. Suggestions are welcome.

mariomaz · 2020-06-12T07:11:27Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.h

+        ieee1905_1::CmduMessageTx response = {response_buffer, sizeof(response_buffer)};
+    };
+
+    sExpectedApMetricsResponse m_expected_ap_metrics_response;


Suggested change

sExpectedApMetricsResponse m_expected_ap_metrics_response;

sApMetricsResponse m_ap_metrics_response_under_construction;

The same goes for the variable.

mariomaz · 2020-06-12T07:23:17Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+        if (!message_com::send_cmdu(socket->slave, cmdu_tx)) {
+            LOG(ERROR) << "Failed forwarding AP_METRICS_QUERY_MESSAGE message to son_slave";
+            ret = false;
+            continue;


No need for continue

mariomaz · 2020-06-12T07:23:49Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    }
+
+    auto ret = false;
+    for (auto socket : slaves_sockets) {


Suggested change

for (auto socket : slaves_sockets) {

for (const auto &socket : slaves_sockets) {

mariomaz · 2020-06-12T07:27:52Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

-        cmdu_rx.swap(); //swap back before forwarding
-        return send_cmdu_to_bus(cmdu_rx, controller_bridge_mac, bridge_info.mac, length);
+    if (mid != m_expected_ap_metrics_response.mid) {
+        LOG(ERROR) << "Received AP_METRICS_RESPONSE_MESSAGE with bad mid=" << std::hex << int(mid);


Please log expected mid and actual mid

It's not a "bad" MID.

More importantly: if it's MID 0, it should be forwarded as is, because then it's a channel utlilization treshold crossing (4.7.6). And in fact, I would always forward it if the MID is not the expected MID - it gives us duplicates if we had two parallel events, but as I mentioned dozens of times: that's fine.

mariomaz · 2020-06-12T07:38:58Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    if (mid == 0) {
+        LOG(DEBUG) << "Forward AP_METRICS_RESPONSE_MESSAGE to controller, mid=" << std::hex
+                   << int(mid);
+        return send_cmdu_to_bus(cmdu_rx, controller_bridge_mac, bridge_info.mac,


Now if mid is equal to 0, it can be either because channel utilization threshold is crossed or because of we are in the middle of a periodic reporting. At this point you don't know it.
However processing of the response should be different in each case: forward as is vs. save response and wait till all slaves have replied.

arnout · 2020-06-12T08:38:57Z

I haven't looked at the code, but the current implementation clearly doesn't do the right thing. E.g. the 4.7.5 tests fail because there are still 2 responses being sent (one of which is empty), as can be seen in the pcap file. There should be only one response.

test_flows should verify this. We currently don't have any test flow for 4.7.5 or 4.7.6, so that really should be added.

arnout · 2020-06-16T10:19:40Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

            return false;
        }
-        bssid.push_back(std::get<1>(bssid_tuple));
+        auto mac = std::get<1>(bssid_tuple);


(nitpick) please use bssid for a bssid.

arnout · 2020-06-16T10:23:19Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    int i = 0;
+    for (const auto &mac : bssid_list) {
+        auto list         = query->bssid_list(i);
+        std::get<0>(list) = true;


This is wrong - you have to check the first element of the tuple, not set it.

arnout · 2020-06-16T10:25:59Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

-                }
+    // Save mid and clear previous query
+    m_expected_ap_metrics_response.mid = mid;
+    m_expected_ap_metrics_response.expected_bssids.clear();


It is cleared here, but never set... This should be

Suggested change

m_expected_ap_metrics_response.expected_bssids.clear();

m_expected_ap_metrics_response.expected_bssids = bssid_list;

arnout · 2020-06-16T10:27:14Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    auto ret = false;
+    for (auto socket : slaves_sockets) {
+        if (!message_com::send_cmdu(socket->slave, cmdu_tx)) {
+            LOG(ERROR) << "Failed forwarding AP_METRICS_QUERY_MESSAGE message to son_slave";


Nitpick: it's not forwarding, just sending.

arnout · 2020-06-16T10:32:19Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

-        cmdu_rx.swap(); //swap back before forwarding
-        return send_cmdu_to_bus(cmdu_rx, controller_bridge_mac, bridge_info.mac, length);
+    if (mid != m_expected_ap_metrics_response.mid) {
+        LOG(ERROR) << "Received AP_METRICS_RESPONSE_MESSAGE with bad mid=" << std::hex << int(mid);


It's not a "bad" MID.

More importantly: if it's MID 0, it should be forwarded as is, because then it's a channel utlilization treshold crossing (4.7.6). And in fact, I would always forward it if the MID is not the expected MID - it gives us duplicates if we had two parallel events, but as I mentioned dozens of times: that's fine.

arnout · 2020-06-16T10:33:04Z

agent/src/beerocks/slave/backhaul_manager/backhaul_manager_thread.cpp

+    if (mid == 0) {
+        LOG(DEBUG) << "Forward AP_METRICS_RESPONSE_MESSAGE to controller, mid=" << std::hex
+                   << int(mid);
+        return send_cmdu_to_bus(cmdu_rx, controller_bridge_mac, bridge_info.mac,
+                                cmdu_rx.getMessageLength());
    }


This entire condition should be removed.

RanRegev · 2020-06-24T07:49:58Z

#1467 replaces this PR.

VladyslavTupikin requested review from RanRegev and arnout June 9, 2020 12:37

VladyslavTupikin self-assigned this Jun 9, 2020

VladyslavTupikin linked an issue Jun 9, 2020 that may be closed by this pull request

[TASK] Refactor AP metrics response collection #1328

Closed

VladyslavTupikin requested review from mariomaz and tomereli June 9, 2020 12:37

VladyslavTupikin force-pushed the refactor/ap_link_metrics branch 2 times, most recently from 255dd94 to 6465bd4 Compare June 9, 2020 16:51

VladyslavTupikin force-pushed the refactor/ap_link_metrics branch from 6465bd4 to e19508c Compare June 10, 2020 09:38

VladyslavTupikin force-pushed the refactor/ap_link_metrics branch from f57cc52 to afc162d Compare June 10, 2020 14:53

VladyslavTupikin marked this pull request as ready for review June 10, 2020 14:53

VladyslavTupikin requested review from kantera800, morantr, rmelotte and vitalybu as code owners June 10, 2020 14:53

VladyslavTupikin force-pushed the refactor/ap_link_metrics branch from afc162d to 8d3ebde Compare June 10, 2020 14:56

RanRegev mentioned this pull request Jun 11, 2020

[TASK] Refactor AP metrics response collection #1328

Closed

mariomaz suggested changes Jun 12, 2020

View reviewed changes

arnout assigned RanRegev and unassigned VladyslavTupikin Jun 12, 2020

arnout added the don't merge This PR is not ready for merge or review label Jun 12, 2020

arnout reviewed Jun 16, 2020

View reviewed changes

ghost added this to the M3 - Workable product milestone Jun 17, 2020

RanRegev closed this Jun 24, 2020

mariomaz mentioned this pull request Jun 29, 2020

refactor ap_metrics #1467

Open

VladyslavTupikin deleted the refactor/ap_link_metrics branch July 20, 2020 18:14

	struct sExpectedApMetricsResponse {
	struct sApMetricsResponse {

	sExpectedApMetricsResponse m_expected_ap_metrics_response;
	sApMetricsResponse m_ap_metrics_response_under_construction;

	for (auto socket : slaves_sockets) {
	for (const auto &socket : slaves_sockets) {

	m_expected_ap_metrics_response.expected_bssids.clear();
	m_expected_ap_metrics_response.expected_bssids = bssid_list;

Conversation

VladyslavTupikin commented Jun 9, 2020

Uh oh!

mariomaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnout commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanRegev commented Jun 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants