feat(controller): support remote worker deploy mode#968
Conversation
Change-Id: Ib147ccf5960d5cca2a64af48e57e0a0853c784f1
📊 CI Metrics ReportSummary
By Role
Per-Test Breakdown
Trends✅ 1 test(s) improved (fewer LLM calls) Generated by HiClaw CI on 2026-07-03 10:02:19 UTC |
|
Thanks for syncing the remote deploy mode work from the internal branch. The overall direction looks good, but I think this PR should not merge yet. A few lifecycle/security pieces need to come over together with the remote placement support, otherwise the public implementation can create resources that are hard to clean up or can authenticate remote tokens too broadly.
Right now the controller takes the caller-provided That means the remote cluster effectively participates in controller authorization just by matching ServiceAccount names. For remote workers, the identity should be bound back to the expected CR-level placement:
The current reconcile/delete paths continue to derive the target cluster from the current
Remote placement needs an applied-target record, e.g.
The new ClusterIP Service is created separately and does not have an ownerRef. However, Please either attach a correct owner/GC story where possible, or explicitly delete Services during member finalizer cleanup. Label-based cleanup is safer than name-only cleanup because it can cover both current and legacy naming formats.
When Expected behavior should be: no expose ports means no Service, or at least any previously created Service should be deleted. These are not just polish issues; they affect remote runtime correctness, cleanup guarantees, and controller auth boundaries. I would suggest fixing these in this PR before merging the remote deploy mode support. |
Change-Id: Ib60c53c8913510460721346f28d3bfa3a6d9f137
|
Thanks for the detailed review. I pushed f609441 to address the scoped fixes here:
For targetCluster changes: the current product contract is that targetCluster should not be changed while runtime resources are active. To move placement, the CR should first be brought to Stopped, then targetCluster can be changed and reconciled from that stopped state. |
Change-Id: I1889c54068c36557e77a273491fdffe0dc457f9c
|
I re-checked the latest revision (
Overall, the feature direction makes sense, but I think the remote auth boundary, deployment target lifecycle, and failing CI should be fixed before merge. |
Change-Id: Ia16e3cf53520b0a1bc6fcdb7a3ed2451bb0de162
|
Thanks for catching this. Fixed the remote Worker token binding by carrying the TokenReview serviceAccount namespace/name into the identity and requiring the matching local Worker CR to be deployMode Remote with the same targetCluster id, target namespace, and expected ServiceAccount name before authorization. Remote identities without a Worker CR now fail instead of falling back to Team lookup. |
Change-Id: Ic2a48a2ceb888db0f36772eabb28919c92c483de
|
Thanks for the detailed follow-up. I pushed 3122eda to address the remaining lifecycle/watch blockers in this PR scope: Workers now persist the applied deployment target in status and keep update/delete operations on that applied target; target changes are rejected until the Worker is fully Stopped, so a stopped-first move can switch targets without orphaning the old cluster. I also wired remote Pod watches for Worker and Team member Pods through the remote client cache, including the controller-name selector and local-namespace enqueue mapping. |
|
Re-checked the latest head ( I still see a few blockers around the Team remote path and the auth target source:
The latest CI state is much better: |
|
Thanks for the follow-up. The Team CR path has already been split out and will be covered by the next PR. For this PR, I will keep the scope limited to the standalone Worker remote deploy path and will not add the Team remote lifecycle/auth changes here. |
|
@Sunrisea @johnlanni @Jing-ze CI is green on the latest head, and I clarified that the Team CR remote path is split to the next PR. Could you take another look when convenient? |
Change-Id: I85fa31e6f7ca0ca1b5c89853b0da98786a9f8943
|
Thanks for pointing this out. Fixed the standalone remote Worker auth target source in 7f74c12: remote auth now prefers status.deployMode/status.targetCluster as the applied target and falls back to spec only before first provisioning. I also added a regression test for the spec/status divergence case. The Team remote path remains split to the next PR. |
|
After re-checking this with the Team/Worker decoupling direction in mind, I think the remaining concern is mainly about API ownership / source of truth. The standalone Worker remote deploy path looks much better now: remote auth validates the applied target, Worker status pins The part that still looks risky is that this PR also adds If the intended model is that Team only owns membership/organization, and every team member has its own Worker CR, then deployment target should be owned by the Worker CR only. Otherwise we end up with two possible sources of truth for the same member’s runtime placement:
I’d suggest removing or explicitly disabling the Team-level remote deployment fields in this PR, unless Team direct runtime deployment is still meant to be supported. In the decoupled model, auth, cleanup, and applied target pinning should all be driven by the member’s Worker CR. |
Change-Id: I33e76453d95f9754b279e6becfb3433f4b282177
|
Thanks for the clarification. Fixed in a7bdfc8 by removing Team-level deployMode/targetCluster from the API/CRD schema and Team member mapping. This keeps remote deployment target ownership on the standalone Worker CR path for this PR; Team runtime placement can stay with the follow-up Team/Worker decoupling work. |
Change-Id: I5f36363b4d6e842551e9fee74313279d848500db
Summary
Scope notes
Tests