diff --git a/modules/disk-load/cmd/disk-load/cpu.go b/modules/disk-load/cmd/disk-load/cpu.go index d5d3122f..efad5cb6 100644 --- a/modules/disk-load/cmd/disk-load/cpu.go +++ b/modules/disk-load/cmd/disk-load/cpu.go @@ -43,11 +43,15 @@ func (m *Machine) unitCPU() map[string]cgroupCPU { return out } -// MachineCPU is the machine's CPU in use over the window, from /proc/stat's cpu line. +// MachineCPU is the machine's CPU in use over the window, from /proc/stat's cpu line. CPUsBusy is +// rounded only for display; raw, unexported, is the same count unrounded, for topCPU's share and +// unaccounted math (review round 1 of novox/hq issue 741: a small busy time that rounds CPUsBusy to +// 0.0 must not zero out every unit's share along with it). type MachineCPU struct { CPUs int `json:"cpus"` BusyPct float64 `json:"busy_pct"` // % of all CPUs together - CPUsBusy float64 `json:"cpus_busy"` // as a count of CPUs, e.g. 7.5 of 12 + CPUsBusy float64 `json:"cpus_busy"` // as a count of CPUs, e.g. 7.5 of 12, rounded for display + raw float64 } // machineCPUOver is the machine's CPU in use between two readings of /proc/stat: everything but idle @@ -62,13 +66,17 @@ func machineCPUOver(a, b cpuTimes) MachineCPU { if busy < 0 { busy = 0 } - pct := round1(busy / total * 100) - return MachineCPU{CPUs: b.cpus, BusyPct: pct, CPUsBusy: round1(pct / 100 * float64(b.cpus))} + raw := busy / total * float64(b.cpus) + return MachineCPU{CPUs: b.cpus, BusyPct: round1(busy / total * 100), CPUsBusy: round1(raw), raw: raw} } // CPUUsage is a unit's or a container's share of CPU between two readings of its cgroup's cpu.stat. +// Cgroup is its cgroup's path, relative to /sys/fs/cgroup, said beside Unit always (review round 1 of +// novox/hq issue 741): two cgroups can share a leaf name (an outer "init.scope" and a user session's +// "user.slice/user@1000.service/init.scope"), and the path is how they are told apart. type CPUUsage struct { Unit string `json:"unit,omitempty"` + Cgroup string `json:"cgroup,omitempty"` Container string `json:"container,omitempty"` CPUPct float64 `json:"cpu_pct"` // % of one CPU: 100 = one CPU busy the whole window SharePct float64 `json:"share_pct"` // this unit's share of the machine's busy time @@ -82,8 +90,8 @@ type CPUUsage struct { func (m *Machine) topCPU(a, b map[string]cgroupCPU, window time.Duration, by string, limit int, cpusBusy float64) ([]CPUUsage, float64) { windowUsec := window.Seconds() * 1e6 type entry struct { - unit, container string - pct float64 + path, unit, container string + pct float64 } var entries []entry var names map[string]string @@ -110,12 +118,16 @@ func (m *Machine) topCPU(a, b map[string]cgroupCPU, window time.Duration, by str container = n } } - entries = append(entries, entry{unit: unit, container: container, pct: pct}) + entries = append(entries, entry{path: path, unit: unit, container: container, pct: pct}) } + // Units are keyed by their cgroup's own path, never by its leaf name (filepath.Base): two cgroups can + // share a leaf name (an outer "init.scope" and a user session's nested one), and keying by the name + // alone would merge them into one row (review round 1 of novox/hq issue 741). Containers stay keyed + // by their name, as before. groups := map[string]*CPUUsage{} for _, e := range entries { - key := e.unit + key := e.path if by == "container" { if e.container == "" { continue @@ -124,9 +136,9 @@ func (m *Machine) topCPU(a, b map[string]cgroupCPU, window time.Duration, by str } g, ok := groups[key] if !ok { - g = &CPUUsage{Unit: e.unit, Container: e.container} + g = &CPUUsage{Unit: e.unit, Cgroup: e.path, Container: e.container} if by == "container" { - g.Unit = "" + g.Unit, g.Cgroup = "", "" } groups[key] = g } @@ -140,7 +152,7 @@ func (m *Machine) topCPU(a, b map[string]cgroupCPU, window time.Duration, by str if out[i].raw != out[j].raw { return out[i].raw > out[j].raw } - return out[i].Unit+out[i].Container < out[j].Unit+out[j].Container + return out[i].Unit+out[i].Cgroup+out[i].Container < out[j].Unit+out[j].Cgroup+out[j].Container }) if len(out) > limit { out = out[:limit] diff --git a/modules/disk-load/cmd/disk-load/cpu_test.go b/modules/disk-load/cmd/disk-load/cpu_test.go index f0886fb8..5acc362a 100644 --- a/modules/disk-load/cmd/disk-load/cpu_test.go +++ b/modules/disk-load/cmd/disk-load/cpu_test.go @@ -105,3 +105,78 @@ func TestCPUTopLeavesOutACgroupThatAppearsOrVanishesWithinTheWindow(t *testing.T t.Errorf("the container runtime was never asked: %v", f.calls) } } + +// aTinyLoad is a machine with 12 CPUs, only 0.04 of them busy over a 2 s window (busy 1 of 300 total +// ticks): rounded to one decimal for display, CPUsBusy reads 0.0, but a unit using half of that busy +// time still has a non-zero share — review round 1's decision that share_pct and unaccounted_pct are +// computed from the unrounded busy time, never from the rounded CPUsBusy. +func aTinyLoad(t *testing.T) *fake { + f := newFake(t) + cpus := "cpu0 0 0 0 0 0 0 0 0 0 0\ncpu1 0 0 0 0 0 0 0 0 0 0\ncpu2 0 0 0 0 0 0 0 0 0 0\ncpu3 0 0 0 0 0 0 0 0 0 0\n" + + "cpu4 0 0 0 0 0 0 0 0 0 0\ncpu5 0 0 0 0 0 0 0 0 0 0\ncpu6 0 0 0 0 0 0 0 0 0 0\ncpu7 0 0 0 0 0 0 0 0 0 0\n" + + "cpu8 0 0 0 0 0 0 0 0 0 0\ncpu9 0 0 0 0 0 0 0 0 0 0\ncpu10 0 0 0 0 0 0 0 0 0 0\ncpu11 0 0 0 0 0 0 0 0 0 0\n" + f.file("/proc/stat", "cpu 0 0 0 0 0 0 0 0 0 0\n"+cpus) + f.then("/proc/stat", "cpu 1 0 0 299 0 0 0 0 0 0\n"+cpus) + + f.file("/sys/fs/cgroup/system.slice/tiny.service/cpu.stat", "usage_usec 0\n") + f.then("/sys/fs/cgroup/system.slice/tiny.service/cpu.stat", "usage_usec 60000\n") // 3 % of one CPU over 2 s + return f +} + +func TestCPUTopSharesAreNonZeroEvenWhenRoundedCPUsBusyIsZero(t *testing.T) { + out, err := tool(t, aTinyLoad(t).machine(), "cpu_top").Run(map[string]any{"seconds": 2.0}) + if err != nil { + t.Fatal(err) + } + m := out.(map[string]any) + cpu := m["cpu"].(MachineCPU) + if cpu.CPUs != 12 || cpu.CPUsBusy != 0 { + t.Fatalf("the machine 0.04 CPUs busy, rounded for display: %+v", cpu) + } + top := m["top"].([]CPUUsage) + if len(top) != 1 || top[0].Unit != "tiny.service" || top[0].CPUPct != 3 || top[0].SharePct != 75 { + t.Fatalf("a non-zero share from the unrounded busy time: %+v", top) + } + if m["unaccounted_pct"] != 25.0 { + t.Fatalf("unaccounted from the unrounded busy time: %+v (want 25, so shares plus unaccounted add to 100)", + m["unaccounted_pct"]) + } +} + +// TestCPUTopKeepsCgroupsWithTheSameUnitNameAsSeparateRows is review round 1's other decision: units are keyed +// by their cgroup path, never by filepath.Base, so two cgroups that both end in "init.scope" — one at the +// root, one under a user's session — are two rows, not one merged by their shared leaf name. +func TestCPUTopKeepsCgroupsWithTheSameUnitNameAsSeparateRows(t *testing.T) { + f := aCPUHog(t) + cg := func(path string, usec int) { + f.file("/sys/fs/cgroup/"+path+"/cpu.stat", "usage_usec 0\n") + f.then("/sys/fs/cgroup/"+path+"/cpu.stat", "usage_usec "+itoa(usec)+"\n") + } + cg("init.scope", 1000000) // the root init, 50 % of one CPU over 2 s + cg("user.slice/user@1000.service/init.scope", 400000) // a user session's init, 20 % + + out, err := tool(t, f.machine(), "cpu_top").Run(map[string]any{"seconds": 2.0, "limit": 10.0}) + if err != nil { + t.Fatal(err) + } + top := out.(map[string]any)["top"].([]CPUUsage) + var inits []CPUUsage + for _, u := range top { + if u.Unit == "init.scope" { + inits = append(inits, u) + } + } + if len(inits) != 2 { + t.Fatalf("two cgroups named init.scope, merged into %d row(s): %+v", len(inits), top) + } + byCgroup := map[string]CPUUsage{} + for _, u := range inits { + byCgroup[u.Cgroup] = u + } + if u, ok := byCgroup["init.scope"]; !ok || u.CPUPct != 50 { + t.Errorf("the root init.scope: %+v", byCgroup) + } + if u, ok := byCgroup["user.slice/user@1000.service/init.scope"]; !ok || u.CPUPct != 20 { + t.Errorf("the user session's init.scope: %+v", byCgroup) + } +} diff --git a/modules/disk-load/cmd/disk-load/tools.go b/modules/disk-load/cmd/disk-load/tools.go index 0d9c48c5..ed29c9c3 100644 --- a/modules/disk-load/cmd/disk-load/tools.go +++ b/modules/disk-load/cmd/disk-load/tools.go @@ -126,7 +126,7 @@ func Tools(m *Machine) []stdio.Tool { } b := m.unitCPU() machine := machineCPUOver(ac, bc) - top, unaccounted := m.topCPU(a, b, w, by, limit, machine.CPUsBusy) + top, unaccounted := m.topCPU(a, b, w, by, limit, machine.raw) return map[string]any{ "window_s": w.Seconds(), "by": by, diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index d1dd7461..8cc6e8f7 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -524,15 +524,23 @@ func (h *Holder) PullMerged(p PullEvent) { if over := startedOverFor(d, p.Number); over != "" { // A delivery stopped only by an earlier, closed duplicate pull request with the same head is not lost when // its merge is heard without the open announcement either: it is started over for the merging pull request - // the same way an announcement would, before the merge is taken as usual (novox/hq issue 783). + // the same way an announcement would, before the merge is taken as usual (novox/hq issue 783). Heard this + // way — its own merge, with no open announcement of this pull request — it is made exactly as a first-heard + // merge in every field but its id: FromForge, and its check the forge's word on the head, so a long-past + // merge replayed from the bus's history still falls under the history rule (issue 313) and is not held for + // a person (review round 1). fresh, t, err := h.freshAnnouncement(p, now, over) if err != nil { h.refuse(err) return } + fresh.FromForge = true + if s := p.HeadChecks["mesh/merge-gate"]; s != "" { + fresh.Check = &Verdict{At: now, Gate: verdictOfState(s), Summary: "the head's mesh/merge-gate, as the forge holds it", + Repo: verdictOfState(p.HeadChecks["mesh/repo-check"])} + } d, ts = fresh, []Transition{t} - if v := decidedOnHead(p.HeadChecks, now); v != nil { - d.Check = v + if d.Check != nil { ts = append(ts, settle(d, Facts{Now: now})...) } } else if d == nil { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go index c8b11518..c0dd6062 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue783_test.go @@ -3,6 +3,7 @@ package main import ( "strings" "testing" + "time" ) // novox/hq issue 783: mesh-catalog commit b5417c28 was pushed while #305 (from branch adr-359-mesh-mail-733) was @@ -20,6 +21,7 @@ const ( head783, merge783 = "d305d305d305d305", "d315d315d315d315" head783b, merge783b = "d306d306d306d306", "d316d316d316d316" head783c = "d307d307d307d307" + head783d, merge783d = "d308d308d308d308", "d318d318d318d318" catalog783 = "novox/mesh-catalog" branch783 = "adr-359-mesh-mail-733" ) @@ -113,3 +115,37 @@ func TestAStoppedDeliveryAnnouncedAgainByItsOwnPullRequestStaysStopped(t *testin t.Fatalf("#307 announced again by itself left it %s, Number %d; want it to stay stopped", d.State, d.Number) } } + +// TestAMergeHeardLongAfterStartingOverAStoppedDuplicateIsHistoryLikeAnyOtherReplayedMerge is review round 1's +// decision: a delivery started over from a merge heard without its open announcement (as in decision 4, above) +// takes FromForge exactly as a first-heard merge does, so one replayed from the bus's history long after it +// happened still falls under the history rule (novox/hq issue 313) and is never held for a person. +func TestAMergeHeardLongAfterStartingOverAStoppedDuplicateIsHistoryLikeAnyOtherReplayedMerge(t *testing.T) { + w := newWorld(t) + id := IDOf(catalog783, head783d) + + w.h.PullUpdated(pr(catalog783, 308, head783d, branch783+"-d", "src/d.go")) + w.h.PullClosed(pr(catalog783, 308, head783d, branch783+"-d")) + if w.state(id) != Stopped { + t.Fatalf("#308 closed unmerged left it %s; want stopped", w.state(id)) + } + + m := merged(catalog783, 318, head783d, merge783d) + m.MergedAt = w.now.Add(-historyLate - time.Minute).UTC().Format(time.RFC3339) + w.h.PullMerged(m) + + d := w.delivery(id) + if d.State == Stopped || d.Number != 318 { + t.Fatalf("#318's merge, heard long after it happened with no announcement of its own, left it %s, Number %d; "+ + "want started over for #318", d.State, d.Number) + } + if !d.FromForge { + t.Fatal("a merge heard without its own announcement is made from the forge's word: FromForge is false") + } + if d.History == "" { + t.Fatalf("a merge heard %s after it happened is history, never held for a person; History is empty", historyLate+time.Minute) + } + if d.State == Held { + t.Fatalf("history is never held for a person; state is %s, HeldWhy %q", d.State, d.HeldWhy) + } +}