Skip to content
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 multiwriter #872

Merged
merged 13 commits into from
Jun 24, 2021

Conversation

spiccinini
Copy link
Contributor

@spiccinini spiccinini commented Apr 14, 2021

This PR implements the Multi Writer proposal from #868 . Thanks @germanferrero for the ideas and implementation, I have only fixed some bugs.

Fixes #868

Currently shared-state can't store information in a way that multiple nodes can modify all its keys (multi-writer).
This is because the author of a key is re-publishing it with the biggest TTL all the time, not leaving space for others to modify the value of that key. It was though for a one writer per key scheme. This scheme can be used to create "streams" of data applying inteligence above shared-state (for example the scheme used by pirania to use a new key for each modification of each voucher) but this is not only more complex, it has to be redone for each use case. So we believe that being able to have keys that can be modified by any node is a step forward to simplicity in some use cases (mainly when the informacion needs persistance).

This adds a multi-writer mode that can also persist the information so that it survives reboots of the full network (for example due to a big power loss in the region).

There are a lot of use cases, for example: List network nodes service #867

The following should me considered as premises when using the multiwriter:

  • Each key is created only by one node "at a time". So for example there is not a "mynetwork" key in the database that all the nodes create on first boot. The typical key may be the name of the the hostname that adds information of itself but other schemes are intended to be suported, for example storing information with uuids like vouchers, notifications, etc.
  • The information stored does not vary often (for example, firmware versions may be stored but not current signal levels)

This PR:

  • Provides the comand shared-state-multiwriter to access this mode/part of the database
  • Uses a permanent storage by default (maybe selectable to temporary in the future if we find an use case for that)
  • Uses a timestamp as an ordered way of modifying the value of a key so that it can be merged "monotonically".

Copy link
Member

@nicopace nicopace left a comment

Choose a reason for hiding this comment

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

great work Ger!
i've started reviewing it, though haven't finished... so gave a few comments and will come back later.

@G10h4ck
Copy link
Member

G10h4ck commented Apr 24, 2021

Uses a timestamp as an ordered way of modifying the value of a key so that it can be merged "monotonically".

shared-state originally used timestamps too, but that was quite problematic because in many situations routers doesn't have the clock synchronized so it became a mess very easily. One quite common case was shared-state running at boot before ntp managed to sync the clock, so we ended up with never expiring records timestamped to 2024 or whatever date in far future (past dates weren't problematic as they would be overwritten once the node clock synced).

How is the case node clock is out of sync (both in far past and in far future) handled?

@spiccinini
Copy link
Contributor Author

Uses a timestamp as an ordered way of modifying the value of a key so that it can be merged "monotonically".

shared-state originally used timestamps too, but that was quite problematic because in many situations routers doesn't have the clock synchronized so it because a mess very easily. One quite common case was shared-state running at boot before ntp managed to sync the clock, so we ended up with never expiring records timestamped to 2024 or whatever date in far future (past dates weren't problematic as they would be overwritten once the node clock synced).

How is the case node clock is out of sync (both in far past and in far future) handled?

Right, nodes with dates in the future are very problematic. If that case was seen in the field I propose to change the method using other merge strategy like the specified here #868 , what do you think about the other strategy? (quoting)

. Another scheme is to use a counter and a random string nounce. For example the key starts at with counter: 1, nonce: "s6qgHkMT", and then other node changes it to counter: 2, nonce: "mEVfaCb7". If the counters are equal the lexicographic order is used is used to select the winner for the merge.

@spiccinini spiccinini force-pushed the add-shared-state-multiwriter branch from 6365432 to dcb099e Compare June 17, 2021 23:45
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2021

Codecov Report

Merging #872 (60b2048) into master (9654a16) will increase coverage by 0.19%.
The diff coverage is 89.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
+ Coverage   72.45%   72.64%   +0.19%     
==========================================
  Files          41       41              
  Lines        3616     3678      +62     
==========================================
+ Hits         2620     2672      +52     
- Misses        996     1006      +10     
Impacted Files Coverage Δ
...kages/lime-system/files/usr/lib/lua/lime/utils.lua 80.23% <46.15%> (-1.39%) ⬇️
...es/shared-state/files/usr/lib/lua/shared-state.lua 76.78% <93.57%> (+4.78%) ⬆️

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 9654a16...60b2048. Read the comment docs.

@spiccinini
Copy link
Contributor Author

I've pushed a new merge strategy using a counter and a random number and one fix for a bug that german spoted.
I think that now this PR can be merged and the following improvements to shared-state to be worked in a new PR.

@germanferrero
Copy link
Contributor

Oh, the comments I made at #873 should have been here!

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 think we are now ready to merge :)

@spiccinini spiccinini requested a review from nicopace June 22, 2021 15:11
@spiccinini
Copy link
Contributor Author

As per the discused roadmap with gio (#884) this is ready for merging.

Copy link
Member

@nicopace nicopace left a comment

Choose a reason for hiding this comment

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

I skimmed through it ... it looks fine, no big issues.
thanks for the contributions!

@spiccinini spiccinini merged commit 06ea3fd into libremesh:master Jun 24, 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.

shared-state multi writer mode proposal
5 participants