Skip to content

Conversation

@rescrv
Copy link
Contributor

@rescrv rescrv commented Dec 30, 2025

Description of changes

The frontend was failing to come up. Extend permissions to access memberlist.

Test plan

CI

Migration plan

N/A

Observability plan

N/A

Documentation Changes

N/A

@github-actions
Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@propel-code-bot
Copy link
Contributor

propel-code-bot bot commented Dec 30, 2025

Tilt updates to grant rust frontend memberlist access

Adds missing rust-frontend-service service account, role, and role binding references to the Tilt-managed Kubernetes resource bootstrap for both chroma and chroma2 environments. Also extends the sysdb bootstrap list to include the rust-log-service memberlist bindings so the frontend can enumerate memberlists at startup.

Key Changes

• Added rust-frontend-service service account and role bindings to the k8s_setup object list
• Mirrored the new rust-frontend-service resources in the k8s_setup2 definition
• Included sysdb-rust-log-service-memberlist-binding in both namespace bootstrap lists

Affected Areas

• Tiltfile

This summary was automatically generated by @propel-code-bot

Comment on lines +270 to +274
'rust-frontend-service-serviceaccount:ServiceAccount:chroma',
'rust-frontend-service-rolebinding:RoleBinding:chroma',
'rust-frontend-service-query-service-memberlist-binding:RoleBinding:chroma',
'rust-log-service-memberlist-readerwriter:Role:chroma',
'rust-frontend-service-rust-log-service-memberlist-binding:RoleBinding:chroma',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended

[Maintainability] These new resource definitions are also duplicated for the chroma2 namespace below (lines 314-318). The entire objects list for k8s_setup and k8s_setup2 is almost identical.

To avoid future inconsistencies and make this file easier to maintain, consider abstracting the resource list into a function that takes the namespace as an argument. This would centralize the resource definitions and ensure both environments stay in sync.

For example:

def get_k8s_setup_objects(namespace):
  return [
    'pod-watcher:Role:{}'.format(namespace),
    # ... (rest of the common resources)
    'rust-frontend-service-serviceaccount:ServiceAccount:{}'.format(namespace),
    'rust-frontend-service-rolebinding:RoleBinding:{}'.format(namespace),
    'rust-frontend-service-query-service-memberlist-binding:RoleBinding:{}'.format(namespace),
    'rust-log-service-memberlist-readerwriter:Role:{}'.format(namespace),
    'rust-frontend-service-rust-log-service-memberlist-binding:RoleBinding:{}'.format(namespace),
    # ...
  ]

k8s_resource(
  objects=get_k8s_setup_objects('chroma'),
  new_name='k8s_setup',
  labels=["infrastructure"],
)

k8s_resource(
  objects=get_k8s_setup_objects('chroma2'),
  new_name='k8s_setup2',
  labels=["infrastructure2"],
)

Applying this pattern would make adding or removing resources for both environments a single-line change.

Context for Agents
These new resource definitions are also duplicated for the `chroma2` namespace below (lines 314-318). The entire `objects` list for `k8s_setup` and `k8s_setup2` is almost identical.

To avoid future inconsistencies and make this file easier to maintain, consider abstracting the resource list into a function that takes the namespace as an argument. This would centralize the resource definitions and ensure both environments stay in sync.

For example:
```python
def get_k8s_setup_objects(namespace):
  return [
    'pod-watcher:Role:{}'.format(namespace),
    # ... (rest of the common resources)
    'rust-frontend-service-serviceaccount:ServiceAccount:{}'.format(namespace),
    'rust-frontend-service-rolebinding:RoleBinding:{}'.format(namespace),
    'rust-frontend-service-query-service-memberlist-binding:RoleBinding:{}'.format(namespace),
    'rust-log-service-memberlist-readerwriter:Role:{}'.format(namespace),
    'rust-frontend-service-rust-log-service-memberlist-binding:RoleBinding:{}'.format(namespace),
    # ...
  ]

k8s_resource(
  objects=get_k8s_setup_objects('chroma'),
  new_name='k8s_setup',
  labels=["infrastructure"],
)

k8s_resource(
  objects=get_k8s_setup_objects('chroma2'),
  new_name='k8s_setup2',
  labels=["infrastructure2"],
)
```
Applying this pattern would make adding or removing resources for both environments a single-line change.

File: Tiltfile
Line: 274

@rescrv rescrv requested a review from sanketkedia December 30, 2025 16:44
@rescrv rescrv changed the base branch from main to rescrv/tonic-upgrade December 30, 2025 21:58
@rescrv rescrv force-pushed the rescrv/tonic-upgrade branch from d901714 to 368def8 Compare December 30, 2025 23:10
@rescrv rescrv force-pushed the rescrv/tilt branch 2 times, most recently from 43c76c7 to c150db7 Compare December 31, 2025 00:13
Copy link
Contributor

@sanketkedia sanketkedia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm my understanding - this isn't MCMR specific right? FE in chroma namespace also has the same issue when accessing query service memberlist for e.g.?

CREATE TABLE IF NOT EXISTS fragments (
log_id STRING(36) NOT NULL,
ident STRING(36) NOT NULL,
path STRING(64) NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical

[Logic] The path column is defined as STRING(64), which may be too short for fragment paths generated with sequence numbers.

The path format for sequence-numbered fragments is log/Bucket={:016x}/FragmentSeqNo={:016x}.parquet, which results in a 66-character string (e.g., log/Bucket=0000000000000000/FragmentSeqNo=0000000000000001.parquet). This will cause an error when inserting into the fragments table.

Consider increasing the size of the path column to accommodate these longer paths, for example, STRING(128).

Context for Agents
The `path` column is defined as `STRING(64)`, which may be too short for fragment paths generated with sequence numbers.

The path format for sequence-numbered fragments is `log/Bucket={:016x}/FragmentSeqNo={:016x}.parquet`, which results in a 66-character string (e.g., `log/Bucket=0000000000000000/FragmentSeqNo=0000000000000001.parquet`). This will cause an error when inserting into the `fragments` table.

Consider increasing the size of the `path` column to accommodate these longer paths, for example, `STRING(128)`.

File: rust/spanner-migrations/log_migrations/0002-create_fragments_table.spanner.sql
Line: 4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's log-uuid that gets used here. log/Uuid={}.parquet fits a 36-char UUID in 64 chars.

@rescrv rescrv force-pushed the rescrv/tonic-upgrade branch from 2e30f26 to 093f00b Compare January 7, 2026 23:04
@rescrv rescrv force-pushed the rescrv/tonic-upgrade branch from 093f00b to 07459d5 Compare January 8, 2026 20:50
Base automatically changed from rescrv/tonic-upgrade to main January 8, 2026 23:13
@rescrv rescrv merged commit 1387b5d into main Jan 9, 2026
119 of 127 checks passed
@rescrv rescrv deleted the rescrv/tilt branch January 9, 2026 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants