-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH] Make it possible to deploy multiple chromas in Tiltfile #5992
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
|
d465973 to
a5c1037
Compare
|
Enable multi-region Chroma deployments via Tilt This PR extends the Key Changes• Expanded Affected Areas• This summary was automatically generated by @propel-code-bot |
| k8s_resource( | ||
| objects=[ | ||
| 'pod-watcher:Role:chroma2', | ||
| 'query-service-memberlist:MemberList:chroma2', | ||
| 'compaction-service-memberlist:MemberList:chroma2', | ||
| 'garbage-collection-service-memberlist:MemberList:chroma2', | ||
| 'rust-log-service-memberlist:MemberList:chroma2', | ||
|
|
||
| 'sysdb-serviceaccount:ServiceAccount:chroma2', | ||
| 'sysdb-serviceaccount-rolebinding:RoleBinding:chroma2', | ||
| 'sysdb-query-service-memberlist-binding:RoleBinding:chroma2', | ||
| 'sysdb-compaction-service-memberlist-binding:RoleBinding:chroma2', | ||
|
|
||
| 'query-service-serviceaccount:ServiceAccount:chroma2', | ||
| 'query-service-serviceaccount-rolebinding:RoleBinding:chroma2', | ||
| 'query-service-memberlist-readerwriter:Role:chroma2', | ||
| 'query-service-query-service-memberlist-binding:RoleBinding:chroma2', | ||
| 'query-service-memberlist-readerwriter-binding:RoleBinding:chroma2', | ||
|
|
||
| 'compaction-service-memberlist-readerwriter:Role:chroma2', | ||
| 'compaction-service-compaction-service-memberlist-binding:RoleBinding:chroma2', | ||
| 'compaction-service-memberlist-readerwriter-binding:RoleBinding:chroma2', | ||
| 'compaction-service-serviceaccount:ServiceAccount:chroma2', | ||
| 'compaction-service-serviceaccount-rolebinding:RoleBinding:chroma2', | ||
|
|
||
| 'lease-watcher:Role:chroma2', | ||
| 'rust-frontend-service-config:ConfigMap:chroma2', | ||
| ], | ||
| new_name='k8s_setup2', | ||
| labels=["infrastructure2"], | ||
| ) |
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.
[Maintainability] The addition of multi-region support has introduced significant duplication in this Tiltfile. For example, the k8s_resource blocks for k8s_setup and k8s_setup2, as well as the service definitions for chroma and chroma2, are nearly identical.
While this works for two regions, it will be hard to maintain and scale to more regions. As a best practice to keep the configuration DRY, consider refactoring this to use a loop over a list of region configurations. This would allow you to generate the resources for each region dynamically, making the Tiltfile more concise and easier to extend.
Context for Agents
The addition of multi-region support has introduced significant duplication in this `Tiltfile`. For example, the `k8s_resource` blocks for `k8s_setup` and `k8s_setup2`, as well as the service definitions for `chroma` and `chroma2`, are nearly identical.
While this works for two regions, it will be hard to maintain and scale to more regions. As a best practice to keep the configuration DRY, consider refactoring this to use a loop over a list of region configurations. This would allow you to generate the resources for each region dynamically, making the `Tiltfile` more concise and easier to extend.
File: Tiltfile
Line: 269| storage: | ||
| admission_controlled_s3: | ||
| s3_config: | ||
| bucket: "chroma-storage2" |
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.
[Documentation] In the PR description, you mentioned that the *2.yaml configs should only differ in namespace. However, I noticed that this file also changes the S3 bucket name to chroma-storage2 (e.g., here and for other services). This seems like a reasonable approach for isolating regions, but it would be helpful to clarify this in the PR description to align the stated intent with the implementation.
Context for Agents
In the PR description, you mentioned that the `*2.yaml` configs should only differ in namespace. However, I noticed that this file also changes the S3 bucket name to `chroma-storage2` (e.g., here and for other services). This seems like a reasonable approach for isolating regions, but it would be helpful to clarify this in the PR description to align the stated intent with the implementation.
File: rust/worker/chroma_config2.yaml
Line: 30To deploy (and tear down) a single chroma: ``` tilt up tilt down ``` To deploy (and tear down) multiple chromas: ``` tilt up -- --multi_region tilt down -- --multi_region ```
a5c1037 to
5198838
Compare
Description of changes
Also, remove hardcoded sysdb namespace. And, update our existing configs to fully specify hostnames (including namespace). The
*2.yamlvariants of our configs should only differ from the original configs in the various places we configure the namespace for the chroma deployment. In the future, we can consider generating the new chroma region's configs based on the original configs.To deploy (and tear down) a single chroma:
To deploy (and tear down) multiple chromas:
Test plan
CI
Migration plan
N/A
Observability plan
N/A
Documentation Changes
N/A