Repository navigation
[feat][RCP] Helm chart changes for RetoolOS - #385
Conversation
[High risk] Adds RetoolOS worker deployment to Helm chart infrastructure. The PR appears safe to merge based on this review. Reviews (3) · Last reviewed commit: "Remove RetoolOS Slack setup guide from c..." |
|
Feedback: 1 2 "Configure Slack" CLIENT_ID So can keep the env var path but additionally allow customers to type these into a text box in app and we store it encrypted in postgres. 3 4 |
…ker-slack-helm # Conflicts: # charts/retool/Chart.yaml
| secretName: "" | ||
|
|
||
| # RetoolOS worker configuration | ||
| config: {} |
There was a problem hiding this comment.
this is very ambiguous. what would even go here?
There was a problem hiding this comment.
It's a specific bag of 3 possible values that the generic worker template recognizes. I've updated with a commented out example.
| # Labels for RetoolOS worker pods | ||
| labels: {} | ||
|
|
||
| # Annotations for RetoolOS worker pods | ||
| annotations: {} |
There was a problem hiding this comment.
it's a bit awkward for labels and annotations to exist under retoolos. but other pod-specific settings to live under retoolos.worker.. I'd vote to move both of these under retoolos.worker.
There was a problem hiding this comment.
I agree, and have updated it the way you describe.
I had originally done it this way because the shared worker template (used by a few workers like agents/agentEvals/r2/workflows etc) reads labels and annotations from the parent block, so this was the only way to do it without special casing retoolos.
Had to change the template to now continue reading parent metadata (for backwards compatibility) but additionally now merge in metadata from the worker block with the worker block winning on conflicting keys.
| # socket uses an outbound Socket Mode connection and works without public | ||
| # ingress. Set http to receive Slack webhooks instead. | ||
| mode: socket |
There was a problem hiding this comment.
it would be worth suggesting what kind of scenario a customer might choose webhooks over socket, if you have some idea.
| description: A Helm chart for Kubernetes | ||
| type: application | ||
| version: 6.11.34 | ||
| version: 6.12.0 |
| {{- else if eq (include "retool.retoolos.enabled" .) "1" }} | ||
| {{- include "retool.blobStorage.envVars" . | nindent 10 }} |
There was a problem hiding this comment.
uhhh this branch looks like a fallthrough from the if eq (include "retool.gitServer.enabled" .) "1" conditional above? should probably make this its own thing, assuming not intentional.
actually, re-reading the helpers changes, I'm guessing this is a result of refactoring retool.gitServer.commonEnv to also embed retool.blobStorage.envVars, so now here if git is disabled but retoolos is enabled, we need to still include blob store env. I guess this is a smaller change from what was already here, but now that these are diverging I'd vote for actually totally isolating retool.gitServer.commonEnv from retool.blobStorage.envVars. I think the logic at call sites like here will be more intuitive that way for the future.
There was a problem hiding this comment.
Thanks! I believe I've implemented it now the way you describe and it does seem cleaner and more intuitive.
…ker-slack-helm # Conflicts: # charts/retool/Chart.yaml
ryanartecona
left a comment
There was a problem hiding this comment.
looks great! just a couple comments on helper organization but this looks ready to ship to me
| {{/* | ||
| Set RetoolOS worker enabled | ||
| Usage: (include "retool.retoolos.enabled" .) | ||
| */}} | ||
| {{- define "retool.retoolos.enabled" -}} | ||
| {{- $output := "" -}} | ||
| {{- if (eq (toString .Values.retoolos.enabled) "true") -}} | ||
| {{- $output = "1" -}} | ||
| {{- end -}} | ||
| {{- $output -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
it's not hurting much, but unless we anticipate this conditional becoming more elaborate to check, this helper isn't simplifying anything as is. all it's doing is changing a call site likeif .Values.retoolos.enabled ...to instead be if (eq (include "retool.retoolos.enabled" .) "1") ... which IMO is slightly harder to read and more characters.
There was a problem hiding this comment.
I agree, but I think we have to keep this helper.
The chart has a shared generic loop that renders all the workers (workflows, agents, ..., now retoolos). It makes you describe your worker like so:
- parent: workflows
type: workflow
- parent: agents
type: agent
- parent: retoolos
type: retoolos
and then dynamically constructs the helper it invokes:
parent = "retoolos"
helper name = "retool." + parent + ".enabled"
= "retool.retoolos.enabled"
Again to avoid special casing retoolOS i'd rather just keep this
| Return "1" when a top-level env setting already owns a variable. | ||
|
|
||
| Chart-derived defaults use this helper to avoid emitting a duplicate EnvVar. | ||
| For example, `env.RETOOLOS_SEARCH_ENABLED: false` must override the chart's | ||
| RetoolOS default. Values supplied through envFrom cannot be inspected while the | ||
| chart renders; use env, environmentVariables, or environmentSecrets to override | ||
| a chart-derived default. | ||
|
|
||
| Usage: include "retool.envVarIsExplicit" (dict "root" . "name" "MY_VAR") |
There was a problem hiding this comment.
mmm this is clever, I like this abstraction
Description
Makes RetoolOS an opt-in, single-toggle feature for self-hosted deployments:
Enabling it creates one new backend-image worker (
retoolos_temporal_worker). It's a monolithic worker that handles everything 'asynchronous' - i.e. everything that does not happen live in the context of a backend endpoint:The core idea is that if the deployment that already has R² configured, then there are no new infra changes needed:
retoolos.enabled: truepgvectoravailable for install. If using managed Postgres in any of the 3 cloud providers (AWS/GCS/Azure), this should already be trueBlob storage
RetoolOS reuses the existing
rr.blobStorageconfiguration. The chart supplies the sameRR_BLOB_STORAGE_PROVIDERandRR_DEFAULT_<PROVIDER>_*values to the backend and RetoolOS worker, even when Git Server itself is disabled.RetoolOS stores its objects under the
retoolos/prefix, so they can safely share the R² bucket or container.The application still supports explicit
RETOOLOS_<PROVIDER>_*configuration for deployments that want a separate store. Those values take precedence over the RR defaults.Slack
Self-hosted deployments default to Socket Mode, which uses an outbound connection and does not require public Slack event or interaction endpoints.
Customers can configure the Slack app through one of three paths:
RETOOLOS_SLACK_*through the chart’s generic environment variablesretoolos.slack.secretNameto an existing Kubernetes Secret containing:client-idclient-secretsigning-secretapp-tokenwhen using Socket ModeThe Kubernetes Secret and explicit environment-variable paths (2 and 3) cannot be combined because they both describe the same deployment-managed Slack app.
For a customer that wants HTTP ingress instead of Socket mode, that can be configured via:
Search
Search is enabled automatically on the backend, Jobs Runner, and RetoolOS worker when RetoolOS is enabled.
Operators can disable search without disabling the rest of RetoolOS:
The Postgres server must have pgvector 0.7 or newer available, and the migration user must be able to create or update the
vectorextension.retoolos_self_hosted_no_rr.md: preserving an old version of this PR description, as it might contain useful info for customers that do not have R² configured.
Test Plan
In a file somewhere, e.g.
./values.njdelima-retoolos.local.yaml:Then, I re-used my existing
njdelimaballoon instance, using the local chart defined here in my branch, and the values override above:I think an actual customer might do something close to this:
Install and verify Slack. After the above, I was able to visit my balloon instance https://njdelima.retool.dev/ and verify RetoolOS works, I can chat with the agent etc. In Retool Settings → RetoolOS → Configure → Messaging, select Add to Slack and finish OAuth as an administrator. DM the bot from a Slack user whose email matches an enabled Retool user and confirm a reply.