diff --git a/internal/catalogue/bootstrap_cycle_test.go b/internal/catalogue/bootstrap_cycle_test.go new file mode 100644 index 0000000..e37ab7f --- /dev/null +++ b/internal/catalogue/bootstrap_cycle_test.go @@ -0,0 +1,71 @@ +package catalogue + +import ( + "strings" + "testing" +) + +// The module that provides the artifact store may not be delivered through it. +// +// Building publishes to the store and the builder will not start without one, so a module that +// provides the store and also builds something asks the mesh to put an artifact into the thing +// that artifact is needed to create. On a mesh new enough to have no registry, that is a build +// that never returns (novox/hq 04-ISSUES/029). +func TestTheArtifactStoreCannotBeDeliveredThroughItself(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"registry","version":"1",` + + `"provides":[{"name":"artifact-store","scope":"mesh"}],` + + `"build":{"artifacts":[{"name":"registry","kind":"upstream","from":"registry:2"}]},` + + `"resources":[{"id":"store","type":"container","name":"mesh-registry",` + + `"artifact":"registry","ports":["5000:5000"]}]}`)) + if err == nil { + t.Fatal("a registry module that builds its own image was accepted; the build has " + + "nowhere to publish until the module it belongs to is already running") + } + if !strings.Contains(err.Error(), "artifact-store") { + t.Fatalf("refused without naming the provision the cycle turns on: %v", err) + } +} + +// Naming the image directly is the way out, and must stay accepted. +func TestAnArtifactStoreThatNamesItsImageIsAccepted(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"registry","version":"1",` + + `"provides":[{"name":"artifact-store","scope":"mesh"}],` + + `"resources":[{"id":"store","type":"container","name":"mesh-registry",` + + `"image":"registry@sha256:` + + `266f282fabd7cd3df053ee7c658c77b42380d1a2f0d8e5a1c0d7a6d5b5c4a3b2",` + + `"ports":["5000:5000"]}]}`)) + if err != nil { + t.Fatalf("the one way an artifact store can be delivered was refused: %v", err) + } +} + +// And an ordinary module still builds whatever it likes. +func TestAModuleThatDoesNotProvideTheStoreStillBuilds(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"forge","version":"1",` + + `"build":{"artifacts":[{"name":"forge","kind":"upstream","from":"gitea/gitea:1.22"}]},` + + `"resources":[{"id":"run","type":"container","name":"forge","artifact":"forge"}]}`)) + if err != nil { + t.Fatalf("an ordinary module was caught by a rule about the artifact store: %v", err) + } +} + +// A mapping that names an address still names the port, and the machine side is still the middle. +// +// The substrate bundle writes "127.0.0.1:5432:5432" today, so the shape is not hypothetical. +// Found in review: the first cut split on the first colon, read "127.0.0.1" as the machine port, +// failed to parse it, and silently skipped the mapping — which put the filter back on the +// declared port, the exact fault MachineSide was written to end. +func TestAnAddressedMappingStillNamesThePort(t *testing.T) { + m := Manifest{Module: "store", Resources: []map[string]any{ + {"type": "container", "id": "server", "ports": []any{"127.0.0.1:8080:80"}}, + }} + for _, named := range []int{8080, 80} { + at, mayAssign := m.MachineSide(named) + if mayAssign { + t.Fatalf("%d was reassigned though the manifest published it explicitly", named) + } + if at != 8080 { + t.Fatalf("naming %d gave %d; the machine side of 127.0.0.1:8080:80 is 8080", named, at) + } + } +} diff --git a/internal/catalogue/machineside_test.go b/internal/catalogue/machineside_test.go index 82d163d..553e1a5 100644 --- a/internal/catalogue/machineside_test.go +++ b/internal/catalogue/machineside_test.go @@ -1,9 +1,6 @@ package catalogue -import ( - "strings" - "testing" -) +import "testing" // A module with nothing that publishes binds what it binds, and the mesh may not move it. // @@ -61,47 +58,3 @@ func TestAPortNotInTheMappingIsNotFound(t *testing.T) { at, mayAssign) } } - -// The module that provides the artifact store may not be delivered through it. -// -// Building publishes to the store and the builder will not start without one, so a module that -// provides the store and also builds something asks the mesh to put an artifact into the thing -// that artifact is needed to create. On a mesh new enough to have no registry, that is a build -// that never returns (novox/hq 04-ISSUES/029). -func TestTheArtifactStoreCannotBeDeliveredThroughItself(t *testing.T) { - _, err := ParseManifest([]byte(`{"module":"registry","version":"1",` + - `"provides":[{"name":"artifact-store","scope":"mesh"}],` + - `"build":{"artifacts":[{"name":"registry","kind":"upstream","from":"registry:2"}]},` + - `"resources":[{"id":"store","type":"container","name":"mesh-registry",` + - `"artifact":"registry","ports":["5000:5000"]}]}`)) - if err == nil { - t.Fatal("a registry module that builds its own image was accepted; the build has " + - "nowhere to publish until the module it belongs to is already running") - } - if !strings.Contains(err.Error(), "artifact-store") { - t.Fatalf("refused without naming the provision the cycle turns on: %v", err) - } -} - -// Naming the image directly is the way out, and must stay accepted. -func TestAnArtifactStoreThatNamesItsImageIsAccepted(t *testing.T) { - _, err := ParseManifest([]byte(`{"module":"registry","version":"1",` + - `"provides":[{"name":"artifact-store","scope":"mesh"}],` + - `"resources":[{"id":"store","type":"container","name":"mesh-registry",` + - `"image":"registry@sha256:` + - `266f282fabd7cd3df053ee7c658c77b42380d1a2f0d8e5a1c0d7a6d5b5c4a3b2",` + - `"ports":["5000:5000"]}]}`)) - if err != nil { - t.Fatalf("the one way an artifact store can be delivered was refused: %v", err) - } -} - -// And an ordinary module still builds whatever it likes. -func TestAModuleThatDoesNotProvideTheStoreStillBuilds(t *testing.T) { - _, err := ParseManifest([]byte(`{"module":"forge","version":"1",` + - `"build":{"artifacts":[{"name":"forge","kind":"upstream","from":"gitea/gitea:1.22"}]},` + - `"resources":[{"id":"run","type":"container","name":"forge","artifact":"forge"}]}`)) - if err != nil { - t.Fatalf("an ordinary module was caught by a rule about the artifact store: %v", err) - } -} diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 5c5bef7..aab61d2 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -778,18 +778,21 @@ func (m Manifest) MachineSide(port int) (at int, mayAssign bool) { } for _, entry := range listed { written := strings.TrimSpace(fmt.Sprint(entry)) - host, inside, long := strings.Cut(written, ":") - if !long { + // "8080", "8080:80", or "127.0.0.1:8080:80" when an address was named — the machine + // side is always the second-from-last part, the same reading the host applies. The + // first shape said only the software's port, so the mesh may choose; the others chose. + parts := strings.Split(written, ":") + if len(parts) == 1 { if n, err := strconv.Atoi(written); err == nil && n == port { return port, true } continue } - outer, err := strconv.Atoi(strings.TrimSpace(host)) + outer, err := strconv.Atoi(strings.TrimSpace(parts[len(parts)-2])) if err != nil { continue } - inner, err := strconv.Atoi(strings.TrimSpace(inside)) + inner, err := strconv.Atoi(strings.TrimSpace(parts[len(parts)-1])) if err == nil && (outer == port || inner == port) { return outer, false } diff --git a/internal/inventory/catalogue.go b/internal/inventory/catalogue.go index d12307f..39a849a 100644 --- a/internal/inventory/catalogue.go +++ b/internal/inventory/catalogue.go @@ -283,7 +283,11 @@ func (i *Inventory) Unassign(ctx context.Context, nodeName, module string) error if tag.RowsAffected() == 0 { return fmt.Errorf("%s is not assigned to %s", module, nodeName) } - return nil + // And its ports go back. Kept-once-chosen is a promise about a module that is still here — + // held past unassignment, a fixed port stays claimed in the name of something that is gone, + // and the next module needing it is refused by a ghost. The same argument that lets a machine + // take a carried port back: a set that only grows keeps a port reserved for nothing. + return i.ReleasePorts(ctx, nodeName, module) } // Assigned is what a person put on this node, which is not the same as what it runs: resolution diff --git a/internal/inventory/ports.go b/internal/inventory/ports.go index b765b87..8ac354e 100644 --- a/internal/inventory/ports.go +++ b/internal/inventory/ports.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" ) // Which port a machine uses for what a module needs reachable. @@ -163,6 +164,38 @@ func (i *Inventory) assignPort( `insert into port_assignment (node, module, wanted, machine, fixed) values ($1, $2, $3, $4, $5)`, nodeID, module, wanted, machine, fixed) + // Two allocations at once can both pick the same lowest free port; the unique index lets one + // through and hands the other a constraint violation in SQL. Said in the mesh's words instead + // — and for a port the mesh chose, simply chosen again: the free list has moved, the retry + // reads it fresh, and the caller never learns the race happened. + var collided *pgconn.PgError + if errors.As(err, &collided) && collided.Code == "23505" { + // Which race decides what happens next. The table has two keys, so this is one of two + // collisions: the racer was *this same assignment* (the primary key), in which case its + // answer is the answer — kept-once-chosen does not care who did the choosing — or it was + // another module taking the machine port (the unique index), in which case the free list + // has moved and an unfixed pick is simply made again. Asking the table tells them apart; + // branching on the constraint's name would couple this to the migration's spelling. + var held Assigned + reread := i.store.Pool().QueryRow(ctx, + `select machine, fixed from port_assignment + where node = $1 and module = $2 and wanted = $3`, + nodeID, module, wanted).Scan(&held.Machine, &held.Fixed) + if reread == nil { + held.Module, held.Wanted = module, wanted + return held, nil + } + if !errors.Is(reread, pgx.ErrNoRows) { + return Assigned{}, reread + } + if !fixed { + return i.assignPort(ctx, nodeID, node, module, wanted, false) + } + return Assigned{}, fmt.Errorf( + "%w: %s needs %d on %s and something else was given it at the same moment — "+ + "two assignments raced, and the port the protocol fixes went to the other one", + ErrPortTaken, module, wanted, node) + } if err != nil { return Assigned{}, err } diff --git a/internal/inventory/ports_test.go b/internal/inventory/ports_test.go index 0a2a835..808876e 100644 --- a/internal/inventory/ports_test.go +++ b/internal/inventory/ports_test.go @@ -211,3 +211,28 @@ func TestWhatAMachineNoLongerHoldsIsAvailableAgain(t *testing.T) { t.Errorf("a port the machine gave back was still reserved: got %d", got.Machine) } } + +// Unassigning a module gives its ports back — the fixed ones are what make this matter. +// +// Held past unassignment, port 25 stays claimed in the name of a mail system that is gone, and +// every mail system after it is refused by a ghost. Found in review: ReleasePorts existed, was +// documented "for when it is unassigned", and was called by nothing. +func TestUnassigningReleasesTheModulesPorts(t *testing.T) { + inv, node := aNodeWithModules(t, "mailu", "other-mail") + ctx := t.Context() + if err := inv.Assign(ctx, node, "mailu"); err != nil { + t.Fatal(err) + } + if _, err := inv.PortFor(ctx, node, "mailu", 25, true); err != nil { + t.Fatal(err) + } + if err := inv.Unassign(ctx, node, "mailu"); err != nil { + t.Fatal(err) + } + if err := inv.Assign(ctx, node, "other-mail"); err != nil { + t.Fatal(err) + } + if _, err := inv.PortFor(ctx, node, "other-mail", 25, true); err != nil { + t.Fatalf("port 25 is still held in the name of a module that was unassigned: %v", err) + } +}