-
Notifications
You must be signed in to change notification settings - Fork 17
TemporalConnection update propagation #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Shivs11
merged 8 commits into
temporalio:main
from
Shivs11:ss/temporal-connection-updates
Aug 15, 2025
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
aac9f1f
connection updates
Shivs11 52af07c
lint
Shivs11 d5a4cbc
Merge branch 'main' into ss/temporal-connection-updates
Shivs11 57b4684
Merge remote-tracking branch 'upstream/main' into ss/temporal-connect…
Shivs11 3bbc280
Merge remote-tracking branch 'upstream/main' into ss/temporal-connect…
Shivs11 fdc6662
update deployment in place to facilitate rolling updates + some more …
Shivs11 ee68d71
nits
Shivs11 ce38023
address comments
Shivs11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,9 @@ import ( | |
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller" | ||
| "sigs.k8s.io/controller-runtime/pkg/handler" | ||
| "sigs.k8s.io/controller-runtime/pkg/log" | ||
| "sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
| ) | ||
|
|
||
| var ( | ||
|
|
@@ -58,7 +60,6 @@ type TemporalWorkerDeploymentReconciler struct { | |
| // | ||
| // For more details, check Reconcile and its Result here: | ||
| // - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile | ||
| // TODO(carlydf): Add watching of temporal connection custom resource (may have issue) | ||
| func (r *TemporalWorkerDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
| // TODO(Shivam): Monitor if the time taken for a successful reconciliation loop is closing in on 5 minutes. If so, we | ||
| // may need to increase the timeout value. | ||
|
|
@@ -112,8 +113,9 @@ func (r *TemporalWorkerDeploymentReconciler) Reconcile(ctx context.Context, req | |
|
|
||
| // Get or update temporal client for connection | ||
| temporalClient, ok := r.TemporalClientPool.GetSDKClient(clientpool.ClientPoolKey{ | ||
| HostPort: temporalConnection.Spec.HostPort, | ||
| Namespace: workerDeploy.Spec.WorkerOptions.TemporalNamespace, | ||
| HostPort: temporalConnection.Spec.HostPort, | ||
| Namespace: workerDeploy.Spec.WorkerOptions.TemporalNamespace, | ||
| MutualTLSSecret: temporalConnection.Spec.MutualTLSSecret, | ||
| }, temporalConnection.Spec.MutualTLSSecret != "") | ||
| if !ok { | ||
| c, err := r.TemporalClientPool.UpsertClient(ctx, clientpool.NewClientOptions{ | ||
|
|
@@ -212,9 +214,35 @@ func (r *TemporalWorkerDeploymentReconciler) SetupWithManager(mgr ctrl.Manager) | |
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&temporaliov1alpha1.TemporalWorkerDeployment{}). | ||
| Owns(&appsv1.Deployment{}). | ||
| Watches(&temporaliov1alpha1.TemporalConnection{}, handler.EnqueueRequestsFromMapFunc(r.findTWDsUsingConnection)). | ||
Shivs11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| WithOptions(controller.Options{ | ||
| MaxConcurrentReconciles: 100, | ||
| RecoverPanic: &recoverPanic, | ||
| }). | ||
| Complete(r) | ||
| } | ||
|
|
||
| func (r *TemporalWorkerDeploymentReconciler) findTWDsUsingConnection(ctx context.Context, tc client.Object) []reconcile.Request { | ||
| var requests []reconcile.Request | ||
|
|
||
| // Find all TWDs in same namespace that reference this TC | ||
| var twds temporaliov1alpha1.TemporalWorkerDeploymentList | ||
| if err := r.List(ctx, &twds, client.InNamespace(tc.GetNamespace())); err != nil { | ||
| return requests | ||
| } | ||
|
|
||
| // Filter to ones using this connection | ||
| for _, twd := range twds.Items { | ||
| if twd.Spec.WorkerOptions.TemporalConnection == tc.GetName() { | ||
| // Enqueue a reconcile request for this TWD | ||
| requests = append(requests, reconcile.Request{ | ||
| NamespacedName: types.NamespacedName{ | ||
| Name: twd.Name, | ||
| Namespace: twd.Namespace, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return requests | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlegrone - curious to hear your thoughts on this:
I added the name of the
MutualTLSSecretas a key over here since a change to the secret name, for the same connectionSpec object, should trigger our worker controller to refresh the client it is using! This was required since I realized that there could be a world where the old secret is not expired but just replaced.However, one area where this breaks is if someone just replaces the contents of an existing secret without changing the name. In this way, the controller does not really get a new client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you solve the problem of content changing without name changing by storing a hash of the name + contents instead of just the name?
Or, if that's inefficient because it means you have to read the secret contents all the time just to generate the map key, we could do a different solution for the contents changing, where if an error occurs when calling the temporal APIs that suggests "wrong credentials", we could try reloading them once just in case it's changed. Then, do a namespace describe to make sure they work, and if they work, restart all the workers.
I don't think this needs to be solved in this PR though, but if you don't solve the "changed content, not expired, same name" thing, do just create an issue about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for responding to this;
yes, I did think about the
storing a hash of the name + contentsidea but I decided against it because in one of my earlier PR's, we discussed that reading the contents of a secret on every reconciliation loop could be intensive.having said that, I like the
APIs that suggests "wrong credentials", we could try reloading them once just in case it's changedidea but don't think it should come in this PR. I can make a separate issue for this!