fix: update checker doesn't require server connection to work #3775
Conversation
frrist
commented
Apr 10, 2024
•
frrist
commented
- and liberally decorate the code with TODOs so we can make this better in the future where someone has some spare cycles
- fixes bacalhau authenication via token CLI borken #3773
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes primarily focuses on enhancing the software's version checking and update mechanisms, improving error handling, and refining the user authentication experience. A notable part of the update seeks to streamline how the application checks for its own updates and interacts with the user for authentication, specifically addressing issues in the CLI token authentication process. Changes
Assessment against linked issues
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| if response, err := util.GetAPIClientV2(cmd).Agent().Version(ctx); err != nil { | ||
| return nil, err | ||
| } else if response != nil { | ||
| return response.BuildVersionInfo, nil | ||
| } else { | ||
| return nil, nil | ||
| } | ||
| return version.Get(), nil |
There was a problem hiding this comment.
This is the fix for: #3773:
This is because the update check runs async on every command and incorrectly required a client, which prompted for a token to ask the server what version is was**. The actual command being run (e.g. job run) also created a client and asked for a token which is why the CLI asked for a token twice until and update.json file existed. The fix here is to simply return the version of the binary running the update checker.
** no idea why we do this, update checker should tell you the binary you are using is out of date, not the server you are talking to. Server operators will become aware their server is out of date via log message the update server prints when its update checker runs
There was a problem hiding this comment.
maybe we need to understand the purpose of the update checker better. Is it to tell that the client is outdated compared to the orchestrator's version? or outdated compared to latest releases from bacalhau?
As a user, I might expect the following:
- If I am running a compute node, I would like the cli to compare my version with the orchestrator's version. I don't want the cli to ask me to update to a version that is beyond the orchestrator's
- If I am running client commands, such as
bacalhau job run, I would like the cli to also compare my version with the orchestrator's - If I a m running an orchestrator node, then I would like the cli to compare my version with the latest release from bacalhau
@aronchick @MichaelHoepler what do you think the right behaviour here?
There was a problem hiding this comment.
I'd propose we reduce the responsibility of the UpdateChecker to only retrieve the latest version of bacalhau from the update server. This will remove the dependency on a bacalhau client - and thus authentication - from the UpdateChecker service - which will fix this bug.
The UpdateChecker may then persist the value it retrieves from the update server to some "system configuration field" (will be very easy to do when #3959 lands). From there we can make the following changes:
- Clients and Compute nodes can compare their version, the orchestrator's version, and the latest version notifying accordingly in stderr or log messages.
- Orchestrator nodes can compare their version with the latest version notifying accordingly in log messages.
Alternatively, and this would be my preference:
- Only Servers (Compute and Requester Nodes) use the update checker (as we already do now) to retrieve the latest version. If they are behind it they produce a log message stating so.
- Clients are only notified when the server they are communicating with is on a version different than theirs. Clients commands do not run the UpdateChecked.
In this flow, clients are only notified they need to update when their server operator updates.
There was a problem hiding this comment.
Yeah those are valid points. I've approved the PR as figuring out the right behaviour of the update checker is a bigger task, and you are not changing the behaviour with this PR
#3980
frrist
changed the title
|
@coderabbitai /review |
|
Based on the provided code snippets from the pull request, here are the observations and recommendations:
In summary, the pull request appears to align with the stated objectives of making the update checker function without requiring a server connection and preparing the codebase for future improvements. However, careful consideration should be given to the points raised above to ensure the changes are robust, maintainable, and well-documented. @frrist, I've reviewed the changes and provided some observations and recommendations. Let me know if you need further details on any specific point or if there are additional aspects of the PR you'd like me to look into. Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# List the files changed in the PR.
git diff --name-only HEAD~1
Length of output: 93 Script: #!/bin/bash
# Display the contents of the modified files.
cat cmd/util/hook/version.go
cat pkg/node/node.go
cat pkg/version/update.go
Length of output: 28806 Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -131,11 +141,14 @@ func RunUpdateChecker( | |||
|
|
|||
| if err == nil { | |||
| responseCallback(ctx, updateResponse) | |||
| // TODO(forrest): similar to the below TODO, shove this in a cache and persit on shutdown. | |||
There was a problem hiding this comment.
Evaluate the feasibility and thread-safety of caching update check times.
| // TODO(forrest) [efficiency]: rather than repeatedly reading a file, set the value in a cache and flush the cache | ||
| // to disk when this service shutwdown, no reason for this IO. |
There was a problem hiding this comment.
Ensure the proposed caching mechanism for update checks is implemented carefully.
| // TODO(forrest) [simplify]: we can make this simpler. Use one timer else we | ||
| // will be checking for updates more than the configured value | ||
| // this time ticks the last time we performed a check + the config default value |
There was a problem hiding this comment.
Simplify the update check scheduling logic to avoid potential timing issues.
| @@ -483,6 +483,7 @@ func NewNode( | |||
| updateCheckCtx, stopUpdateChecks := context.WithCancel(ctx) | |||
| version.RunUpdateChecker( | |||
| updateCheckCtx, | |||
| // TODO(forrest) [correctness]: this code is literally the server, why are we returning nil???!!! | |||
There was a problem hiding this comment.
Investigate the rationale behind returning nil in the update check context.
frrist
force-pushed
the
frrist/fix/update-checker
branch
from
eaded7b
to
536e2d5
Compare
frrist
force-pushed
the
frrist/fix/update-checker
branch
3 times, most recently
from
c800556
to
3c2dc61
Compare
frrist
force-pushed
the
frrist/fix/update-checker
branch
from
3c2dc61
to
d59e08e
Compare
