From 53a79a17a1c73a95ce820a19c4712ff4fd1e8ea3 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 21 Sep 2026 19:23:02 +0200 Subject: [PATCH] Review: a failure is the same by resource id, not by the host's words; bound files and /run/docker.sock are declared The host's error text may carry a duration or a counter, and a resource looping on it would never have read as stuck. The previous row is read and compared here. Stuck needs a start to say. A container may mount the file a binding lands in; the runtime socket is declared under both of its spellings; the catalogue-wide test takes MESH_CATALOG. --- internal/catalogue/manifest.go | 6 +- internal/catalogue/mounts_test.go | 18 ++++- internal/inventory/doing_test.go | 14 ++++ ...ilure-that-repeats-is-said-to-be-stuck.sql | 5 +- internal/inventory/nodes.go | 80 ++++++++++++++----- 5 files changed, 98 insertions(+), 25 deletions(-) diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 4c82a6f..4976795 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -1098,7 +1098,8 @@ func (m Manifest) MachineSide(port int) (at int, mayAssign bool) { // facilitiesOf is what each capability lets a container mount: paths the machine owns and a module // is granted the use of by declaring the capability, never by declaring them as its own. var facilitiesOf = map[string][]string{ - "container-runtime": {"/var/run/docker.sock"}, + // Both spellings: /var/run is a link to /run on every machine the mesh runs on. + "container-runtime": {"/var/run/docker.sock", "/run/docker.sock"}, } // undeclaredMounts is every bind-mount source no declaration covers — see the check above. @@ -1127,6 +1128,9 @@ func (m Manifest) undeclaredMounts() []string { for _, where := range m.Grants { claim(where) } + for _, where := range m.Binds { + claim(where) + } for _, a := range m.Accesses { claim(a.Path) } diff --git a/internal/catalogue/mounts_test.go b/internal/catalogue/mounts_test.go index 0e323c5..15125cc 100644 --- a/internal/catalogue/mounts_test.go +++ b/internal/catalogue/mounts_test.go @@ -70,9 +70,13 @@ func TestTheRuntimeSocketIsGrantedByTheCapabilityAndNotOtherwise(t *testing.T) { // **Every manifest in the catalogue beside this checkout passes**, so the rule is not one the // catalogue is already breaking. Skipped, aloud, where the catalogue is not there. func TestEveryCatalogueManifestDeclaresWhatItMounts(t *testing.T) { - files, _ := filepath.Glob("../../../mesh-catalog/modules/*/module.json") + root := os.Getenv("MESH_CATALOG") + if root == "" { + root = "../../../mesh-catalog" + } + files, _ := filepath.Glob(filepath.Join(root, "modules", "*", "module.json")) if len(files) == 0 { - t.Skip("the catalogue is not beside this checkout") + t.Skipf("no catalogue at %s (set MESH_CATALOG to a checkout)", root) } for _, file := range files { raw, err := os.ReadFile(file) @@ -84,3 +88,13 @@ func TestEveryCatalogueManifestDeclaresWhatItMounts(t *testing.T) { } } } + +// Where a bound fact lands is the mesh's file too, and a container may mount it directly. +func TestAMountOfABoundFactIsAccepted(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"store","requires":["model-access"],` + + `"binds":{"model-access":"/var/lib/store/model.json"},"resources":[` + + strings.Replace(aContainerMounting, "%s", "/var/lib/store/model.json", 1) + `]}`)) + if err != nil { + t.Fatalf("a mount of the file the mesh writes a binding to was refused: %v", err) + } +} diff --git a/internal/inventory/doing_test.go b/internal/inventory/doing_test.go index e560fbb..55a3552 100644 --- a/internal/inventory/doing_test.go +++ b/internal/inventory/doing_test.go @@ -284,6 +284,20 @@ func TestTheSameFailureReportedAgainIsCountedNotRestarted(t *testing.T) { t.Fatalf("the failure began at %s and the row now says %s", *first.Since, *again.Since) } + // The same resource failing with different words — a duration, a counter — is still the same + // failure: it is the resource that loops, not the sentence. + reworded := Doing{Outcome: OutcomeFailed, Failed: []FailedResource{{ID: "img", Error: "no such image (after 31s)"}}} + if err := inv.RecordDoing(ctx, id, reworded); err != nil { + t.Fatal(err) + } + still, _, err := inv.DoingOf(ctx, "looping") + if err != nil { + t.Fatal(err) + } + if still.Times != StuckAfter+1 || !still.Stuck() { + t.Fatalf("the same resource failing in other words restarted the count: %+v", still) + } + // A different failure is a new situation, not a longer one. other := Doing{Outcome: OutcomeFailed, Failed: []FailedResource{{ID: "svc", Error: "unit not found"}}} if err := inv.RecordDoing(ctx, id, other); err != nil { diff --git a/internal/inventory/migrations/0025-a-failure-that-repeats-is-said-to-be-stuck.sql b/internal/inventory/migrations/0025-a-failure-that-repeats-is-said-to-be-stuck.sql index 266ea5b..f034e77 100644 --- a/internal/inventory/migrations/0025-a-failure-that-repeats-is-said-to-be-stuck.sql +++ b/internal/inventory/migrations/0025-a-failure-that-repeats-is-said-to-be-stuck.sql @@ -7,8 +7,9 @@ -- when its dependency arrives" from "failed identically for ever", and nothing escalated the second. -- -- Still one row per node. What is added is how long the CURRENT failure has been the same one: --- when it first appeared, and how many reports in a row have said it. A report that says something --- different starts the count again; a clean apply clears it. +-- when it first appeared, and how many reports in a row have said it -- the same outcome, the same +-- refusal, the same failed resources by id (not by the host's words, which may carry a duration). +-- A report that says something different starts the count again; a clean apply clears it. alter table node_report add column failing_since timestamptz, diff --git a/internal/inventory/nodes.go b/internal/inventory/nodes.go index 6729cb5..8cb1ccb 100644 --- a/internal/inventory/nodes.go +++ b/internal/inventory/nodes.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "sort" "strings" "time" @@ -468,7 +469,7 @@ type Doing struct { Declared string // Since is when the machine first reported THIS failure — the same outcome, the same refusal, - // the same failed resources — and Times is how many reports in a row have said it. A node + // the same failed resources by id — and Times is how many reports in a row have said it. A node // re-applies on a steady interval and reports each time (novox/hq ADR 0010), so a failure // that will never succeed arrives as the same report over and over, indistinguishable from // one that just happened until somebody counts (novox/hq 04-ISSUES/065). Nil and zero for a @@ -496,7 +497,32 @@ const StuckAfter = 3 // Stuck reports whether this machine has been failing the same way for long enough that waiting // is no longer a plan. The failure is still the host's own words; this only says it is not new. -func (d Doing) Stuck() bool { return d.Wrong() && d.Times >= StuckAfter } +func (d Doing) Stuck() bool { return d.Wrong() && d.Times >= StuckAfter && d.Since != nil } + +// sameFailure is whether two reports describe one failure: the same outcome, the same refusal, and +// the same failed resources BY ID. Not by the host's words: an error that carries a duration, a +// counter or a temporary path would read as new on every report, and the resource looping on it — +// which is what stuck is for — would never be said to be (novox/hq 04-ISSUES/065). +func sameFailure(a, b Doing) bool { + if a.Outcome != b.Outcome || a.Refused != b.Refused || len(a.Failed) != len(b.Failed) { + return false + } + ids := func(d Doing) []string { + out := make([]string, 0, len(d.Failed)) + for _, f := range d.Failed { + out = append(out, f.ID) + } + sort.Strings(out) + return out + } + x, y := ids(a), ids(b) + for i := range x { + if x[i] != y[i] { + return false + } + } + return true +} // RecordDoing keeps what a node said it did. // @@ -505,35 +531,49 @@ func (d Doing) Stuck() bool { return d.Wrong() && d.Times >= StuckAfter } // would bury the ones that matter. // // **What the row also keeps is whether this failure is the one before.** The same outcome, the -// same refusal, the same failed resources with the same words: then the failure did not just -// happen, it is still happening, and the row keeps when it began and counts one more report. Any -// difference starts again — a machine failing on a new resource is a new situation, not a longer -// one — and a clean apply clears both (novox/hq 04-ISSUES/065). +// same refusal, the same failed resources by id: then the failure did not just happen, it is +// still happening, and the row keeps when it began and counts one more report. Any difference +// starts again — a machine failing on a new resource is a new situation, not a longer one — and +// a clean apply clears both (novox/hq 04-ISSUES/065). The previous row is read first and the +// comparison made here, so "the same" is a rule this package states rather than a jsonb equality +// that would restart the count on a changed word in an error. func (i *Inventory) RecordDoing(ctx context.Context, node string, d Doing) error { failed, err := json.Marshal(d.Failed) if err != nil { return err } + var since *time.Time + times := 0 + if d.Outcome != OutcomeApplied { + var before Doing + var beforeFailed []byte + err := i.store.Pool().QueryRow(ctx, + `select outcome, refused, failed, failing_since, failures from node_report where node = $1`, + node).Scan(&before.Outcome, &before.Refused, &beforeFailed, &before.Since, &before.Times) + switch { + case errors.Is(err, pgx.ErrNoRows): + case err != nil: + return err + default: + if err := json.Unmarshal(beforeFailed, &before.Failed); err != nil { + return err + } + } + now := time.Now() + since, times = &now, 1 + if err == nil && sameFailure(before, d) && before.Since != nil { + since, times = before.Since, before.Times+1 + } + } _, err = i.store.Pool().Exec(ctx, `insert into node_report (node, outcome, refused, failed, applied, at, declared, failing_since, failures) - values ($1, $2, $3, $4, $5, now(), $6, - case when $2 <> $7 then now() end, - case when $2 <> $7 then 1 else 0 end) + values ($1, $2, $3, $4, $5, now(), $6, $7, $8) on conflict (node) do update set outcome = excluded.outcome, refused = excluded.refused, failed = excluded.failed, applied = excluded.applied, at = excluded.at, declared = excluded.declared, - failing_since = case - when excluded.outcome = $7 then null - when node_report.outcome = excluded.outcome and node_report.refused = excluded.refused - and node_report.failed = excluded.failed then node_report.failing_since - else excluded.at end, - failures = case - when excluded.outcome = $7 then 0 - when node_report.outcome = excluded.outcome and node_report.refused = excluded.refused - and node_report.failed = excluded.failed then node_report.failures + 1 - else 1 end`, - node, d.Outcome, d.Refused, failed, d.Applied, d.Declared, OutcomeApplied) + failing_since = excluded.failing_since, failures = excluded.failures`, + node, d.Outcome, d.Refused, failed, d.Applied, d.Declared, since, times) return err }