troglobit / mrouted

The original DVMRP (dynamic multicast routing) implementation for UNIX
https://troglobit.com/projects/mrouted/
Other
83 stars 17 forks source link

[WIP] Add manual reload functionality to mrouted for hotplugged network interfaces #63

Open cguimaraes opened 3 weeks ago

cguimaraes commented 3 weeks ago

Summary

This pull request introduces a new feature to the mrouted daemon, allowing users to manually reload the daemon (via mroutectl) in cases where new network interfaces, either virtual or physical, are hotplugged. This reload command functions similarly to the existing restart command, with the key difference being that it preserves existing multicast routing rules, making it less disruptive in running network environments.

Feature Details

Testing and Validation


Not In Scope: This change does not automate the reload when interfaces are hotplugged. In future versions, the reload process could be made automatic via:

cguimaraes commented 3 weeks ago

I have rushed a bit with this PR, and it seems I overlooked too many aspects.

But your feedback is very helpful to pin point several things to improve. I will fix this PR accordingly.

As for the automated part, I would leave it for a future-self to tackle it.

cguimaraes commented 3 weeks ago

This looks like a good idea, the PR description is descriptive and the new code looks great ...

... until looking into what the code actually does -- it adds a new function reload_iface(), that's a copy-paste of restart() without free_all_prunes() and free_all_routes().

Meaning, this is essentially the same functionality, but with memory leaks. You should see loss of routing as soon as the routing socket is closed and the kernel wipes all entries from the MRIB and removes all VIFs set up. Sure, the function is quick to reopen the routing socket, and the kernel is probed again for new interfaces, but all state is just recreated, only now with uncreachable rtentrys since rtable is reset to NULL.

I have been working a bit on this PR and, this time, taking more time to understand the codebase. I hope that it now resembles a more adequate implementation.

I did some quick tests with the mroutectl show interfaces and the interfaces seem to be added and removed as expected. But, I still need to test it with the changing (virtual) mesh topology of 10 nodes and with mcjoin, as state in the original PR's message.

Also, it seems that line below is segfaulting whenever I kill mrouted with CTRL+C. I need to investigate this a little more.

n = poll(pfd, nhandlers, timeout(n) * 1000);

To top it all off, the PR describes itself as adding a 'reload' command to mroutectl, but there is no such command. Not even a single change to mroutectl in this PR, not even an update to the man page, so I suspect you are using some other method of accessing the IPC directly.

Looks like something was not synced between my machines. Now, it also includes changes in the mroutectl

troglobit commented 1 week ago

Also, it seems that line below is segfaulting whenever I kill mrouted with CTRL+C. I need to investigate this a little more.

n = poll(pfd, nhandlers, timeout(n) * 1000);

The timer handling has been completely refactored on the master branch, fixing this issue among other things, see #56