| index d42704637..0cf4333ca 100644 |
| --- a/internal/rollout/status_test.go |
| +++ b/internal/rollout/status_test.go |
| @@ -251,9 +251,13 @@ func TestStatusReportsAStartingOrStoppingHostAsTransient(t *testing.T) { |
| } |
| |
| // Status asks the activator with no Manager or scheduler lock held (the lock |
| -// order on Manager). The probe only TryLocks, so a violation fails the test |
| -// instead of deadlocking it. No attempt runs (nothing is installed), so no |
| -// other goroutine holds these locks. |
| +// order on Manager), for both of its questions: Current for every host, and |
| +// Owns for one with no current generation (b, asleep here). The probe only |
| +// TryLocks, so a violation fails the test instead of deadlocking it. No |
| +// attempt runs (nothing is installed), so no other goroutine holds these |
| +// locks for long. The probe is the harness's (probeAct), wired in before |
| +// any scheduler started; its hook is installed atomically (final review |
| +// round 1, finding 7). |
| func TestStatusAsksTheActivatorWithNoLockHeld(t *testing.T) { |
| h := newHarness(t, "a", "b") |
| h.pass() |
| @@ -266,29 +270,32 @@ func TestStatusAsksTheActivatorWithNoLockHeld(t *testing.T) { |
| if len(scheds) == 0 { |
| t.Fatal("the pass built no scheduler") |
| } |
| - probe := &lockProbeAct{fakeAct: h.act, m: h.m, scheds: scheds, t: t} |
| - h.m.d.Act = probe |
| + h.act.mu.Lock() |
| + delete(h.act.gens, "b") // asleep: Status asks Owns about it |
| + h.act.mu.Unlock() |
| + lp := &lockProbe{m: h.m, scheds: scheds, t: t, asked: map[string]*atomic.Int64{"Owns": {}, "Current": {}}} |
| + h.probe.onAsk(lp.check) |
| + defer h.probe.onAsk(nil) |
| h.m.Status() |
| - if probe.asked.Load() == 0 { |
| - t.Fatal("Status never asked the activator") |
| + if lp.asked["Current"].Load() == 0 || lp.asked["Owns"].Load() == 0 { |
| + t.Fatalf("Status asked Current %d and Owns %d time(s): want both asked", lp.asked["Current"].Load(), lp.asked["Owns"].Load()) |
| } |
| } |
| |
| -// lockProbeAct fails the test when Owns or Current is called while the |
| +// lockProbe fails the test when Owns or Current is asked by Status while the |
| // Manager's or a scheduler's mutex is held. |
| -type lockProbeAct struct { |
| - *fakeAct |
| +type lockProbe struct { |
| m *Manager |
| scheds []*scheduler |
| t *testing.T |
| - asked atomic.Int64 |
| + asked map[string]*atomic.Int64 // by method; built before the hook is installed, never written after |
| } |
| |
| -func (p *lockProbeAct) check() { |
| +func (p *lockProbe) check(method, _ string) { |
| if !askedByStatus() { |
| return // the scheduler's own goroutine asks too; only Status is under test |
| } |
| - p.asked.Add(1) |
| + p.asked[method].Add(1) |
| // A lock that Status holds stays held for the whole ask, while a |
| // scheduler goroutine's own hold is brief, so a violation is a lock that |
| // cannot be taken for a long while. TryLock, never Lock: a violation |
| @@ -303,11 +310,11 @@ func (p *lockProbeAct) check() { |
| return false |
| } |
| if !free(&p.m.mu) { |
| - p.t.Error("activator asked with Manager.mu held") |
| + p.t.Errorf("activator asked (%s) with Manager.mu held", method) |
| } |
| for _, s := range p.scheds { |
| if !free(&s.mu) { |
| - p.t.Error("activator asked with a scheduler.mu held") |
| + p.t.Errorf("activator asked (%s) with a scheduler.mu held", method) |
| } |
| } |
| } |
| @@ -328,16 +335,6 @@ func askedByStatus() bool { |
| } |
| } |
| |
| -func (p *lockProbeAct) Owns(host string) bool { |
| - p.check() |
| - return p.fakeAct.Owns(host) |
| -} |
| - |
| -func (p *lockProbeAct) Current(host string) (activator.Gen, bool) { |
| - p.check() |
| - return p.fakeAct.Current(host) |
| -} |
| - |
| // A host that joins a scheduler after Status read the names was never asked of |
| // the activator, so nothing says it is settled: that report calls it |
| // transient, and the next one asks. |
| @@ -350,13 +347,17 @@ func TestStatusCallsAHostThatJoinedAfterTheNamesWereReadTransient(t *testing.T) |
| } |
| h.m.mu.Unlock() |
| var once atomic.Bool |
| - h.m.d.Act = &joinProbeAct{fakeAct: h.act, onAsk: func() { |
| + h.probe.onAsk(func(method, _ string) { |
| + if method != "Current" || !askedByStatus() { // not the scheduler's own goroutine |
| + return |
| + } |
| if once.CompareAndSwap(false, true) { // the names are read; now a host joins |
| s.mu.Lock() |
| s.q.hosts["late"] = &hostSlot{wantBuild: "v1", good: "v1", serving: "v1"} |
| s.mu.Unlock() |
| } |
| - }} |
| + }) |
| + defer h.probe.onAsk(nil) |
| r := h.m.Status() |
| if late := r.Hosts["late"]; !late.Transient || late.Serving != "" || late.Awake { |
| t.Fatalf("late = %+v, want transient for the report that never asked", late) |
| @@ -365,15 +366,3 @@ func TestStatusCallsAHostThatJoinedAfterTheNamesWereReadTransient(t *testing.T) |
| t.Fatalf("a = %+v, want awake on v1", a) |
| } |
| } |
| - |
| -type joinProbeAct struct { |
| - *fakeAct |
| - onAsk func() |
| -} |
| - |
| -func (p *joinProbeAct) Current(host string) (activator.Gen, bool) { |
| - if askedByStatus() { // not the scheduler's own goroutine |
| - p.onAsk() |
| - } |
| - return p.fakeAct.Current(host) |
| -} |
amadan