feat(etcd): Version the etcd keys #3458
Conversation
Prefix all our etcd keys with `v1/` to give us upgrade options in the future. Note there is no effort to manage versions centrally, we aren't enforcing all keys upgrade in lockstep, or even implementing an upgrade feature. We're giving future-us space to work. Signed-off-by: Graham King <[email protected]>
WalkthroughIntroduces versioned etcd key prefixes ("v1/...") across components, updates KvCache to accept a version parameter and construct prefixed paths, tightens watcher key parsing/validation, removes slugifying from storage keys, adjusts some log levels, and updates tests/docs accordingly. Control flow largely unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Svc as Service/Component
participant KC as KvCache
participant ET as etcd
Note over Svc,KC: Initialization (versioned prefix)
Svc->>KC: new(client, "v1", prefix, initial_values)
KC->>KC: store prefix = "v1/" + prefix
Note over KC,ET: Watch + Cache
KC->>ET: watch("v1/" + prefix)
ET-->>KC: events (put/delete)
KC->>KC: update in-memory cache
KC-->>Svc: getter returns cached values
Note over Svc,ET: Writes use versioned keys
Svc->>ET: put/get/delete with keys under "v1/..."
sequenceDiagram
autonumber
participant W as Model Watcher
participant ET as etcd
participant L as LLM Registry
Note over W: Key extraction stricter
W->>ET: watch("v1/mdc/...")
ET-->>W: event with key
W->>W: validate starts_with("v1/mdc") and parts count
alt valid key
W->>L: add/update model(card.name(), checksum, engine)
else invalid
W-->>W: error: reject key
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/llm/src/discovery/watcher.rs (1)
586-589: Fix the test to avoid double "v1/" prefix.The test creates a key with format
"v1/{}/..."wheremodel_card::ROOT_PATHis already"v1/mdc", resulting in"v1/v1/mdc/...". This doesn't match the expected key format described in the comment at line 559.Apply this diff to fix the test:
- let input = format!( - "v1/{}/dynamo/backend/generate/694d9981145a61ad", - model_card::ROOT_PATH - ); + let input = format!( + "{}/dynamo/backend/generate/694d9981145a61ad", + model_card::ROOT_PATH + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/bindings/python/rust/lib.rs(1 hunks)lib/bindings/python/rust/planner.rs(1 hunks)lib/llm/src/discovery.rs(1 hunks)lib/llm/src/discovery/watcher.rs(2 hunks)lib/llm/src/model_card.rs(1 hunks)lib/runtime/src/component.rs(3 hunks)lib/runtime/src/storage/key_value_store.rs(1 hunks)lib/runtime/src/storage/key_value_store/etcd.rs(2 hunks)lib/runtime/src/transports/etcd.rs(3 hunks)lib/runtime/src/transports/etcd/path.rs(9 hunks)lib/runtime/src/utils/pool.rs(1 hunks)lib/runtime/src/utils/typed_prefix_watcher.rs(1 hunks)lib/runtime/src/utils/worker_monitor.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/bindings/python/rust/planner.rs (1)
lib/runtime/src/transports/etcd.rs (2)
new(93-126)new(516-562)
lib/llm/src/discovery/watcher.rs (3)
lib/llm/src/local_model.rs (3)
card(331-333)endpoint_id(106-109)endpoint_id(389-391)lib/llm/src/discovery/model_manager.rs (1)
checksum(397-399)lib/runtime/src/component.rs (1)
endpoint_id(107-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build and Test - dynamo
- GitHub Check: clippy (launch/dynamo-run)
- GitHub Check: tests (launch/dynamo-run)
- GitHub Check: tests (.)
- GitHub Check: tests (lib/runtime/examples)
- GitHub Check: tests (lib/bindings/python)
- GitHub Check: clippy (.)
- GitHub Check: clippy (lib/bindings/python)
🔇 Additional comments (18)
lib/runtime/src/utils/typed_prefix_watcher.rs (1)
73-73: LGTM! Documentation updated to reflect versioned keys.The example correctly demonstrates the new versioned prefix scheme ("v1/mdc/") introduced by this PR. This documentation update helps developers understand the expected key format without affecting any runtime behavior.
lib/runtime/src/storage/key_value_store.rs (1)
255-255: LGTM!Test bucket name correctly updated to include the v1 prefix, consistent with the versioning scheme.
lib/runtime/src/utils/worker_monitor.rs (1)
97-97: LGTM!The watched prefix correctly updated to use the versioned path, aligning with the model_card::ROOT_PATH constant change.
lib/bindings/python/rust/lib.rs (1)
597-597: LGTM!Port key format correctly updated to use the v1 prefix, replacing the previous "dyn://" scheme.
lib/llm/src/discovery.rs (1)
11-11: LGTM!The KV Router root path correctly updated to include the v1 prefix. Note that as a public constant, this is technically a breaking change for any external code referencing it directly.
lib/llm/src/model_card.rs (1)
37-37: LGTM!The model card root path correctly updated to include the v1 prefix. As a public constant, this is technically a breaking change for any external code referencing it directly.
lib/llm/src/discovery/watcher.rs (2)
415-415: LGTM!The embeddings model addition correctly uses
card.name()and passes the checksum parameter.
562-577: LGTM!The tightened validation and simplified path parsing logic correctly enforces the versioned key format. The requirement that keys must start with
model_card::ROOT_PATHprovides clear validation upfront.lib/runtime/src/transports/etcd/path.rs (3)
11-11: LGTM!The etcd root path correctly updated to include the v1 prefix, removing the scheme-based format ("dynamo://") in favor of a simple versioned path.
17-17: LGTM!The addition of the
ENDPOINT_KEYWORDconstant complements the existingCOMPONENT_KEYWORDand maintains consistency in the path structure.
376-539: LGTM!All tests correctly updated to use the new
ETCD_ROOT_PATHformat. The test coverage remains comprehensive, including edge cases for lease IDs and path validation.lib/runtime/src/storage/key_value_store/etcd.rs (1)
243-243: Confirm callers use version-prefixed bucket names
Removed slugification in Etcd storage meansbucket_nameis now used raw. Ensure everyget_or_create_bucketandget_bucketinvocation passes a pre-formatted, version-prefixed name (e.g."v1/mdc"instead of"mdc").lib/runtime/src/transports/etcd.rs (3)
516-524: LGTM! Version parameter correctly implemented.The addition of the
versionparameter and the prefix construction usingformat!("{version}/{prefix}")properly implements the versioned key strategy described in the PR objectives.
579-579: LGTM! Appropriate log level adjustments.Changing KvCache update/delete logs to
trace!and the watcher stop message todebug!appropriately reduces log noise for cache operations while maintaining observability.Also applies to: 586-586, 593-593
723-723: LGTM! Test correctly updated.The test properly passes the
"v1"version parameter to align with the new API signature.lib/runtime/src/component.rs (3)
37-37: LGTM! Import consolidation.Importing
ETCD_ROOT_PATHfrom the etcd module centralizes the path definition and aligns with the versioned key strategy.
75-75: LGTM! Consistent versioned path.Updating
INSTANCE_ROOT_PATHto"v1/instances"consistently applies the version prefix across the codebase.
601-601: LGTM! Correct usage of imported constant.The usage of the imported
ETCD_ROOT_PATHcorrectly constructs the namespace path.
Signed-off-by: Graham King <[email protected]>
nv-anants
left a comment
nv-anants
left a comment
There was a problem hiding this comment.
not too familiar with etcd migration changes, but otherwise lgtm
Signed-off-by: Graham King <[email protected]> Signed-off-by: Piotr Tarasiewicz <[email protected]>
Signed-off-by: Graham King <[email protected]>
Prefix all our etcd keys with
v1/to give us upgrade options in the future.Note there is no effort to manage versions centrally, we aren't enforcing all keys upgrade in lockstep, or even implementing an upgrade feature. We're giving future-us space to work.
Summary by CodeRabbit