Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 25, 2026, 4:05 PM ET / 20:05 UTC. ClawSweeper reviewWhat this changesThe PR makes Azure ephemeral OS disks use GA full caching through the Go SDK and coordinator, enables that mode for direct fixed-ID leases, and updates SKU validation, tests, and documentation. Merge readiness⛔ Blocked before merge - 6 items remain The GA Azure path is useful and has credible live CLI proof, but current main and v0.66.0 still support both older disk modes. This PR needs an explicit compatibility decision because it rejects one shipped setting and changes the provisioning requirements of another. Priority: P2 Review scores
Verification
How this fits togetherCrabbox turns CLI flags or coordinator lease requests into Azure VM settings. The Azure provider selects a VM size and OS disk mode, then sends a VM creation request that determines whether the lease can provision and support checkpoints. flowchart LR
A[CLI flags or lease request] --> B[Azure disk setting]
B --> C[VM size selection]
C --> D[Eligibility check]
D --> E[Azure VM request]
E --> F[Provisioned lease]
F --> G[Run or checkpoint]
Decision needed
Why: The PR deliberately changes a released configuration contract, and the discussion records no maintainer acceptance of that upgrade impact. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve both shipped disk-mode meanings during upgrade, add an explicit GA full-caching choice, and verify fresh and existing settings through CLI and coordinator provisioning. Do we have a high-confidence way to reproduce the issue? Not applicable as a feature migration. Current main and v0.66.0 clearly define the two existing settings, and the introduced validation shows where each upgrade break occurs. Is this the best way to solve the issue? No. The GA request path is supported by live proof, but assigning it to the shipped ephemeral value and rejecting ephemeral-preview is not the safest upgrade path without explicit maintainer approval. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against fc3cb7d44ef1. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Rebuild #2565 on current main. Use the pinned SDK's native full-caching field and matching coordinator API, preserving conditional fixed-ID creates and replay protection. Reject removed preview settings across config show, flags, and coordinator requests; return actionable migration errors and count constrained active vCPUs for GA eligibility. Document the breaking upgrade and retain contributor coverage alongside request/default rejection and replay regression tests. Co-authored-by: Jonathan Moss <2729151+jwmoss@users.noreply.github.com>
Rebuild #2565 on current main. Use the pinned SDK's native full-caching field and matching coordinator API, preserving conditional fixed-ID creates and replay protection. Reject removed preview settings across config show, flags, and coordinator requests; return actionable migration errors and count constrained active vCPUs for GA eligibility. Document the breaking upgrade and retain contributor coverage alongside request/default rejection and replay regression tests. Co-authored-by: Jonathan Moss <2729151+jwmoss@users.noreply.github.com>
Rebuild #2565 on current main. Use the pinned SDK's native full-caching field and matching coordinator API, preserving conditional fixed-ID creates and replay protection. Reject removed preview settings across config show, flags, and coordinator requests; return actionable migration errors and count constrained active vCPUs for GA eligibility. Document the breaking upgrade and retain contributor coverage alongside request/default rejection and replay regression tests. Co-authored-by: Jonathan Moss <2729151+jwmoss@users.noreply.github.com>
* feat(azure): use GA full caching for ephemeral OS disks Rebuild #2565 on current main. Use the pinned SDK's native full-caching field and matching coordinator API, preserving conditional fixed-ID creates and replay protection. Reject removed preview settings across config show, flags, and coordinator requests; return actionable migration errors and count constrained active vCPUs for GA eligibility. Document the breaking upgrade and retain contributor coverage alongside request/default rejection and replay regression tests. Co-authored-by: Jonathan Moss <2729151+jwmoss@users.noreply.github.com> * refactor(worker): deslop --------- Co-authored-by: Jonathan Moss <2729151+jwmoss@users.noreply.github.com>
|
Thanks @jwmoss! This landed via #2567 (merged as 2d694e6 and deployed to the production coordinator, with a passing post-deploy live smoke), rebuilt on current main with your implementation and tests preserved and co-author credit in the commit. Follow-ups on top: constrained-vCPU eligibility per Microsoft's GA prerequisites, explicit coordinator rejection of the legacy |
What Problem This Solves
Azure full caching is generally available, but Crabbox still exposes it as a separate preview disk mode.
User Impact
--azure-os-disk ephemeralnow enables GA full caching in the CLI and coordinator, including direct fixed-ID leases.Migration: replace
ephemeral-previewwithephemeralin flags, config, and environment variables. The removed value returns a clear replacement error. Existingephemeralsettings now require full-caching support: at least eight vCPUs and a supported VM family. Smaller VMs and Fsv2 no longer qualify. Managed disks remain the default; ephemeral disks still cannot provide native snapshot checkpoints. No new credentials are required.Why This Change Was Made
The preview implementation in #186 introduced
ephemeral-previewand a raw ARM request because the SDK lacked the full-caching field. Azure now documents GA support, and Crabbox's pinned Go SDK exposes that field.Use the native SDK and matching coordinator API, remove the preview request/polling workaround, and apply GA family and vCPU requirements. Fixed-ID creates retain
If-None-Match: *and replay protection.Evidence
Selected Azure evidence appears below. Account IDs, addresses, resource identities, local paths, and credential material are omitted.
scripts/live-smoke.sh: passes on Linux x64,Standard_D8ads_v6, Ubuntu 26.04, ineastus. Covers provision, readiness, inspect, SSH, cache inventory, sync, command execution, and stop. ARM confirmsenableFullCaching: true,Local,NvmeDisk, andReadOnly.false.The full Go race gate is not a pass: 101 packages pass, but the CLI package hits localhost SSH failures and its 20-minute timeout. The static SSH failure also reproduces on unchanged upstream. One unchanged script test fails under macOS Bash 3.2; its full 44-test file passes in Linux Docker.
Live proof covers direct Linux x64. Coordinator coverage uses HTTP fixtures; Windows and ARM64 are not live-tested here. The ARM flag proves Azure accepted full caching, not completion of the background cache copy. This PR does not deploy or release Crabbox.
Sanitized Azure readback and command output
Selected fields from the raw ARM
2026-04-01response:{ "size": "Standard_D8ads_v6", "state": "Succeeded", "osDisk": { "osType": "Linux", "diskSizeGB": 30, "createOption": "FromImage", "caching": "ReadOnly", "diffDiskSettings": { "enableFullCaching": true, "option": "Local", "placement": "NvmeDisk" }, "managedDisk": { "storageAccountType": "StandardSSD_LRS" } } }Selected terminal output from the successful live smoke:
The command exits zero. Fixed-ID replay preserves the same resource ID and immutable VM ID. The final resource-group existence check returns
false.Stop reports SSH exit-255 warnings for guest-side cleanup; Azure resource deletion succeeds. The disk settings do not measure background cache-copy completion.