Create interface state monitor #1579

mariomaz · 2020-07-31T19:15:42Z

See PPM-18

A new class InterfaceStateMonitor has been created and used in ieee1905_transport to monitor changes in the up-and-running state of network interfaces.

vitalii-komisarenko

Great commit messages!

I added a bunch of comments, but addressing them is optional. I would rather like this code to merged and changed later if needed.

Approving.

vitalii-komisarenko · 2020-08-14T06:58:09Z

common/beerocks/bcl/include/bcl/beerocks_event_loop_impl.h

+    /**
+     * @see EventLoop::register_handlers
+     */
+    bool register_handlers(int fd, const EventHandlers &handlers) override;


Do we have to register all handlers at once?
Do we need ability to register/remove a single handler? Does underlying interface prevent us from doing that?

EventHandlers is a struct containing several EventHandler functions (on_read, on_write, on_error, ...). Method register_handlers registers all of them at once. This way the method can check if handlers have already been installed or not.

I cannot think of a situation where we would want to register/remove a single handler but that's possible by passing a EventHandlers struct with its members set accordingly. You can call register_handlers as many times as you want, provided you call remove_handlers in between.

vitalii-komisarenko · 2020-08-14T06:58:52Z

common/beerocks/bcl/source/beerocks_event_loop_impl.cpp

+//////////////////////////////////////////////////////////////////////////////
+
+// Maximal number of events to process in a single epoll_wait call
+static constexpr int MAX_POLL_EVENTS = 17;


It's a curious number, yes.
This question is for @vitalybu

No particular reason.. Just a good looking number :)

common/beerocks/bcl/source/beerocks_event_loop_impl.cpp

vitalii-komisarenko · 2020-08-14T07:05:29Z

common/beerocks/bcl/source/beerocks_event_loop_impl.cpp

+    auto add_fd_to_epoll = [&](int fd) -> bool {
+        epoll_event event = {};
+        event.data.fd     = fd;
+        event.events      = EPOLLRDHUP | EPOLLERR | EPOLLHUP;
+
+        // If read handler was set, also listen for POLL-IN events
+        if (handlers.on_read) {
+            event.events |= EPOLLIN;
+        }
+
+        // If write handler was set, also listen for POLL-OUT events
+        if (handlers.on_write) {
+            event.events |= EPOLLOUT;
+        }
+
+        if (epoll_ctl(m_epoll_fd, EPOLL_CTL_ADD, fd, &event) == -1) {
+            LOG(ERROR) << "Failed adding FD (" << fd << ") to the poll: " << strerror(errno);
+            return false;
+        }
+
+        // Map the file descriptor to the event handlers structure
+        m_fd_to_event_handlers[fd] = handlers;
+
+        return true;
+    };
+
+    // Add the file descriptor to the poll
+    if (!add_fd_to_epoll(fd)) {
+        return false;
+    }


I like this part of code :)

You should congratulate @vitalybu for it.
IMHO using a lambda here is not necessary and adds an extra level of indentation.

But it groups related code together.

Before removing the the timeout related source code, this lambda was called from two different places (that's why it was created in the first place)...

vitalii-komisarenko · 2020-08-14T07:24:20Z

common/beerocks/bcl/include/bcl/network/file_descriptor_impl.h

+     *
+     * @return True on success and false otherwise (for example, if it was already closed)
+     */
+    bool close()


I would expect function named close() to return void, but maybe other people disagree.

The close() system call returns an int to report the result. Returning bool here is meant to forward such a result.

A user aware of close system can be confused by if (close()) statement. In the system call it checks for failure, but in FileDescriptorImpl it checks for success.
Can we just return int?

common/beerocks/bcl/include/bcl/network/file_descriptor_impl.h

vitalii-komisarenko · 2020-08-14T07:29:18Z

common/beerocks/bcl/include/bcl/network/interface_state_monitor.h

+     * @param iface_name Interface name.
+     * @param iface_state Interface state (true if it is up and running).
+     */
+    using StateChangeHandler = std::function<void(const std::string &iface_name, bool iface_state)>;


Why the same function for both states? Why not on_iface_up() and on_iface_down()? Would they share too much code?

Both solutions are equivalent, but I think mine is more concise: just one handler type definition, just one setter method to register it and just one member variable to store it. From the caller's perspective it's also simpler (despite it needs an if in the handler function to check what's the new interface state)

vitalii-komisarenko · 2020-08-14T07:31:59Z

common/beerocks/bcl/include/bcl/network/sockets_impl.h

+
+class UdsSocket : public SocketAbstractImpl {
+public:
+    UdsSocket() : SocketAbstractImpl(socket(AF_UNIX, SOCK_STREAM, 0)) {}


SOCK_SEQPACKET or add an intermediate class StreamSocket to share code of TcpSocket and UdsSocket.

If we end up using SOCK_SEQPACKET, we'll derive a new class from SocketAbstractImpl

common/beerocks/bcl/source/network/interface_state_manager_impl.cpp

https://jira.prplfoundation.org/browse/PPM-18 This is a preparatory commit. As discussed in code reviews, associating a timeout to a read or write operation on a specific file descriptor is doubtedly useful and adds unnecessary complexity to the reactor design pattern. Moreover, current implementation of this feature has some flaws. This is not yet a problem because timeouts are not being used anywhere. As discussed in architecture meetings related to socket refactorings, operation timeouts and periodic actions will be handled by a new yet to be implemented Timer class (a wrapper around timerfd_create). Therefore current per-file-descriptor timeout handling will be removed. In order to use the current event loop with the new sockets API to implement the interface state monitor, some modifications are required in the event loop implementation. To simplify this task and avoid having to fix code that will be removed after all, this preparatory commit removes the timeout handling mechanism. Signed-off-by: Mario Maz <[email protected]>

https://jira.prplfoundation.org/browse/PPM-18 This is a preparatory commit. EventLoop interface and implementation are not intended to deal with sockets only (or any other specific class). Instead, EventLoop will deal with any OS resource implementing the file descriptor interface. So there is no need to couple EventLoop whith any specific class, even if using a template for that. In order to use the current event loop with the new sockets API to implement the interface state monitor, event loop interface and implementation have to be modified to use `int` type to refer to the source of events, being that integer the file descriptor of the OS resource where events occur. In the future, the file descriptor will be represented by a `FileDescriptor` class which, among other features, will close the file descriptor in its destructor. Current `Socket` class will be eventually deprecated in the future. Method to register handlers for events on a given file descriptor has been renamed from `add_event` to the more appropriate name `register_handlers`. For the same reason, the method to remove those handlers when they are not needed any more has been renamed from `del_event` to `remove_handlers`. `SocketEventLoop` class has been renamed to `EventLoopImpl` and the unit tests modified accordingly. While we are at it, a bug in the constructor of BrokerServer has been fixed (it was creating a shared_ptr from a reference). New `start()` and `stop()` methods have been added to BrokerServer to register and remove event handlers respectively. Registering handlers in `start()` instead of in the constructor prevents cppcheck from complaining about virtual methods being called from constructor. Signed-off-by: Mario Maz <[email protected]>

https://jira.prplfoundation.org/browse/PPM-18 This is a preparatory commit. Application event loop is currently defined in Ieee1905Transport::run(). Since event loop is going to be a dependency for the interface state monitor as well as for the transport class, move it to the main function so it can later be reused. The event loop must be then injected as a parameter in constructor to all classes that depend on it like, for example, the BrokerServer class. Since event loop is now a member variable of the Ieee1905Transport class, remove the methods add_event, del_event and run which are not necessary any more and update unit tests accordingly. Signed-off-by: Mario Maz <[email protected]>

https://jira.prplfoundation.org/browse/PPM-18 Message broker is currently defined in Ieee1905Transport::run(). As well as the event loop, the message broker is a dependency of the class. In order to have testable code, move it to the main function so it can later be replaced with a mock while unit testing (in a future PR). Strictly speaking, these changes are not required for this PR but produce clearer code as all dependencies are create and injected the same way. While we are at it, optimize parameter passing in methods to register a message handler (by const-ref instead of by value). Also call the stop() method of the message broker on application exit, which was not being called. Signed-off-by: Mario Maz <[email protected]>

https://jira.prplfoundation.org/browse/PPM-18 Create InterfaceStateManager class and all its dependencies. Follow-up commits will use this class in the ieee1905_transport process as a new mechanism to detect changes in the state of network interfaces, based on the new sockets API. Signed-off-by: Mario Maz <[email protected]>

https://jira.prplfoundation.org/browse/PPM-18 Use the InterfaceStateManager class in the ieee1905_transport process as a new mechanism to read and detect changes in the state of the network interfaces. Existing code in `ieee1905_transport_netlink.cpp` is deprecated and will be removed in next commit. InterfaceStateManager uses interface names. Since we should use interface names everywhere rather than interface indexes (because interfaces are not renamed while prplMesh is running and conversely, indexes can change, and because names are easier to debug than indexes), modify existing code to favor names over indexes. BTW, this change also fixes a bug in calls to `handle_interface_status_change` that use a file descriptor when they shall be using an interface index. Signed-off-by: Mario Maz <[email protected]>

https://jira.prplfoundation.org/browse/PPM-18 Remove dead code after adding the InterfaceStateManager class. Signed-off-by: Mario Maz <[email protected]>

mariomaz requested a review from RanRegev July 31, 2020 19:15

github-actions bot assigned mariomaz Jul 31, 2020

mariomaz requested review from vitalybu and arnout July 31, 2020 19:16

mariomaz force-pushed the feature/PPM-18_create_interface_state_monitor branch from 882bfee to fd7b0a1 Compare August 13, 2020 19:07

mariomaz marked this pull request as ready for review August 13, 2020 19:08

mariomaz requested review from adam1985d, kantera800, morantr, rmelotte and tomereli as code owners August 13, 2020 19:08

mariomaz requested a review from vitalii-komisarenko August 13, 2020 19:09

vitalii-komisarenko approved these changes Aug 14, 2020

View reviewed changes

mariomaz added 5 commits August 14, 2020 17:06

mariomaz force-pushed the feature/PPM-18_create_interface_state_monitor branch from 6a7d518 to 91289f4 Compare August 14, 2020 15:07

mariomaz added 2 commits August 14, 2020 17:41

transport: remove unused code

5016b5b

https://jira.prplfoundation.org/browse/PPM-18 Remove dead code after adding the InterfaceStateManager class. Signed-off-by: Mario Maz <[email protected]>

mariomaz force-pushed the feature/PPM-18_create_interface_state_monitor branch from 91289f4 to 5016b5b Compare August 14, 2020 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create interface state monitor #1579

Create interface state monitor #1579

mariomaz commented Jul 31, 2020

vitalii-komisarenko left a comment

vitalii-komisarenko Aug 14, 2020

mariomaz Aug 14, 2020

vitalii-komisarenko Aug 14, 2020

mariomaz Aug 14, 2020

vitalybu Aug 15, 2020

vitalii-komisarenko Aug 14, 2020

mariomaz Aug 14, 2020

vitalii-komisarenko Aug 14, 2020

vitalybu Aug 15, 2020

vitalii-komisarenko Aug 14, 2020

mariomaz Aug 14, 2020

vitalii-komisarenko Aug 14, 2020

vitalii-komisarenko Aug 14, 2020

mariomaz Aug 14, 2020

vitalii-komisarenko Aug 14, 2020

mariomaz Aug 14, 2020

Create interface state monitor #1579

Are you sure you want to change the base?

Create interface state monitor #1579

Conversation

mariomaz commented Jul 31, 2020

vitalii-komisarenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment