diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go index bc193f5..22c6de0 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/engines.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "sort" "sync" @@ -51,24 +52,25 @@ func (e engineAsks) ComposeCandidate(ctx context.Context, node string, ask deliv } // placedNodes names every node the seat mesh-placement places now, read from its state when a check composes (no -// watch: a check reads it once). A state that cannot be read places no node, said in the journal. +// watch: a check reads it once). A state that cannot be read is an error, so the check says its composition was not +// made rather than passing with no node asked (review finding 4). type placedNodes struct { - kv func(ctx context.Context) (jetstream.KeyValue, error) - logf func(string, ...any) + kv func(ctx context.Context) (jetstream.KeyValue, error) } -func (p placedNodes) Placed() []string { +func (p placedNodes) Placed() ([]string, error) { ctx, cancel := context.WithTimeout(context.Background(), delivery.ComposeAnswerBound) defer cancel() kv, err := p.kv(ctx) if err != nil { - p.logf("the state %s cannot be read, so no node is asked to compose: %v", delivery.PlacementBucket, err) - return nil + return nil, fmt.Errorf("the state %s cannot be read: %w", delivery.PlacementBucket, err) } lister, err := kv.ListKeys(ctx) + if errors.Is(err, jetstream.ErrNoKeysFound) { + return nil, nil // a placement that places nothing + } if err != nil { - p.logf("the state %s cannot be listed, so no node is asked to compose: %v", delivery.PlacementBucket, err) - return nil + return nil, fmt.Errorf("the state %s cannot be listed: %w", delivery.PlacementBucket, err) } seen := map[string]bool{} for key := range lister.Keys() { @@ -81,7 +83,7 @@ func (p placedNodes) Placed() []string { out = append(out, n) } sort.Strings(out) - return out + return out, nil } // seatSandbox runs a compose job on the Go build seat's check stream, as the checker's own ask, and answers the diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go index 1f03586..a5fbef9 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go @@ -176,9 +176,8 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro } verdicts := &manifestVerdicts{} compositions := check.EngineCompositions{ - Asker: engineAsks{call: caller.CallOn}, - Placed: placedNodes{kv: func(at context.Context) (jetstream.KeyValue, error) { return js.KeyValue(at, delivery.PlacementBucket) }, - logf: logger.Printf}.Placed, + Asker: engineAsks{call: caller.CallOn}, + Placed: placedNodes{kv: func(at context.Context) (jetstream.KeyValue, error) { return js.KeyValue(at, delivery.PlacementBucket) }}.Placed, Bound: answerBound, Sandbox: seatSandbox{asker: buildrun.Asker{Publisher: asks, Waiter: waiter, Bound: stepBound}}, Verdicts: verdicts.all, diff --git a/modules/mesh-delivery-checker/internal/check/engines.go b/modules/mesh-delivery-checker/internal/check/engines.go index aef0ef1..64c8579 100644 --- a/modules/mesh-delivery-checker/internal/check/engines.go +++ b/modules/mesh-delivery-checker/internal/check/engines.go @@ -39,8 +39,9 @@ type Sandbox interface { // EngineCompositions is the checker's composition of a check (Examiner.Compositions). type EngineCompositions struct { Asker EngineAsker - // Placed names every node in placement now (the seat mesh-placement's state). - Placed func() []string + // Placed names every node in placement now (the seat mesh-placement's state), or why it cannot be read: then no node + // can be asked, and the composition is not made (an error), never passed with nobody asked. + Placed func() ([]string, error) // Bound is the answer bound (the setting compose-answer-bound); zero is delivery.ComposeAnswerBound. Bound time.Duration // Composer is the commit of the node-engine the mesh runs, named as what composed (ADR 0548 ยง6's running commit). @@ -71,7 +72,9 @@ func (e EngineCompositions) Compose(ctx context.Context, c *Case) (delivery.Comp } var nodes []string if e.Placed != nil { - nodes = e.Placed() + if nodes, err = e.Placed(); err != nil { + return delivery.CompositionVerdict{}, fmt.Errorf("the placement cannot be read, so no node can be asked: %w", err) + } } bound := e.Bound if bound <= 0 { diff --git a/modules/mesh-delivery-checker/internal/check/engines_test.go b/modules/mesh-delivery-checker/internal/check/engines_test.go index 29cee46..52e98c6 100644 --- a/modules/mesh-delivery-checker/internal/check/engines_test.go +++ b/modules/mesh-delivery-checker/internal/check/engines_test.go @@ -81,7 +81,7 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { key, _ := delivery.CompositionKey(cs.Group, cs.HeadsHash) compose := func(e *fakeEngines, nodes ...string) delivery.CompositionVerdict { t.Helper() - ec := EngineCompositions{Asker: e, Placed: func() []string { return nodes }, Bound: 50 * time.Millisecond, + ec := EngineCompositions{Asker: e, Placed: func() ([]string, error) { return nodes, nil }, Bound: 50 * time.Millisecond, Composer: func() string { return strings.Repeat("7", 40) }} v, err := ec.Compose(context.Background(), cs) if err != nil { @@ -149,7 +149,7 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { return composer.Verdict{V: composer.Version, Cases: []composer.CaseVerdict{{Name: "change", Machines: map[string]composer.Machine{node: {Composes: false, Reason: "the new composer refuses it"}}}}} }} - ec := EngineCompositions{Asker: e, Placed: func() []string { return []string{"anchor"} }, Bound: time.Second, + ec := EngineCompositions{Asker: e, Placed: func() ([]string, error) { return []string{"anchor"}, nil }, Bound: time.Second, Sandbox: sb, Manifests: digests{"web": "sha256:" + strings.Repeat("c", 64)}} v, err := ec.Compose(context.Background(), host) if err != nil { @@ -178,4 +178,17 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { } }) + // Review finding 4: a placement that cannot be read is no mesh with no node: the composition is not made, said. + t.Run("an unreadable placement makes no composition", func(t *testing.T) { + ec := EngineCompositions{Asker: &fakeEngines{}, Placed: func() ([]string, error) { return nil, errors.New("no such bucket") }} + if _, err := ec.Compose(context.Background(), cs); err == nil || !strings.Contains(err.Error(), "no such bucket") { + t.Errorf("an unreadable placement: %v", err) + } + b := newBench(t) + b.ex.Compositions = ec + v := b.judge(t, cs).Verdict + if v.Composition == nil || v.Composition.State != delivery.CompositionNotRun || !strings.Contains(v.Composition.Words, "no such bucket") { + t.Errorf("the verdict's composition = %+v", v.Composition) + } + }) } diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index 5328576..b561a1f 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -279,7 +279,9 @@ func (st *state) composed(ctx context.Context, v *delivery.Verdict) { } comp, err := st.j.Compositions.Compose(ctx, st.c) if err != nil { + // Said in the verdict, never left as a composition not yet run (review finding 4). st.j.logf("%s: the composition could not be made, so it is said as not run: %v", st.c.Key, err) + v.Composition = &delivery.Composition{State: delivery.CompositionNotRun, Words: "not made: " + err.Error()} return } withComposition(v, comp)