[go: nahoru, domu]

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

feat: periodic version check #10438

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Matrix89
Copy link
@Matrix89 Matrix89 commented Jun 4, 2024

This PR builds on top of @schomatis work from #8839, it moves his version checking logic to its own file and adds a ticker which runs it every hour.

Closes #6487

@Matrix89 Matrix89 requested a review from a team as a code owner June 4, 2024 22:49
@lidel lidel mentioned this pull request Jun 11, 2024
12 tasks
@Matrix89 Matrix89 force-pushed the feat/notify-outdated-version branch from b4a5919 to 73f5b36 Compare June 15, 2024 11:50
@Matrix89
Copy link
Author

Hey, thanks for triggering the CI. I've synced examples/kubo-as-a-library/go.mod with go.mod and added the version check command to the commands test, I don't know if the other workflow failures are related.

Is there an easy way to run workflows locally(act doesn't seem to work)?

@lidel lidel changed the title Add periodic version check feat: periodic version check Jun 18, 2024
@lidel
Copy link
Member
lidel commented Jun 19, 2024

Thank you @Matrix89, I'll try to review and squeeze this into 0.30, the sooner we ship the notice the better.

If you want to run tests locally, you get 90% of them by running cd test/cli and go test ./.... Some sharness tests are flaky, but the one below suggest a panic:

To expedite this PR I'll look into this later today, but will timebox and let you know if I've run out of time.

TODO for self

  • fix panic from CI
  • UX
    • move warning to a logger, like we already do with provider warning in core/node/provider.go
    • link to releases page
    • ability to disable check
  • future-proof / defensive programming
    • disable check when ipfs daemon --offline
    • test with custom Routing.Routers
    • (aka fix or disable when no WAN/LAN DHT)

Preview

$ ipfs daemon

[..]

WebUI: http://127.0.0.1:5021/webui
Gateway server listening on /ip4/127.0.0.1/tcp/8580
Daemon is ready
2024-06-19T23:27:54.111+0200	ERROR	cmd/ipfs	kubo/daemon.go:1076	
⚠️ A NEW VERSION OF KUBO DETECTED

This Kubo node is running an outdated version (0.21.0).
63% of the sampled Kubo peers are running a higher version.
Visit https://github.com/ipfs/kubo/releases or https://dist.ipfs.tech/#kubo and update to version 0.29.0 or later.

schomatis and others added 4 commits June 20, 2024 00:38
This refactor applies changes listed in
ipfs#10438 (comment)
namely
- removes surface for panics on custom routing configurations
- avoids running the check until one minute after node start, to allow
  for peerbook to populate
- allow disabling version check via env. variable  (because there will
  be ask for this anyway)
- make RPC command JSON more useful by including information
  about the size of sampled kubo nodes
- make message more friendly to less technical users
@lidel lidel force-pushed the feat/notify-outdated-version branch from 73f5b36 to e29ffe8 Compare June 19, 2024 22:57
Copy link
Member
@lidel lidel left a comment

Choose a reason for hiding this comment

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

Lgtm.

Had to moved some things around and fix panics with custom routers. Listed issues are no longer happening, and it's more future-proof. Added docs and changelog.

Looking at https://github.com/probe-lab/network-measurements/blob/main/reports/2024/calendar-week-23/ipfs/README.md#kubo made me adjust min-fraction to 5% to start showing notification bit faster.

In this form it should be a low risk quality of life improvement, so planning to include it in 0.30 (#10436).

Will keep PR open for a few more days, just in case someone wants to drop feedback.

TODO from reviews

configuratbility

  • replace env variable with Version.RunSwarmCheck (Flag)
  • while at it, add Version.Suffix (optionalString)

@lidel lidel marked this pull request as draft July 2, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated Version Notice
3 participants