guild-operators icon indicating copy to clipboard operation
guild-operators copied to clipboard

[mithril] Fix bug in checkUpdate using cncli.sh

Open TrevorBenson opened this issue 1 year ago • 6 comments

  1. Includes a syntax fix for #1757
  2. Removes unecessary UPDATE_CHECK=N exports in the workflow now that it is a default value in the Dockerfile ENV / Image.
  3. Exports G_ACCOUNT before calls to ./guild-deploy.sh to ensure if images are built in a fork that the files come from the fork, not cardano-community.
  4. Sets default G_ACCOUNT and GUILD_DEPLOY_BRANCH so manual docker build commands do not require passing --build-arg's except to alter default values.
  5. Fixes a bug leftover in the update logic and abstracts common functions from mithril scripts into a new mithril.library sourced by the scripts, reduces maintenance overhead and code duplication.
  6. Prepares for #1744 by getting the minimum versions for cardano node from mithrils new networks.json file.
  7. Implements changes suggested by @Fuma419 since branch node-8.10.0 will merge into alpha, but not node-8.9.0.
  8. Fixes a bug leftover in the update logic

1756 update bug

  • This changes the call to use $(basename "$0") for any instance of a mithril script calling checkUpdate for itself, should future proof against script renames.
    • Also runs checkUpdate for env and mithril.library.

1759 changes to mithril-client

  • The cardano-db is now the base command

    • snapshot is relocated inside cardano-db
    • download is relocated from snapshot directly into cardano-db
  • Implemented cleanup on crashes as well as user interupts. A new node should either have a full copy of the most recent snapshot, or be empty so cnode.sh has no corruption or incomplete files preventing a good sync.

  • This also changes mithril-signer.sh

    • Existing flags in mithril-signer.sh changed to have a better overlap in mithril scripts using similar OPTARGS (-u always skip updates, etc.).
    • New flags implemented that integrates the upstream mithril scripts to Verify signer registration and signer signatures.

Documentation has been updated to account for changes in the scripts usage, flags, commands and subcommands. Also the MITHRIL_DOWNLOAD variable is documented into the node-cli.md.

closes #1756 closes #1757 closes #1759

TrevorBenson avatar Apr 17 '24 22:04 TrevorBenson

Taking this back out of draft. After further testing the edge case I thought I may have found seems specific to the container setup, not the branch or the images it generates.

TrevorBenson avatar Apr 19 '24 06:04 TrevorBenson

The only bit am unsure is what does the title refer to (cncli.sh)? That file is not touched in the PR - was it something you encountered when using docker (entrypoint.sh)?

rdlrt avatar Apr 19 '24 15:04 rdlrt

The only bit am unsure is what does the title refer to (cncli.sh)? That file is not touched in the PR - was it something you encountered when using docker (entrypoint.sh)?

I think it started small and grew, hence the title. This was the referenced issue, calling update on cncli.sh in mithril-client.sh https://github.com/cardano-community/guild-operators/pull/1761/files#diff-dcb7fafc553f752e887e28294b4c42ffcea644be175251669936690333f576c1L144

Scitz0 avatar Apr 19 '24 15:04 Scitz0

I found an issue that go hidden by the final exit message being >> redirected into the log instead of | tee. The sourcing of the mithril.env doesn't export the vars for mithril signer, and it doesn't have flags to explicitely pass each value like mithril-client does, so it ends up missing CARDANO_CLI_PATH and other variables.

@rdlrt Since you approved already I wanted to bring commit eac10bf to your attention. This loops over each line in mithril.env and exports them for the signer. I also changed logging from >> to | tee -a so that its not hidden from the user in the log if an issue occurs. This should also make the journalctl contain all the signer log content for review

TrevorBenson avatar Apr 19 '24 19:04 TrevorBenson

@rdlrt Since you approved already I wanted to bring commit https://github.com/cardano-community/guild-operators/commit/eac10bff4d11d5a3d8f8d71b8bb3bdf6511d6425 to your attention.

I am alright with that, mithril in general is in early/fluid state and expect a lot of changes (personally, I treat it as in alpha state) - it's primary use-case that brings value is very simplistic (ofcourse, quite useful for those synching nodes) , bring nodes to sync using archive instead of validating chain, should one opt to.

Accordingly, I am not too fussed with how the script is being designed atm, once it stabilises enough (especially caters for multiple versions and the releases / tags are not dates 🙂 ), we can improve the scripts to be a bit more aligned.

rdlrt avatar Apr 21 '24 01:04 rdlrt

@Scitz0 Support for Sanchonet added to the current PR as discussed/requested.

  • guild-deploy.sh has been modified to accept CARDANO_NODE_VERSION from the environment when defined.
  • Sanchonet / Preview often relies on the pre-release binaries for either mithril, cardano node, or both.
    • Sanchonet branch
      • Created from and rebased continuously on alpha.
      • Uses the latest pre-release of cardano-node binaries.
      • Uses the unstable release of mithril binaries (as suggested from mithril).
    • Preview
      • Created from and rebased continuously on alpha.
      • Uses the latest full release of cardano-node binaries.
      • Uses the pre-release of mithril binaries (as suggested from mithril).
        • Except when "${release}-pre" == "${prerelease} ", then it defaults to using the full release binaries.
  • The Docker Image, docker_bin.yml, workflow has been modified to account for new testing branches.
    • Uses the CNVERSION from the files/docker/node/release-versions/cardano-node-prerelease.txt contained in the sanchonet branch.
    • Sanchonet uses configs from cardano-community, so may result in incompatible configs to pre-release node when breaking changes exist in the pre-release.
      • An alternate approach might be to get sanchonet configs from the book.world.dev.cardano.org environments. However I've left this open for discussion since breaking changes in configs are not a guarantee from a pre-release, just a posibility.
    • Preview uses the CNVERSION from the files/docker/node/release-versions/cardano-node-latest.txt contained in the preview branch.
  • A new Autoupdate testing branches autoupdate-testing.yml workflow has been added.
    • Fetches the cardano-node and mithril pre-release versions and updates them in the sanchonet and preview branches.
    • Fetches the mithril unstable version and updates it in the sanchonet branch.
      • Currently this is unstable and does not change between releases. Logic included in case mithril changes to a XXXX.Y-unstable naming convention similar to what is used in pre-release versions, in which case the workflow and CI builds should continue to function as expected.
    • Rebases both preview and sanchonet on the alpha branch.
    • If any changes occur throughout the workflow the branches are updated. If no change occurs throughout the entire worflow no update/push occurs.

The ~~cnode.sh~~ env script appears to have strong dependencies on the koios script support. The preview branch works and only uses the version in alpha. However, the sanchonet container using the suggested node and mithril binary versions results in an error on cnode.sh startup:

Koios scripts have now been upgraded to support cardano-node 8.9.x ('8.10.1' found) / cardano-cli 8.20.x.x ('8.22.0.0' found).
Please update cardano-node binaries (ensure to read release notes and update various configs using guild-deploy (use appropriate options to download/install/overwrite parts you need) or use tagged branches for older node version (eg: ./<script>.sh -b node-8.1.2 to switch scripts to an older branch).

When asked about adding support for sanchonet I had not noticed the versionCheck for versions Koios is compatible with. My first thought is to add a environment variable which allows bypassing this check on sanchonet network. However I figured I would open this up for discussion in case there was a preference on the way to handle this for sanchonet.


@rdlrt This PR has additional content added around sanchonet, workflow changes, and modifications to guild-deploy.sh. As such I am clearing the current approval and requesting an additional review. Please let me know if you have any questions or suggestions.

Thanks

TrevorBenson avatar Apr 28 '24 19:04 TrevorBenson

#@TrevorBenson - comments inline:

  • Sanchonet / Preview often relies on the pre-release binaries for either mithril, cardano node, or both.

    • Sanchonet branch
      • Created from and rebased continuously on alpha.
      • Uses the latest pre-release of cardano-node binaries.
      • Uses the unstable release of mithril binaries (as suggested from mithril).
    • Uses the CNVERSION from the files/docker/node/release-versions/cardano-node-prerelease.txt contained in the sanchonet branch.
    • Sanchonet uses configs from cardano-community, so may result in incompatible configs to pre-release node when breaking changes exist in the pre-release.

I dont think node for preview itself should rely on pre-release node binaries, it does not have fork dependency (where stable version doesnt work post a hard fork) atleast. Accordingly, I'd propose remove preview branch completely. I am OK with sanchonet branch and it's status above (as long as core scripts on alpha/master are not changed to add an error when there's a breaking change on test networks) 👍🏼 Accordingly, my comment to remove -pre reference in guild-deploy.sh

* An alternate approach might be to get sanchonet configs from the [book.world.dev.cardano.org environments](https://book.world.dev.cardano.org/environments.html). However I've left this open for discussion since breaking changes in configs are not a guarantee from a pre-release, just a posibility.

I am OK with using configs from book.play (it's quite confusing to have book.world vs book.play, but as per my understanding - more automated updates for pre-release go to book.play). The node binary itself is a bit of a playground atm, so I am not too worried about sanchonet nodes breaking between commits. The only note is that using config from book.play will require adding config file manipulation to substitute logging and change some Tracers so that it doesnt break tools that read logs

  • Preview uses the CNVERSION from the files/docker/node/release-versions/cardano-node-latest.txt contained in the preview branch.

Preview should stick to stable node release (it is OK to use test/pre-release mithril versions), thus - should not require seperate branch.

The ~cnode.sh~ env script appears to have strong dependencies on the koios script support. The preview branch works and only uses the version in alpha. However, the sanchonet container using the suggested node and mithril binary versions results in an error on cnode.sh startup:

When asked about adding support for sanchonet I had not noticed the versionCheck for versions Koios is compatible with. My first thought is to add a environment variable which allows bypassing this check on sanchonet network. However I figured I would open this up for discussion in case there was a preference on the way to handle this for sanchonet.

@rdlrt This PR has additional content added around sanchonet, workflow changes, and modifications to guild-deploy.sh. As such I am clearing the current approval and requesting an additional review. Please let me know if you have any questions or suggestions.

versionCheck can be overridden by environment variable STRICT_VERSION_CHECK. Could be something for the workflow.

rdlrt avatar May 03 '24 03:05 rdlrt

At a higher level, I'd recommend to keep the PR regarding sanchonet addition or workflow improvements seperate - mithril script changes could've been easier to merge =] Becomes easier to review as well

rdlrt avatar May 03 '24 03:05 rdlrt

At a higher level, I'd recommend to keep the PR regarding sanchonet addition or workflow improvements seperate - mithril script changes could've been easier to merge =] Becomes easier to review as well

The support for testing mithril and node pre-releases has been dropped, and will be opened in a separate PR.

TrevorBenson avatar May 03 '24 20:05 TrevorBenson

versionCheck can be overridden by environment variable STRICT_VERSION_CHECK. Could be something for the workflow.

:+1: I had overlooked the STRICT_VERSION_CHECK, but I tested this out yesterday on Sanchonet and it works perfectly.

TrevorBenson avatar May 03 '24 23:05 TrevorBenson

I am OK with using configs from book.play (it's quite confusing to have book.world vs book.play, but as per my understanding - more automated updates for pre-release go to book.play). The node binary itself is a bit of a playground atm, so I am not too worried about sanchonet nodes breaking between commits. The only note is that using config from book.play will require adding config file manipulation to substitute logging and change some Tracers so that it doesnt break tools that read logs

I'll retool this for book.play and look for changes that would have been missed when comparing book.world to configurations in the repo.

TrevorBenson avatar May 03 '24 23:05 TrevorBenson

  • Preview uses the CNVERSION from the files/docker/node/release-versions/cardano-node-latest.txt contained in the preview branch.

Preview should stick to stable node release (it is OK to use test/pre-release mithril versions), thus - should not require seperate branch.

I'll split changes for handling pre-releases of cardano node and mithril binaries required in guild-deploy.sh out of alpha and into only sanchonet/preview. The workflow to automatically rebase these branches on changes to alpha should suffice to keep it functional until merge conflicts arise, in which case I will manually resolve them.


That said, the intent to run only a stable release on the Preview network doesn't quite match upstream suggestions. Both teams state that Preview is for testing pre-release (release candidates):

Creating a Docker Image from the preview (or sanchonet) branch should create a non public image. Per GitHub docs contributors to cardano-community/guild-operators won't even have access to the images. Someone with full control of the account would need to grant individual contributors access on the cardano community packages page. Or an SPO could build them in their own fork.

I believe this covers both use case :

  • Running a stable releases on Preview:
    • Using -b master, with stable/merged guild operators tools.
    • Using -b alpha, to test out the latest changes to guild operators tools
  • Running release candidates on Preview:
    • By using -b preview, which may result in non compatible combinations of cardano-node and cntools, cncli, gLiveView, but should be compatible for prerelease node and mithril client/signer.

We can discuss this further once a new PR is opened since the work is no longer inside this branch/PR.

TrevorBenson avatar May 03 '24 23:05 TrevorBenson

That said, the intent to run only a stable release on the Preview network doesn't quite match upstream suggestions

While that maybe (I dont think I agree, some releases are specifically made available for sancho and later on said OK to run on preview) true for them, it does have a direct impact on scripts on this repository. The breaking changes added on releases need adjustments beyond download/run to be able to support. Having a higher version downloaded will automatically cause cascading effect of abundant requests to add support in scripts (which personally over the years I am not keen to - as the goal posts and comms can change quickly).

Another side-effect of having something ready is folks will reuse it preprod/mainnet too - we would not want to contribute to network issues. Sancho network is easy to reset if things go south, same cant be said for preview, as cardano does not have a comparable testnet with long enough history for any performance impact assessment on mainnet.

rdlrt avatar May 03 '24 23:05 rdlrt