Disable upgrades on unsupported kernel (fixes #9290)#9635
Disable upgrades on unsupported kernel (fixes #9290)#9635rasa wants to merge 23 commits intosyncthing:mainfrom
Conversation
|
We need to be buildable with Go 1.21 until at least Go 1.23 is released, unfortunately. (But that's soon) |
@calmh So, do you wanna wait until we bump go.mod to 1.22, or should I backport go/version's functionality? Probably the easiest way would be to simply copy its code locally, and tag it |
|
I didn't check what you're really after but there already is a CompareVersions in the upgrade package, since doing that is part of its core business. It should be mostly applicable. |
@calmh Yeah, I switched to that and it actually works better, as we not only need to check for equality, but allow for a minor version mismatch in the CI env. |
|
When it comes to Windows, I think one potential problem is that everything starting from the first release of Windows 10 is version |
lib/upgrade/upgrade_test.go
Outdated
| // If we're running inside a GitHub action, we need to compare the go | ||
| // version in compatibility.json with both the current runtime version, and | ||
| // the previous one, as build-syncthing.yaml runs our unit tests with | ||
| // both versions. |
There was a problem hiding this comment.
So this brings to light the fact that this check is going to be a pain in the ass for anyone wanting to use a different Go version for whatever reason and not planning to distribute Syncthing or care about the compat file. We should probably have this test run only with a specific release tag or something, to make sure we run it but others don't have to.
There was a problem hiding this comment.
Good idea, I'll add that.
A dev could also tweak compatibility.json to match their env, or even delete the file. Of course, they would need to be careful not to commit their tweak.
compatibility.json
Outdated
| "darwin@go1.23": "11", | ||
| "linux@go1.24": "3.10" |
There was a problem hiding this comment.
It's just documentation, so the dev who needs to update this file, won't have to do any research, but simply cut-n-paste.
There was a problem hiding this comment.
Mmm let's not do that, it's confusing and how do you know what will be the requirements for Go 1.24? This should be updated at release time, or at least just shortly before.
What we could do to be more flexible is to have a source file with multiple sets of data in it, for example in yaml format:
- runtime: go1.21
requirements:
darwin: 20
windows: 10.0
...
- runtime: go1.22
requirements:
darwin: 21
windows: 10.1
...
and then we could at build time pick the set based on runtime.Version to create compatibility.json, and then it could be a build failure (for everyone) when there aren't any requirements for the current version. Then you could in principle add things for the future as well, though I think we should be careful about that.
There was a problem hiding this comment.
@calmh I implemented your suggestion, so compatibility.json is now generated dynamically from compatibility.yaml based on the current runtime.Version().
That's true, but the patch level should allow us to disable Windows 10 or 2016, when and if that ever happens. |
|
I think we can do a refinement here. We can include the compatibility data directly in the upgrade server response, so it's available in the list of releases that the upgrade code looks at. Then it becomes just one more filtering criteria there, and the upgrade code can simply filter out upgrades that are no longer compatible. |
|
Delivering the metadata via the server would also have the positive side effect that we can continue to intervene. Otherwise, if we make a mistake, we may already have dozens of installations that no longer update automatically. |
@calmh Proposed solution: The release bundle includes a old commentSomehow the upgrade server will need to know what the `runtime.Version()` of the latest release of syncthing is. We could:
Which method would you prefer? EDIT: Alternatively, the upgrade server could include the compatibility info for all runtime versions, and the upgrading syncthing could determine the runtime of the downloaded release, per below. The upgrade server would use debug.ReadBuildInfo to determine the version of go that was used to compile a binary. |
|
The point is to reduce the amount of unnecessary downloads the upgrading Syncthing instance needs to do. I suggest the following:
|
1. The build process creates a compatability.json for the runtime used. 2. The release process adds compatability.json as a release artifact 3. The upgrade server reads compatability.yaml and merges it into its /meta.json response. 4. The upgrading client reads the compatability.json file in the release bundle, and using the go runtime setting found in it, looks up the minimum OS versions listed in the upgrade server's /meta.json response. 5. If the upgrading client's OS version is lower than the minimum OS version listed, the upgrade is aborted.
There was a problem hiding this comment.
I feel I'm not sure we're envisioning the same solution. Looking the code I'm not sure of the intention. There's stuff to extract the compat.json data from inside the archive, but I think it should be returned by the upgrade server. There's something like it in the upgrade server response, but it does not seem targeted for the release but a general blob of all the compat data?
I think we should:
- Generate a compat.json for the current runtime when building Syncthing
- Have that compat.json be one of the files uploaded as part of the release (this is not in scope for this PR, the release process is separate and I'll deal with it)
- Have the upgrade server look for (the release specific) compat.json among the assets, when it gets the release metadata from Github, load it, parse it, and add it to the response for that release.
- Have Syncthing look at this when it decides to try to upgrade or not to a given release, before downloading the archive.
@calmh Done.
Gotcha.
Gotcha, I will implement this, but it will make the code slightly more complex, but in other ways simpler, too. tl;dr: I was trying to avoid the upgrade server having to download anything, as it's just another point of failure, and you and other's liked re @bt90's comment:
So the only way to "intervene" is for the upgrade server to always send the contents of the most recent compatibility.yaml, and not the compat.json contained in the release. But I agree that the upgrade server could download each compat.json for each active release, and using the
Agreed! My apologies for not figuring this out sooner, and thank you for your patience. |
|
I think by "intervene" above we meant that we control the upgrade server, so we can change what it does and what it returns if we want, by programming. |
|
@calmh OK, I implemented the changes you requested. The upgrade server simply downloads config.json from GitHub and includes its contents in its |
|
I think I'm failing to communicate here, unfortunately. I do not want a json file in the release archive; I don't want the upgrade server nor Syncthing itself downloading a release archive in order to look for a json file. The compatibility file should be an entirely standalone thing that can be checked before even beginning to download a release archive. I'll take a stab at showing what I mean. |
|
This is what I mean #9656 |
Purpose
See #9290.
This code uses go/version which was first provided in go1.22. Is that acceptable?Testing
Tested on Linux, macOS Catalina, and Windows.
Documentation
Do we need to update the docs?