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

[Mithril] - Add version check if update_check is set to 'Y' #1744

Closed
rdlrt opened this issue Mar 27, 2024 · 7 comments
Closed

[Mithril] - Add version check if update_check is set to 'Y' #1744

rdlrt opened this issue Mar 27, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request scripts Update to scripts in guild-operators

Comments

@rdlrt
Copy link
Contributor

rdlrt commented Mar 27, 2024

Describe the bug

Mithril versions need to be updated - especially post a release of node release, and can easily be missed given that not everyone would use guild-deploy scripts to perform updates. Hence, the scripts themselves should be able to check the latest version of mithril from github (if UPDATE_CHECK is set to 'Y') and prompt for update - similar to cncli.sh

@TrevorBenson
Copy link
Collaborator

What is the current logic to avoid introducing breaking changes, to set UPDATE_CHECK default to N, or do the existing update checks have a separate method to avoid updating the release binaries to an incompatible version with the current node, introducing breaking changes?

@TrevorBenson
Copy link
Collaborator

I did a cursory review after work today of env, cntools.sh and cntools.library it appears UPDATE_CHECK and checkUpdate are specific to the scripts and not any third party release binaries. I thought there might be similar updates for other binaries in the previous question.

At the moment nothing else comes to mind to prevent breaking changes from Mithril release binaries intended only for the latest node version. I'll put something together to update binaries from the scripts without guild-deoloy.sh and consider if other options exist besides setting UPDATE_CHECK to default to N.

@TrevorBenson
Copy link
Collaborator

@rdlrt

I communicated with the Mithril developers on the topic of avoiding breaking changes in input-output-hk/mithril discussion #1606 . A method should be implemented in input-output-hk/mithril issue #1615 to provide a simple way to avoid updates with breaking changes, or allow the SPO to update anyway once they understand it will lead to breaking changes between the updated Mithril binaries and the installed Cardano binaries.

After that I'll implement the logic requested in this issue.

@TrevorBenson TrevorBenson added enhancement New feature or request scripts Update to scripts in guild-operators labels Apr 14, 2024
@TrevorBenson
Copy link
Collaborator

Describe the bug

Mithril versions need to be updated - especially post a release of node release, and can easily be missed given that not everyone would use guild-deploy scripts to perform updates. Hence, the scripts themselves should be able to check the latest version of mithril from github (if UPDATE_CHECK is set to 'Y') and prompt for update - similar to cncli.sh

@rdlrt

After further review I'm a bit confused by this issues request. I checked the referenced cncli.sh script, it's cncliInit and logic for when UPDATE_CHECK is set to 'Y'. I've also reviewed the checkUpdate function inherited by cncli.sh from env. I don't see any logic where cncli.sh updates its own binary. I only see it updating the script itself, similar to other scripts, but no method for cncli.sh to actualy update the binary cncli it wraps.

From what I can tell if cardano node updates and creates a breaking change between the node version and cncli binary version the SPO would still be required to use guild-deploy.sh to update the CNCLI binary regardless if they update the cncli.sh script with its built in checks.

I'm not against building something new for Mithril to handle this outside of guild-deploy.sh, but after review I do not see a method an SPO would use to update cncli, cardano-node, or any other binary without relying on guild-deploy.sh, please advise.

@rdlrt
Copy link
Contributor Author

rdlrt commented Apr 18, 2024

After further review I'm a bit confused by this issues request. I checked the referenced cncli.sh script, it's cncliInit and logic for when UPDATE_CHECK is set to 'Y'. I've also reviewed the checkUpdate function inherited by cncli.sh from env. I don't see any logic where cncli.sh updates its own binary. I only see it updating the script itself, similar to other scripts, but no method for cncli.sh to actualy update the binary cncli it wraps.

I was referring to cncli version check (indeed it is in gLiveView.sh not cncli.sh itself) here.

TrevorBenson added a commit that referenced this issue May 4, 2024
…ID for verification / Add mithril client download to non container docs (#1761)

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
Copy link
Collaborator

@rdlrt I think the majority of this request was handled in #1761. The part which may need additional testing is the ensuring the minimum node version required for mithril against the currently installed node version, to avoid version incompatibility.

I'm tempted to close this issue as resolved, as the original issue description I believe was achieved already. If this is closed and I find that minimum version compatibility is not working as intended I will open a new issue for tracking and a PR to resolve.

@rdlrt
Copy link
Contributor Author

rdlrt commented Jul 30, 2024

Agreed

@rdlrt rdlrt closed this as completed Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request scripts Update to scripts in guild-operators
Projects
None yet
Development

No branches or pull requests

2 participants