containers: add setLabels API to mutate container labels at runtime#6787
containers: add setLabels API to mutate container labels at runtime#6787martinezjandrew wants to merge 2 commits into
Conversation
Adds a new container.setLabels(labels: Record<string, string>): Promise<void> JS method, gated behind the workerdExperimental compatibility flag, backed by a new capnp RPC method setLabels @15 (labels :List(Label)) on the Container interface. Each call fully replaces the existing label set; the container must be running. Label-name and label-value validation is shared with start() via a new requireValidLabels() helper.
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Adds a setLabels() runtime API to mutate container labels in-memory, with validation refactoring, capnp schema addition, and Docker shim support.
Issues found (ranked by severity):
- [MEDIUM]
setLabelsWarningLoggedis an instance member but the comment claims it's process-wide — it fires once perContainerClient, not once per process. - [LOW]
testSetLabelsSerializedasserts{ b: '2' }wins, relying on FIFO RPC-turn ordering — the comment's wording ("whichever call was awaited last") is misleading.
This review was generated by an AI assistant and may contain inaccuracies.
|
|
||
| // Process-wide latch ensuring the "Docker does not propagate runtime label changes" | ||
| // warning is emitted at most once per workerd process, on the first setLabels() call. | ||
| std::atomic_bool setLabelsWarningLogged{false}; |
There was a problem hiding this comment.
setLabelsWarningLogged is a non-static instance member, so every ContainerClient gets its own copy. The comment says "process-wide latch" but it only gates per-instance.
The existing staleSnapshotVolumeCheckScheduled (which IS process-wide) is declared as a file-scope variable in the anonymous namespace in container-client.c++:67. If the intent is truly once-per-process, this should follow the same pattern:
| std::atomic_bool setLabelsWarningLogged{false}; | |
| static inline std::atomic_bool setLabelsWarningLogged{false}; |
Alternatively, if once-per-container is acceptable, just update the comment to say "per-container" instead of "process-wide".
|
|
||
| // Issue two concurrent setLabels calls without awaiting between them. The server-side | ||
| // RpcTurn mechanism serializes them, so the final inspect() must equal whichever call | ||
| // was awaited last. |
There was a problem hiding this comment.
Nit: the comment says "the final inspect() must equal whichever call was awaited last" — but it's the last call queued (FIFO RPC-turn order) that wins, not the last awaited. Both are awaited simultaneously via Promise.all. Consider rewording to:
| // was awaited last. | |
| // Issue two concurrent setLabels calls without awaiting between them. The server-side | |
| // RpcTurn mechanism serializes them in FIFO order, so the final inspect() must reflect | |
| // the second (last-queued) call. |
|
Review posted on PR #6787 with two findings:
The rest of the PR looks solid: the capnp schema addition ( |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6787 +/- ##
==========================================
- Coverage 66.62% 66.60% -0.02%
==========================================
Files 402 402
Lines 115914 115944 +30
Branches 19425 19427 +2
==========================================
- Hits 77231 77230 -1
- Misses 27094 27124 +30
- Partials 11589 11590 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Keep the local ContainerClient label state entirely in memory so setLabels() reflects the real runtime behavior by actually replacing the current label set, without depending on Docker object labels.
8469717 to
5ba0ea4
Compare
Customers keep containers in a per-customer "warm pool" and need to attach customer-identifying labels after the container has been claimed.
Today labels can only be set at start() time (#6352), so there's no way to label an already-running container without destroying and restarting it.
This PR adds a
setLabels()runtime API to mutate labels on a running container: