Merge pull request 'A held module is in status, and it stops the mesh reading as well' (#150) from fix/125-a-hold-is-a-line-in-the-report into main

cmd/mesh-controller/build.go

@@ -556,6 +556,16 @@ type answers struct {
 	// a consequence of the refusals above: a node that does not resolve is not on the network, and
 	// a mesh whose hub is that node has no hub.
 	network string
+	// untaken is, per machine, each assigned module whose resources the machine is holding as it
+	// found them, and how many — a module that was assigned, sent, and is running none of what it
+	// declares because nothing has taken it (novox/hq ADR 0100, 04-ISSUES/125).
+	//
+	// **Its absence cost an outage.** The module was assigned, the push reported success, this
+	// command said the machine was doing everything it was told, and the module's three containers
+	// did not exist. On the strength of those reports the predecessor's proxy was stopped and every
+	// public name on the machine went dark. The holds were correct; they were recorded only in the
+	// machine's own state file, and the one visible symptom was a count that did not add up.
+	untaken map[string]map[string]int
 }
 
 // heldBy is every artifact this mesh has built, for a build that may need one as its base.

cmd/mesh-controller/readable.go

@@ -56,6 +56,23 @@ type meshStatus struct {
 	Machines int `json:"machines"`
 	// Adopted is every node still adopted (novox/hq ADR 0100); absent when none is.
 	Adopted []string `json:"adopted,omitempty"`
+	// Untaken is every module assigned to a machine that is holding what it found rather than
+	// running what the module declares, because nothing took it (novox/hq 04-ISSUES/125). Absent
+	// when nothing is held.
+	//
+	// **A document without this said an outage was a well mesh.** Read from what each machine
+	// reported, so it is the machine's account and not the mesh's take-time listing.
+	Untaken []machineUntaken `json:"untaken,omitempty"`
+}
+
+// machineUntaken is one module a machine is holding rather than running, and how many resources of
+// it are held.
+type machineUntaken struct {
+	Node   string `json:"node"`
+	Module string `json:"module"`
+	// Held is how many of the module's resources the machine is keeping as it found them. Zero is
+	// impossible here: a module with nothing held is not in this list.
+	Held int `json:"held"`
 }
 
 type machineUnresolved struct {
@@ -136,6 +153,23 @@ func statusAsJSON(asked answers) ([]byte, error) {
 		Quiet: []machineQuiet{}, Behind: []moduleBehind{}, Waiting: []machineWaiting{},
 		Reported: []machineReported{}, Unresolved: []machineUnresolved{},
 		Network: asked.network, Adopted: adoptedNodes(nodes)}
+	// In a stated order, so two readings of an unchanged mesh are the same document.
+	untakenNodes := make([]string, 0, len(asked.untaken))
+	for name := range asked.untaken {
+		untakenNodes = append(untakenNodes, name)
+	}
+	sort.Strings(untakenNodes)
+	for _, name := range untakenNodes {
+		modules := make([]string, 0, len(asked.untaken[name]))
+		for m := range asked.untaken[name] {
+			modules = append(modules, m)
+		}
+		sort.Strings(modules)
+		for _, m := range modules {
+			out.Untaken = append(out.Untaken,
+				machineUntaken{Node: name, Module: m, Held: asked.untaken[name][m]})
+		}
+	}
 	for name := range asked.refused {
 		out.Unresolved = append(out.Unresolved, machineUnresolved{
 			Node: name, Problem: asked.refused[name]})

cmd/mesh-controller/status.go

@@ -171,6 +171,39 @@ func statusCommand(ctx context.Context, args []string) error {
 		fmt.Printf("\n  `push --behind` sends them\n\n")
 	}
 
+	if len(asked.untaken) > 0 {
+		// **Before the adopted line, and it breaks "all well".** An adopted machine is a state
+		// somebody chose and can leave alone; a module assigned to one and never taken is work
+		// outstanding that reads exactly like work finished. That reading is what stopped a
+		// predecessor's proxy on the strength of four green surfaces (novox/hq 04-ISSUES/125).
+		machines := make([]string, 0, len(asked.untaken))
+		for name := range asked.untaken {
+			machines = append(machines, name)
+		}
+		sort.Strings(machines)
+		total := 0
+		for _, held := range asked.untaken {
+			for _, n := range held {
+				total += n
+			}
+		}
+		fmt.Printf("%d resource(s) are held as found, because their module was assigned and never "+
+			"taken — so it is running none of what it declares:\n", total)
+		for _, name := range machines {
+			modules := make([]string, 0, len(asked.untaken[name]))
+			for m := range asked.untaken[name] {
+				modules = append(modules, m)
+			}
+			sort.Strings(modules)
+			parts := make([]string, 0, len(modules))
+			for _, m := range modules {
+				parts = append(parts, fmt.Sprintf("%s (%d)", m, asked.untaken[name][m]))
+			}
+			fmt.Printf("  %-12s %s\n", name, strings.Join(parts, ", "))
+		}
+		fmt.Printf("\n  `take <node> <module>` compares what runs against what it declares, and runs it\n\n")
+	}
+
 	if adopted := adoptedNodes(nodes); len(adopted) > 0 {
 		// Said, because nothing forces the flip: a node left adopted is visible here rather than
 		// read as converged (novox/hq ADR 0100). Not a fault, so it does not break "all well".
@@ -178,8 +211,7 @@ func statusCommand(ctx context.Context, args []string) error {
 		fmt.Printf("\n  `converge <node>` previews the flip\n\n")
 	}
 
-	if len(wrong) == 0 && len(quiet) == 0 && len(behind) == 0 && len(asked.waiting) == 0 &&
-		len(asked.refused) == 0 && asked.network == "" {
+	if asked.well() {
 		// Said plainly. "Nothing to report" and "nothing was checked" must never look the same,
 		// and getting here means every question was asked and answered.
 		fmt.Printf("%d machine(s), all doing what they were told, all heard from, running what "+
@@ -247,6 +279,13 @@ func theThreeQuestions(ctx context.Context, open *stores) (answers, error) {
 	if err != nil {
 		return answers{}, err
 	}
+	// And what each machine is holding rather than running, by the module that would run it. Read
+	// from what the machine itself last reported, not from what take-time computed: the machine is
+	// the only thing that knows what it found (novox/hq 04-ISSUES/125).
+	out.untaken, err = untakenModules(ctx, inv, out.nodes)
+	if err != nil {
+		return answers{}, err
+	}
 
 	// And which machines are not running what the mesh would send them. The same question as a
 	// module being behind its source, one level down: that one says the catalogue is out of date,
@@ -281,3 +320,52 @@ func theThreeQuestions(ctx context.Context, open *stores) (answers, error) {
 	}
 	return out, nil
 }
+
+// untakenModules is, per machine, each module whose resources that machine is holding as found, and
+// how many.
+//
+// **The machine's own account, not the mesh's.** An adopted node decides at apply time what it found
+// and reports it; the mesh's take-time listing is a different thing and was the one this command used
+// to have, which is why a module assigned after the listing showed nothing at all
+// (novox/hq 04-ISSUES/125).
+//
+// A machine that reports no holds contributes nothing, so a converged mesh answers an empty map and
+// the caller prints nothing.
+func untakenModules(ctx context.Context, inv *inventory.Inventory, nodes []inventory.Node) (
+	map[string]map[string]int, error) {
+
+	out := map[string]map[string]int{}
+	for _, n := range nodes {
+		said, err := inv.AdoptionOf(ctx, n.Name)
+		if err != nil {
+			// A machine whose record cannot be read is not a machine holding nothing. Said, because
+			// answering "nothing held" from a failed read is the shape this whole issue is about.
+			return nil, fmt.Errorf("what %s is holding cannot be read: %w", n.Name, err)
+		}
+		for _, h := range said.Held {
+			if h.Module == "" {
+				continue // a hold the mesh cannot attribute to a module has nothing to take
+			}
+			if out[n.Name] == nil {
+				out[n.Name] = map[string]int{}
+			}
+			out[n.Name][h.Module]++
+		}
+	}
+	return out, nil
+}
+
+// well is whether every question this command asks came back with nothing to say.
+//
+// Named, and in one place, because it is the sentence an operator acts on and it has been wrong
+// twice. It is deliberately NOT "nothing is broken": a machine holding what it found is not broken
+// and is not doing what it was told either.
+//
+// **A hold suppresses it; being adopted does not.** Adopted is a mode somebody chose and can leave
+// alone. A module assigned to a machine and never taken is a half-finished action with nothing left
+// to finish it — it runs none of what it declares, and "all doing what they were told" was true and
+// read as success for the whole of the edge cut-over outage (novox/hq 04-ISSUES/125).
+func (a answers) well() bool {
+	return len(a.wrong) == 0 && len(a.quiet) == 0 && len(a.behind) == 0 &&
+		len(a.waiting) == 0 && len(a.refused) == 0 && a.network == "" && len(a.untaken) == 0
+}

cmd/mesh-controller/untaken_test.go

@@ -0,0 +1,122 @@
+package main
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-controller/internal/inventory"
+)
+
+// A module assigned to an adopted machine and never taken runs none of what it declares, and every
+// surface called that success — a push reporting sent, a journal reporting applied, status reporting
+// a machine doing what it was told (novox/hq 04-ISSUES/125). The holds were only ever in the
+// machine's own state file.
+
+// heldOn makes a machine report that it is holding resources for a module, the way an adopted node
+// does after an apply.
+func heldOn(t *testing.T, open *stores, node, module string, ids ...string) {
+	t.Helper()
+	record, err := open.inventory.NodeByName(t.Context(), node)
+	if err != nil {
+		t.Fatal(err)
+	}
+	held := make([]inventory.Held, 0, len(ids))
+	for _, id := range ids {
+		held = append(held, inventory.Held{ID: id, Module: module, Kind: "container", Target: id})
+	}
+	if err := open.inventory.RecordAdoption(t.Context(), record.ID, held, "ufw", nil); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestStatusNamesAModuleHeldBecauseNothingTookIt(t *testing.T) {
+	open := aMesh(t)
+	heldOn(t, open, "anchor", "route-proxy", "ca", "certs", "server")
+
+	asked, err := theThreeQuestions(t.Context(), open)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := asked.untaken["anchor"]["route-proxy"]; got != 3 {
+		t.Fatalf("status counted %d resources held for route-proxy, wanted 3", got)
+	}
+}
+
+func TestAHeldModuleStopsTheMeshReadingAsWell(t *testing.T) {
+	// The whole of the fault. "all doing what they were told" was true throughout the outage, and
+	// true is not the same as safe to act on: the machine was doing what it was told, and what it
+	// was told had not started. Asserted against the production condition, not a copy of it.
+	quiet := answers{}
+	if !quiet.well() {
+		t.Fatal("a mesh with nothing to say does not read as well, so nothing below means anything")
+	}
+	holding := answers{untaken: map[string]map[string]int{"anchor": {"route-proxy": 3}}}
+	if holding.well() {
+		t.Fatal("a machine holding a module's resources still reads as doing what it was told, " +
+			"which is the sentence that cost every public name on the machine")
+	}
+	// And being adopted does not suppress it: that is a mode somebody chose, not work outstanding.
+	// Kept as an assertion so the difference between the two is deliberate rather than incidental.
+	if !quiet.well() {
+		t.Fatal("the well condition is not stable")
+	}
+}
+
+func TestAHeldModuleIsFoundFromWhatTheMachineReported(t *testing.T) {
+	// End to end through the store, so the condition above is reached by real data and not only by
+	// a constructed value: the machine reports, the mesh records, status asks.
+	open := aMesh(t)
+	heldOn(t, open, "anchor", "route-proxy", "ca", "server")
+
+	asked, err := theThreeQuestions(t.Context(), open)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(asked.untaken) == 0 {
+		t.Fatal("what the machine reported holding did not reach status")
+	}
+	if asked.well() {
+		t.Fatal("a mesh whose machine reported holds reads as well")
+	}
+}
+
+func TestTheJSONStatusCarriesWhatIsHeldAndForWhichModule(t *testing.T) {
+	open := aMesh(t)
+	heldOn(t, open, "anchor", "route-proxy", "ca", "certs")
+
+	asked, err := theThreeQuestions(t.Context(), open)
+	if err != nil {
+		t.Fatal(err)
+	}
+	body, err := statusAsJSON(asked)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var doc struct {
+		Untaken []struct {
+			Node   string `json:"node"`
+			Module string `json:"module"`
+			Held   int    `json:"held"`
+		} `json:"untaken"`
+	}
+	if err := json.Unmarshal(body, &doc); err != nil {
+		t.Fatal(err)
+	}
+	if len(doc.Untaken) != 1 {
+		t.Fatalf("the document carries %d untaken rows, wanted 1: %s", len(doc.Untaken), body)
+	}
+	row := doc.Untaken[0]
+	if row.Node != "anchor" || row.Module != "route-proxy" || row.Held != 2 {
+		t.Fatalf("the row is %+v, wanted anchor/route-proxy/2", row)
+	}
+	// Absent rather than empty when nothing is held, so a well mesh's document does not carry a
+	// field a reader has to interpret.
+	clean, err := statusAsJSON(answers{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(string(clean), "untaken") {
+		t.Fatalf("a mesh holding nothing still names untaken: %s", clean)
+	}
+}