Merge pull request 'An apply says what it held, not only what it applied' (#52) from fix/125-a-hold-is-a-line-in-the-report into main

internal/link/held_note_test.go

@@ -0,0 +1,46 @@
+package link
+
+import (
+	"strings"
+	"testing"
+)
+
+// The count that did not add up was the only symptom sixteen held resources had, and reading it meant
+// opening the node's state file by hand (novox/hq 04-ISSUES/125). The line that says what an apply did
+// says what it did not, too.
+
+func TestTheApplyLineSaysWhatItHeldAndForWhichModule(t *testing.T) {
+	got := heldNote([]Held{
+		{ID: "ca", Module: "route-proxy", Kind: "directory"},
+		{ID: "certs", Module: "route-proxy", Kind: "directory"},
+		{ID: "server", Module: "route-proxy", Kind: "container"},
+		{ID: "mail", Module: "mailu", Kind: "container"},
+	})
+	if !strings.Contains(got, "4 held") {
+		t.Fatalf("the count of what was held is not in the line: %q", got)
+	}
+	// The module is the thing an operator can act on: `take` takes a module.
+	if !strings.Contains(got, "route-proxy: 3") || !strings.Contains(got, "mailu: 1") {
+		t.Fatalf("the line does not break the holds down by module: %q", got)
+	}
+	// Ordered, so two machines holding the same things read the same and a diff of two reports is
+	// about what changed.
+	if strings.Index(got, "mailu") > strings.Index(got, "route-proxy") {
+		t.Fatalf("modules are not in a stated order: %q", got)
+	}
+	// It says why, because "held" alone reads as a failure and this is correct behaviour.
+	if !strings.Contains(got, "taken") {
+		t.Fatalf("the line does not say a hold ends when the module is taken: %q", got)
+	}
+}
+
+func TestAnApplyThatHeldNothingSaysNothingExtra(t *testing.T) {
+	// A converged machine holds nothing, which is most applies. Reporting "0 held" on every one of
+	// them is how a line stops being read.
+	if got := heldNote(nil); got != "" {
+		t.Fatalf("an apply with no holds added %q to its line", got)
+	}
+	if got := heldNote([]Held{}); got != "" {
+		t.Fatalf("an apply with no holds added %q to its line", got)
+	}
+}

internal/link/run.go

@@ -6,6 +6,8 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"sort"
+	"strings"
 	"time"
 )
 
@@ -250,9 +252,11 @@ func Run(ctx context.Context, m Membership, apply Applier, say Announce, timeout
 			case report.Refused != "":
 				say("refused a declaration: " + report.Refused)
 			case len(report.Failed) > 0:
-				say(fmt.Sprintf("applied %d and failed: %v", len(report.Applied), report.Failed))
+				say(fmt.Sprintf("applied %d and failed: %v%s",
+					len(report.Applied), report.Failed, heldNote(report.Held)))
 			default:
-				say(fmt.Sprintf("applied %d resource(s)", len(report.Applied)))
+				say(fmt.Sprintf("applied %d resource(s)%s",
+					len(report.Applied), heldNote(report.Held)))
 			}
 			publishReport(ctx, link, m, report, say, timeout)
 			// Settled after the report is published. A node that dies between applying and
@@ -392,3 +396,36 @@ func publishAlive(ctx context.Context, bus Bus, m Membership, say Announce,
 		say("could not tell the mesh this node is here: " + err.Error())
 	}
 }
+
+// heldNote is what this apply did NOT do, for the line that says what it did.
+//
+// **A count that does not add up is the only symptom a held resource had** (novox/hq 04-ISSUES/125).
+// An adopted node keeps what it found until its module is taken (ADR 0100), and that is correct — but
+// it was recorded only in the node's own state file. On the edge cut-over the mesh sent 346 resources,
+// the journal said it applied 330, and nothing anywhere said which sixteen or why. Reading it took
+// opening state.json by hand; not reading it took every public name on the machine down, because the
+// operator had four green surfaces and a discrepancy nobody could interpret.
+//
+// So the line that reports the apply carries it. Grouped by module and ordered by name, because the
+// sentence an operator needs is "route-proxy is assigned and not taken", and the module is the thing
+// they can act on — `take` is the verb, and it takes a module.
+func heldNote(held []Held) string {
+	if len(held) == 0 {
+		return ""
+	}
+	byModule := map[string]int{}
+	for _, h := range held {
+		byModule[h.Module]++
+	}
+	names := make([]string, 0, len(byModule))
+	for name := range byModule {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	parts := make([]string, 0, len(names))
+	for _, name := range names {
+		parts = append(parts, fmt.Sprintf("%s: %d", name, byModule[name]))
+	}
+	return fmt.Sprintf(", %d held until their module is taken (%s)",
+		len(held), strings.Join(parts, ", "))
+}