Update instance connectors on a connector reconcile #4992
Conversation
runtime/reconcilers/connector.go
Outdated
| // Get and sync repo | ||
| repo, release, err := r.C.Runtime.Repo(ctx, r.C.InstanceID) | ||
| if err != nil { | ||
| return runtime.ReconcileResult{Err: fmt.Errorf("failed to access repo: %w", err)} | ||
| } | ||
| defer release() | ||
| err = repo.Sync(ctx) | ||
| if err != nil { | ||
| return runtime.ReconcileResult{Err: fmt.Errorf("failed to sync repo: %w", err)} | ||
| } | ||
|
|
||
| // Get instance | ||
| inst, err := r.C.Runtime.Instance(ctx, r.C.InstanceID) | ||
| if err != nil { | ||
| return runtime.ReconcileResult{Err: fmt.Errorf("failed to find instance: %w", err)} | ||
| } | ||
|
|
||
| // Parse the project | ||
| parser, err := rillv1.Parse(ctx, repo, r.C.InstanceID, inst.Environment, inst.OLAPConnector) | ||
| if err != nil { | ||
| return runtime.ReconcileResult{Err: fmt.Errorf("failed to parse: %w", err)} | ||
| } | ||
|
|
||
| // Update instance connectors | ||
| err = r.C.Runtime.UpdateInstanceWithRillYAML(ctx, inst.ID, parser, false) | ||
| if err != nil { | ||
| return runtime.ReconcileResult{Err: fmt.Errorf("failed to update instance: %w", err)} | ||
| } |
There was a problem hiding this comment.
The file watcher already parsed the file and created the Connector resource (returned from self.GetConnector() above). Doing a repo sync (clone or pull) and full project parse again here is redundant, and potentially quite slow if there are many connectors.
Instead, you will probably want to add new functions, separate from UpdateInstanceWithRillYAML, for upserting/deleting connectors for an instance. They should also take care only to update in the DB (invalidating various caches) if the connector actually changed. (Since the reconciler can trigger even if there are no changes, e.g. on process restart.)
The delete case needs to be handled in the if self.Meta.DeletedOn != nil { case below.
# Conflicts: # proto/gen/rill/runtime/v1/resources.pb.go
esevastyanov
changed the title
runtime/controller_test.go
Outdated
| @@ -1390,7 +1390,8 @@ name: my-gcs | |||
| }, | |||
| Resource: &runtimev1.Resource_Connector{ | |||
| Connector: &runtimev1.ConnectorV2{ | |||
| Spec: &runtimev1.ConnectorSpec{Driver: "s3", Properties: map[string]string{"region": "us-west-2"}}, | |||
| Spec: &runtimev1.ConnectorSpec{Driver: "s3", Properties: map[string]string{"region": "us-west-2"}}, | |||
| State: &runtimev1.ConnectorState{SpecHash: "76b3819c673b0a4e4755418614d8cb62"}, | |||
There was a problem hiding this comment.
You can nil the field here to omit it from tests:
rill/runtime/testruntime/reconcile.go
Line 155 in abb730b
runtime/reconcilers/connector.go
Outdated
| if t.State == nil { | ||
| t.State = &runtimev1.ConnectorState{} | ||
| } |
There was a problem hiding this comment.
If you update func (r *ConnectorReconciler) ResetState(res *runtimev1.Resource) error { to set an empty state, then this isn't needed. We do that in most other reconcilers, but actually looks like there's some inconsistency, so I understand the confusion. There might be some other code that assumes the state is not nil, so it's probably best to set it there.
* UpdateInstanceWithRillYAML on connector reconcile * Simplified connector reconcile * Merged main * Fixed test * fmt fix * ResetState
The aim is to update project connectors on connector update