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 and json config #10438

Merged
merged 10 commits into from
Jul 24, 2024

Conversation

Matrix89
Copy link
Contributor

@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
32 tasks
@Matrix89 Matrix89 force-pushed the feat/notify-outdated-version branch from b4a5919 to 73f5b36 Compare June 15, 2024 11:50
@Matrix89
Copy link
Contributor 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
@lidel lidel changed the title feat: periodic version check feat: periodic version check and json config Jul 24, 2024
replacing env variable with proper configuration flag
that allows user to enable/disable checks on init etc.
wire up json config at Version.AgentSuffix to be applied
when present, otherwise use --agent-version-suffix from ipfs daemon
as before
@lidel lidel marked this pull request as ready for review July 24, 2024 21:28
@lidel
Copy link
Member

lidel commented Jul 24, 2024

Pushed:

  • cosmetic refactor that adds proper JSON configuration under Version.* namespace, and also moves related agent-version-suffix under that, making it easier for people using Kubo in their infra to identify their nodes
  • docs and changelog

Merging to include in 0.30.0-rc1 (#10436) and gather feedback.
Once again, thanks for poking at this @Matrix89 ❤️

@lidel lidel merged commit 225dbe6 into ipfs:master Jul 24, 2024
14 checks passed
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