Shared state improvements

[germanferrero]

Improvements to shared-state

• Run all publishers on boot, so the database for all shared-state db gets populated early enough with the node data at least and also all the publisher hooks run (creating for example the files /etc/bat-hosts, etc). This also allows other nodes to sync on demand with another node with high probability that that node will have it's own data available to be exchanged.
• Move load and lock/unlock logic to the lua library methods, in order to be able to interact with shared-state using the lua library.

Bug fixes

• Do use the logger passed at class initialization.

New unit test:

• New empty db is created correctly

Motivation

• The usecase that triggered these change requests is the one of a node that is aligning against another node, but cannot resolve the hostname (and interface) because that node shared-state data didn't reach this node yet. In that case we want to call shared state sync from lua against that node in an ad-hoc way.

Thanks @spiccinini :)

[germanferrero]

More tests will come :)

[G10h4ck]

I have done a quick code review without testing, github differ doesn't help much but looks good overall

[codecov-io]

Codecov Report

Merging #841 (1eea1f9) into master (0dbbc8a) will decrease coverage by 1.68%.
The diff coverage is 37.20%.

@@            Coverage Diff             @@
##           master     #841      +/-   ##
==========================================
- Coverage   72.19%   70.51%   -1.69%     
==========================================
  Files          38       39       +1     
  Lines        3334     3510     +176     
==========================================
+ Hits         2407     2475      +68     
- Misses        927     1035     +108     

Impacted Files	Coverage Δ
...es/shared-state/files/usr/lib/lua/shared-state.lua	38.28% <36.84%> (ø)
tests/utils.lua	98.86% <100.00%> (+0.01%)	⬆️

---

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 0dbbc8a...1eea1f9. Read the comment docs.

[nicopace]

I've noticed the change from procedural to OOP model, and though it might seem more organized... is that a movement that we want to do?
All of the system is built around procedural programming, and it might be confusing for others to read this approach over the other one.
Just my two cents :)

[germanferrero]

I've noticed the change from procedural to OOP model, and though it might seem more organized... is that a movement that we want to do?

Hi @nicopace, thanks for reviewing. One of the goodness of this OOP model is that it allows us to mock SharedState methods while unit testing other functions that use its services. You can see an example of this here
I didn't found a way to mock the procedural implementation, since each call to SharedState returned an independent tablewith no pointer to something you can mock in your tests.

Do we think this is enough to adopt OOP model in this case? I think it is

[nicopace]

I understand your point... there should be a way of mocking the behaviour, or at least not shifting programming paradigms, as doing so I believe shouldn't be a decision made on the basis of facilitating the tests, but as an overall design decision.

Will try to find alternatives, but also would love to hear how you feel about it... maybe if you agree on the previous statement, we can search together.

We can do a pair programming session for this for sure.

[germanferrero]

I understand your concern. From my understanding, the previous implementation was modeled following OOP aswell. You first had to "instantiate" SharedState, which returns you an "object" with kind-of methods that behaved differently depending on how you instantiate SharedState.
A more "pure" procedural implementation would be one in which we eliminate this instantiation step and pass the dataType to each function (ex: shared_state.sync('bat-hosts')). In this way we could do the mocking :)

I'm not against using OOP model when it fits well, even for a small part of the system, but yes, we can unify to procedural.
Let me know your thoughts on this

[spiccinini]

As German says, the original code was OOP in a way that you can't mock the inner methods until you have the instance at hand. This makes mocking code that instance SharedState harder (the way would be to add a constructor helper method so an instance can be provided from the outside. So I think that the new version is better than the old in this regard.

As you have sayd, the other option is to go to a "non OOP way" and exposing all the methods as module functions. This last option is what I have been doing mostly (but in pirania I used a metatable but for other reasons, I wanted to be able to overload the toString method).
I agree that OOP is harder to read for newcomers to Lua. But it gets a lot easier when used a lot (like in this module) and it is also much more like Python that there are a lot of people that know it.
I would love to hear @G10h4ck opinion.

[spiccinini]

I want to add that I am +1 on the last patch. More tests would be needed though.

[nicopace]

I am ok with it if there is general agreement... mine is not a blocker then.

[germanferrero]

It did need tests indeed! I found some (pretty sure all) bugs coming from the refactor.
I've add some tests. We still need to test sync and get_candidates_neigh, but for that more refactoring effort will be needed. I think we can merge this PR as it is, addressing those later, so we can merge new align screen changes based on this (PR is coming).

[G10h4ck]

For me it is ok to have it refactored to OOP style +1

[spiccinini]

So we tested it in a node in quintanalibre and it is working as expected, also in qemu, I think it is ready. Great work @germanferrero !

[germanferrero]

great work was yours @spiccinini

[spiccinini]

Oh I forgot that we have to change the logging as now that the logger bug is fixed it is spaming the syslog

[germanferrero]

I've test the logger behavior in a node in quintanalibre and it is not spamming the log anymore, thanks @spiccinini