-
-
Notifications
You must be signed in to change notification settings - Fork 113
Add shared state network nodes #873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add shared state network nodes #873
Conversation
|
haven't reviewed yet, but looks very nice! thanks for the work put in this! |
|
The motivation for |
After chatting with @spiccinini, |
germanferrero
left a comment
There was a problem hiding this 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 :)
0dc59bc to
4477880
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
07a7afa to
0dd489c
Compare
germanferrero
left a comment
There was a problem hiding this 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!
52a05cd to
b6331ef
Compare
| @@ -0,0 +1,10 @@ | |||
| #!/bin/sh | |||
|
|
|||
| unique_append() | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
packages/shared-state-network_nodes/files/usr/lib/lua/network-nodes.lua
Outdated
Show resolved
Hide resolved
b6331ef to
1cc901c
Compare
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):
hostname, ipv4, ipv6, board, fw_version, membernodes_and_linksdatabase plus themembershipattribute: 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 thenodes_and_linksdatabase. 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 isrecently_connectedand not justconnectedso that it does not give "false information".This is the CLI provided, it only shows and can't be used to remove nodes (use the ubus interface for that):
