From bd5cc4ef1fbccdc68d18b836b6b9f8234c8a060e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 22:01:13 +0200 Subject: [PATCH 1/4] fix(scaleway): a disk in the block product is reachable from its server, and the detach that answered 200 released nothing (#571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #8 served `sbs_volume`, one server's disks can live in two stores. Every operation on the server-volume relationship resolved `kindVolume` alone, so a disk the emulator itself had just published was unreachable through all of them. Measured with scw 2.56.3 against a binary built from 3b00d23, on 2026-08-28, on a server anybody can create with `root-volume=sbs:20GB`: attach-volume, on a volume `scw block volume create` made 404 attach-volume, on another server's block root 404 server update volumes.0.id= refused, "does not exist" instance snapshot create volume-id= 404 instance volume delete 404 (correct, see below) detach-volume 200, and released nothing The last line is the one that costs a client its command. `scw instance server terminate with-block=true` walks GetVolume (instance, 404) → GetVolume (block, 200) → detach-volume → then polls the block volume until its status leaves `in_use`. The detach answered 200 while the disk kept its server, so the status never moved: rc=124 at twenty-five seconds, five identical block GETs in the CLI's own -D trace. `anyVolume` resolves both products, and the five doors that take a server and a volume id go through it: attach-volume, detach-volume, the update's volume map, a create naming a volume, and instance CreateSnapshot. `serverVolumeView` dispatches the rendering, so a block disk is published inside a server as an instance VolumeServer carrying `volume_type: sbs_volume` — the value the Terraform provider branches on — whichever door attached it. **Two operations deliberately do NOT resolve both kinds**, and that is the half this change had to get right. instance/v1 GetVolume and DeleteVolume keep answering 404 for a block volume, because the SDK's own dual-product reader (`api/instance/v1/volume_utils.go`, `getUnknownVolume`) falls back to `block.GetVolume` only on a typed `ResourceNotFoundError`. A symmetrical fix would have ended the search before it reached the product that owns the disk, which is the failure #8 exists to prevent. The mutation that makes `volumeOf` resolve both kinds is in the spec, and it reddens `TestAnSbsRootVolumeIsReadableThroughTheBlockFallback`. Three defects of the same family came with it: * `attachStoredVolume` never marked a block volume `in_use`, while `detachStoredVolume` had always marked it `available` — so an attached disk answered `references: [attached]` and `status: available` at once, and `scw` polls the status; * an instance snapshot of a block volume fell through to the `b_ssd` default, naming a product it was not taken from. `sbs_snapshot` is read from the SDK (`VolumeVolumeType` declares it beside `sbs_volume`, and `Snapshot .VolumeType` is a `VolumeVolumeType`, while `CreateSnapshotRequest .VolumeType` cannot spell it) and declared as a reading, not a measurement: no recorded account here holds one. A type the client names still wins, because that field "overrides" in the SDK's own words; * `Owns` declared `kindPrivateNIC` and `kindVolume` and not `kindBlockVolume`, so `storetest.Orphans` — the invariant that no disk names a machine that is gone — skipped every disk of one product. The witness is planted rather than hoped for: a sweep that reports nothing is indistinguishable from a sweep that looked nowhere. `TestAttachingDoesNotStealAnotherServersVolume` now runs its three doors once per product. The honest reason is not that it caught a theft: before the shared resolver a block root was unstealable because it was unreachable, so the guard read as present and stood on an accident. Neutralise the owner comparison in `attachStoredVolume` and both halves go red — which is the property the instance half alone could not give. Nothing changes for any existing client: every answer that was a 200 is the same 200. `mise run conformance:leg -- scw-cli` and `-- fields` green, `mise run prepush` green, and the twelve mutations of `tools/falsify/specs/block-volumes-reach-their-server.json` all bite. One existing mutation was retargeted rather than rewritten: the `volumesOf` fragment of `scaleway-cloud-fidelity.json` became ambiguous when `anyVolume` introduced a second identical loop header, and `falsify:lint` refused it before this commit existed. It now carries the inner line that only `volumesOf` has. Refs #571. This is step 1 of two; it changes no default and closes nothing. Assisted-by: Claude Code (claude-opus-5) --- internal/providers/scaleway/block.go | 11 +- .../providers/scaleway/block_attach_test.go | 300 ++++++++++++++++++ .../scaleway/ownership_audit_test.go | 148 ++++++--- internal/providers/scaleway/privatenics.go | 9 +- internal/providers/scaleway/servers.go | 55 +++- internal/providers/scaleway/snapshots.go | 29 +- internal/providers/scaleway/volumes.go | 73 +++++ .../block-volumes-reach-their-server.json | 103 ++++++ .../specs/scaleway-cloud-fidelity.json | 4 +- 9 files changed, 664 insertions(+), 68 deletions(-) create mode 100644 internal/providers/scaleway/block_attach_test.go create mode 100644 tools/falsify/specs/block-volumes-reach-their-server.json diff --git a/internal/providers/scaleway/block.go b/internal/providers/scaleway/block.go index 034d8ca5..184152cf 100644 --- a/internal/providers/scaleway/block.go +++ b/internal/providers/scaleway/block.go @@ -774,8 +774,9 @@ func (p *Pack) listBlockVolumeTypes(w http.ResponseWriter, r *http.Request) { // ---- The bridge with instance/v1 ------------------------------------------- -// blockRootVolumeServerView renders a block volume the way instance/v1 lists it -// inside a server. +// blockVolumeServerView renders a block volume the way instance/v1 lists it +// inside a server. Reached through serverVolumeView, which is what every builder +// of a `volumes` map calls. // // Two shapes for one disk, and both are needed: the server's `volumes` map is an // instance VolumeServer whatever product owns the volume, and the fallback read @@ -785,7 +786,11 @@ func (p *Pack) listBlockVolumeTypes(w http.ResponseWriter, r *http.Request) { // // volume_type is "sbs_volume", which is what tells the provider to fall back at // all: it reads instance.GetVolume first and only tries block on a typed 404. -func blockRootVolumeServerView(res *resource.Resource) map[string]any { +// +// It was named blockRootVolumeServerView while a root disk was the only block +// volume a server could carry. It is not: `scw instance server attach-volume +// volume-type=sbs_volume` puts one under any key, and the name said otherwise. +func blockVolumeServerView(res *resource.Resource) map[string]any { out := map[string]any{ "id": res.ID, "name": textOf(res.Attrs["name"]), diff --git a/internal/providers/scaleway/block_attach_test.go b/internal/providers/scaleway/block_attach_test.go new file mode 100644 index 00000000..2367a6f0 --- /dev/null +++ b/internal/providers/scaleway/block_attach_test.go @@ -0,0 +1,300 @@ +package scaleway_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/core/resource" + "github.com/stephrobert/feint/internal/core/store/storetest" + "github.com/stephrobert/feint/internal/providers/scaleway" +) + +// The server-volume relationship, over a disk that lives in the block product. +// +// Every operation here was measured against the binary built from 3b00d23 with +// scw 2.56.3 on 2026-08-28, and every one of them failed: attach-volume, +// detach-volume, the update's volume map and a create naming a block volume +// resolved instance/v1 alone, so a disk `scw block volume create` had just made +// — or a root disk the same emulator had published under volumes["0"] — was +// unreachable through all of them. The one that did not answer 404 answered +// worse: detach-volume answered 200 and released nothing. +// +// These are not tests about a root volume. A block volume reaches a server under +// any key, and AttachServerVolumeRequest declares volume_type with "sbs_volume" +// among its values (instance_sdk.go), so the operation is defined over one. + +// blockStatusOf reads a block volume's status and how many servers reference it. +func blockStatusOf(t *testing.T, ts *httptest.Server, id string) (status string, references int) { + t.Helper() + code, out := do(t, ts, "GET", blockURL+"/volumes/"+id, "") + if code != http.StatusOK { + t.Fatalf("block volume %s answers %d, want 200", id, code) + } + status, _ = out["status"].(string) + refs, _ := out["references"].([]any) + return status, len(refs) +} + +// serverVolumeMap reads a server's volumes map. +func serverVolumeMap(t *testing.T, ts *httptest.Server, serverID string) map[string]any { + t.Helper() + _, out := do(t, ts, "GET", zone+"/servers/"+serverID, "") + srv, _ := out["server"].(map[string]any) + volumes, _ := srv["volumes"].(map[string]any) + return volumes +} + +// A block volume attaches and detaches through the instance server routes, and +// says so in both products afterwards. +// +// `scw instance server attach-volume server-id=… volume-id= +// volume-type=sbs_volume` answered "cannot find resource 'volume'" before #571, +// on a volume created two commands earlier by `scw block volume create`. +func TestABlockVolumeAttachesAndDetachesThroughTheServerRoutes(t *testing.T) { + ts := newTestServer(t) + srv := aServer(t, ts, "host") + vol := blockVolumeWith(t, ts, "data", 10000000000) + + status, body := do(t, ts, "POST", zone+"/servers/"+srv+"/attach-volume", `{"volume_id":"`+vol+`","volume_type":"sbs_volume"}`) + if status != http.StatusOK { + t.Fatalf("attach-volume answered %d, want 200: %v", status, body) + } + + // The entry the server publishes is an instance VolumeServer carrying + // volume_type "sbs_volume": that value is what sends the Terraform provider + // to the block fallback, and the instance rendering of a block volume has no + // volume_type at all. + volumes := serverVolumeMap(t, ts, srv) + var entry map[string]any + for _, v := range volumes { + listed, _ := v.(map[string]any) + if id, _ := listed["id"].(string); id == vol { + entry = listed + } + } + if entry == nil { + t.Fatalf("the server does not list the attached block volume: %v", volumes) + } + if entry["volume_type"] != "sbs_volume" { + t.Errorf("the attached block volume is published as %v, want sbs_volume", entry["volume_type"]) + } + + if got, refs := blockStatusOf(t, ts, vol); got != "in_use" || refs != 1 { + t.Errorf("after attach the block volume reads %s/%d references, want in_use/1", got, refs) + } + + status, body = do(t, ts, "POST", zone+"/servers/"+srv+"/detach-volume", `{"volume_id":"`+vol+`"}`) + if status != http.StatusOK { + t.Fatalf("detach-volume answered %d, want 200: %v", status, body) + } + if got, refs := blockStatusOf(t, ts, vol); got != "available" || refs != 0 { + t.Errorf("after detach the block volume reads %s/%d references, want available/0", got, refs) + } + for key, v := range serverVolumeMap(t, ts, srv) { + listed, _ := v.(map[string]any) + if id, _ := listed["id"].(string); id == vol { + t.Errorf("the server still lists the detached block volume under %q", key) + } + } +} + +// A block volume a server holds says in_use, not available. +// +// block/v1's `status` IS the resource state here, and detachStoredVolume already +// set it back to available on the way out while nothing set it to in_use on the +// way in. So a volume created free and then attached answered references: +// [attached] and status: available at the same time — and `scw` polls the +// status, never the references (its own -D trace of `server terminate` shows +// five identical GETs waiting on that field). +func TestAttachingABlockVolumeMarksItInUse(t *testing.T) { + ts := newTestServer(t) + srv := aServer(t, ts, "host") + vol := blockVolumeWith(t, ts, "fresh", 10000000000) + + if got, _ := blockStatusOf(t, ts, vol); got != "available" { + t.Fatalf("a fresh block volume reads %s, want available: the test measures a transition", got) + } + if status, body := do(t, ts, "POST", zone+"/servers/"+srv+"/attach-volume", `{"volume_id":"`+vol+`"}`); status != http.StatusOK { + t.Fatalf("attach-volume answered %d: %v", status, body) + } + if got, refs := blockStatusOf(t, ts, vol); got != "in_use" || refs != 1 { + t.Errorf("an attached block volume reads %s with %d references, want in_use with 1", got, refs) + } +} + +// Detaching a block ROOT volume really releases it, which is what stops the CLI +// hanging. +// +// `scw instance server terminate` walks GetVolume (instance, 404) → GetVolume +// (block, 200) → detach-volume → then polls the block volume until its status +// leaves in_use. detach-volume resolved kindVolume alone, so it answered 200 and +// changed nothing: measured on 2026-08-28 against a binary built from 3b00d23, +// `scw instance server terminate with-block=true` returned rc=124 at +// twenty-five seconds with five identical block GETs in its own trace, on a +// server anybody can create with `root-volume=sbs:20GB`. +func TestTerminateReleasesABlockRootVolume(t *testing.T) { + ts := newTestServer(t) + srv, body := serverWith(t, ts, + `{"name":"sbs","commercial_type":"DEV1-S","volumes":{"0":{"volume_type":"sbs_volume","size":20000000000}}}`) + volumes, _ := body["volumes"].(map[string]any) + root, _ := volumes["0"].(map[string]any) + rootID, _ := root["id"].(string) + if rootID == "" { + t.Fatalf("the server carries no root volume: %v", body) + } + if got, _ := blockStatusOf(t, ts, rootID); got != "in_use" { + t.Fatalf("a root volume in use reads %s, want in_use: the test measures a transition", got) + } + + if status, out := do(t, ts, "POST", zone+"/servers/"+srv+"/detach-volume", `{"volume_id":"`+rootID+`"}`); status != http.StatusOK { + t.Fatalf("detach-volume answered %d: %v", status, out) + } + // The field the client polls. A 200 that leaves this at in_use is the hang. + if got, refs := blockStatusOf(t, ts, rootID); got != "available" || refs != 0 { + t.Fatalf("after detach-volume the block root reads %s with %d references, want available with 0 — this is the poll that never ends", got, refs) + } +} + +// An update's volume map reaches a block volume. +// +// The map is the field a Terraform plan writes, and it refused with "volume … +// does not exist in fr-par-1" about a disk the same emulator had created. +func TestAnUpdatesVolumeMapReachesABlockVolume(t *testing.T) { + ts := newTestServer(t) + srv := aServer(t, ts, "host") + root, _ := serverVolumeMap(t, ts, srv)["0"].(map[string]any) + rootID, _ := root["id"].(string) + vol := blockVolumeWith(t, ts, "extra", 10000000000) + + status, body := do(t, ts, "PATCH", zone+"/servers/"+srv, + `{"volumes":{"0":{"id":"`+rootID+`"},"1":{"id":"`+vol+`"}}}`) + if status != http.StatusOK { + t.Fatalf("the update answered %d, want 200: %v", status, body) + } + entry, _ := serverVolumeMap(t, ts, srv)["1"].(map[string]any) + if entry == nil || entry["id"] != vol { + t.Fatalf("the update did not attach the block volume: %v", serverVolumeMap(t, ts, srv)) + } + if entry["volume_type"] != "sbs_volume" { + t.Errorf("the block volume is published as %v, want sbs_volume", entry["volume_type"]) + } + if got, refs := blockStatusOf(t, ts, vol); got != "in_use" || refs != 1 { + t.Errorf("the block volume reads %s/%d after the update, want in_use/1", got, refs) + } +} + +// A create naming a block volume attaches it, the way +// additional_volume_ids does. +// +// It was skipped silently: the create answered 201 with the volume left +// detached and nothing saying so, which is the defect +// TestAdditionalVolumesAreAttachedAtCreate was written for, one product +// further on. +func TestACreateNamingABlockVolumeAttachesIt(t *testing.T) { + ts := newTestServer(t) + vol := blockVolumeWith(t, ts, "carried", 10000000000) + + srv, body := serverWith(t, ts, + `{"name":"carrier","commercial_type":"DEV1-S","image":"ubuntu_jammy","volumes":{"1":{"id":"`+vol+`"}}}`) + volumes, _ := body["volumes"].(map[string]any) + entry, _ := volumes["1"].(map[string]any) + if entry == nil || entry["id"] != vol { + t.Fatalf("the create did not attach the block volume: %v", volumes) + } + if entry["volume_type"] != "sbs_volume" { + t.Errorf("the block volume is published as %v, want sbs_volume", entry["volume_type"]) + } + if holder := holderOf(t, ts, vol); holder != srv { + t.Errorf("the block volume names %q, want the server %q", holder, srv) + } +} + +// An instance snapshot of a block volume is an sbs_snapshot. +// +// The route resolved kindVolume alone, so it answered 404 on the disk the same +// emulator published under the server's volumes["0"]. The type is read from the +// SDK rather than from a recording, and the reading is narrow: instance/v1 +// VolumeVolumeType declares sbs_snapshot beside sbs_volume and Snapshot +// .VolumeType is a VolumeVolumeType, while CreateSnapshotRequest.VolumeType +// (SnapshotVolumeType) cannot spell it — b_ssd would name a different product. +// +// What a client asks for still wins, because the request field "overrides the +// volume_type of the snapshot" in the SDK's own words. +func TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot(t *testing.T) { + ts := newTestServer(t) + _, body := serverWith(t, ts, + `{"name":"sbs","commercial_type":"DEV1-S","volumes":{"0":{"volume_type":"sbs_volume","size":20000000000}}}`) + volumes, _ := body["volumes"].(map[string]any) + root, _ := volumes["0"].(map[string]any) + rootID, _ := root["id"].(string) + + status, out := do(t, ts, "POST", zone+"/snapshots", `{"name":"snap","volume_id":"`+rootID+`"}`) + if status != http.StatusCreated { + t.Fatalf("snapshot of a block volume answered %d, want 201: %v", status, out) + } + snap, _ := out["snapshot"].(map[string]any) + if snap["volume_type"] != "sbs_snapshot" { + t.Errorf("the snapshot reports volume_type %v, want sbs_snapshot", snap["volume_type"]) + } + base, _ := snap["base_volume"].(map[string]any) + if base == nil || base["id"] != rootID { + t.Errorf("the snapshot does not name the volume it was taken of: %v", snap["base_volume"]) + } + // The size comes from the volume, not from the catalogue default: a + // snapshot that reports 20 GB of a 40 GB disk is a lie a client stores. + if size, _ := snap["size"].(float64); size != 20000000000 { + t.Errorf("the snapshot reports size %v, want the volume's 20000000000", snap["size"]) + } + + // A named type wins, because the request field overrides. + status, out = do(t, ts, "POST", zone+"/snapshots", + `{"name":"unified","volume_id":"`+rootID+`","volume_type":"unified"}`) + if status != http.StatusCreated { + t.Fatalf("snapshot with an explicit type answered %d: %v", status, out) + } + snap, _ = out["snapshot"].(map[string]any) + if snap["volume_type"] != "unified" { + t.Errorf("the client named unified and the snapshot reports %v", snap["volume_type"]) + } +} + +// The orphan sweep knows a block volume belongs to a server. +// +// `Owns` declared kindPrivateNIC and kindVolume and not kindBlockVolume, so +// storetest.Orphans — the invariant that no disk names a machine that is gone — +// skipped every disk of one product for as long as that product has existed +// here. This is measurement-integrity's rule 1 made executable: a sweep that +// reports nothing is indistinguishable from a sweep that looked nowhere, so the +// witness is planted. The instance volume beside it is the control: if the +// planting itself were broken, neither would be reported and the test would +// still be green on the mutation. +func TestTheSweepSeesABlockVolumeThatNamesADeadServer(t *testing.T) { + orphanBlock := &resource.Resource{ + ID: "block-orphan", + Kind: "block/volume", + Tenant: resource.Tenant{Provider: scaleway.Name}, + Runtime: map[string]string{"server": "a-server-that-is-gone"}, + } + orphanInstance := &resource.Resource{ + ID: "instance-orphan", + Kind: "instance/volume", + Tenant: resource.Tenant{Provider: scaleway.Name}, + Runtime: map[string]string{"server": "a-server-that-is-gone"}, + } + + found := storetest.Orphans([]*resource.Resource{orphanBlock, orphanInstance}, scaleway.Owns, nil) + if len(found) != 2 { + t.Fatalf("the sweep reported %d orphan(s), want both the block and the instance disk: %v", len(found), found) + } + var sawBlock bool + for _, line := range found { + if strings.Contains(line, "block-orphan") { + sawBlock = true + } + } + if !sawBlock { + t.Errorf("the sweep did not report the block volume that names a dead server: %v", found) + } +} diff --git a/internal/providers/scaleway/ownership_audit_test.go b/internal/providers/scaleway/ownership_audit_test.go index 0510412a..1b0554e7 100644 --- a/internal/providers/scaleway/ownership_audit_test.go +++ b/internal/providers/scaleway/ownership_audit_test.go @@ -81,17 +81,62 @@ func TestAVolumeStateIsOneTheSDKDeclares(t *testing.T) { } } +// holderOf answers which server holds a volume, whichever product it lives in. +// +// It walks instance first and block on a 404, which is not a convenience: it is +// the SDK's own reader (api/instance/v1/volume_utils.go, getUnknownVolume), and +// a test that asked only the instance side would report "nobody holds it" about +// every disk of the block product — the exact shape of the defect it is here to +// catch (measurement-integrity, rule 2: a reader with two outcomes reports +// "absent" when it means "elsewhere"). +func holderOf(t *testing.T, ts *httptest.Server, volumeID string) string { + t.Helper() + if status, out := do(t, ts, "GET", zone+"/volumes/"+volumeID, ""); status == http.StatusOK { + vol, _ := out["volume"].(map[string]any) + server, _ := vol["server"].(map[string]any) + if server == nil { + return "" + } + id, _ := server["id"].(string) + return id + } + status, out := do(t, ts, "GET", blockURL+"/volumes/"+volumeID, "") + if status != http.StatusOK { + t.Fatalf("volume %s answers on neither product (block said %d): the test cannot read its owner", volumeID, status) + } + refs, _ := out["references"].([]any) + for _, entry := range refs { + ref, _ := entry.(map[string]any) + if ref["product_resource_type"] == "instance_server" { + id, _ := ref["product_resource_id"].(string) + return id + } + } + return "" +} + // Attaching a volume must not take it from the server that owns it. // // The guard read Attrs["server"] while every other reader in the pack — the // view, volumesOf, the delete and terminate paths — reads // Runtime[runtimeServerKey]. So it saw nothing on a root volume, and an audit // moved one server's root volume onto another: both then listed it. +// +// It is run once per PRODUCT since #571, and the honest account of why is not +// "it caught a theft". Before the shared resolver the three doors resolved +// kindVolume alone, so a block root was unstealable because it was unreachable: +// this table would have passed on the block half for a reason that had nothing +// to do with ownership. That is the failure mode this repository keeps +// measuring — a control that reads as present and is standing on an accident — +// and the resolver is what removes the accident. What the block half asserts is +// therefore forward-looking and it is checked by mutation, not by history: +// neutralise the owner comparison in attachStoredVolume and BOTH halves fail, +// which is the property the instance half alone could not give. func TestAttachingDoesNotStealAnotherServersVolume(t *testing.T) { // Three doors onto the same fact. A third audit walked the two the previous // fix had not touched: only attach-volume asked the question, so a create // naming another server's root volume moved it and both servers listed it. - for _, door := range []struct { + doors := []struct { what string take func(t *testing.T, ts *httptest.Server, thief, volume string) int }{ @@ -109,59 +154,64 @@ func TestAttachingDoesNotStealAnotherServersVolume(t *testing.T) { `{"volumes":{"1":{"id":"`+volume+`"}}}`) return status }}, - } { - t.Run(door.what, func(t *testing.T) { - ts := newTestServer(t) - owner := aServer(t, ts, "owner") - thief := aServer(t, ts, "thief-host") - - _, out := do(t, ts, "GET", zone+"/servers/"+owner, "") - srv, _ := out["server"].(map[string]any) - volumes, _ := srv["volumes"].(map[string]any) - root, _ := volumes["0"].(map[string]any) - rootID, _ := root["id"].(string) - if rootID == "" { - t.Fatalf("the owner has no root volume: %v", srv) - } + } + // The owner's root disk, in each of the two products a Scaleway server's + // disks can live in. + products := []struct { + what string + owner string + }{ + {"an instance root", `{"name":"owner","commercial_type":"DEV1-S","image":"ubuntu_jammy"}`}, + {"a block root", `{"name":"owner","commercial_type":"DEV1-S","image":"ubuntu_jammy","volumes":{"0":{"volume_type":"sbs_volume","size":20000000000}}}`}, + } + for _, product := range products { + for _, door := range doors { + t.Run(product.what+"/"+door.what, func(t *testing.T) { + ts := newTestServer(t) + owner, ownerBody := serverWith(t, ts, product.owner) + thief := aServer(t, ts, "thief-host") + + volumes, _ := ownerBody["volumes"].(map[string]any) + root, _ := volumes["0"].(map[string]any) + rootID, _ := root["id"].(string) + if rootID == "" { + t.Fatalf("the owner has no root volume: %v", ownerBody) + } - // The thief's own root, which a failed steal must not cost it. - _, out = do(t, ts, "GET", zone+"/servers/"+thief, "") - srv, _ = out["server"].(map[string]any) - volumes, _ = srv["volumes"].(map[string]any) - own, _ := volumes["0"].(map[string]any) - ownRoot, _ := own["id"].(string) + // The thief's own root, which a failed steal must not cost it. + _, out := do(t, ts, "GET", zone+"/servers/"+thief, "") + srv, _ := out["server"].(map[string]any) + volumes, _ = srv["volumes"].(map[string]any) + own, _ := volumes["0"].(map[string]any) + ownRoot, _ := own["id"].(string) - door.take(t, ts, thief, rootID) + door.take(t, ts, thief, rootID) - // Whatever the status, the volume must not have moved: a create that - // skips an unavailable volume answers 201, and that is fine — what is - // not fine is the owner losing its disk. - _, out = do(t, ts, "GET", zone+"/volumes/"+rootID, "") - vol, _ := out["volume"].(map[string]any) - server, _ := vol["server"].(map[string]any) - if server == nil || server["id"] != owner { - t.Errorf("%s moved the root volume: it now names %v", door.what, vol["server"]) - } + // Whatever the status, the volume must not have moved: a create that + // skips an unavailable volume answers 201, and that is fine — what is + // not fine is the owner losing its disk. + if holder := holderOf(t, ts, rootID); holder != owner { + t.Errorf("%s moved the root volume: it now names %q, want the owner %q", door.what, holder, owner) + } - // The two consequences the first version of this test did not - // assert, which is how the PATCH door went on stealing through - // three audits: the thief must not list the volume, and must not - // have lost its own root doing so. - _, out = do(t, ts, "GET", zone+"/servers/"+thief, "") - srv, _ = out["server"].(map[string]any) - volumes, _ = srv["volumes"].(map[string]any) - for key, entry := range volumes { - listed, _ := entry.(map[string]any) - if id, _ := listed["id"].(string); id == rootID { - t.Errorf("%s: the thief lists the owner's volume under %q — both servers hold it", door.what, key) + // The two consequences the first version of this test did not + // assert, which is how the PATCH door went on stealing through + // three audits: the thief must not list the volume, and must not + // have lost its own root doing so. + _, out = do(t, ts, "GET", zone+"/servers/"+thief, "") + srv, _ = out["server"].(map[string]any) + volumes, _ = srv["volumes"].(map[string]any) + for key, entry := range volumes { + listed, _ := entry.(map[string]any) + if id, _ := listed["id"].(string); id == rootID { + t.Errorf("%s: the thief lists the owner's volume under %q — both servers hold it", door.what, key) + } } - } - _, out = do(t, ts, "GET", zone+"/volumes/"+ownRoot, "") - vol, _ = out["volume"].(map[string]any) - if holder, _ := vol["server"].(map[string]any); holder == nil || holder["id"] != thief { - t.Errorf("%s: the thief's own root was detached by the attempt: %v", door.what, vol["server"]) - } - }) + if holder := holderOf(t, ts, ownRoot); holder != thief { + t.Errorf("%s: the thief's own root was detached by the attempt: it names %q", door.what, holder) + } + }) + } } } diff --git a/internal/providers/scaleway/privatenics.go b/internal/providers/scaleway/privatenics.go index b2fba578..2482fe0e 100644 --- a/internal/providers/scaleway/privatenics.go +++ b/internal/providers/scaleway/privatenics.go @@ -588,9 +588,16 @@ func (p *Pack) privateNICView(res *resource.Resource) map[string]any { // volume that named a deleted server, then a NIC that did (#214). The vocabulary // is Scaleway's, so it is declared here; the invariant is everyone's, so it lives // in storetest. +// +// kindBlockVolume was missing from this list for as long as the block product +// has existed here (#571). It holds its server in the same Runtime key, it is +// released by the same detachStoredVolume, and it was simply not declared — so +// the sweep that proves no disk names a dead machine skipped every disk of one +// product. Not a hypothesis: a kind absent from this switch cannot be reported +// by storetest.Orphans, whatever it holds. func Owns(res *resource.Resource) (kind, id string, ok bool) { switch res.Kind { - case kindPrivateNIC, kindVolume: + case kindPrivateNIC, kindVolume, kindBlockVolume: if serverID := res.Runtime[runtimeServerKey]; serverID != "" { return kindServer, serverID, true } diff --git a/internal/providers/scaleway/servers.go b/internal/providers/scaleway/servers.go index fc629a5c..8e41742e 100644 --- a/internal/providers/scaleway/servers.go +++ b/internal/providers/scaleway/servers.go @@ -1284,7 +1284,12 @@ func (p *Pack) setServerVolumes(server *resource.Resource, wanted map[string]vol for key, tmpl := range wanted { switch { case tmpl.ID != "": - vol, found := p.env.Store.Get(Name, kindVolume, tmpl.ID) + // Both products, through the shared resolver: the map named a + // volume by id and only instance/v1 was searched, so + // `volumes.0.id=` answered "volume … does not exist + // in fr-par-1" about a disk the same emulator had just created + // (#571). + vol, found := p.anyVolume(tmpl.ID) if !found || vol.Tenant.Zone != server.Tenant.Zone { return fmt.Errorf("volume %s does not exist in %s", tmpl.ID, server.Tenant.Zone) } @@ -1302,7 +1307,7 @@ func (p *Pack) setServerVolumes(server *resource.Resource, wanted map[string]vol return err } keep[vol.ID] = true - view[key] = volumeView(vol) + view[key] = serverVolumeView(vol) case tmpl.Size > 0: // A template with a size and no id asks for a new disk, the way // creation does. Same helper, so the two paths cannot diverge. @@ -1547,19 +1552,21 @@ func deref(p *bool, fallback bool) bool { // // TestAdditionalVolumesAreAttachedAtCreate fails without this. func (p *Pack) attachTemplateVolumes(templates map[string]volumeTemplate, root, server *resource.Resource, zone, serverName string) map[string]any { - // A root volume that lives in block gets block's rendering inside the server, + // A volume that lives in block gets block's rendering inside the server, // which is an instance VolumeServer carrying volume_type "sbs_volume" — the // value that sends the Terraform provider to the block fallback (#8). // Copying the instance view here would publish a volume with no type at all. - out := map[string]any{"0": volumeView(root)} - if root.Kind == kindBlockVolume { - out["0"] = blockRootVolumeServerView(root) - } + out := map[string]any{"0": serverVolumeView(root)} for key, tpl := range templates { if key == "0" || tpl.ID == "" { continue } - vol, ok := p.env.Store.Get(Name, kindVolume, tpl.ID) + // Both products: `additional-volumes.0=` resolved + // instance/v1 alone, so a create naming a disk of the block product + // skipped it silently and answered 201 with the volume unattached — + // the same "declared, read, not to the end" shape this function was + // written for, one product further on (#571). + vol, ok := p.anyVolume(tpl.ID) if !ok || vol.Tenant.Zone != zone { continue } @@ -1569,7 +1576,7 @@ func (p *Pack) attachTemplateVolumes(templates map[string]volumeTemplate, root, if err := p.attachStoredVolume(vol, server, serverName); err != nil { continue } - out[key] = volumeView(vol) + out[key] = serverVolumeView(vol) } return out } @@ -1605,13 +1612,25 @@ func (p *Pack) attachServerVolume(w http.ResponseWriter, r *http.Request) { writeInvalidArguments(w, ArgumentError{ArgumentName: "body", Reason: "format", HelpMessage: err.Error()}) return } - vol, ok := p.env.Store.Get(Name, kindVolume, req.VolumeID) + // Both products. AttachServerVolumeRequest declares volume_type with + // "sbs_volume" among its values (instance_sdk.go), so the operation this + // route claims is defined over a block volume — and resolving kindVolume + // alone answered 404 for every one of them, including a disk `scw block + // volume create` had just made. Measured with scw 2.56.3 (#571). + vol, ok := p.anyVolume(req.VolumeID) if !ok || vol.Tenant.Zone != zone { writeNotFound(w, "volume", req.VolumeID) return } // The API refuses to move a volume already in use, and Terraform reads that // error rather than guessing. + // + // This is also where the anti-theft guard of #202 starts covering a block + // disk: while the resolution above found nothing, one server could not take + // another's block root because nobody could reach it at all, which is an + // accident and not a control. attachStoredVolume asks the question for both + // kinds — TestAttachingDoesNotStealAnotherServersVolume now walks its three + // doors twice, once per product. key := p.nextVolumeKey(res) serverName, _ := res.Attrs["name"].(string) if err := p.attachStoredVolume(vol, res, serverName); err != nil { @@ -1623,7 +1642,7 @@ func (p *Pack) attachServerVolume(w http.ResponseWriter, r *http.Request) { // stored map — never through the clone's, which resource.Clone shares with // the store, and never by Commit, whose wholesale write erased a concurrent // write to another field of the same server after its 200 (#295). - entry := volumeView(vol) + entry := serverVolumeView(vol) var updated *resource.Resource err := p.env.Store.Update(Name, kindServer, id, func(stored *resource.Resource) error { volumes := make(map[string]any, len(volumeMapOf(stored))+1) @@ -1669,7 +1688,19 @@ func (p *Pack) detachServerVolume(w http.ResponseWriter, r *http.Request) { writeNotFound(w, "volume", req.VolumeID) return } - if vol, ok := p.env.Store.Get(Name, kindVolume, req.VolumeID); ok { + // Both products, and this is the operation of the family that did not answer + // 404 — it answered 200 and released nothing, which is worse. + // + // `scw instance server terminate` walks GetVolume (instance, 404) → + // GetVolume (block, 200) → detach-volume → and then polls the block volume + // until its status leaves `in_use`. With the resolution below reading + // kindVolume alone, the detach came back 200 while the disk kept its server + // in Runtime, so the status never moved and the CLI never returned: rc=124 + // at twenty-five seconds, five identical block GETs in its own -D trace, + // measured on 2026-08-28 against a binary built from 3b00d23. + // + // TestTerminateReleasesABlockRootVolume fails without this. + if vol, ok := p.anyVolume(req.VolumeID); ok && vol.Tenant.Zone == zone { p.detachStoredVolume(vol) } // The server's map shrinks inside the store lock, on a fresh copy — the diff --git a/internal/providers/scaleway/snapshots.go b/internal/providers/scaleway/snapshots.go index edf2fb05..801fcada 100644 --- a/internal/providers/scaleway/snapshots.go +++ b/internal/providers/scaleway/snapshots.go @@ -83,7 +83,14 @@ func (p *Pack) createSnapshot(w http.ResponseWriter, r *http.Request) { size := uint64(rootVolumeSize) base := map[string]any{"id": "", "name": ""} if req.VolumeID != nil && *req.VolumeID != "" { - volume, found := p.env.Store.Get(Name, kindVolume, *req.VolumeID) + // Both products. A server's root disk lives in block as soon as the + // client asks for sbs_volume, and this resolved kindVolume alone — so + // `scw instance snapshot create volume-id=` answered 404 on + // the disk the same emulator had just published in the server's own + // volumes map (#571). The conformance suite's golden-image path takes + // exactly that route: it snapshots volumes["0"] and cuts an image from + // the snapshot. + volume, found := p.anyVolume(*req.VolumeID) if !found { writeNotFound(w, "volume", *req.VolumeID) return @@ -95,6 +102,26 @@ func (p *Pack) createSnapshot(w http.ResponseWriter, r *http.Request) { if volumeType == "" { volumeType = textOf(volume.Attrs["volume_type"]) } + // A block volume carries no volume_type attribute — its product has one + // class, "sbs" — so the reading above leaves it empty and the default + // below would call the snapshot b_ssd, which is a different product. + // + // The value comes from the SDK's enum, not from the wire: instance/v1 + // VolumeVolumeType declares sbs_snapshot beside sbs_volume, and + // Snapshot.VolumeType is a VolumeVolumeType, while CreateSnapshotRequest + // .VolumeType (SnapshotVolumeType) cannot even spell it — its four + // values are unknown_volume_type, l_ssd, b_ssd and unified, and its + // documentation says "if omitted, the volume type of the original volume + // will be used". So the request cannot ask for this and the answer has + // to derive it. No recorded account here holds one: this is a reading, + // declared as such, like the block snapshot shape above it. + // + // Only when the client named none: the request field "overrides the + // volume_type of the snapshot", which is the SDK's own wording, so a + // client that asked for one keeps it. + if volumeType == "" && volume.Kind == kindBlockVolume { + volumeType = "sbs_snapshot" + } // Through the shared reader: the assertion this replaces answered // ok=false on a volume that had crossed a snapshot, so a snapshot taken // after a `feint snapshot load` recorded a size of zero (#542). diff --git a/internal/providers/scaleway/volumes.go b/internal/providers/scaleway/volumes.go index 5354885f..036bf93f 100644 --- a/internal/providers/scaleway/volumes.go +++ b/internal/providers/scaleway/volumes.go @@ -272,6 +272,18 @@ func (p *Pack) attachStoredVolume(vol *resource.Resource, server *resource.Resou } stored.Runtime[runtimeServerKey] = server.ID stored.Attrs["server_name"] = serverName + // The mirror of what detachStoredVolume does on the way out, and the + // same asymmetry: block/v1's `status` IS res.State, so a volume a server + // holds reads `in_use` there while an instance volume has no such state. + // Without it, `scw block volume create` then `scw instance server + // attach-volume` left a disk whose references say "attached" and whose + // status says "available" — and `scw` polls the status, not the + // references. + // + // TestAttachingABlockVolumeMarksItInUse fails without this. + if stored.Kind == kindBlockVolume { + stored.State = blockVolumeInUse + } stored.Updated = p.env.Now() return nil }) @@ -283,6 +295,9 @@ func (p *Pack) attachStoredVolume(vol *resource.Resource, server *resource.Resou } vol.Runtime[runtimeServerKey] = server.ID vol.Attrs["server_name"] = serverName + if vol.Kind == kindBlockVolume { + vol.State = blockVolumeInUse + } return nil } @@ -340,6 +355,22 @@ func (p *Pack) volumesOf(serverID string) []*resource.Resource { return out } +// volumeOf resolves the {id} of an instance/v1 volume route, and resolves +// kindVolume ALONE on purpose. +// +// This is the one place where answering about a block volume would be a defect +// rather than a fix. The SDK's own dual-product reader +// (api/instance/v1/volume_utils.go, getUnknownVolume) calls instance GetVolume +// first and falls back to block.GetVolume only on a typed ResourceNotFoundError, +// so an instance route that answered here for a block volume would end the +// search before it reached the product that owns the disk. `scw instance server +// terminate` walks exactly that pair, and the trace of a block-root server shows +// it: GET /instance/v1/…/volumes/{id} → 404, then +// GET /block/v1alpha1/…/volumes/{id} → 200. +// +// TestAnSbsRootVolumeIsReadableThroughTheBlockFallback fails if this ever +// resolves both kinds — which is why anyVolume below is a separate function and +// not a change to this one. func (p *Pack) volumeOf(w http.ResponseWriter, r *http.Request) (*resource.Resource, bool) { zone, ok := zoneOf(w, r) if !ok { @@ -354,6 +385,48 @@ func (p *Pack) volumeOf(w http.ResponseWriter, r *http.Request) (*resource.Resou return res, true } +// anyVolume resolves a volume id in BOTH products, which is what every operation +// on the SERVER-volume relationship has to do. +// +// A server's disks can live in two stores since #8 served sbs_volume, and every +// operation that takes a server and a volume id resolved kindVolume alone. So a +// disk created by `root-volume=sbs:20GB` — or by `scw block volume create` — +// could not be attached, detached, put in an update's volume map, named by a +// create, or snapshotted. Measured with scw 2.56.3 against the binary built from +// 3b00d23, and the worst of the six was not a 404: `detach-volume` answered 200 +// and released nothing, so `scw instance server terminate` polled +// GET /block/v1alpha1/…/volumes/{id} for `in_use` to clear and never returned. +// +// It is deliberately NOT used by the instance/v1 volume routes themselves: see +// volumeOf, whose 404 is the fallback the SDK depends on. +// +// The zone is the caller's business, because the callers disagree and both are +// right: the server operations check it (a volume of another zone is not +// attachable), CreateSnapshot never did and gains no refusal here. +func (p *Pack) anyVolume(id string) (*resource.Resource, bool) { + for _, kind := range []string{kindVolume, kindBlockVolume} { + if res, found := p.env.Store.Get(Name, kind, id); found { + return res, true + } + } + return nil, false +} + +// serverVolumeView renders a volume the way instance/v1 lists it inside a +// server's `volumes` map, whichever product owns it. +// +// One dispatch rather than a branch per caller: the map is built in four places +// (a create's root, a create's additional volumes, an update, attach-volume) and +// three of them rendered the instance view unconditionally. On a block volume +// that view publishes no volume_type at all, and volume_type is the field the +// Terraform provider branches on to fall back to block/v1. +func serverVolumeView(res *resource.Resource) map[string]any { + if res.Kind == kindBlockVolume { + return blockVolumeServerView(res) + } + return volumeView(res) +} + // volumeView is the wire shape, shared by the volume endpoints and by the // volume map a server carries: the two must not drift apart, because a client // reads the same volume through both. diff --git a/tools/falsify/specs/block-volumes-reach-their-server.json b/tools/falsify/specs/block-volumes-reach-their-server.json new file mode 100644 index 00000000..63cb06fd --- /dev/null +++ b/tools/falsify/specs/block-volumes-reach-their-server.json @@ -0,0 +1,103 @@ +{ + "subject": "a disk in the block product is reachable through the operations that take a server and a volume id, and stays unreachable through the ones a client uses to fall back (#571)", + "why": "Since #8 served sbs_volume, one server's disks can live in two stores, and every operation on the SERVER-volume relationship resolved instance/v1 alone: attach-volume, detach-volume, the update's volume map, a create naming a volume, and CreateSnapshot. Measured with scw 2.56.3 against a binary built from 3b00d23 on 2026-08-28. Four answered 404 on a disk the same emulator had just published; the fifth, detach-volume, answered 200 and released nothing, so `scw instance server terminate with-block=true` polled GET /block/v1alpha1/.../volumes/{id} for a status that could never move and returned rc=124 at twenty-five seconds. The mutations below split into two families, and the second is the one this spec exists for: instance/v1 GetVolume must KEEP answering 404 for a block volume, because the SDK's own dual-product reader (api/instance/v1/volume_utils.go, getUnknownVolume) falls back to block only on a typed ResourceNotFoundError. A fix that made every route resolve both kinds would have ended the search before it reached the product that owns the disk.", + "package": "./internal/providers/scaleway/", + "mutations": [ + { + "label": "the shared resolver looks in one product again", + "file": "internal/providers/scaleway/volumes.go", + "find": "\tfor _, kind := range []string{kindVolume, kindBlockVolume} {\n\t\tif res, found := p.env.Store.Get(Name, kind, id); found {\n\t\t\treturn res, true\n\t\t}\n\t}", + "replace": "\tfor _, kind := range []string{kindVolume, kindBlockVolume}[:1] {\n\t\tif res, found := p.env.Store.Get(Name, kind, id); found {\n\t\t\treturn res, true\n\t\t}\n\t}", + "expect": "every operation on the server-volume relationship goes back to instance/v1 alone, which is the state #571 measured", + "test": "TestABlockVolumeAttachesAndDetachesThroughTheServerRoutes" + }, + { + "label": "detach-volume stops releasing a block disk and keeps answering 200", + "file": "internal/providers/scaleway/servers.go", + "find": "\tif vol, ok := p.anyVolume(req.VolumeID); ok && vol.Tenant.Zone == zone {\n\t\tp.detachStoredVolume(vol)\n\t}", + "replace": "\tif vol, ok := p.anyVolume(req.VolumeID); ok && vol.Tenant.Zone == zone && vol.Kind == kindVolume {\n\t\tp.detachStoredVolume(vol)\n\t}", + "expect": "the block root keeps its server in Runtime while the server's map loses it, which is the 200 that makes `scw instance server terminate` poll for ever", + "test": "TestTerminateReleasesABlockRootVolume" + }, + { + "label": "attach-volume refuses a block disk again", + "file": "internal/providers/scaleway/servers.go", + "find": "\tvol, ok := p.anyVolume(req.VolumeID)\n\tif !ok || vol.Tenant.Zone != zone {\n\t\twriteNotFound(w, \"volume\", req.VolumeID)\n\t\treturn\n\t}", + "replace": "\tvol, ok := p.anyVolume(req.VolumeID)\n\tif !ok || vol.Tenant.Zone != zone || vol.Kind == kindBlockVolume {\n\t\twriteNotFound(w, \"volume\", req.VolumeID)\n\t\treturn\n\t}", + "expect": "`scw instance server attach-volume volume-type=sbs_volume` answers \"cannot find resource 'volume'\" on a volume `scw block volume create` has just made", + "test": "TestABlockVolumeAttachesAndDetachesThroughTheServerRoutes" + }, + { + "label": "the update's volume map cannot name a block disk", + "file": "internal/providers/scaleway/servers.go", + "find": "\t\t\tvol, found := p.anyVolume(tmpl.ID)\n\t\t\tif !found || vol.Tenant.Zone != server.Tenant.Zone {", + "replace": "\t\t\tvol, found := p.anyVolume(tmpl.ID)\n\t\t\tif !found || vol.Tenant.Zone != server.Tenant.Zone || vol.Kind == kindBlockVolume {", + "expect": "a Terraform plan writing volumes.1.id over a block disk is refused with \"volume … does not exist in fr-par-1\" about a disk the same emulator holds", + "test": "TestAnUpdatesVolumeMapReachesABlockVolume" + }, + { + "label": "a create naming a block disk skips it silently", + "file": "internal/providers/scaleway/servers.go", + "find": "\t\tvol, ok := p.anyVolume(tpl.ID)\n\t\tif !ok || vol.Tenant.Zone != zone {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tvol, ok := p.anyVolume(tpl.ID)\n\t\tif !ok || vol.Tenant.Zone != zone || vol.Kind == kindBlockVolume {\n\t\t\tcontinue\n\t\t}", + "expect": "additional_volume_ids naming a block disk answers 201 with the volume left detached and nothing saying so", + "test": "TestACreateNamingABlockVolumeAttachesIt" + }, + { + "label": "an instance snapshot cannot be taken of a block disk", + "file": "internal/providers/scaleway/snapshots.go", + "find": "\t\tvolume, found := p.anyVolume(*req.VolumeID)\n\t\tif !found {", + "replace": "\t\tvolume, found := p.anyVolume(*req.VolumeID)\n\t\tif !found || volume.Kind == kindBlockVolume {", + "expect": "the golden-image path answers 404 on the disk the server publishes under volumes[\"0\"]", + "test": "TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot" + }, + { + "label": "a snapshot of a block disk is typed as the other product's", + "file": "internal/providers/scaleway/snapshots.go", + "find": "\t\tif volumeType == \"\" && volume.Kind == kindBlockVolume {\n\t\t\tvolumeType = \"sbs_snapshot\"\n\t\t}", + "replace": "\t\tif volumeType == \"\" && volume.Kind == kindBlockVolume && false {\n\t\t\tvolumeType = \"sbs_snapshot\"\n\t\t}", + "expect": "the snapshot falls through to the b_ssd default, naming a product it was not taken from", + "test": "TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot" + }, + { + "label": "a block disk is published inside a server with no type at all", + "file": "internal/providers/scaleway/volumes.go", + "find": "\tif res.Kind == kindBlockVolume {\n\t\treturn blockVolumeServerView(res)\n\t}\n\treturn volumeView(res)", + "replace": "\tif res.Kind == kindBlockVolume && false {\n\t\treturn blockVolumeServerView(res)\n\t}\n\treturn volumeView(res)", + "expect": "the server's volumes map carries no volume_type for a block disk, and volume_type is the field the Terraform provider branches on to fall back to block/v1", + "test": "TestABlockVolumeAttachesAndDetachesThroughTheServerRoutes" + }, + { + "label": "a block disk a server holds keeps saying it is available", + "file": "internal/providers/scaleway/volumes.go", + "find": "\t\tif stored.Kind == kindBlockVolume {\n\t\t\tstored.State = blockVolumeInUse\n\t\t}", + "replace": "\t\tif stored.Kind == kindBlockVolume && false {\n\t\t\tstored.State = blockVolumeInUse\n\t\t}", + "expect": "an attached block volume answers references [attached] and status available at once, and `scw` polls the status", + "test": "TestAttachingABlockVolumeMarksItInUse" + }, + { + "label": "the ownership question is asked and its answer thrown away, over both products", + "file": "internal/providers/scaleway/volumes.go", + "find": "\treleasable := \"\"\n\tif owner := vol.Runtime[runtimeServerKey]; owner != \"\" && owner != server.ID {\n\t\tif _, alive := p.env.Store.Get(Name, kindServer, owner); alive {\n\t\t\treturn fmt.Errorf(\"volume %s is attached to server %s\", vol.ID, owner)\n\t\t}\n\t\treleasable = owner\n\t}", + "replace": "\treleasable := vol.Runtime[runtimeServerKey]\n\tif owner := vol.Runtime[runtimeServerKey]; owner != \"\" && owner != server.ID && false {\n\t\tif _, alive := p.env.Store.Get(Name, kindServer, owner); alive {\n\t\t\treturn fmt.Errorf(\"volume %s is attached to server %s\", vol.ID, owner)\n\t\t}\n\t\treleasable = owner\n\t}", + "expect": "one server takes another's root disk through all three doors, in both products — the block half of this table is what the shared resolver made assertable at all", + "test": "TestAttachingDoesNotStealAnotherServersVolume" + }, + { + "label": "the orphan sweep stops declaring that a block disk belongs to a server", + "file": "internal/providers/scaleway/privatenics.go", + "find": "\tcase kindPrivateNIC, kindVolume, kindBlockVolume:", + "replace": "\tcase kindPrivateNIC, kindVolume, kindBlockVolume[:0]:", + "expect": "storetest.Orphans cannot report a block disk that names a machine that is gone, whatever it holds — a kind absent from this switch is a kind the sweep never looks at", + "test": "TestTheSweepSeesABlockVolumeThatNamesADeadServer" + }, + { + "label": "instance/v1 GetVolume answers for a block disk, and the SDK's fallback never fires", + "file": "internal/providers/scaleway/volumes.go", + "find": "\tid := r.PathValue(\"id\")\n\tres, found := p.env.Store.Get(Name, kindVolume, id)\n\tif !found || res.Tenant.Zone != zone {", + "replace": "\tid := r.PathValue(\"id\")\n\tres, found := p.anyVolume(id)\n\tif !found {\n\t\tres, found = p.env.Store.Get(Name, kindVolume, id)\n\t}\n\tif !found || res.Tenant.Zone != zone {", + "expect": "the over-correction: getUnknownVolume stops at the instance answer and never reads block, so the Terraform provider gets a field set without size, references or status — the failure #8 exists to prevent, reintroduced by a fix that looked symmetrical", + "test": "TestAnSbsRootVolumeIsReadableThroughTheBlockFallback" + } + ] +} diff --git a/tools/falsify/specs/scaleway-cloud-fidelity.json b/tools/falsify/specs/scaleway-cloud-fidelity.json index 75b12467..43cac335 100644 --- a/tools/falsify/specs/scaleway-cloud-fidelity.json +++ b/tools/falsify/specs/scaleway-cloud-fidelity.json @@ -53,8 +53,8 @@ { "label": "volumesOf walks instance/v1 alone again, so a gone server never releases the block volume it held (#365)", "file": "internal/providers/scaleway/volumes.go", - "find": "\tfor _, kind := range []string{kindVolume, kindBlockVolume} {", - "replace": "\tfor _, kind := range []string{kindVolume, kindBlockVolume}[:1] {", + "find": "\tfor _, kind := range []string{kindVolume, kindBlockVolume} {\n\t\tfor _, res := range p.env.Store.List(kind, resource.Tenant{Provider: Name}) {", + "replace": "\tfor _, kind := range []string{kindVolume, kindBlockVolume}[:1] {\n\t\tfor _, res := range p.env.Store.List(kind, resource.Tenant{Provider: Name}) {", "test": "TestABlockVolumeIsAvailableOnceItsServerIsGone" }, { From 4f9ef56a01fee57bd08ab4ee562f7b2ed7a4e88e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 22:22:39 +0200 Subject: [PATCH 2/4] feat(scaleway): a DEV1-S root disk lives in block, like the cloud's, and eighteen acceptance entries go with it (#365) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default flip #365 asked for. `scw` follows the server's own `volumes["0"].id` into `block/v1alpha1` and is answered there now, where this emulator used to 404 on a path every `scw instance server delete` takes. scw block volume get before: rc=1, "cannot find resource 'volume' with ID …" after: rc=0, the volume, in_use, one reference naming its server Measured on a real fr-par-1 account and recorded in corpus/scaleway/scw-instance.jsonl: `CreateServer` with no `volumes` in the body is answered with `volumes: {"0": {"volume_type": "sbs_volume"}}`, read back three times through block and deleted there. It is one line because #571 landed first. The reason not to flip was real and was measured — five operations resolved `kindVolume` alone — but it was a defect that already existed for anybody writing `root_volume { volume_type = "sbs_volume" }`, not a price this change would have paid. Reading a defect as a cost is what kept this open; docs/limits.md now says so where the old limit was. **Eighteen acceptance entries in corpus/accepted.json are deleted**, which the staleness rule makes compulsory rather than optional: the gate reported each of them as excusing nothing, and they all said the same thing — "a server's root volume lives in instance/v1 here and in block upstream". They were removed from the gate's own output rather than by hand. `feint corpus --check` now compares those exchanges for real: 0 divergent findings nothing accepts. Comparing them for the first time found two fields, and both are served rather than accepted, because a recording is the strongest source this repository has: * **`parent_snapshot_id`** — the cloud's root disk names the image snapshot it was restored from (scw-instance.jsonl seq 9, scw-billed-shapes.jsonl seq 13), and this answered null. Not invented: `root_volume` on an instance/v1 Image IS that snapshot — `createImage` reads the client's snapshot id out of that very field — so the identifier was already published and the disk now points back at it. * **`last_detached_at`** — null on every read while the volume is held, a timestamp on the read that follows the detach (seq 9/14 then 18; seq 2/13/27 then 33). Written where the state already moves to `available`, which is the one place a volume stops being held. The four unit tests that encoded a root disk in `instance/v1` state the same facts one product further on, and one of them now asserts the cloud's own sequence: delete the server (204), read the volume in block (200), delete it there (204). `tools/conformance/scaleway/scw-cli.sh` changes, and the reason is a client behaviour rather than an emulator one: `scw instance snapshot create volume-id=` calls `instance.GetVolume` itself before it sends anything (scaleway-cli 2.56.3, `internal/namespaces/instance/v1/ custom_snapshot.go`) and returns that error, so the golden-image path can no longer take the server's root as its subject. It takes an instance volume the client creates, which is what a client does, and the suite gained the step that proves the new reality: the server's root disk is snapshotted through `scw block snapshot create` and names the disk it came from. The instance route does resolve a block volume — `unified=true` reaches it and answers an `sbs_snapshot` — and that is asserted by unit test and by the raw route, since no client walks it. Whether the real cloud answers `instance.GetVolume` for an SBS volume is **not measured**: no recording carries that call. docs/limits.md says so rather than guessing. `volumes_constraint.min_size` is untouched and the trap stays disarmed, checked rather than assumed: `scw instance server create type=DEV1-S` is the first thing the conformance suite does and it passes, and a block root sums to nothing local. Gates: `mise run prepush` green, `mise run conformance:leg -- scw-cli` and `-- fields` green, `feint corpus --check` green with 174 acceptance entries where there were 192, and the sixteen mutations of tools/falsify/specs/block-volumes-reach-their-server.json all bite. Closes #365. Assisted-by: Claude Code (claude-opus-5) --- corpus/accepted.json | 144 ------------------ coverage/scaleway-coverage.json | 2 +- docs/limits.md | 62 +++++--- docs/routes.md | 2 +- internal/providers/scaleway/block.go | 21 ++- .../providers/scaleway/block_attach_test.go | 109 +++++++++++++ internal/providers/scaleway/images.go | 30 +++- internal/providers/scaleway/lifecycle_test.go | 13 +- internal/providers/scaleway/pack.go | 8 +- internal/providers/scaleway/servers.go | 49 +++--- internal/providers/scaleway/volumes.go | 12 ++ internal/providers/scaleway/volumes_test.go | 66 +++++--- tools/conformance/scaleway/scw-cli.sh | 38 ++++- .../block-volumes-reach-their-server.json | 32 ++++ .../specs/scaleway-cloud-fidelity.json | 4 +- 15 files changed, 365 insertions(+), 227 deletions(-) diff --git a/corpus/accepted.json b/corpus/accepted.json index 94af900c..022fd89a 100644 --- a/corpus/accepted.json +++ b/corpus/accepted.json @@ -621,14 +621,6 @@ "reason": "THIS EMULATOR'S LIFECYCLE TRANSITIONS ARE IMMEDIATE, which docs/limits.md states as a decision, and this is that decision seen from block/v1alpha1. A snapshot is `available` the instant it is cut here; upstream it is born in a transient state and a DeleteSnapshot issued while it settles is refused with 412 and a body. corpus/scaleway/scw-billed-shapes.jsonl seq 9-12 is exactly that sequence: the refused delete, a read that still finds the snapshot, the accepted delete, a read that does not. Here the FIRST delete succeeds, so the read between them finds nothing (404 against 200, which reports all twelve fields of the snapshot absent) and the second delete meets nothing (404 against 204). A refusal whose state is not reachable here cannot be served: a guard for a state nothing can enter is a control that can never fire, which is the rule docs/limits.md already draws for Outscale's InvalidVolumeState. It goes the day a snapshot has a settling state. Recorded 2026-08-24 (#427).", "issue": "https://github.com/stephrobert/feint/issues/433" }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.DeleteVolume", - "kind": "status", - "path": "", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, { "file": "scaleway/scw-billed-shapes.jsonl", "operation": "block/v1alpha1/API.GetSnapshot", @@ -733,118 +725,6 @@ "reason": "THIS EMULATOR'S LIFECYCLE TRANSITIONS ARE IMMEDIATE, which docs/limits.md states as a decision, and this is that decision seen from block/v1alpha1. A snapshot is `available` the instant it is cut here; upstream it is born in a transient state and a DeleteSnapshot issued while it settles is refused with 412 and a body. corpus/scaleway/scw-billed-shapes.jsonl seq 9-12 is exactly that sequence: the refused delete, a read that still finds the snapshot, the accepted delete, a read that does not. Here the FIRST delete succeeds, so the read between them finds nothing (404 against 200, which reports all twelve fields of the snapshot absent) and the second delete meets nothing (404 against 204). A refusal whose state is not reachable here cannot be served: a guard for a state nothing can enter is a control that can never fire, which is the rule docs/limits.md already draws for Outscale's InvalidVolumeState. It goes the day a snapshot has a settling state. Recorded 2026-08-24 (#427).", "issue": "https://github.com/stephrobert/feint/issues/433" }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "created_at", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "id", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "last_detached_at", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "name", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "parent_snapshot_id", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "project_id", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "references", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "size", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "specs", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "status", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "tags", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "updated_at", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "zone", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, - { - "file": "scaleway/scw-billed-shapes.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "status", - "path": "", - "reason": "A SERVER'S ROOT VOLUME LIVES IN instance/v1 HERE AND IN block UPSTREAM, and every finding on this operation is that one default. corpus/scaleway/scw-billed-shapes.jsonl seq 11 is a CreateServer whose body names no volume, and fr-par answered volumes {\"0\": {volume_type: sbs_volume}} -- a block volume -- then read it three times through block/v1alpha1 (seq 13, 27, 33) and deleted it there (seq 34). This emulator gives such a server a b_ssd volume in instance/v1, so all four calls answer 404, and a 404 reports every field of the expected body as absent. sbs_volume IS honoured when a client asks for it, and tools/conformance/scaleway/terraform/main.tf asks for it, so the path is proven end to end by the real provider; what is not done is the default. MEASURED WHY NOT: flipping it reds ten tests at once, because the whole instance/v1 volume surface reads a server's root disk out of the instance store -- CreateSnapshot and CreateImage cannot find the volume to snapshot, attach-volume and detach-volume refuse it, and terminate stops carrying it away. docs/limits.md carries it. It goes the day the default moves, which is a batch of its own and not a line in a handler. Recorded 2026-08-24 (#427).", - "issue": "https://github.com/stephrobert/feint/issues/433" - }, { "file": "scaleway/scw-billed-shapes.jsonl", "operation": "lb/v1/ZonedAPI.GetLB", @@ -1309,30 +1189,6 @@ "reason": "THE RECORDING PREDATES THE RENAME, AND ONLY AN ACCOUNT CAN RE-RECORD IT. Scaleway renamed the vpc/v2 Object Storage family on 2026-08-25: the VPC's s3_integration_enabled became object_storage_private_access_enabled and the Private Network's has_s3_integration became has_object_storage_private_access. Both upstream sources agree and neither declares a deprecation alias, read on 2026-08-28: .upstream/scaleway-sdk-go/api/vpc/v2/vpc_sdk.go:1024 and :646, and .upstream/scaleway-openapi/vpc-v2.yml, whose x-properties-order -- the document's own exhaustive property list for each object -- carries the new name and not the old one. So the emulator answers the new name alone; answering both would invent a deprecation window no source declares. This corpus was recorded on 2026-08-20, five days before, so it is evidence of what the cloud answered BEFORE the rename and it still settles the two things a rename does not touch: the field is present on every answer, and its value is false on an account with nothing attached. Re-recording needs a paid vpc/v2 account this repository does not have, and `mise run corpus:cloud` is what would arbitrate it. It goes the day that recording is made. (#570)", "issue": "https://github.com/stephrobert/feint/issues/570" }, - { - "file": "scaleway/scw-instance.jsonl", - "operation": "block/v1alpha1/API.DeleteVolume", - "kind": "status", - "path": "", - "reason": "scw deletes the root volume through block/v1alpha1 after deleting the server, and the cloud answers 204 where this emulator answers 404 because the volume it created is an instance/v1 one", - "issue": "#365" - }, - { - "file": "scaleway/scw-instance.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "absent", - "path": "*", - "reason": "every top-level field of a body this emulator does not serve at all: the block volume behind the root disk does not exist here, so its fourteen fields are absent for the one reason the status divergence above states", - "issue": "#365" - }, - { - "file": "scaleway/scw-instance.jsonl", - "operation": "block/v1alpha1/API.GetVolume", - "kind": "status", - "path": "", - "reason": "the cloud gave the DEV1-S an SBS root volume and answers 200 on the block path scw follows; this emulator attaches a local instance/v1 volume, so the same read answers 404", - "issue": "#365" - }, { "file": "scaleway/scw-instance.jsonl", "operation": "instance/v1/API.ListServers", diff --git a/coverage/scaleway-coverage.json b/coverage/scaleway-coverage.json index 7f3b8260..4cf70221 100644 --- a/coverage/scaleway-coverage.json +++ b/coverage/scaleway-coverage.json @@ -1414,7 +1414,7 @@ { "operation": "instance/v1/API.ListVolumesTypes", "product": "instance", - "reason": "the emulator serves one volume type, b_ssd, because that is what its catalogue attaches, so a type list would describe capabilities nothing here can create", + "reason": "the instance volumes this emulator makes are b_ssd, its servers' root disks are sbs_volume in the block product, and neither is backed by storage: a type list would describe capabilities and constraints nothing here can honour", "version": "v1", "status": "declined" }, diff --git a/docs/limits.md b/docs/limits.md index a55776bb..b1f7232c 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -803,9 +803,10 @@ Measured by @vde-dis on #8, with OpenTofu 1.12.5 and `scaleway/scaleway` 2.80.0: migrate to sbs or downgrade terraform."* - **`sbs_volume` used to plan for ever**, because the emulator overrode the type to `b_ssd` and the value read back never matched the value sent. It is now - honoured: the disk is created in `block/v1`, and the provider reads it back - through the fallback it always used — `instance.GetVolume` first, then - `block.GetVolume` on a typed 404. + honoured, and since #365 it is also what a request naming no type gets: the + disk is created in `block/v1`, and the provider reads it back through the + fallback it always used — `instance.GetVolume` first, then `block.GetVolume` + on a typed 404. - **The local types (`l_ssd`, `scratch`) are still overridden**, and that has its own reason, unchanged: the emulated catalogue declares `volumes_constraint.min_size` at 0 and the CLI sums local volumes against it, @@ -897,27 +898,40 @@ the pack: this emulator was never asked to delete the gateway, and answering to avoid. They go when the gateway is recorded again with its destruction in the transcript. -### A server's root volume lives in `instance/v1` here and in `block` upstream - -The largest single divergence the 2026-08-24 recording found, and it is one -default. `CreateServer` with no `volumes` in the body is answered by `fr-par` -with `volumes: {"0": {"volume_type": "sbs_volume"}}` — a *block* volume — and -the recording then reads that volume three times through -`block/v1alpha1/API.GetVolume` and deletes it there. This emulator gives such a -server a `b_ssd` volume in `instance/v1`, so all four of those calls answer -`404`: **forty-three findings, one default.** - -`sbs_volume` is honoured when a client asks for it, and -`tools/conformance/scaleway/terraform/main.tf` asks for it, so the path itself -is proven end to end by the real provider. What is not done is making it the -default, and the reason is measured rather than assumed: the whole -`instance/v1` volume surface reads a server's root disk out of the instance -store. Flipping the default reds ten tests at once — `CreateSnapshot` and -`CreateImage` cannot find the volume to snapshot, `attach-volume` and -`detach-volume` refuse it, and terminate stops carrying it away. That is a -batch of its own, not a line in a handler, and it is the same shape of decision -as the asynchronous-delete entry above: a lifecycle that belongs to every kind, -changed in one place. +### A server's root volume lives in `block`, like the cloud's — since #365 + +This entry used to be the largest single divergence the 2026-08-24 recording +found, and it is over. `CreateServer` with no `volumes` in the body is answered +by `fr-par` with `volumes: {"0": {"volume_type": "sbs_volume"}}` — a *block* +volume — and the recording then reads that volume three times through +`block/v1alpha1/API.GetVolume` and deletes it there. This emulator gave such a +server a `b_ssd` volume in `instance/v1`, so all four of those calls answered +`404`: forty-three findings, one default. The eighteen acceptance entries that +carried them are deleted, and `feint corpus --check` compares those exchanges +for real now. + +It is worth keeping why it took two steps, because the first was mistaken for a +price rather than a defect. The reason not to flip was measured: the whole +`instance/v1` volume surface read a server's root disk out of the instance +store, so a block root was invisible to `attach-volume`, `detach-volume`, the +update's volume map, a create naming a volume and `CreateSnapshot`. But that was +already true for anybody who wrote `root_volume { volume_type = "sbs_volume" }`, +which has worked since SW-3 — the defect existed, unmeasured, and the flip only +made it universal. **The cost of a change and a defect it exposes are not the +same thing**, and reading one as the other is what left this open for a month. +#571 fixed the five resolutions first; this became one line. + +What the flip left standing, and it is a client behaviour rather than an +emulator one: **`scw instance snapshot create volume-id=` +no longer works without `unified=true`**. The CLI calls `instance.GetVolume` +itself before it sends anything (scaleway-cli 2.56.3, +`internal/namespaces/instance/v1/custom_snapshot.go`) and returns that error, so +the command stops one call before the emulator. The instance route does resolve +a block volume — `unified=true` reaches it and answers an `sbs_snapshot` — and +`scw block snapshot create volume-id=` is the path the conformance suite +walks. Whether the real cloud answers `instance.GetVolume` for an SBS volume is +**not measured here**: no recording carries that call, and the SDK's own +`getUnknownVolume` only makes sense if it can 404. ## What survives a dead emulator, in one table diff --git a/docs/routes.md b/docs/routes.md index 05b747d4..0bee0157 100644 --- a/docs/routes.md +++ b/docs/routes.md @@ -409,7 +409,7 @@ are in `coverage/`, one artefact per provider. - `instance` — 1 operation — it writes into Object Storage, which is not emulated because the Terraform provider builds the S3 endpoint in code: supporting it needs DNS interception and a certificate, measured in docs/limits.md - `instance` — 1 operation — its request carries tags and nothing else, and the pack stores no tag on a private NIC, so it would answer success over a field nothing reads back - `instance` — 1 operation — its thirteen counters span resources this pack does not serve, so every total would be short by the unemulated remainder with nothing saying which -- `instance` — 1 operation — the emulator serves one volume type, b_ssd, because that is what its catalogue attaches, so a type list would describe capabilities nothing here can create +- `instance` — 1 operation — the instance volumes this emulator makes are b_ssd, its servers' root disks are sbs_volume in the block product, and neither is backed by storage: a type list would describe capabilities and constraints nothing here can honour - `instance` — 1 operation — the server already publishes allowed_actions, derived from its state, so a second listing would be a second place to keep in step with the first - `ipam` — 1 operation — ipam/v1alpha1 is the superseded draft of ipam/v1, which is served - `lb` — 53 operations — the regional lb/v1 API is deprecated upstream in favour of the zoned one, which is served: the portal publishes only the zoned document, and every measured client calls ZonedAPI diff --git a/internal/providers/scaleway/block.go b/internal/providers/scaleway/block.go index 184152cf..507f854a 100644 --- a/internal/providers/scaleway/block.go +++ b/internal/providers/scaleway/block.go @@ -228,11 +228,18 @@ func (p *Pack) blockVolumeView(res *resource.Resource) map[string]any { "class": blockStorageClass, "perf_iops": res.Attrs["perf_iops"], }, - // Present and null on every volume the recorded account returned. Neither - // is emulated: no Key Manager, and no detachment history. - "kms_key_id": nil, + // Present and null on every volume the recorded account returned. Not + // emulated: there is no Key Manager here. + "kms_key_id": nil, + // A timestamp once something has released this volume, null before — + // which is what both recordings show, null while attached and a string + // on the read that follows the detach. Written by detachStoredVolume, + // the one place a volume stops being held. "last_detached_at": nil, } + if detached := textOf(res.Attrs["last_detached_at"]); detached != "" { + view["last_detached_at"] = detached + } // A string when the volume came from a snapshot, null otherwise. The SDK // declares a pointer and the recorded account only had volumes with a parent, // so the null branch is the SDK's reading and the string branch is measured. @@ -833,7 +840,11 @@ const ( // block one — being in both would answer the first call and never exercise the // fallback, which is precisely the path #8 exists to unblock. // TestAnSbsRootVolumeIsReadableThroughTheBlockFallback fails without this. -func (p *Pack) newBlockRootVolume(zone, project, name string, size uint64) *resource.Resource { +// +// parentSnapshot is the image snapshot the disk was restored from, which is what +// the cloud publishes and what a client reads to know where its root came from +// (see imageRootSnapshot). +func (p *Pack) newBlockRootVolume(zone, project, name string, size uint64, parentSnapshot string) *resource.Resource { now := p.env.Now() return &resource.Resource{ ID: p.env.NewID(), @@ -848,7 +859,7 @@ func (p *Pack) newBlockRootVolume(zone, project, name string, size uint64) *reso "tags": []any{}, "size": size, "zone": zone, - "parent_snapshot_id": "", + "parent_snapshot_id": parentSnapshot, "perf_iops": uint32(blockDefaultIOPS), }, } diff --git a/internal/providers/scaleway/block_attach_test.go b/internal/providers/scaleway/block_attach_test.go index 2367a6f0..828fee14 100644 --- a/internal/providers/scaleway/block_attach_test.go +++ b/internal/providers/scaleway/block_attach_test.go @@ -298,3 +298,112 @@ func TestTheSweepSeesABlockVolumeThatNamesADeadServer(t *testing.T) { t.Errorf("the sweep did not report the block volume that names a dead server: %v", found) } } + +// A default DEV1-S gets its root disk in the block product, like the cloud. +// +// #365, and the whole of it. A DEV1-S created on a real fr-par-1 account was +// given an SBS root volume — corpus/scaleway/scw-instance.jsonl, recorded +// 2026-08-21 and confirmed 2026-08-24 — and `scw` then read it back through +// GET /block/v1alpha1/zones/fr-par-1/volumes/{id} (200) and deleted it there +// (204). This emulator answered 404 on both, on a path every `scw instance +// server delete` takes. +// +// The client asks for nothing here: no `volumes` map at all, which is what `scw +// instance server create type=DEV1-S image=ubuntu_jammy` actually sends (`scw +// -D`, 2.56.3). So this is the default and not an opt-in. +func TestADefaultRootVolumeLivesInBlockLikeTheCloud(t *testing.T) { + ts := newTestServer(t) + _, server := serverWith(t, ts, `{"name":"plain","commercial_type":"DEV1-S","image":"ubuntu_jammy"}`) + + volumes, _ := server["volumes"].(map[string]any) + root, _ := volumes["0"].(map[string]any) + if root == nil { + t.Fatalf("the server carries no root volume: %v", server["volumes"]) + } + if root["volume_type"] != "sbs_volume" { + t.Fatalf("a default root volume is %v, want sbs_volume: the cloud gives a DEV1-S an SBS root", root["volume_type"]) + } + id, _ := root["id"].(string) + + // The read #365 is titled after. Instance keeps its typed 404 so the SDK's + // fallback fires, and block answers. + if status, _ := do(t, ts, "GET", zone+"/volumes/"+id, ""); status != http.StatusNotFound { + t.Errorf("instance answered %d for the root volume, want 404 so getUnknownVolume falls back", status) + } + status, got := do(t, ts, "GET", blockURL+"/volumes/"+id, "") + if status != http.StatusOK { + t.Fatalf("block answered %d for the root volume, want 200: this is #365", status) + } + if got["id"] != id { + t.Errorf("block answered another volume: %v", got) + } +} + +// A root disk restored from an image names the snapshot it came from. +// +// The cloud does: corpus/scaleway/scw-instance.jsonl seq 9 answers a +// parent_snapshot_id on the root volume of a DEV1-S created from ubuntu_jammy, +// and this emulator answered null there. It became comparable only when #365 +// made the volume answerable at all — before that the whole object was missing +// and `corpus --check` excused it wholesale. +func TestARootVolumeNamesTheImageSnapshotItCameFrom(t *testing.T) { + ts := newTestServer(t) + _, server := serverWith(t, ts, `{"name":"plain","commercial_type":"DEV1-S","image":"ubuntu_jammy"}`) + volumes, _ := server["volumes"].(map[string]any) + root, _ := volumes["0"].(map[string]any) + id, _ := root["id"].(string) + + _, got := do(t, ts, "GET", blockURL+"/volumes/"+id, "") + parent, _ := got["parent_snapshot_id"].(string) + if parent == "" { + t.Fatalf("the root volume names no parent snapshot: %v", got["parent_snapshot_id"]) + } + // The identifier the emulator itself publishes for that image's root volume, + // read back through the image door: two views of one fact, and inventing a + // third id here would give a client two answers to the same question. + _, image := do(t, ts, "GET", zone+"/images/"+imageIDOf(t, server), "") + img, _ := image["image"].(map[string]any) + imageRoot, _ := img["root_volume"].(map[string]any) + if imageRoot == nil || imageRoot["id"] != parent { + t.Errorf("the root volume's parent is %q and the image's root volume is %v: the two must be the same snapshot", parent, image["image"]) + } +} + +// imageIDOf reads the image id off a server body. +func imageIDOf(t *testing.T, server map[string]any) string { + t.Helper() + image, _ := server["image"].(map[string]any) + id, _ := image["id"].(string) + if id == "" { + t.Fatalf("the server carries no image: %v", server["image"]) + } + return id +} + +// A released block volume says WHEN it was released. +// +// Both recordings carry null on every read while the volume is held and a +// timestamp on the read that follows the detach: scw-instance.jsonl seq 9/14 +// null then seq 18 a string, scw-billed-shapes.jsonl seq 2/13/27 null then seq +// 33 a string. `feint corpus --check` reported "last_detached_at is string +// upstream, null here" the moment #365 made this volume comparable. +func TestAReleasedBlockVolumeSaysWhenItWasDetached(t *testing.T) { + ts := newTestServer(t) + srv, body := serverWith(t, ts, `{"name":"plain","commercial_type":"DEV1-S"}`) + volumes, _ := body["volumes"].(map[string]any) + root, _ := volumes["0"].(map[string]any) + id, _ := root["id"].(string) + + _, got := do(t, ts, "GET", blockURL+"/volumes/"+id, "") + if got["last_detached_at"] != nil { + t.Fatalf("a volume that was never detached reports %v, want null", got["last_detached_at"]) + } + + if status, out := do(t, ts, "DELETE", zone+"/servers/"+srv, ""); status != http.StatusNoContent { + t.Fatalf("delete server answered %d: %v", status, out) + } + _, got = do(t, ts, "GET", blockURL+"/volumes/"+id, "") + if _, ok := got["last_detached_at"].(string); !ok { + t.Errorf("a released volume reports last_detached_at %v, want the moment it was released", got["last_detached_at"]) + } +} diff --git a/internal/providers/scaleway/images.go b/internal/providers/scaleway/images.go index 0905dbb0..207218a5 100644 --- a/internal/providers/scaleway/images.go +++ b/internal/providers/scaleway/images.go @@ -100,7 +100,7 @@ func (p *Pack) imageView(zone, id, label string) map[string]any { // TestTheCatalogueImageTypesFromServerLikeTheCloudDoes fails without this. "from_server": "", "root_volume": map[string]any{ - "id": "33333333-3333-4333-8333-333333333333", + "id": catalogueImageSnapshot, "name": label + "-root", "size": 20_000_000_000, "volume_type": "b_ssd", @@ -108,6 +108,34 @@ func (p *Pack) imageView(zone, id, label string) map[string]any { } } +// catalogueImageSnapshot is the snapshot every catalogue image's root volume +// names. Fixed, like the rest of the catalogue, and named here because a second +// reader appeared: a root disk restored from an image carries this identifier as +// its parent_snapshot_id, and a literal written twice is a literal that will one +// day be written differently. +const catalogueImageSnapshot = "33333333-3333-4333-8333-333333333333" + +// imageRootSnapshot is the snapshot an image's root volume names, which is what +// a disk created from that image carries as its parent. +// +// Not an invention: `root_volume` on an instance/v1 Image IS the snapshot the +// image was built from — createImage below reads the client's snapshot id out of +// that very field — so the emulator already publishes this identifier, and a +// root volume restored from the image points back at it. The cloud does the +// same, measured: corpus/scaleway/scw-instance.jsonl seq 9 answers a +// parent_snapshot_id on the root volume of a DEV1-S created from ubuntu_jammy, +// and scw-billed-shapes.jsonl seq 13 does on another. +// +// TestARootVolumeNamesTheImageSnapshotItCameFrom fails without this. +func (p *Pack) imageRootSnapshot(imageID string) string { + if res, found := p.env.Store.Get(Name, kindImage, imageID); found { + if root, _ := res.Attrs["root_volume"].(map[string]any); root != nil { + return textOf(root["id"]) + } + } + return catalogueImageSnapshot +} + // resolveImage maps what a create request put in `image` onto what the // emulator needs: the ID to publish, the name to display, and the catalogue // label the machine driver turns into a base image. diff --git a/internal/providers/scaleway/lifecycle_test.go b/internal/providers/scaleway/lifecycle_test.go index fba1dcf4..6df41247 100644 --- a/internal/providers/scaleway/lifecycle_test.go +++ b/internal/providers/scaleway/lifecycle_test.go @@ -151,13 +151,18 @@ func TestDeletingAServerLeavesItsVolumeAvailable(t *testing.T) { t.Fatalf("delete: status %d", status) } - status, out = do(t, ts, "GET", zone+"/volumes/"+rootID, "") + // In block, where the root disk lives since #365, and where `scw instance + // server delete` goes to read it: the recorded account answered 200 there + // after the server was gone, then 204 to the delete. + status, out = do(t, ts, "GET", blockURL+"/volumes/"+rootID, "") if status != http.StatusOK { t.Fatalf("the root volume went with the server: status %d", status) } - volume, _ := out["volume"].(map[string]any) - if server := volume["server"]; server != nil { - t.Fatalf("the volume still belongs to the deleted server: %v", server) + if refs, _ := out["references"].([]any); len(refs) != 0 { + t.Fatalf("the volume still belongs to the deleted server: %v", out["references"]) + } + if out["status"] != "available" { + t.Fatalf("the released volume reads status %v, want available: this is the field the CLI polls", out["status"]) } } diff --git a/internal/providers/scaleway/pack.go b/internal/providers/scaleway/pack.go index 814ca621..b53093d0 100644 --- a/internal/providers/scaleway/pack.go +++ b/internal/providers/scaleway/pack.go @@ -760,7 +760,13 @@ func (p *Pack) Declined() []emulator.Decline { // say so: it returns a catalogue of volume types with their constraints, // which is the same nature as ListServersTypes — served. It is declined // for the reason that actually applies to it. - emulator.Because("the emulator serves one volume type, b_ssd, because that is what its catalogue attaches, so a type list would describe capabilities nothing here can create", + // The reason it names has to stay true, and #365 moved the fact under + // it: a server's root disk is an sbs_volume now, like the cloud's, and + // b_ssd is what `scw instance volume create` makes when a client asks + // for a disk of its own. Two types, both of them the same single + // capability — a size, recorded and answered, with nothing written + // anywhere — so the decline stands and its sentence does not. + emulator.Because("the instance volumes this emulator makes are b_ssd, its servers' root disks are sbs_volume in the block product, and neither is backed by storage: a type list would describe capabilities and constraints nothing here can honour", "instance/v1/API.ListVolumesTypes"), emulator.Because("its thirteen counters span resources this pack does not serve, so every total would be short by the unemulated remainder with nothing saying which", diff --git a/internal/providers/scaleway/servers.go b/internal/providers/scaleway/servers.go index 8e41742e..94928fce 100644 --- a/internal/providers/scaleway/servers.go +++ b/internal/providers/scaleway/servers.go @@ -214,7 +214,7 @@ const rootVolumeSize = 20_000_000_000 // ever. Omitting the root_volume block is the way through, which is what the // conformance fixture happens to do — which is also why nothing here shows it. // docs/limits.md carries that as a stated limit rather than a surprise. -func (p *Pack) rootVolume(server *resource.Resource, name, project, organization string, wanted volumeTemplate) *resource.Resource { +func (p *Pack) rootVolume(server *resource.Resource, name, project, organization, parentSnapshot string, wanted volumeTemplate) *resource.Resource { // The size the client asked for, when it asked. Ignoring it gave every // server the catalogue's disk whatever the request said. size := uint64(rootVolumeSize) @@ -225,26 +225,33 @@ func (p *Pack) rootVolume(server *resource.Resource, name, project, organization if wanted.Name != "" { volumeName = wanted.Name } - // sbs_volume is honoured since SW-3, and it is the only asked-for type that - // is: the local ones stay overridden for the reason above, which is unchanged. - // The disk lands in block/v1 rather than instance/v1, which is where the - // Terraform provider goes to read it back. + // The default is block, which is what the cloud does (#365). // - // THE DEFAULT IS STILL INSTANCE/V1, AND #365 IS THE OPEN QUESTION ABOUT IT. - // The cloud gives a DEV1-S an SBS root volume — measured twice, 2026-08-21 - // and 2026-08-24 — so `scw` follows the server's own volumes["0"].id into - // block/v1alpha1 and this emulator answers 404 there. Flipping this one - // condition to `wanted.VolumeType == "" || …` makes that read answer, and - // was tried on 2026-08-27: it moves EVERY server's root disk out of - // instance/v1, where this pack implements the whole server-volume - // relationship. attach-volume, the update's volume map, CreateSnapshot, - // GetVolume and DeleteVolume all resolve kindVolume alone, so a client's - // own root disk stops being reachable through any of them — and the - // ownership guard that keeps one server from stealing another's root volume - // stops covering the root volume at all. That is a decision about where a - // root disk lives, not a line here, and it is the maintainer's to take. - if wanted.VolumeType == "sbs_volume" { - vol := p.newBlockRootVolume(server.Tenant.Zone, project, volumeName, size) + // A DEV1-S created on a real fr-par-1 account was given an SBS root volume, + // measured twice — 2026-08-21 and 2026-08-24, recorded in + // corpus/scaleway/scw-instance.jsonl — and `scw` then read it back through + // GET /block/v1alpha1/zones/fr-par-1/volumes/{id} and deleted it there. An + // instance root made that read a 404 on the path the cloud answers 200 on, + // on a command every client runs. + // + // This flip was tried on 2026-08-27 and reverted, because at the time it + // moved every root disk into a product where nothing could reach it: + // attach-volume, detach-volume, the update's volume map, a create naming a + // volume and CreateSnapshot all resolved kindVolume alone. That is fixed + // first and separately (#571, step 1): those five go through anyVolume, and + // the ownership guard covers both products. The flip is this line only + // because that work landed before it. + // + // The local types stay overridden for the reason above, which is unchanged: + // the CLI sums LOCAL volumes against volumes_constraint.min_size and would + // refuse the very creation it just asked for. A block root sums to nothing + // local, which is why this default is reachable at all — + // TestCatalogueKeepsTheLocalVolumeTrapDisarmed and the `scw instance server + // create` of the conformance suite are the two halves of that check. + // + // TestADefaultRootVolumeLivesInBlockLikeTheCloud fails without this. + if wanted.VolumeType == "" || wanted.VolumeType == "sbs_volume" { + vol := p.newBlockRootVolume(server.Tenant.Zone, project, volumeName, size, parentSnapshot) // A volume this call just built: it can belong to nobody else, so the // only error attachVolume returns cannot happen here. _ = p.attachVolume(vol, server, name) @@ -371,7 +378,7 @@ func (p *Pack) createServer(w http.ResponseWriter, r *http.Request) { resolvedImageID, imageDisplay, imageLabel := resolveImage(req.Image) res := resource.New(p.env.NewID(), kindServer, resource.Tenant{Provider: Name, Project: project, Zone: zone}, "stopped", now) - rootVol := p.rootVolume(res, req.Name, project, organization, req.Volumes["0"]) + rootVol := p.rootVolume(res, req.Name, project, organization, p.imageRootSnapshot(resolvedImageID), req.Volumes["0"]) res.Attrs = map[string]any{ "name": req.Name, diff --git a/internal/providers/scaleway/volumes.go b/internal/providers/scaleway/volumes.go index 036bf93f..9dac95ea 100644 --- a/internal/providers/scaleway/volumes.go +++ b/internal/providers/scaleway/volumes.go @@ -326,6 +326,18 @@ func (p *Pack) detachStoredVolume(vol *resource.Resource) { // TestABlockVolumeIsAvailableOnceItsServerIsGone fails without this. if stored.Kind == kindBlockVolume { stored.State = blockVolumeAvailable + // And it says WHEN, which is the other half of the same fact and + // the one nothing could compare while a default root disk lived in + // instance/v1. Both recordings of a real account carry a timestamp + // here on the read that follows the detach and null on every read + // before it: corpus/scaleway/scw-instance.jsonl seq 18 and + // scw-billed-shapes.jsonl seq 33, against null on seq 9/14 and + // 2/13/27. `feint corpus --check` reported "last_detached_at is + // string upstream, null here" the moment #365 made this volume + // comparable at all. + // + // TestAReleasedBlockVolumeSaysWhenItWasDetached fails without this. + stored.Attrs["last_detached_at"] = p.env.Now().Format(time.RFC3339) } stored.Updated = p.env.Now() return nil diff --git a/internal/providers/scaleway/volumes_test.go b/internal/providers/scaleway/volumes_test.go index 87cbbe32..af84a061 100644 --- a/internal/providers/scaleway/volumes_test.go +++ b/internal/providers/scaleway/volumes_test.go @@ -24,6 +24,10 @@ func serverWith(t *testing.T, ts *httptest.Server, body string) (string, map[str // A server always carries a root volume under key "0". The Terraform provider // reads it there and sizes the rest with len(volumes)-1, so an empty map is not // a missing field: it panics the plugin. +// +// Since #365 that disk lives in the BLOCK product, which is where the cloud puts +// it, so this test states the same fact one product further on: the key, the id, +// the type the client branches on, and the read that answers. func TestServerCarriesARootVolume(t *testing.T) { ts := newTestServer(t) @@ -41,27 +45,37 @@ func TestServerCarriesARootVolume(t *testing.T) { if volumeID == "" { t.Fatalf("the root volume has no id: %v", root) } - // A local volume would make the CLI refuse the creation it just asked for, - // because it sums local volumes against the catalogue constraint. - if root["volume_type"] != "b_ssd" { - t.Errorf("root volume type is %v, want b_ssd", root["volume_type"]) + // A LOCAL volume would make the CLI refuse the creation it just asked for, + // because it sums local volumes against the catalogue constraint. sbs_volume + // is not local, sums to nothing there, and is what a real DEV1-S is given. + if root["volume_type"] != "sbs_volume" { + t.Errorf("root volume type is %v, want sbs_volume", root["volume_type"]) } - // Readable through the volumes endpoint: the provider fetches it by id right - // after the create, and a 404 there fails the apply. - status, got := do(t, ts, "GET", zoneURL+"/volumes/"+volumeID, "") + // Readable where the client goes to read it: block, after a typed 404 on the + // instance side. Both halves matter — the provider fetches the volume by id + // right after the create, and it only tries block once instance has refused. + if status, got := do(t, ts, "GET", zoneURL+"/volumes/"+volumeID, ""); status != http.StatusNotFound { + t.Fatalf("instance answered %d for a block root, want 404 so the fallback happens (%v)", status, got) + } + status, got := do(t, ts, "GET", blockURL+"/volumes/"+volumeID, "") if status != http.StatusOK { - t.Fatalf("get volume: expected 200, got %d (%v)", status, got) + t.Fatalf("get block volume: expected 200, got %d (%v)", status, got) + } + if got["id"] != volumeID { + t.Errorf("block answered another volume: %v", got) } - vol, _ := got["volume"].(map[string]any) - attached, _ := vol["server"].(map[string]any) - if attached == nil || attached["name"] != "vol" { - t.Errorf("the volume does not name its server: %v", vol) + if holder := holderOf(t, ts, volumeID); holder != server["id"] { + t.Errorf("the volume names %q, want its server %v", holder, server["id"]) } } // Deleting a server detaches its volumes and keeps them: on Scaleway the disk // outlives the machine, and the CLI polls each volume after the server is gone. +// +// The sequence asserted here is the one the real cloud answered, recorded in +// corpus/scaleway/scw-instance.jsonl: DELETE the server (204), GET the volume in +// block/v1alpha1 (200), DELETE it there (204). func TestDeletingAServerKeepsItsVolume(t *testing.T) { ts := newTestServer(t) @@ -74,30 +88,40 @@ func TestDeletingAServerKeepsItsVolume(t *testing.T) { t.Fatalf("delete server: expected 204, got %d", status) } - status, got := do(t, ts, "GET", zoneURL+"/volumes/"+volumeID, "") + status, got := do(t, ts, "GET", blockURL+"/volumes/"+volumeID, "") if status != http.StatusOK { t.Fatalf("the volume vanished with its server: get returned %d (%v)", status, got) } - vol, _ := got["volume"].(map[string]any) - if vol["server"] != nil { - t.Errorf("the volume is still attached to a deleted server: %v", vol["server"]) + // The field `scw` polls before it deletes: a root left in_use is the hang + // #571 measured, and its references must be empty as well. + if got["status"] != "available" { + t.Errorf("the released volume reads status %v, want available", got["status"]) + } + if refs, _ := got["references"].([]any); len(refs) != 0 { + t.Errorf("the released volume still references %d server(s): %v", len(refs), got["references"]) } // Detached, it can now be deleted, which is what the CLI does next. - if status, _ := do(t, ts, "DELETE", zoneURL+"/volumes/"+volumeID, ""); status != http.StatusNoContent { + if status, _ := do(t, ts, "DELETE", blockURL+"/volumes/"+volumeID, ""); status != http.StatusNoContent { t.Errorf("delete a detached volume: expected 204, got %d", status) } } // An attached volume cannot be deleted, and a client that destroys in the wrong // order depends on that error to retry. +// +// On an INSTANCE volume, which a client still creates explicitly with `scw +// instance volume create` and attaches as an additional disk. The block half of +// the same refusal is TestABlockVolumeAttachedToAServerDoesNotDelete: two +// products, two error shapes, and neither may be inferred from the other. func TestAttachedVolumeRefusesDeletion(t *testing.T) { ts := newTestServer(t) - _, server := serverWith(t, ts, `{"name":"busy","commercial_type":"DEV1-S"}`) - volumes, _ := server["volumes"].(map[string]any) - root, _ := volumes["0"].(map[string]any) - volumeID, _ := root["id"].(string) + server := aServer(t, ts, "busy") + volumeID := aVolume(t, ts, "busy-extra") + if status, out := do(t, ts, "POST", zoneURL+"/servers/"+server+"/attach-volume", `{"volume_id":"`+volumeID+`"}`); status != http.StatusOK { + t.Fatalf("attach-volume answered %d: %v", status, out) + } status, denied := do(t, ts, "DELETE", zoneURL+"/volumes/"+volumeID, "") if status != http.StatusBadRequest { diff --git a/tools/conformance/scaleway/scw-cli.sh b/tools/conformance/scaleway/scw-cli.sh index 37cfd215..89199ddc 100755 --- a/tools/conformance/scaleway/scw-cli.sh +++ b/tools/conformance/scaleway/scw-cli.sh @@ -387,7 +387,39 @@ img_server_id="$(printf '%s' "$img_server" | jq -r '(.server // .).id')" img_root="$(printf '%s' "$img_server" | jq -r '(.server // .).volumes["0"].id')" [ -n "$img_root" ] && [ "$img_root" != null ] || fail "the server carries no root volume: $img_server" -snap="$(scw instance snapshot create name=conformance-snap volume-id="$img_root" zone="$ZONE" -o json)" \ +# The root disk is a BLOCK volume since #365, which is what the cloud gives a +# DEV1-S, so it is snapshotted through the product that owns it. This step is +# new and it is not decoration: it is the whole reason the subject of the +# instance snapshot below had to change. +# +# `scw instance snapshot create volume-id=` cannot be that +# subject, and the refusal comes from the CLI rather than from here: without +# `unified=true` the command calls instance.GetVolume itself before it sends +# anything (scaleway-cli 2.56.3, internal/namespaces/instance/v1/ +# custom_snapshot.go) and returns that error. The instance route DOES resolve a +# block volume — TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot, and +# `unified=true` reaches it — but a fixture cannot assert it through a client +# that stops one call earlier. +root_snap="$(scw block snapshot create name=conformance-root-snap volume-id="$img_root" \ + zone="$ZONE" -o json)" || fail "block snapshot of the server root rejected: $root_snap" +root_snap_id="$(printf '%s' "$root_snap" | jq -r '(.snapshot // .).id')" +[ -n "$root_snap_id" ] && [ "$root_snap_id" != null ] \ + || fail "no id in the root snapshot response: $root_snap" +scw block snapshot get "$root_snap_id" zone="$ZONE" -o json \ + | jq -e --arg v "$img_root" '(.snapshot // .).parent_volume.id == $v' >/dev/null \ + || fail "the snapshot of the root disk does not name the root disk" +scw block snapshot delete "$root_snap_id" zone="$ZONE" >/dev/null \ + || fail "delete of the root snapshot rejected" + +# An instance volume for the instance snapshot, created by the client the way a +# client does. It used to be the server's root disk, which was an instance +# volume until #365 moved it where the cloud keeps it. +img_vol="$(scw instance volume create name=conformance-golden-vol volume-type=b_ssd size=10G \ + zone="$ZONE" -o json)" || fail "volume create for the image test rejected: $img_vol" +img_vol_id="$(printf '%s' "$img_vol" | jq -r '(.volume // .).id')" +[ -n "$img_vol_id" ] && [ "$img_vol_id" != null ] || fail "no id in the volume create response: $img_vol" + +snap="$(scw instance snapshot create name=conformance-snap volume-id="$img_vol_id" zone="$ZONE" -o json)" \ || fail "snapshot create rejected: $snap" snap_id="$(printf '%s' "$snap" | jq -r '(.snapshot // .).id')" [ -n "$snap_id" ] && [ "$snap_id" != null ] || fail "no id in the snapshot create response: $snap" @@ -395,7 +427,7 @@ snap_id="$(printf '%s' "$snap" | jq -r '(.snapshot // .).id')" printf '%s' "$snap" | jq -e '(.snapshot // .).state == "available"' >/dev/null \ || fail "the snapshot is not available on creation: $snap" scw instance snapshot get "$snap_id" zone="$ZONE" -o json \ - | jq -e --arg v "$img_root" '(.snapshot // .).base_volume.id == $v' >/dev/null \ + | jq -e --arg v "$img_vol_id" '(.snapshot // .).base_volume.id == $v' >/dev/null \ || fail "the snapshot does not name the volume it was taken of" # arch is required by the CLI, not by the API: `scw instance image create` @@ -423,6 +455,8 @@ prove_end "$neg" scw instance image delete "$img_id" zone="$ZONE" >/dev/null || fail "image delete rejected" scw instance snapshot delete "$snap_id" zone="$ZONE" >/dev/null \ || fail "snapshot delete rejected once its image was gone" +scw instance volume delete "$img_vol_id" zone="$ZONE" >/dev/null \ + || fail "delete of the snapshotted volume rejected" scw instance server stop "$img_server_id" zone="$ZONE" >/dev/null || fail "cleanup: poweroff rejected" scw instance server delete "$img_server_id" zone="$ZONE" >/dev/null || fail "cleanup: delete rejected" prove_end "$span" diff --git a/tools/falsify/specs/block-volumes-reach-their-server.json b/tools/falsify/specs/block-volumes-reach-their-server.json index 63cb06fd..71d902b7 100644 --- a/tools/falsify/specs/block-volumes-reach-their-server.json +++ b/tools/falsify/specs/block-volumes-reach-their-server.json @@ -98,6 +98,38 @@ "replace": "\tid := r.PathValue(\"id\")\n\tres, found := p.anyVolume(id)\n\tif !found {\n\t\tres, found = p.env.Store.Get(Name, kindVolume, id)\n\t}\n\tif !found || res.Tenant.Zone != zone {", "expect": "the over-correction: getUnknownVolume stops at the instance answer and never reads block, so the Terraform provider gets a field set without size, references or status — the failure #8 exists to prevent, reintroduced by a fix that looked symmetrical", "test": "TestAnSbsRootVolumeIsReadableThroughTheBlockFallback" + }, + { + "label": "a default root disk goes back to instance/v1, where the cloud does not keep it", + "file": "internal/providers/scaleway/servers.go", + "find": "\tif wanted.VolumeType == \"\" || wanted.VolumeType == \"sbs_volume\" {", + "replace": "\tif (wanted.VolumeType == \"\" && false) || wanted.VolumeType == \"sbs_volume\" {", + "expect": "`scw` follows the server's own volumes[\"0\"].id into block/v1alpha1 and meets a 404, which is #365's title", + "test": "TestADefaultRootVolumeLivesInBlockLikeTheCloud" + }, + { + "label": "a root disk stops naming the image snapshot it was restored from", + "file": "internal/providers/scaleway/servers.go", + "find": "\trootVol := p.rootVolume(res, req.Name, project, organization, p.imageRootSnapshot(resolvedImageID), req.Volumes[\"0\"])", + "replace": "\trootVol := p.rootVolume(res, req.Name, project, organization, p.imageRootSnapshot(resolvedImageID)[:0], req.Volumes[\"0\"])", + "expect": "parent_snapshot_id answers null where the recorded cloud answers the image's snapshot, and a client asking where its disk came from is told nowhere", + "test": "TestARootVolumeNamesTheImageSnapshotItCameFrom" + }, + { + "label": "a released block volume stops saying when it was released", + "file": "internal/providers/scaleway/volumes.go", + "find": "\t\t\tstored.Attrs[\"last_detached_at\"] = p.env.Now().Format(time.RFC3339)", + "replace": "\t\t\tstored.Attrs[\"last_detached_at-not-answered\"] = p.env.Now().Format(time.RFC3339)", + "expect": "last_detached_at stays null for ever, where both recordings of a real account carry a timestamp on the read that follows the detach", + "test": "TestAReleasedBlockVolumeSaysWhenItWasDetached" + }, + { + "label": "the view stops publishing the detachment it recorded", + "file": "internal/providers/scaleway/block.go", + "find": "\tif detached := textOf(res.Attrs[\"last_detached_at\"]); detached != \"\" {\n\t\tview[\"last_detached_at\"] = detached\n\t}", + "replace": "\tif detached := textOf(res.Attrs[\"last_detached_at\"]); detached != \"\" && false {\n\t\tview[\"last_detached_at\"] = detached\n\t}", + "expect": "the field is recorded and never answered, which reads to a client exactly like a volume nothing has ever released", + "test": "TestAReleasedBlockVolumeSaysWhenItWasDetached" } ] } diff --git a/tools/falsify/specs/scaleway-cloud-fidelity.json b/tools/falsify/specs/scaleway-cloud-fidelity.json index 43cac335..ed7e50ae 100644 --- a/tools/falsify/specs/scaleway-cloud-fidelity.json +++ b/tools/falsify/specs/scaleway-cloud-fidelity.json @@ -46,8 +46,8 @@ { "label": "a released block volume keeps reading in_use, and `scw instance server delete` polls it for ever (#365)", "file": "internal/providers/scaleway/volumes.go", - "find": "\t\tif stored.Kind == kindBlockVolume {\n\t\t\tstored.State = blockVolumeAvailable\n\t\t}", - "replace": "\t\tif stored.Kind == kindBlockVolume && false {\n\t\t\tstored.State = blockVolumeAvailable\n\t\t}", + "find": "\t\t\tstored.State = blockVolumeAvailable\n", + "replace": "\t\t\tstored.State = stored.State + blockVolumeAvailable[:0]\n", "test": "TestABlockVolumeIsAvailableOnceItsServerIsGone" }, { From 1e937626b3b05035ee8392c242edb1ed4896bd53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 22:29:44 +0200 Subject: [PATCH 3/4] fix(scaleway): a snapshot that promises the block product has to be answerable there, and this one was not (#571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by measuring my own fix. The instance snapshot of a block volume that the commit before this one added was typed `sbs_snapshot`, read straight off the SDK's `VolumeVolumeType` enum. That value is not a label, it is a promise: `scw instance image list` calls `block.GetSnapshot` for every image whose `root_volume.volume_type` is `sbs_snapshot` and fails the WHOLE listing on error (scaleway-cli 2.56.3, `internal/namespaces/instance/v1/custom_image.go:222`). The snapshot lives in `instance/v1`, so the promise was false, and cutting an image from it broke a command that has nothing to do with it: scw instance image list before: rc=1, "cannot find resource 'snapshot' with ID …" — for the whole zone after: rc=0, the image listed `unified` instead, which is not a compromise: it is the value the CLI itself sends for this exact input. With `unified=true` it skips the volume lookup and asks for `SnapshotVolumeTypeUnified` whatever the volume is; without that flag it reads the volume through `instance.GetVolume` and gives up on the 404. So a unified snapshot is the only instance snapshot of a block volume any `scw` user can ask for. The test asserts the promise rather than the spelling — if the type ever is `sbs_snapshot`, `block/v1alpha1` must answer for that id — so it keeps holding the day a snapshot really does cross the two products. Which is the finding this commit also writes down and does NOT fix, because it is a body of work and the maintainer asked to be asked. Measured 2026-08-28: scw block snapshot create volume-id= works scw instance image create snapshot-id= 404 scw instance snapshot get 404 scw block volume create from-snapshot= 404 A block snapshot is now the only kind a client can take of a server's root disk, and `scw instance image create` cannot cut an image from one — so the golden-image chain is walkable from a volume the client created and no longer from the server's own root. It is the shape #571 fixed for volumes, one product over, and it is real rather than a decision: the SDK says images built on block snapshots exist. docs/limits.md carries the table. Assisted-by: Claude Code (claude-opus-5) --- docs/limits.md | 25 +++++++++++ .../providers/scaleway/block_attach_test.go | 42 ++++++++++++++----- internal/providers/scaleway/snapshots.go | 38 ++++++++++++----- .../block-volumes-reach-their-server.json | 18 +++++--- 4 files changed, 98 insertions(+), 25 deletions(-) diff --git a/docs/limits.md b/docs/limits.md index b1f7232c..b87d445f 100644 --- a/docs/limits.md +++ b/docs/limits.md @@ -933,6 +933,31 @@ walks. Whether the real cloud answers `instance.GetVolume` for an SBS volume is **not measured here**: no recording carries that call, and the SDK's own `getUnknownVolume` only makes sense if it can 404. +**And the snapshots have not crossed, which the flip put on the default path.** +Measured on 2026-08-28, against this emulator, with `scw` 2.56.3: + +| naming a *block* snapshot | naming an *instance* snapshot | +|---|---| +| `scw block snapshot create volume-id=` — works | n/a | +| `scw instance image create snapshot-id=…` — **404** | works | +| `scw instance snapshot get …` — 404 (the fallback's own shape) | works | +| `scw block volume create from-snapshot.snapshot-id=…` — works | **404** | + +A block snapshot is now the only kind a client can take of a server's root disk, +and `scw instance image create` cannot cut an image from one. So the golden-image +chain — snapshot a disk, cut an image, boot from it — is walkable from a volume +the client created and no longer from the server's own root. This is the shape +#571 fixed for volumes, one product over, and it is **named rather than fixed**: +the SDK says images built on block snapshots exist (`Image.RootVolume.VolumeType` +can be `sbs_snapshot`, and `scw instance image list` reads `block.GetSnapshot` +for exactly that value), so the gap is real and not a decision. + +One consequence is already guarded, because it was created and measured inside +this change: an instance snapshot of a block volume is typed `unified`, never +`sbs_snapshot`. `sbs_snapshot` is a promise that the id resolves in +`block/v1alpha1`, and an image cut from a snapshot that broke that promise made +`scw instance image list` fail for the whole zone. + ## What survives a dead emulator, in one table The store is memory: a dead process loses every emulated resource, and that is diff --git a/internal/providers/scaleway/block_attach_test.go b/internal/providers/scaleway/block_attach_test.go index 828fee14..57cbc740 100644 --- a/internal/providers/scaleway/block_attach_test.go +++ b/internal/providers/scaleway/block_attach_test.go @@ -211,18 +211,31 @@ func TestACreateNamingABlockVolumeAttachesIt(t *testing.T) { } } -// An instance snapshot of a block volume is an sbs_snapshot. +// An instance snapshot of a block volume works, and does not promise the block +// product. // // The route resolved kindVolume alone, so it answered 404 on the disk the same -// emulator published under the server's volumes["0"]. The type is read from the -// SDK rather than from a recording, and the reading is narrow: instance/v1 -// VolumeVolumeType declares sbs_snapshot beside sbs_volume and Snapshot -// .VolumeType is a VolumeVolumeType, while CreateSnapshotRequest.VolumeType -// (SnapshotVolumeType) cannot spell it — b_ssd would name a different product. +// emulator published under the server's volumes["0"]. It answers now — and the +// TYPE it answers was got wrong once, in this same change, which is why the +// assertion below is about what the value must NOT be. +// +// sbs_snapshot was the first answer, read straight off the SDK's +// VolumeVolumeType enum, and it broke a command: `scw instance image list` calls +// block.GetSnapshot for every image whose root_volume.volume_type is +// sbs_snapshot and fails the WHOLE listing on error (scaleway-cli 2.56.3, +// internal/namespaces/instance/v1/custom_image.go:222). Cutting an image from +// such a snapshot made `scw instance image list` answer "cannot find resource +// 'snapshot'" for the entire zone — measured 2026-08-28, against the emulator +// this test runs in. +// +// So the invariant is not "the type is unified". It is: **a type that promises +// the block product must be answerable by the block product**, and this test +// asserts the promise rather than the spelling, so it keeps holding the day a +// snapshot really does cross the two. // // What a client asks for still wins, because the request field "overrides the // volume_type of the snapshot" in the SDK's own words. -func TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot(t *testing.T) { +func TestAnInstanceSnapshotOfABlockVolumeDoesNotPromiseTheBlockProduct(t *testing.T) { ts := newTestServer(t) _, body := serverWith(t, ts, `{"name":"sbs","commercial_type":"DEV1-S","volumes":{"0":{"volume_type":"sbs_volume","size":20000000000}}}`) @@ -235,9 +248,7 @@ func TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot(t *testing.T) { t.Fatalf("snapshot of a block volume answered %d, want 201: %v", status, out) } snap, _ := out["snapshot"].(map[string]any) - if snap["volume_type"] != "sbs_snapshot" { - t.Errorf("the snapshot reports volume_type %v, want sbs_snapshot", snap["volume_type"]) - } + snapID, _ := snap["id"].(string) base, _ := snap["base_volume"].(map[string]any) if base == nil || base["id"] != rootID { t.Errorf("the snapshot does not name the volume it was taken of: %v", snap["base_volume"]) @@ -247,6 +258,17 @@ func TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot(t *testing.T) { if size, _ := snap["size"].(float64); size != 20000000000 { t.Errorf("the snapshot reports size %v, want the volume's 20000000000", snap["size"]) } + // b_ssd would name the product it was NOT taken from. + if snap["volume_type"] == "b_ssd" { + t.Errorf("the snapshot of a block volume is typed b_ssd, which is the other product") + } + // And the promise: sbs_snapshot means "this id resolves in block/v1alpha1". + if snap["volume_type"] == "sbs_snapshot" { + if status, _ := do(t, ts, "GET", blockURL+"/snapshots/"+snapID, ""); status != http.StatusOK { + t.Errorf("the snapshot is typed sbs_snapshot and block answers %d for it: "+ + "`scw instance image list` reads block.GetSnapshot on exactly that type and fails the whole listing", status) + } + } // A named type wins, because the request field overrides. status, out = do(t, ts, "POST", zone+"/snapshots", diff --git a/internal/providers/scaleway/snapshots.go b/internal/providers/scaleway/snapshots.go index 801fcada..ae2bd4f1 100644 --- a/internal/providers/scaleway/snapshots.go +++ b/internal/providers/scaleway/snapshots.go @@ -106,21 +106,39 @@ func (p *Pack) createSnapshot(w http.ResponseWriter, r *http.Request) { // class, "sbs" — so the reading above leaves it empty and the default // below would call the snapshot b_ssd, which is a different product. // - // The value comes from the SDK's enum, not from the wire: instance/v1 - // VolumeVolumeType declares sbs_snapshot beside sbs_volume, and - // Snapshot.VolumeType is a VolumeVolumeType, while CreateSnapshotRequest - // .VolumeType (SnapshotVolumeType) cannot even spell it — its four - // values are unknown_volume_type, l_ssd, b_ssd and unified, and its - // documentation says "if omitted, the volume type of the original volume - // will be used". So the request cannot ask for this and the answer has - // to derive it. No recorded account here holds one: this is a reading, - // declared as such, like the block snapshot shape above it. + // `unified` rather than `sbs_snapshot`, and the difference was MEASURED + // rather than reasoned. sbs_snapshot was the first answer here, read + // straight off the SDK's VolumeVolumeType enum, and it broke a command: + // `scw instance image list` calls block.GetSnapshot for every image whose + // root_volume.volume_type is sbs_snapshot and fails the WHOLE listing on + // error (scaleway-cli 2.56.3, + // internal/namespaces/instance/v1/custom_image.go:222). Cutting an image + // from such a snapshot therefore made `scw instance image list` answer + // "cannot find resource 'snapshot'" for the entire zone — measured + // 2026-08-28. sbs_snapshot is a promise that the id resolves in the + // BLOCK product, and this snapshot lives in instance/v1. + // + // `unified` is the value the CLI itself sends for this very input: with + // `unified=true` it skips the volume lookup entirely and asks for + // SnapshotVolumeTypeUnified, whatever the volume is. And without that + // flag it reads the volume through instance.GetVolume and gives up on a + // 404 — so unified is the ONLY instance snapshot of a block volume any + // scw user can ask for. + // + // What is not settled, and is not this change's to settle: whether the + // cloud makes such a snapshot readable through block/v1alpha1 as well. + // If it does, the honest answer is sbs_snapshot AND a snapshot that + // answers on both doors — which is the volume work of #571 done again + // for snapshots, and nothing here has measured it. + // + // TestAnInstanceSnapshotOfABlockVolumeDoesNotPromiseTheBlockProduct + // fails without this. // // Only when the client named none: the request field "overrides the // volume_type of the snapshot", which is the SDK's own wording, so a // client that asked for one keeps it. if volumeType == "" && volume.Kind == kindBlockVolume { - volumeType = "sbs_snapshot" + volumeType = "unified" } // Through the shared reader: the assertion this replaces answered // ok=false on a volume that had crossed a snapshot, so a snapshot taken diff --git a/tools/falsify/specs/block-volumes-reach-their-server.json b/tools/falsify/specs/block-volumes-reach-their-server.json index 71d902b7..498930f0 100644 --- a/tools/falsify/specs/block-volumes-reach-their-server.json +++ b/tools/falsify/specs/block-volumes-reach-their-server.json @@ -49,15 +49,15 @@ "find": "\t\tvolume, found := p.anyVolume(*req.VolumeID)\n\t\tif !found {", "replace": "\t\tvolume, found := p.anyVolume(*req.VolumeID)\n\t\tif !found || volume.Kind == kindBlockVolume {", "expect": "the golden-image path answers 404 on the disk the server publishes under volumes[\"0\"]", - "test": "TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot" + "test": "TestAnInstanceSnapshotOfABlockVolumeDoesNotPromiseTheBlockProduct" }, { - "label": "a snapshot of a block disk is typed as the other product's", + "label": "a snapshot of a block disk falls back to the other product's type", "file": "internal/providers/scaleway/snapshots.go", - "find": "\t\tif volumeType == \"\" && volume.Kind == kindBlockVolume {\n\t\t\tvolumeType = \"sbs_snapshot\"\n\t\t}", - "replace": "\t\tif volumeType == \"\" && volume.Kind == kindBlockVolume && false {\n\t\t\tvolumeType = \"sbs_snapshot\"\n\t\t}", + "find": "\t\tif volumeType == \"\" && volume.Kind == kindBlockVolume {\n\t\t\tvolumeType = \"unified\"\n\t\t}", + "replace": "\t\tif volumeType == \"\" && volume.Kind == kindBlockVolume && false {\n\t\t\tvolumeType = \"unified\"\n\t\t}", "expect": "the snapshot falls through to the b_ssd default, naming a product it was not taken from", - "test": "TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot" + "test": "TestAnInstanceSnapshotOfABlockVolumeDoesNotPromiseTheBlockProduct" }, { "label": "a block disk is published inside a server with no type at all", @@ -130,6 +130,14 @@ "replace": "\tif detached := textOf(res.Attrs[\"last_detached_at\"]); detached != \"\" && false {\n\t\tview[\"last_detached_at\"] = detached\n\t}", "expect": "the field is recorded and never answered, which reads to a client exactly like a volume nothing has ever released", "test": "TestAReleasedBlockVolumeSaysWhenItWasDetached" + }, + { + "label": "an instance snapshot of a block disk promises the block product again", + "file": "internal/providers/scaleway/snapshots.go", + "find": "\t\t\tvolumeType = \"unified\"\n", + "replace": "\t\t\tvolumeType = \"unified\"[:0] + \"sbs_snapshot\"\n", + "expect": "the type says the id resolves in block/v1alpha1 and it does not, so an image cut from it makes `scw instance image list` fail for the whole zone (custom_image.go:222) — measured 2026-08-28", + "test": "TestAnInstanceSnapshotOfABlockVolumeDoesNotPromiseTheBlockProduct" } ] } From 2b09d6851782b7d3cb9581ec0f7ca8ecadc08597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 22:33:38 +0200 Subject: [PATCH 4/4] docs(scaleway): rootVolume's own comment described a restriction it stopped enforcing at SW-3 (#571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not prose tidying. The comment above `rootVolume` opened with "the type stays b_ssd whatever is asked" and closed with "so today there is no writable value" — both false since #8 served `sbs_volume`, and flatly wrong now that no type at all gets a block disk. The next reader of that function would have been told the opposite of what the code does, by the comment written to stop exactly that. The two reasons it gives are kept and separated, because that separation is the lesson #8 paid for: the local types are still overridden (the CLI sums LOCAL volumes against `volumes_constraint.min_size`), `sbs_volume` is honoured, and omitting the block now gets `sbs_volume` too. One stale citation with it: the conformance suite named TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot, which was renamed in the commit before this one when its subject turned out to be the promise rather than the spelling. Assisted-by: Claude Code (claude-opus-5) --- internal/providers/scaleway/servers.go | 32 +++++++++++++++----------- tools/conformance/scaleway/scw-cli.sh | 6 ++--- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/internal/providers/scaleway/servers.go b/internal/providers/scaleway/servers.go index 94928fce..1397d27e 100644 --- a/internal/providers/scaleway/servers.go +++ b/internal/providers/scaleway/servers.go @@ -192,28 +192,32 @@ const rootVolumeSize = 20_000_000_000 // volumes endpoint, so the volume has to be a stored resource, not an inline // object. // -// The type stays b_ssd whatever is asked, and there are two reasons, not one. -// The single reason this comment used to give covered only half the values it -// refuses, which is how a reader came to lift the restriction for the other -// half — reported by @vde-dis on #8, with the measurement below. +// The requested type is honoured for sbs_volume and refused for the LOCAL ones, +// and there are two reasons, not one. The single reason this comment used to +// give covered only half the values it refuses, which is how a reader came to +// lift the restriction for the other half — reported by @vde-dis on #8, with the +// measurement below. Both halves are stated here for that reason, and neither is +// repeated inside the function. // // Against a *local* type (l_ssd, scratch): the catalogue declares // volumes_constraint.min_size at 0 and the CLI sums local volumes against it, // so attaching one here would make the CLI refuse the very creation it just -// asked for. +// asked for. That is unchanged and it is why the local branch is still an +// override rather than an honouring. // // Against sbs_volume, which is block and sums to nothing there: honouring it // sends the Terraform provider to GET /block/v1/zones/{zone}/volumes/{id} to -// read the volume back, no pack serves block/v1, and the apply dies on -// "waiting for Volume failed: http error 404 Not Found". A permanent diff is -// bad; an apply that cannot finish is worse. It becomes honourable with #8 -// (SW-3) and not before — the two belong in one batch. +// read the volume back. No pack served block/v1 before SW-3, and the apply died +// on "waiting for Volume failed: http error 404 Not Found"; block/v1 is served +// now, so it is honoured — and since #365 it is also what a request naming no +// type gets, because it is what the cloud gives a DEV1-S. // -// So today there is no writable value: the provider refuses b_ssd outright from -// 2.79 on ("b_ssd volumes are not supported anymore"), and sbs_volume plans for -// ever. Omitting the root_volume block is the way through, which is what the -// conformance fixture happens to do — which is also why nothing here shows it. -// docs/limits.md carries that as a stated limit rather than a surprise. +// What that leaves a client: b_ssd is refused by the provider itself from 2.79 +// on ("b_ssd volumes are not supported anymore"), sbs_volume works, and omitting +// the block gets sbs_volume too. The conformance fixture declares the block +// rather than omitting it, which is the whole point of #8 — a fixture that +// avoids the one input that breaks is a test that cannot fail. docs/limits.md +// carries what is still not emulated behind an SBS volume: the storage itself. func (p *Pack) rootVolume(server *resource.Resource, name, project, organization, parentSnapshot string, wanted volumeTemplate) *resource.Resource { // The size the client asked for, when it asked. Ignoring it gave every // server the catalogue's disk whatever the request said. diff --git a/tools/conformance/scaleway/scw-cli.sh b/tools/conformance/scaleway/scw-cli.sh index 89199ddc..e599b2ab 100755 --- a/tools/conformance/scaleway/scw-cli.sh +++ b/tools/conformance/scaleway/scw-cli.sh @@ -397,9 +397,9 @@ img_root="$(printf '%s' "$img_server" | jq -r '(.server // .).volumes["0"].id')" # `unified=true` the command calls instance.GetVolume itself before it sends # anything (scaleway-cli 2.56.3, internal/namespaces/instance/v1/ # custom_snapshot.go) and returns that error. The instance route DOES resolve a -# block volume — TestAnInstanceSnapshotOfABlockVolumeIsAnSbsSnapshot, and -# `unified=true` reaches it — but a fixture cannot assert it through a client -# that stops one call earlier. +# block volume — TestAnInstanceSnapshotOfABlockVolumeDoesNotPromiseTheBlockProduct +# covers it and `unified=true` reaches it — but a fixture cannot assert it +# through a client that stops one call earlier. root_snap="$(scw block snapshot create name=conformance-root-snap volume-id="$img_root" \ zone="$ZONE" -o json)" || fail "block snapshot of the server root rejected: $root_snap" root_snap_id="$(printf '%s' "$root_snap" | jq -r '(.snapshot // .).id')"