fix(appliance): always self-update to latest#63848
Conversation
03bb1f1 to
fd4c14c
Compare
There was a problem hiding this comment.
Minor nit: I don't think we guarantee that versions are ordered, especially with recent release of 5.3.27455 and other "custom" builds we may run into issues here.
There was a problem hiding this comment.
Ah, it's totally not obvious from signatures alone but ParseVersions() returns them in ascending semver order: https://github.com/sourcegraph/sourcegraph/blob/2947657755fec31cfa226157a3e00e711d6c480b/internal/appliance/versions.go#L128
Back in selfupdate.go, a few lines up, we filter the relreg versions by public-ness.
Do those 2 aspects make this OK? If 5.2.27455 is the latest public version, it will be returned whether it's a "custom" build or not. If that's not OK we might need to add richer metadata to relreg - or is there already something in there we can use in our filtering?
There was a problem hiding this comment.
@Chickensoupwithrice I'm going to merge this because I think the issue we're discussing is orthogonal to this PR, but please review my last comment and let me know if you think we have anything we need to change
There was a problem hiding this comment.
I know I'm late to the party, but we definitely only want stuff marked public.
There was a problem hiding this comment.
Yep, look a few lines up - we do filter for public-only
Chickensoupwithrice
left a comment
There was a problem hiding this comment.
I think this is a good idea. In practice I rarely see changes to the helm / deployment repos, and agree that versioned if statements will not be a cause of major concern. Keeping appliance up to date seems like a worthwhile endeavor.
Instead of a a maximum of 2 minor versions beyond the deployed Sourcegraph instance.
fd4c14c to
00c4bd2
Compare
Instead of a a maximum of 2 minor versions beyond the deployed Sourcegraph instance. The main advantage of removing the 2-minor-version constraint, is that it allows admins to always be able to update to the latest SG with one click, instead of having to repeat that process through intermediate versions if they have fallen far behind.
Instead of a a maximum of 2 minor versions beyond the deployed Sourcegraph instance.
The main advantage of removing the 2-minor-version constraint, is that it allows admins to always be able to update to the latest SG with one click, instead of having to repeat that process through intermediate versions if they have fallen far behind.
A disadvantage of removing this, is the same reason we introduced it in the first place. Some background context (and please see https://docs.google.com/document/d/1hlioMCiPNwig7QqV1Q7jjCpBloEpQoKE64rPZUmi1fA/edit#heading=h.j7ihwlbta4sj for more info): most upgrades to most services are image-tag bumps, requiring no special logic in the appliance. In general however, any individual upgrade may introduce changes to kubernetes resource configs, e.g. new env vars, or even whole new resources. In Helm, we incorporate such changes by simply changing the chart and publishing a new version to the helm repository. In the appliance, we'll need to incorporate such changes with if-statements that do logic based on the current and target version (see RFC doc for code examples).
Having the appliance only support N versions back from its own version would allow us to rotate out old code branches, simplifying maintenance. However, this imposes a multi-step upgrade process on the admin.
We could in principle have our cake and eat it by separating out the reconciler component, that contains this logic, into a separately-versioned thing, and have the appliance orchestrate upgrades to that to implement single-click upgrades while allowing us to rotate out old branches from reconciler revisions. IMO, that is something we could consider introducing in the future - by not doing it today, we don't block ourselves from doing it later. It is a cool idea but does introduce a fair amount of solution complexity. Since most upgrades are just image tag metadata, I suspect it'll be a long time before the version branching logic builds to a point where it's a pain. So in summary, I think this commit a good compromise for the first few appliance versions.
If this is 👍, I'll update this section of the relevant RFC: https://docs.google.com/document/d/1hlioMCiPNwig7QqV1Q7jjCpBloEpQoKE64rPZUmi1fA/edit#heading=h.j7ihwlbta4sj
Don't think that because this is already in a PR it is "too late to push back" - it's not - this was just a small enough proposal code-wise that it seemed easier to anchor the conversation in a PR that we can either merge or close.
Test plan
Tests included.
Changelog