mvu: autoupgrade in frontend#52242
Conversation
|
|
||
| const appName = "frontend-autoupgrader" | ||
|
|
||
| func NewRunnerWithSchemas(observationCtx *observation.Context, out *output.Output, schemaNames []string, schemas []*schemas.Schema) (*runner.Runner, error) { |
There was a problem hiding this comment.
Maybe instead of passing around a runner that we need to extract the db from we can pass around an object that contains map[string]SchemaAndConnection for each of the requested schemas as well the runner. Then it's readily available as an explicit parameter.
(Can be a future refactor if you think it's beneficial)
There was a problem hiding this comment.
Yea thats something I want decoupled somehow sometime, theres a lot going on underneath in terms of factories, factory caching, connection caching etc that is super fiddly to pull out
163d565 to
eebb449
Compare
3b19646 to
e957865
Compare
There was a problem hiding this comment.
@Numbers88s @efritz heres the lovely UI, feel free to do whatever
5e9acac to
52bb352
Compare
| // claims a "lock" to prevent other frontends from attempting to autoupgrade concurrently, looping while the lock couldn't be claimed until either | ||
| // 1) the version is where we want to be at or | ||
| // 2) the lock was claimed by us | ||
| func claimAutoUpgradeLock(ctx context.Context, obsvCtx *observation.Context, upgradestore UpgradeStore, toVersion oobmigration.Version) (stillNeedsUpgrade bool, err error) { |
There was a problem hiding this comment.
This would be a good function to test (but we could add one after branch cut).
Co-authored-by: Eric Fritz <eric@sourcegraph.com>
| // 1) we know old frontends are retired and not coming back (due to new frontends running health/ready server) | ||
| // 2) dependent services have picked up the magic DSN and restarted | ||
| // TODO: can we surface this in the UI? | ||
| func blockForDisconnects(ctx context.Context, logger log.Logger, db database.DB) error { |
There was a problem hiding this comment.
This would be a good function to test (but we could add one after branch cut).
Co-authored-by: Eric Fritz <eric@sourcegraph.com>
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff e79e5ac...29a9820.
|
| # echo STABLE_VERSION "$stamp_version" | ||
| echo STABLE_VERSION "$VERSION" |
| defer func() { | ||
| close(AutoUpgradeDone) | ||
| }() |
There was a problem hiding this comment.
This is only called once, right? Double-close of a channel will panic.
There was a problem hiding this comment.
yep only once, in Main
Implements (basically) fully automated multi-version upgrades as an optional startup step in the `frontend` service. It was put there for a few reasons: 1) We can display a UI with whatever we want (not possible in init containers) 2) We can support rolling upgrades, by booting up ready/health + conf servers to shutting down of old services and blocking new services from connecting until autoupgrade is complete [brain expansion](https://i.kym-cdn.com/entries/icons/original/000/014/401/tumblr_inline_mqrh26jIU11qz4rgp.jpg) ## Test plan Expansive testing on a local docker-compose setup (from 3.37.0 to 5.0.5) and some lighter testing on k3s (from 4.3.1 to 5.0.5) --------- Co-authored-by: Eric Fritz <eric@sourcegraph.com>
| if os.Getenv("WEBPACK_DEV_SERVER") == "1" { | ||
| assets.UseDevAssetsProvider() | ||
| } | ||
| enterprisecmd.SingleServiceMainEnterprise(shared.Service) |
There was a problem hiding this comment.
This broke the Outbound Requests admin dashboard and possibly other things that rely on frontend also having been initialized with config.
In this specific case, we realised that frontend only returns requests in the admin dashboard when limit != 0: https://github.com/sourcegraph/sourcegraph/blob/e235066282729db3d4541cf982faf1af6e1cddd1/internal/httpcli/redis_logger_middleware.go#L131-L133
But that limit is only set in this conf.Init() function: https://github.com/sourcegraph/sourcegraph/blob/e235066282729db3d4541cf982faf1af6e1cddd1/internal/conf/init.go#L35-L37
Meaning: in frontend the above function is never called and thus the Watch function is never called and thus the values are never updated from their default value to the value in the config.
There was a problem hiding this comment.
Is this a possible fix?
diff --git a/enterprise/cmd/frontend/shared/shared.go b/enterprise/cmd/frontend/shared/shared.go
index d9b082395b..f074c4a4bb 100644
--- a/enterprise/cmd/frontend/shared/shared.go
+++ b/enterprise/cmd/frontend/shared/shared.go
@@ -140,6 +140,7 @@ func SwitchableSiteConfig() conftypes.WatchableSiteConfig {
go func() {
<-shared.AutoUpgradeDone
+ conf.Init()
switchable.WatchableSiteConfig = confClient
for _, watcher := range switchable.watchers {
confClient.Watch(watcher)
I'm not sure about how SwitchableSiteConfig is supposed to work and whether this is safe, though.
See [this comment](https://github.com/sourcegraph/sourcegraph/pull/52242/files#r1254598066) for the source issue. Looks like #52242 introduced a condition where certain httpcli settings were not being configured in the frontend due to some cyclic package dependency-avoidance trickery in the `conf.Init` function. This PR ensures that this setup code is hit in the frontend initialization process. No other service should need this (as they hit the `conf.Init` function directly from `SingleServiceMain` on startup), and no other feature should have been affected (as these are the only settings missing from that path). ## Test plan Tested locally. <br> Backport e715340 from #54745 Co-authored-by: Eric Fritz <eric@sourcegraph.com>
Implements (basically) fully automated multi-version upgrades as an optional startup step in the
frontendservice. It was put there for a few reasons:brain expansion
Test plan
Expansive testing on a local docker-compose setup (from 3.37.0 to 5.0.5) and some lighter testing on k3s (from 4.3.1 to 5.0.5)