feat: file watcher for cosmovisor

[robert-zaremba]

Adding upgrade file watcher for cosmovisor.

Currently the comswisor upgrade mechanism relays on parsing log messages. This is not reliable:

• depends on the log level output (x/upgrade uses INFO)
• can be hacked by accidentally logging user user content
• can be broken by using upgrade name which will break the regex pattern.

closes: #7703
closes: #8523
closes: #8651
closes: #8793
closes: #8964

Depends on:

• feat: Add backup option for cosmovisor #9652

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov]

Codecov Report

Merging #8590 (ed5c0cd) into master (0bcce5d) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ed5c0cd differs from pull request most recent head 25a2378. Consider uploading reports for the commit 25a2378 to get more accurate results

@@            Coverage Diff             @@
##           master    #8590      +/-   ##
==========================================
- Coverage   63.60%   63.60%   -0.01%     
==========================================
  Files         568      568              
  Lines       53509    53492      -17     
==========================================
- Hits        34034    34021      -13     
+ Misses      17550    17547       -3     
+ Partials     1925     1924       -1     

Impacted Files	Coverage Δ
store/types/gas.go	84.44% <ø> (ø)
store/types/store.go	68.75% <ø> (ø)
x/upgrade/types/plan.go	96.29% <ø> (ø)
x/upgrade/abci.go	94.73% <100.00%> (+0.14%)	⬆️
x/upgrade/keeper/keeper.go	80.18% <100.00%> (+0.08%)	⬆️
x/bank/types/key.go	70.00% <0.00%> (-12.36%)	⬇️
x/bank/migrations/v044/store.go	68.42% <0.00%> (-5.27%)	⬇️
x/upgrade/keeper/grpc_query.go	75.60% <0.00%> (ø)
crypto/keys/internal/ecdsa/privkey.go	84.21% <0.00%> (+1.75%)	⬆️
x/bank/migrations/v043/keys.go	15.38% <0.00%> (+2.88%)	⬆️

[alessio]

What problem does this solve?

[robert-zaremba]

What problem does this solve?

@alessio , This is related to our Slack chat. Cosmovisor doesn't handle correctly the upgrade restarts. The upgrade module generates an upgrade info file - which should be used for upgrade handling instead of logs.

[alessio]

@robert-zaremba yeah but tread carefully. There is no real OS-agnostic file watching mechanism. Such a thing just does not exist, operating systems provide their own primitives. Behaviour may vary quite much across different OSes.

[robert-zaremba]

@robert-zaremba yeah but tread carefully. There is no real OS-agnostic file watching mechanism. Such a thing just does not exist, operating systems provide their own primitives. Behaviour may vary quite much across different OSes.

Exactly, that's why I'm using the fsnotify package.

[alessio]

And this is exactly what I'm talking about:

From https://github.com/fsnotify/fsnotify's README

[robert-zaremba]

Re OSX - this is fine - w need at least one event.

For NFS - I don't think anyone will store blockchain using a network filesystem in production. Cosmovisor is for production setups.

Do you have a better solution in mind?

[alessio]

Yes. Don't use libraries that try to be OS-agnostic whilst it's clearly impossible to guarantee behavior consistency across different platforms. Use a timer-based mechanism maybe? Or maybe just develop a socket-based interface between the two processes.

[robert-zaremba]

@alessio - what do you mean with time based mechanism? I was thinking about creating a socket, but that would require updating SDK. @aaronc what do you think?

[alessio]

what do you mean with time based mechanism?

A time watch, e.g. checking the file for changes every interval. But that is bad too - at least it's consistently bad across all archs.

However, Robert mind updating the PR's description with rationale for this changeset, please? I'm thinking of any casual journeyman passing by here, I think they would struggle to understand what is the problem you're trying to solve 👍

[robert-zaremba]

Updated the description.

[robert-zaremba]

@alessio , @aaronc - can we reach a consensus on the strategy?

• The solution proposed in this PR (using fsnotify) works but may have problem with network file systems. I tested fsnotify on mac and linux with different partitions and it was working.
• Alessio proposal of checking frequently a file change will work. We could do it once a second.
• Previously with Aaron we were discussing socket communication between x/upgrade and cosmovisor, but we didn't want to change the SDK.

[alessio]

Previously with Aaron we were discussing socket communication between x/upgrade and cosmovisor,

That sounds like a good recipe for backdoors. It too risks to break x/upgrade determinism.

Alessio proposal of checking frequently a file change will work. We could do it once a second.

Mmm one second may be a bit too much. Why don't we better integrate external solutions such as systemd on GNU/Linux and launchd on Mac OS? They're designed to take care of things such as process start, stop and restart. Else we risk reinventing the wheel.

[alessio]

How about using signals? @robert-zaremba

SIGUSR{1,2} would be good for this

[aaronc]

ConceptACK.

Using sockets or signals might be a reasonable approach in the future, but x/upgrade would need to support it and it doesn't yet. So first we should make sure cosmovisor supports what x/upgrade already supports (which is log messages + the file) so that this can be used to migrate Stargate networks -> 0.42. We can add sockets or signals to x/upgrade if that's desirable but it's a separate issue.

[robert-zaremba]

@aaronc - why supporting log message if we have a file? Log approach seams useless and as we identified - it might be error prone.

[robert-zaremba]

SIGUSR{1,2} would be good for this

@alessio - so an App would send a signal to cosmovisor (it's parent process)? So we would need to pass a PID as a parameter to the App?

[alessio]

SIGUSR{1,2} would be good for this

@alessio - so an App would send a signal to cosmovisor (it's parent process)? So we would need to pass a PID as a parameter to the App?

Yep! That's correct. And conversely, cosmovisor may send a signal back to the app too - if necessary

[alessio]

Signals are not very well supported in Windows. But that's OK, I don't think there are actual clients running validators on Windows in prod. The good of this is that signals are standard across operating systems (e.g. Linux, Mac OS, others) that abide by the POSIX standards, so their behaviour would be consistent. Not only file watching mechanisms are very much OS-dependent but also their implementations and APIs have changed over time (and risk is that they will continue do to so).

[clevinson]

We should make sure this gets merged before changing how many logs we're putting in INFO:

• Groom Tendermint Logs tendermint/tendermint#5912
• Are there too many logs by default? #8132

[ryanchristo]

#9652 has been merged. Pre-approving with a few minor documentation suggestions.

The first line of the documentation needs to be updated to reflect changes (no long monitoring stdout):

cosmovisor is a small process manager for Cosmos SDK application binaries that monitors the governance module via stdout for incoming chain upgrade proposals.

We will need to make it clear in the release notes that auto-download with this version of cosmovisor will not work with Stargate chains. It might not hurt to add a warning at the top of the documentation as well that explains this.

[robert-zaremba]

I've merged master (and the auto backup feature)

[tac0turtle]

it seems this PR broke CI. Other prs are failing on cosmos visor tests now fail.

[robert-zaremba]

Yes, because some tests were downloading a file which was using a branch path. We fixed it.

[RaulBernal]

So, this release should works with SDK v0.42.9 or not?

[robert-zaremba]

cosmovisor @ master and upcoming cosmovisor-v1.0 doesn't support autodownload with chains running on SDK v0.42.x. Otherwise it works like a charm.

For latest cosmovisor and autodownload you need SDK v0.43.x or later.