From 50734095b8b77dbb43f64c847da63cf421205a10 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 26 Sep 2026 19:02:35 +0200 Subject: [PATCH] A repeat assignment says nothing changed (ADR 0115) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One assignment of a module per node is now the rule, not a limitation — the operator dropped the multi-assignment requirement, and the schema's (node, module) key has been the decision since migration 0005. What changed: Assign reports whether the assignment was new, and the command says 'already runs — one node runs one of each (ADR 0115); nothing changed' instead of printing 'is assigned' for a no-op, which read as an action that happened. Idempotence stays: a repeat is exit 0, because a script stating what is already true is not wrong. --- cmd/mesh-controller/acts.go | 9 ++++++++- cmd/mesh-controller/adoption.go | 2 +- cmd/mesh-controller/mesh_for_test.go | 2 +- internal/inventory/adoption_test.go | 4 ++-- internal/inventory/catalogue.go | 23 ++++++++++++++++------- internal/inventory/catalogue_test.go | 28 +++++++++++++++++++--------- internal/inventory/forget_test.go | 2 +- internal/inventory/ports_test.go | 4 ++-- internal/inventory/secrets_test.go | 2 +- 9 files changed, 51 insertions(+), 25 deletions(-) diff --git a/cmd/mesh-controller/acts.go b/cmd/mesh-controller/acts.go index c5bae72..9e7ca23 100644 --- a/cmd/mesh-controller/acts.go +++ b/cmd/mesh-controller/acts.go @@ -46,9 +46,16 @@ func assign(ctx context.Context, open *stores, node, module string) (string, err return "", err } defer release() - if err := open.inventory.Assign(ctx, node, module); err != nil { + fresh, err := open.inventory.Assign(ctx, node, module) + if err != nil { return "", err } + if !fresh { + // Nothing changed, and saying "is assigned" would read as an action. One node runs one + // of each — the module's name is the assignment's identity (novox/hq ADR 0115). + return fmt.Sprintf("%s already runs %s — one node runs one of each (ADR 0115); nothing changed", + node, module), nil + } said := fmt.Sprintf("%s is assigned %s", node, module) plan, _, err := planFor(ctx, open, node) if err != nil { diff --git a/cmd/mesh-controller/adoption.go b/cmd/mesh-controller/adoption.go index 9941728..6b9d95b 100644 --- a/cmd/mesh-controller/adoption.go +++ b/cmd/mesh-controller/adoption.go @@ -355,7 +355,7 @@ func converge(ctx context.Context, open *stores, node string, yes bool, digest s strings.Join(assigned, ", ")) } if !slices.Contains(assigned, filter) { - if err := inv.Assign(ctx, node, filter); err != nil { + if _, err := inv.Assign(ctx, node, filter); err != nil { return "", err } if _, _, err := planFor(ctx, open, node); err != nil { diff --git a/cmd/mesh-controller/mesh_for_test.go b/cmd/mesh-controller/mesh_for_test.go index 35f3627..3008697 100644 --- a/cmd/mesh-controller/mesh_for_test.go +++ b/cmd/mesh-controller/mesh_for_test.go @@ -73,7 +73,7 @@ func aMesh(t *testing.T) *stores { if err := open.inventory.RecordOverlayKey(t.Context(), record.ID, aPublicKey(t)); err != nil { t.Fatal(err) } - if err := open.inventory.Assign(t.Context(), name, overlay.Name); err != nil { + if _, err := open.inventory.Assign(t.Context(), name, overlay.Name); err != nil { t.Fatal(err) } } diff --git a/internal/inventory/adoption_test.go b/internal/inventory/adoption_test.go index a0443f5..c779d32 100644 --- a/internal/inventory/adoption_test.go +++ b/internal/inventory/adoption_test.go @@ -65,7 +65,7 @@ func TestTakingIsRefusedOnAConvergedNodeAndForAnUnassignedModule(t *testing.T) { if _, err := inv.AddNode(t.Context(), "converged"); err != nil { t.Fatal(err) } - if err := inv.Assign(t.Context(), "converged", "hello-web"); err != nil { + if _, err := inv.Assign(t.Context(), "converged", "hello-web"); err != nil { t.Fatal(err) } if err := inv.Take(t.Context(), "converged", "hello-web"); !errors.Is(err, ErrNotAdopted) { @@ -91,7 +91,7 @@ func TestATakenModuleOutlivesItsAssignmentAndReturningToAdopted(t *testing.T) { t.Fatal(err) } for _, m := range []string{"hello-web", "postgres"} { - if err := inv.Assign(t.Context(), "anchor", m); err != nil { + if _, err := inv.Assign(t.Context(), "anchor", m); err != nil { t.Fatal(err) } } diff --git a/internal/inventory/catalogue.go b/internal/inventory/catalogue.go index bcf3e55..51eaa02 100644 --- a/internal/inventory/catalogue.go +++ b/internal/inventory/catalogue.go @@ -430,27 +430,36 @@ func (i *Inventory) discard(ctx context.Context, name string) error { return nil } -// Assign puts a module on a node. +// Assign puts a module on a node, and says whether that is new. // // Records the intention and checks nothing. Whether the set of assignments can actually become a // declaration is resolution's question, asked over the whole set at once — and asking it here, // one module at a time, would let an assignment look accepted and then refuse when a second // arrives. -func (i *Inventory) Assign(ctx context.Context, nodeName, module string) error { +// +// **One assignment of a module per node is the rule, not a race lost** (novox/hq ADR 0115). The +// module's name is the assignment's identity — its database user, its broker account, its +// containers and its placed directory are all named by it — so the schema's (node, module) key +// is the decision, and a repeat is absorbed rather than refused. Absorbed audibly: the caller is +// told nothing changed, because "is assigned" printed for a no-op reads as an action. +func (i *Inventory) Assign(ctx context.Context, nodeName, module string) (bool, error) { node, err := i.NodeByName(ctx, nodeName) if err != nil { - return err + return false, err } if err := i.runsSomewhere(ctx, module); err != nil { - return err + return false, err } - _, err = i.store.Pool().Exec(ctx, + tag, err := i.store.Pool().Exec(ctx, `insert into assignment (node, module) values ($1, $2) on conflict do nothing`, node.ID, module) if err != nil && strings.Contains(err.Error(), "assignment_module_fkey") { - return fmt.Errorf("%w: %s", ErrNoSuchModule, module) + return false, fmt.Errorf("%w: %s", ErrNoSuchModule, module) } - return err + if err != nil { + return false, err + } + return tag.RowsAffected() > 0, nil } // Unassign takes a module off a node. diff --git a/internal/inventory/catalogue_test.go b/internal/inventory/catalogue_test.go index 92c61a3..1fd4e9e 100644 --- a/internal/inventory/catalogue_test.go +++ b/internal/inventory/catalogue_test.go @@ -77,7 +77,7 @@ func TestAModuleAMachineIsRunningCannotBeForgotten(t *testing.T) { if err := inv.RegisterModule(t.Context(), manifest("thing", nil, nil), Source{}); err != nil { t.Fatal(err) } - if err := inv.Assign(t.Context(), "laptop", "thing"); err != nil { + if _, err := inv.Assign(t.Context(), "laptop", "thing"); err != nil { t.Fatal(err) } @@ -104,7 +104,7 @@ func TestRemovingANodeTakesItsAssignments(t *testing.T) { if err := inv.RegisterModule(t.Context(), manifest("thing", nil, nil), Source{}); err != nil { t.Fatal(err) } - if err := inv.Assign(t.Context(), "laptop", "thing"); err != nil { + if _, err := inv.Assign(t.Context(), "laptop", "thing"); err != nil { t.Fatal(err) } if _, err := inv.store.Pool().Exec(t.Context(), `delete from node where id = $1`, node.ID); err != nil { @@ -136,14 +136,16 @@ func TestAssigningAModuleTheMeshDoesNotKnowIsRefused(t *testing.T) { if _, err := inv.AddNode(t.Context(), "laptop"); err != nil { t.Fatal(err) } - err := inv.Assign(t.Context(), "laptop", "not-a-module") + _, err := inv.Assign(t.Context(), "laptop", "not-a-module") if !errors.Is(err, ErrNoSuchModule) { t.Fatalf("assigning an unknown module gave %v", err) } } -func TestAssigningTwiceIsNotAnError(t *testing.T) { - // It is a statement of what should be true, and it already is. +func TestAssigningTwiceIsNotAnErrorAndSaysSo(t *testing.T) { + // It is a statement of what should be true, and it already is — one assignment of a module + // per node is the rule (novox/hq ADR 0115), so a repeat is absorbed, audibly: the caller is + // told nothing was new. inv := fresh(t) if _, err := inv.AddNode(t.Context(), "laptop"); err != nil { t.Fatal(err) @@ -151,10 +153,18 @@ func TestAssigningTwiceIsNotAnError(t *testing.T) { if err := inv.RegisterModule(t.Context(), manifest("thing", nil, nil), Source{}); err != nil { t.Fatal(err) } - for i := 0; i < 3; i++ { - if err := inv.Assign(t.Context(), "laptop", "thing"); err != nil { + first, err := inv.Assign(t.Context(), "laptop", "thing") + if err != nil || !first { + t.Fatalf("the first assignment is the new one; got fresh=%v err=%v", first, err) + } + for i := 0; i < 2; i++ { + again, err := inv.Assign(t.Context(), "laptop", "thing") + if err != nil { t.Fatalf("assigning again failed: %v", err) } + if again { + t.Fatal("a repeat must say nothing was new") + } } assigned, err := inv.Assigned(t.Context(), "laptop") if err != nil { @@ -277,7 +287,7 @@ func TestBeingBehindNamesTheMachinesRunningTheOldOne(t *testing.T) { t.Fatal(err) } for _, n := range []string{"laptop", "workstation"} { - if err := inv.Assign(t.Context(), n, "thing"); err != nil { + if _, err := inv.Assign(t.Context(), n, "thing"); err != nil { t.Fatal(err) } } @@ -451,7 +461,7 @@ func TestTheCatalogueSaysWhereEachModuleCameFromAndWhoRunsIt(t *testing.T) { t.Fatal(err) } for _, n := range []string{"workstation", "laptop"} { - if err := inv.Assign(ctx, n, "shell"); err != nil { + if _, err := inv.Assign(ctx, n, "shell"); err != nil { t.Fatal(err) } } diff --git a/internal/inventory/forget_test.go b/internal/inventory/forget_test.go index 5c675a0..75e5a36 100644 --- a/internal/inventory/forget_test.go +++ b/internal/inventory/forget_test.go @@ -212,7 +212,7 @@ func TestAModuleStillAssignedRefusesBeforeAnythingAboutWhatItHolds(t *testing.T) if err := inv.SetSettings(ctx, "anchor", "step-ca", map[string]any{"a": 1}); err != nil { t.Fatal(err) } - if err := inv.Assign(ctx, "anchor", "step-ca"); err != nil { + if _, err := inv.Assign(ctx, "anchor", "step-ca"); err != nil { t.Fatal(err) } for _, forget := range []func() error{ diff --git a/internal/inventory/ports_test.go b/internal/inventory/ports_test.go index 2ac93c8..76f45ab 100644 --- a/internal/inventory/ports_test.go +++ b/internal/inventory/ports_test.go @@ -221,7 +221,7 @@ func TestWhatAMachineNoLongerHoldsIsAvailableAgain(t *testing.T) { func TestUnassigningReleasesTheModulesPorts(t *testing.T) { inv, node := aNodeWithModules(t, "mailu", "other-mail") ctx := t.Context() - if err := inv.Assign(ctx, node, "mailu"); err != nil { + if _, err := inv.Assign(ctx, node, "mailu"); err != nil { t.Fatal(err) } if _, err := inv.PortFor(ctx, node, "mailu", 25, true); err != nil { @@ -230,7 +230,7 @@ func TestUnassigningReleasesTheModulesPorts(t *testing.T) { if err := inv.Unassign(ctx, node, "mailu"); err != nil { t.Fatal(err) } - if err := inv.Assign(ctx, node, "other-mail"); err != nil { + 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 { diff --git a/internal/inventory/secrets_test.go b/internal/inventory/secrets_test.go index 6cbcc69..e5b1610 100644 --- a/internal/inventory/secrets_test.go +++ b/internal/inventory/secrets_test.go @@ -350,7 +350,7 @@ func TestACredentialGoesWhenTheConsumerStopsAskingForIt(t *testing.T) { }, Source{}); err != nil { t.Fatal(err) } - if err := inv.Assign(ctx, "consumer", "meshboard"); err != nil { + if _, err := inv.Assign(ctx, "consumer", "meshboard"); err != nil { t.Fatal(err) } if _, err := inv.SecretFor(ctx, "postgres-database", "consumer", "gitea", "provider", ""); err != nil { -- 2.54.0