Feature/ucc listener run internally on controller and agent #452

morantr · 2019-10-28T18:03:58Z

Replace cli_ucc_listener and wfa_ca module on the controller with controller and agent ucc listener threads.

tomereli

Can you explain a bit more in the commit message?

agent/src/beerocks/slave/son_slave_thread.cpp

tomereli

The first commit message is very informative, but it looks more like a brainstorming between you and yourself than a commit message. I would rephrase it to be something like:

The ucc listener in the agent should ideally reside in the backhaul manager, since there should be only one. Unfortunately, doing so causes inconsistency on send_cmdu function which on the controller uses a socket, while in the backhaul manager is using the bus.
The son slave which runs the ucc listener is defined in the agent configuration file for the sake of simplicity.

However, I have several issues with this approach:

Having 2 different ports for UCC listeners for controller and agent is confusing. Why should we run both? We are either doing controller certification OR agent certification, never in the same time, right? So I would expect a single UCC listener which supports all types of commands. In this case I would still prefer the listener to be an external application which has access to the bus and can send CMDUs to the agent and controller - this way we can have a single termination point for all UCC commands.
Regarding "First I thought to implement it in the backhaul manager, but it caused
inconsistency on send cmdu function which on the controller, uses
a socket, and on the backhaul manager uses the bus, so I have decided to
implement it on son_slave." - I think the backhaul manager is the only logical place to run this thread, if we decide to go with this approach. Also you said yourself that we need to remove the mrouter ([Refactor] Agent: single executable per radio #435 (comment)) so why not do that and align send_cmdu?

tomereli

The second commit is impossible to review, it would have been better to do it gradually so we can see which parts are copy paste from the wfa_ca / ucc listener. Is there any way you can make it easier to review? Perhaps add comments where the new implementation was modified?

tomereli

Fourth commit - I assume that a followup commit fixes test_flows.sh? Note that it has to be part of this PR, otherwise it won't pass CI..

tomereli

8th commit - I think that from this point this deserves a separate PR, after fixing the test_flows.sh.

tomereli

"Modify backhaul manager constructor" commit - Based on the commit message, this should be a fixup commit to the one that added agent_ucc_listener

morantr · 2019-10-29T16:24:06Z

The first commit message is very informative, but it looks more like a brainstorming between you and yourself than a commit message. I would rephrase it to be something like:

The ucc listener in the agent should ideally reside in the backhaul manager, since there should be only one. Unfortunately, doing so causes inconsistency on send_cmdu function which on the controller uses a socket, while in the backhaul manager is using the bus.
The son slave which runs the ucc listener is defined in the agent configuration file for the sake of simplicity.

Ok, acceptable, I will change it when I'll rebase.

However, I have several issues with this approach:

Having 2 different ports for UCC listeners for controller and agent is confusing. Why should we run both? We are either doing controller certification OR agent certification, never in the same time, right? So I would expect a single UCC listener which supports all types of commands. In this case I would still prefer the listener to be an external application which has access to the bus and can send CMDUs to the agent and controller - this way we can have a single termination point for all UCC commands.

You can use whatever port you wish, it is configurable on the controller/agent config file.
You can even shut the ucc_listener by setting the port number to zero.
But you can't use them both on the same port, because it is impossible to bind a specific port twice.
Anyway, If I remember correctly, the ucc uses different ports for Agent and Controller. Also, even if we would have use external ucc_listener we still would have to use 2 instances which listen to 2 different port because it is impossible to know which command is destined for for each entity.

Regarding "First I thought to implement it in the backhaul manager, but it caused
inconsistency on send cmdu function which on the controller, uses
a socket, and on the backhaul manager uses the bus, so I have decided to
implement it on son_slave." - I think the backhaul manager is the only logical place to run this thread, if we decide to go with this approach. Also you said yourself that we need to remove the mrouter (#435 (comment)) so why not do that and align send_cmdu?

You are right, but for that to happen, first need to do some refactoring - deleting mrouter, and turn the master thread to bus socket thread.

The second commit is impossible to review, it would have been better to do it gradually so we can see which parts are copy paste from the wfa_ca / ucc listener. Is there any way you can make it easier to review? Perhaps add comments where the new implementation was modified?

Basically I didn't change much. Most of the code is copy-past. The only changes I did is to remove all bml related code from ucc_listener parts , and on wfa_ca part, create use the newly defined virtual functions on the command handler instead of just use them directly on the file.

Fourth commit - I assume that a followup commit fixes test_flows.sh? Note that it has to be part of this PR, otherwise it won't pass CI..

I know. I am waiting for @rmelotte to finish the python script which will simulate the UCC, and then I'll push a fixup for that commit that aligns test_flows.sh with the new script.

8th commit - I think that from this point this deserves a separate PR, after fixing the test_flows.sh.

These commit stand on its own and can be part of this PR since they are related. Besides, I already did it here, and I don't think it will give us any benefit to move it now.

"Modify backhaul manager constructor" commit - Based on the commit message, this should be a fixup commit to the one that added agent_ucc_listener

Why? How is it related to it? This commit is something I "fixed" which is not related to the PR (except the reason I did it).

rmelotte · 2019-10-29T17:32:12Z

@morantr I didn't want to add commits on your branch directly in case you need to rewrite history, so I pushed to a separate branch instead: dev/test_flows_capi_commands.
At the moment, only one test is failing: test higher layer data payload.
It fails with:

status,INVALID,errorCode,param name 'tlv_value' has value with invalid format

I will look into this tomorrow, but as you can see there seem to be an issue with one of the validations of the tlv_value field.

rmelotte · 2019-10-30T09:38:55Z

I added a commit on dev/test_flows_capi_commands to fix the test that was failing.
The issue was in test_flows.sh, not the validation.

tomereli

I really like commit 162f9e2 and especially the elaborate commit message. Kudos.

agent/src/beerocks/slave/agent_ucc_listener.cpp

agent/src/beerocks/slave/agent_ucc_listener.h

agent/src/beerocks/slave/son_slave_thread.cpp

tomereli

Mostly nitpicks, apart from the socket handling by the UCC listener. It should be notified by the slave on any changes to the socket. After this we can merge it.

tomereli

I'm approving already so not to block this. But if you can fix the remaining comments it would be great.

agent/src/beerocks/slave/agent_ucc_listener.cpp

arnout

This is an excellent pull request, with nice commit messages and of course good content as well. I really like how the UCC listener has been implemented.

I've got two nitpick comments so it can be merged as is.

I'm not actually so sure about the "controller: Change database cetification buffer return value type" commit. It doesn't explain why we need to do that. But then, I don't really understand when smart pointers should be passed by reference and when by value.

arnout · 2019-11-04T15:51:40Z

common/beerocks/bcl/source/beerocks_ucc_listener.cpp

+ *     echo -n <command> | nc <bridge_ip_addr> <port>
+ */
+
+#include "../include/beerocks/bcl/beerocks_ucc_listener.h"


AFAICS the include directory is passed with a -I option, so #include <beerocks/bcl/beerocks_ucc_listener.h> should work.

(nitpick)

arnout · 2019-11-07T11:41:27Z

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

-                std::string const_bh_slave_);
+    main_thread(const config_file::sConfigSlave &config,
+                const std::set<std::string> &slave_ap_ifaces_,
+                const std::set<std::string> &slave_sta_ifaces_);


Since you're storing references to the arguments in the main_thread object, I think there should be a (doxygen) comment that clarifies that the lifetime of those arguments must surpass that of the main_thread object itself.

Add new ucc_listener configuration fields both to Controller and Agent configurations. On Controller configuration add only "ucc_listener_port" field. On Agent configuration add also "ucc_listener_port" field, but add as well 'vendor' and 'model' for version command, and 'ucc_listener_slave_hostap_iface' to decide on which son_slave to run the ucc_listenrer. First I thought to implement it in the backhaul manager, but it caused inconsistency on send cmdu function which on the controller, uses a socket, and on the backhaul manager uses the bus, so I have decided to implement it on son_slave. I had two options to decide on which son_slave to run it: 1. Implement it as a singleton class. 2. Explicitly set on configuration file on which son_slave to run it. I have chosen to set it on the configuration file because it is more simple IMO. Signed-off-by: Moran Shoeg <[email protected]>

Add ucc listener thread that is a combination of cli ucc listener and wfa_ca controller module. To make common CA_GET_VERSION command handle to work, add "BEEROCKS_VERSION" macro to common cmake file. Signed-off-by: Moran Shoeg <[email protected]>

Signed-off-by: Moran Shoeg <[email protected]>

Previously, the commands were directly sent to beerocks_cli. With wfa_ca moving away from beerocks_cli, there will be a dedicated socket for the CAPI commands. This commit adds a python script that makes it easier to send CAPI commands to the listening socket, from the shell test script (i.e. test_flows.sh). The script expects three positional arguments: the host, the port and the actual command. It connects to the target socket, send the command, and waits until it receives a COMPLETE, INVALID, or ERROR reply. The replies are printed by the script as they are received. If an INVALID or ERROR reply is received, the script ends with an exception. If the device replies with an unexpected string, the script also ends with an exception. Signed-off-by: Raphaël Mélotte <[email protected]>

The Travis builds currently only use scripts that are python3 compatible. As python2 will be deprecated soon anyway, this commit replaces it with python3. Should any later build fail because of the python version, they will need to be fixed to use python3. Signed-off-by: Raphaël Mélotte <[email protected]>

Previously the payload was built by adding a space then the gateway mac address to the payload variable. As the variable was initially empty, the resulting payload contained an extra space at the beginning. This commit initializes the payload variable so that there is no space at the beginning of the resulting payload. As the payload already contains one mac address when entering the for loop, it has to run one time less. Signed-off-by: Raphaël Mélotte <[email protected]>

controller changes: Add a new controller ucc listener thread to the controller. This change required to remove wfa_ca module from the controller to prevent compilation error due to conflicts. tools changes: update tests_flows.sh to use the python script to send commands test_flows.sh should be able to send CAPI commands to both the controller and agents. Instead of using beerocks_cli to send CAPI commands, this commit uses the new "send_CAPI_command.py". Three new functions have been added to get the container port, IP, and to send the CAPI commands. Existing calls to send_bml_command have to be updated to remove the "bml_wfa_ca_controller" and give the container name instead. Also, the eval call and additional quoting are no longer necessary and have been removed. The send_bml_command has been kept because it's still being used, to send steering requests for example. Unfortunately, it's not currently possible to retrieve the container IP without knowing the bridge name, because the IP that docker assigns itself is not the one that is actually used for the bridge. For this reason, the bridge_name had to be hard-coded to "br-lan". The changes in controller and tools are not in a separated commit, to not break the commit. Signed-off-by: Moran Shoeg <[email protected]> Signed-off-by: Raphaël Mélotte <[email protected]>

Some tests were already using check() and check_error, but not all. Also, some tests were using them, but did not return check_error, which made the test always pass if the last command succeeded (even if any previous ones failed). With the new python script to send commands, it's now also possible to check the output of the call (the script returns an error code if the device replied with an error for example). This commit adds check() calls where possible, and set and return check_error accordingly. As a consequence, failing tests should no longer be marked as "OK", even if the last command of the test succeeded. Signed-off-by: Raphaël Mélotte <[email protected]>

Remove cli_ucc_listener from cli since it is not used anymore. Signed-off-by: Moran Shoeg <[email protected]>

Signed-off-by: Moran Shoeg <[email protected]>

Remove wfa_ca messages from tlvf since there are no longer used. Signed-off-by: Moran Shoeg <[email protected]>

Signed-off-by: Moran Shoeg <[email protected]>

Preparative commit. Save controller bridge mac on controller socket so it could be used on send_cmdu_to_destination function on agent_ucc_listener. Also, change the update of master_socket (aka socket to the controller) to be in a single place. Signed-off-by: Moran Shoeg <[email protected]>

Add agent_ucc_listener to son_slave. In this scope, add to son_slave lock and unlock calls to before and after select, because it has a shared memory with the ucc_listener. Signed-off-by: Moran Shoeg <[email protected]>

While implementing agent_ucc_listener, the thought was to locate it on the backhaul manager. Eventually, it was not implemented on the backhaul manager, but some changes made during the implementation process, on class constructor and initialization which are more efficient, and makes the code cleaner, so I decided to push it. Signed-off-by: Moran Shoeg <[email protected]>

Removed the reference from the return value type of the certification buffer allocate and get function. Signed-off-by: Moran Shoeg <[email protected]>

morantr requested review from arnout and tomereli as code owners October 28, 2019 18:03

morantr requested review from Galvien and CoralMalachi October 28, 2019 18:04

tomereli suggested changes Oct 29, 2019

View reviewed changes

agent/src/beerocks/slave/son_slave_thread.cpp Show resolved Hide resolved

tomereli reviewed Oct 29, 2019

View reviewed changes

tomereli suggested changes Oct 29, 2019

View reviewed changes

tomereli reviewed Oct 29, 2019

View reviewed changes

morantr force-pushed the feature/ucc-listener-run-internally-on-controller-and-agent branch 3 times, most recently from 2c78ce2 to 865e1f4 Compare October 31, 2019 08:56

rmelotte force-pushed the feature/ucc-listener-run-internally-on-controller-and-agent branch 4 times, most recently from 3a75be2 to 3e46504 Compare November 4, 2019 11:25

rmelotte requested a review from tomereli November 5, 2019 08:40