Pull based config changes from admin to runtime #8548

k-anshul · 2025-12-19T10:49:34Z

closes https://linear.app/rilldata/issue/PLAT-324/switch-to-pull-instead-of-push-for-config-changes-from-admin-to

Checklist:

Covered by tests
Ran it and it works as intended
Reviewed the diff before requesting a review
Checked for unhandled edge cases
Linked the issues it closes
Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
Intend to cherry-pick into the release branch
I'm proud of this work!

proto/rill/admin/v1/api.proto

admin/deployments.go

admin/server/deployment.go

runtime/registry.go

runtime/runtime.go

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>

k-anshul · 2025-12-30T06:51:39Z

runtime/server/instances.go

 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}

+	err = s.runtime.ReloadConfig(ctx, req.InstanceId)


The reload happens after an instance has been started, if this call fails then the controller will be running without annotations or frontendurl set (assuming vars can anyways not be set).

This feels problematic as it can give an initial flash of failed reconciles after the first deploy. Which may be quite confusing if looking at logs/telemetry or the status page (where I believe it retains old errors until after the next succesful reconcile).

Can you see a simple way to pull the config before creating the instance here? Otherwise, is there a simple way to pull config before starting the controller the first time (this would not block runtime server start since controllers are started in the background, and if we persist and check updated_on, might also only need to be done after creation).

Otherwise we can also just revert to the old way of sending it manually in the CreateInstance call

Can missing annotations or frontendurl lead to reconcile failures ?

I think pulling the config before adding the instance will require many changes. We can take one of the following options :

Revert to the old way of sending it manually in the CreateInstance call (most preferable).

Retry config reloads.

k-anshul · 2025-12-30T07:54:54Z

runtime/runtime.go

 		return nil, err
 	}

+	rt.configReloader = newConfigReloader(rt)


This can lead to controller restarting twice for an instance that missed it's variable updates.
This is difficult to fix if we want config reloads to not block runtime starts.

I think that's okay – it would be quite an edge case, right?

Yeah should be an edge case.

begelundmuller · 2025-12-30T11:28:06Z

admin/deployments.go

+		// do not trigger reconcile for stopped deployments
+		if d.Status == database.DeploymentStatusDeleting || d.Status == database.DeploymentStatusStopped || d.Status == database.DeploymentStatusStopping {
+			continue
+		}


Not sure this is safe – what if it's running at this point, but is changed to deleting afterwards (but before the call to UpdateDeployment?

Isn't the real problem just that UpdateDeployment changes the desired status? It seems like it shouldn't do that (since only start/stop/redeploy RPCs should change the provisioned status).

Isn't the real problem just that UpdateDeployment changes the desired status? It seems like it shouldn't do that

Yeah true. Basically all updates to project are handled in the same way whereas only some like slots and version change require a redeployment and thus require a desired status change.

Do you think we should revisit my earlier solution i.e. just directly trigger reload configs on all deployments if vars change/annotations change? Can also differentiate between update to projects i.e. slots change -> trigger reconcile but branch updates do not.
We don't need to trigger the full reconcile deployment flow for just a variable change.

only some like slots and version change require a redeployment and thus require a desired status change.

I don't think slots or version changes require a redeployment or status change? Or am I missing something? They just require the reconcile job to run to propagate the changes.

Can this issue be fixed just by removing the change of the desired status in UpdateDeployment? Or if that's tricky, since starts/stops are rare, maybe using depl.DesiredStatus instead of database.DeploymentStatusRunning might also be okay.

We don't need to trigger the full reconcile deployment flow for just a variable change.

It's pretty cheap to run, any specific concerns about running the job? The idea with a "reconcile" job is that it provides a single point for config changes to a deployment (which gives nice guarantees about config changes not being made in parallel).

begelundmuller · 2025-12-30T11:32:36Z

runtime/runtime.go

 }

+func (r *Runtime) ReloadConfig(ctx context.Context, instanceID string) error {
+	return r.configReloader.reloadConfig(ctx, instanceID)


Need to check if r.configReloader != nil {

Also, shouldn't it run in the background? Since the admin service may have to call this on many runtimes at a time, and since the runtime already is capable of pulling config in the background, it'd be nice for it to just be a quick op

begelundmuller · 2025-12-30T11:33:01Z

runtime/runtime.go

+// 1. via rt.ReloadConfig whenever admin wants runtime to reload its configs
+// 2. whenever runtime is started to pick up any configs changed while it was down
+// 3. periodically every hour to pick up any changes done outside of runtime. This just adds extra resilience
+type configReloader struct {


Maybe move to a dedicated file runtime/config_reloader.go?

begelundmuller · 2025-12-30T11:39:04Z

runtime/runtime.go

+func (r *configReloader) reloadConfig(ctx context.Context, instanceID string) error {
+	_, err := r.group.Do(ctx, instanceID, func(ctx context.Context) (string, error) {


This singleflight deduplicates calls, so I don't think it's safe. For example, if I add two variables rapidly after each other, the second call may get de-duplicated and only the first new variable will be pulled.

I think this use case requires a non-deduplicating singleflight (basically a keyed mutex). Realistically, given there usually aren't many instances, it's probably sufficient to just use ctxsync.RWMutex

Good catch. I totally overlooked it.

Realistically, given there usually aren't many instances, it's probably sufficient to just use ctxsync.RWMutex

Do we really need RWMutex or a simple mutex is enough ? If it is for cancelation feature then can use it but just clarifying if we need read locks here.

I just suggested that for the cancellation yes – isn't it considered bad practice to use normal mutexes for long running operations (since callers become unresponsive to ctx cancellations)?

k-anshul added 2 commits December 19, 2025 12:11

pull configs

a52fc0f

integrate in admin and runtime

93b989f

k-anshul self-assigned this Dec 19, 2025

k-anshul marked this pull request as draft December 19, 2025 10:49

k-anshul changed the title ~~Pull configs~~ Pull based config changes from admin to runtime Dec 19, 2025

k-anshul added 2 commits December 23, 2025 10:28

Merge remote-tracking branch 'origin/main' into pull_configs

f106e62

implement GetDeploymentConfig API

da20661

k-anshul marked this pull request as ready for review December 23, 2025 06:40

k-anshul added 3 commits December 23, 2025 12:20

modify getdeploymentconfig api

76eb9cc

trigger reload instead of reconcile on variables update

2ee3580

check deployment status

d093c5f

k-anshul requested a review from begelundmuller December 29, 2025 04:39

Merge remote-tracking branch 'origin/main' into pull_configs

7fcd49c

begelundmuller requested changes Dec 29, 2025

View reviewed changes

k-anshul and others added 4 commits December 29, 2025 17:38

Update proto/rill/admin/v1/api.proto

e4d03f4

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>

review comments

da96e67

lint fix

106f595

dev test

1953469

k-anshul commented Dec 30, 2025

View reviewed changes

k-anshul added 2 commits December 30, 2025 13:07

self review

f7c8a95

add tracing

d6cd239

k-anshul commented Dec 30, 2025

View reviewed changes

k-anshul added 2 commits December 30, 2025 15:09

archive handling

4e0c407

renames

baddd63

k-anshul requested a review from begelundmuller December 30, 2025 10:40

begelundmuller requested changes Dec 30, 2025

View reviewed changes

		func (r *configReloader) reloadConfig(ctx context.Context, instanceID string) error {
		_, err := r.group.Do(ctx, instanceID, func(ctx context.Context) (string, error) {

Pull based config changes from admin to runtime #8548

Are you sure you want to change the base?

Pull based config changes from admin to runtime #8548

Conversation

k-anshul commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-anshul Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-anshul Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begelundmuller Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begelundmuller Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

k-anshul commented Dec 19, 2025 •

edited

Loading

k-anshul Dec 30, 2025 •

edited

Loading

k-anshul Dec 30, 2025 •

edited

Loading

begelundmuller Dec 30, 2025 •

edited

Loading

begelundmuller Dec 30, 2025 •

edited

Loading