Skip to content

Conversation

@spiccinini
Copy link
Contributor

@spiccinini spiccinini commented Apr 14, 2021

This PR adds an rpcd/ubus service that provides the list of the nodes of the network and the ability to remove one node from the list. This uses the shared-state multiwriter as main storage so it should be easiear to first review and merge the shared-state changes of PR #872 .

Most of the code was done by @germanferrero , great work!

Fixes #867

Implementation details (see rationale in the related issue #867):

  • Use the new shared-state-multiwriter:
    • The keys are the hostnames of the nodes
    • Each node inserts its own key in the database
    • Any node can modify the membership of other node (because the community should be able to mark a node as not participating anymore, for example people moved, or the name of the node changed). This is provided by ubus in this PR and its frontend in the Lime-App PR Add Reachable/Unreachable Nodes and Delete Nodes Screens lime-app#301
    • This database is persistent
    • attributes stored in the multiwriter database: hostname, ipv4, ipv6, board, fw_version, member
    • attributes computed from the nodes_and_links database plus the membership attribute: status. It can be: recently_connected, disconnected, gone. A node is considered disconnected if its member attribute is True but its key is not in the nodes_and_links database. As its key can be present but the node may be off or disconnected for a couple of minutes (until the TTL drops to 0 and it is removed from the nodes_and_links db) the status is recently_connected and not just connected so that it does not give "false information".
    • A cli to show the network nodes list

This is the CLI provided, it only shows and can't be used to remove nodes (use the ubus interface for that):
image

@nicopace
Copy link
Contributor

haven't reviewed yet, but looks very nice! thanks for the work put in this!

@germanferrero
Copy link
Contributor

The motivation for recently_connected also applies for recently_disconnected as the "disconnected" node could recently come back to the network but synchronization of it's nodes_and_links key between that node and the current node didn't happened yet.
Should we named it recently_disconnected too?

@germanferrero
Copy link
Contributor

The motivation for recently_connected also applies for recently_disconnected as the "disconnected" node could recently come back to the network but synchronization of it's nodes_and_links key between that node and the current node didn't happened yet.
Should we named it recently_disconnected too?

After chatting with @spiccinini, recently_disconnected wouldn't be accurate neither, since the node not in sync may be the current node. disconnected_or_not_sync or very_likely_disconnected would, as the time needed to spread nodes_and_links key after boot should be very short.

Copy link
Contributor

@germanferrero germanferrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @spiccinini good job :)

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2021

Codecov Report

Merging #873 (d6cd542) into master (55ec0b8) will increase coverage by 0.58%.
The diff coverage is 98.57%.

❗ Current head d6cd542 differs from pull request most recent head 1cc901c. Consider uploading reports for the commit 1cc901c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
+ Coverage   73.92%   74.51%   +0.58%     
==========================================
  Files          41       42       +1     
  Lines        3559     3629      +70     
==========================================
+ Hits         2631     2704      +73     
+ Misses        928      925       -3     
Impacted Files Coverage Δ
...kages/lime-system/files/usr/lib/lua/lime/utils.lua 81.06% <75.00%> (+0.82%) ⬆️
...-network_nodes/files/usr/lib/lua/network-nodes.lua 100.00% <100.00%> (ø)
...es/shared-state/files/usr/lib/lua/shared-state.lua 77.23% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ec0b8...1cc901c. Read the comment docs.

@spiccinini spiccinini force-pushed the add-shared-state-network-nodes branch 2 times, most recently from 07a7afa to 0dd489c Compare June 18, 2021 00:06
Copy link
Contributor

@germanferrero germanferrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left minor changes request in the comments :) Looks great!

@spiccinini spiccinini requested a review from germanferrero June 25, 2021 15:50
@spiccinini spiccinini force-pushed the add-shared-state-network-nodes branch from 52a05cd to b6331ef Compare June 25, 2021 15:54
@@ -0,0 +1,10 @@
#!/bin/sh

unique_append()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not the first time I see this function.
should it become part of a library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when we create the lime-utils package :)


local network_nodes = {}

function network_nodes.node(hostname, member, fw_version, board, ipv4, ipv6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function feels like a private function within the module... could it be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it. I've pushed a refactor for this and all the non public functions to be accesed with a leading underscore (as they need to be reachable for the tests).

@spiccinini spiccinini force-pushed the add-shared-state-network-nodes branch from b6331ef to 1cc901c Compare July 8, 2021 22:06
@spiccinini spiccinini merged commit 7e5a78e into libremesh:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List network nodes service

4 participants