diff --git a/cmd/mesh-controller/backups_before_send_integration_test.go b/cmd/mesh-controller/backups_before_send_integration_test.go index 58f89ed0..10da5daa 100644 --- a/cmd/mesh-controller/backups_before_send_integration_test.go +++ b/cmd/mesh-controller/backups_before_send_integration_test.go @@ -4,6 +4,7 @@ package main import ( "encoding/json" + "os" "strings" "sync" "testing" @@ -217,3 +218,91 @@ func TestADeliveryThatMovesABackedUpModuleWaitsForItsBackupAndAFailedOneHoldsItA t.Fatalf("the condition stays: %+v", said) } } + +// Issue 1086: a plan cut from a merge asked its first tier at the cut, so a module keeping data was sent without the +// backup the plan promised. The cut now takes the backups first (askFirstTierAtCut, batches.go), and the walk's own +// step asks the tier once they are done. +func TestAPlanCutFromAMergeOfABackedUpModuleAsksItsBackupAndNotItsTierUntilItIsDone(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + inv := open.inventory + settings := t.TempDir() + "/merge-window.json" + if err := os.WriteFile(settings, []byte(`{"merge-window": "90s", "merge-window-at-most": "10m"}`), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(mergeWindowFileVar, settings) + everyHeadWaits(t) + asked := asksRecorded(t) + js, err := broker.Dial(testbus.URL(t)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(js.Close) + before := handActConn + handActConn = js.Conn() + t.Cleanup(func() { handActConn = before }) + + if _, err := inv.SeedSeats(ctx, catalogue.DefaultSeats()); err != nil { + t.Fatal(err) + } + register(t, open, catalogue.Manifest{Module: "keeper", Version: "1", + Claims: []catalogue.Claim{{Name: catalogue.BackupSeat, Scope: catalogue.ScopeNode, Serves: []string{"backed-up", "now", "restore"}}}}) + if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "mesh-controller", Version: "1"}, + inventory.Source{Repository: "novox/mesh-controller", Seat: "git", Ref: "main", BuiltFrom: "c0", Head: "c0"}); err != nil { + t.Fatal(err) + } + notes, err := catalogue.ParseManifest([]byte(`{"module":"notes","version":"1","data":{"own":[` + + `{"id":"files","path":"${dir:files}","class":"valuable","why":"people's notes"}]},` + + `"resources":[{"id":"files","type":"directory"}]}`)) + if err != nil { + t.Fatal(err) + } + if err := inv.RegisterModule(ctx, notes, inventory.Source{Repository: "novox/notes", Seat: "git", Ref: "main", + BuiltFrom: "c0", Head: "c0"}); err != nil { + t.Fatal(err) + } + for _, m := range []string{"keeper", "notes"} { + if _, err := assign(ctx, open, "anchor", m); err != nil { + t.Fatal(err) + } + } + seat := &aBackupSeatOnTheBus{} + seat.serve(t, js.Conn(), "anchor") + wasWithin, wasEvery := backupsWithin, backupsPollEvery + backupsWithin, backupsPollEvery = time.Minute, 10*time.Millisecond + t.Cleanup(func() { backupsWithin, backupsPollEvery = wasWithin, wasEvery }) + + // 1. A merge of notes is heard and its window closes: the batch is cut through cutBatch. + hear(t, open, repoMerge("notes", "c-notes", t0), t0.Add(time.Second)) + cutAt(t, open, t0.Add(time.Second+90*time.Second)) + ws, _ := walks(t, open) + if len(ws) != 1 { + t.Fatalf("%d walk(s)", len(ws)) + } + // 2. Right after the cut no tier is asked, the record has anchor/notes pending, the seat heard one `now`. + cut, err := inv.PlanByID(ctx, ws[0].ID) + if err != nil { + t.Fatal(err) + } + if len(*asked) != 0 || askedInTier(cut) { + t.Fatalf("the first tier was asked at the cut: %v %+v", *asked, cut.Modules) + } + if cut.Backups == nil || cut.Backups.State != inventory.BackupPending || len(cut.Backups.Asked) != 1 || + cut.Backups.Asked[0].Machine != "anchor" || cut.Backups.Asked[0].Module != "notes" { + t.Fatalf("the backups of the cut: %+v", cut.Backups) + } + if seat.asks() != 1 { + t.Fatalf("the seat heard %d `now`", seat.asks()) + } + // 3. The backup answers done: the walk's own step asks the first tier, once, and asks no second backup. + seat.answers("notes", true, "") + advancePlans(ctx, open) + advancePlans(ctx, open) + after, err := inv.PlanByID(ctx, ws[0].ID) + if err != nil { + t.Fatal(err) + } + if len(*asked) != 1 || after.Backups.State != inventory.BackupDone || seat.asks() != 1 { + t.Fatalf("builds=%v backups=%+v asks=%d", *asked, after.Backups, seat.asks()) + } +} diff --git a/cmd/mesh-controller/batches.go b/cmd/mesh-controller/batches.go index 96235aa1..b3c09aff 100644 --- a/cmd/mesh-controller/batches.go +++ b/cmd/mesh-controller/batches.go @@ -1224,7 +1224,9 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [ return nil } // Written and its first tier asked as one act on the plans (novox/hq issue 213), under the caller's hold. - if err := askTier(ctx, inv, &plan, plan.Tiers[plan.Tier]); err != nil { + if err := askFirstTierAtCut(&plan, + func(p *inventory.Plan) (bool, error) { return backupsFirst(ctx, inv, p) }, + func() error { return askTier(ctx, inv, &plan, plan.Tiers[plan.Tier]) }); err != nil { return err } if err := inv.SavePlan(ctx, &plan); err != nil { @@ -1234,6 +1236,23 @@ func planBatch(ctx context.Context, open *stores, batch *inventory.Plan, carry [ return nil } +// askFirstTierAtCut asks a freshly cut plan's first tier, after the backups of the data it moves (novox/hq ADR 0404, +// issue 1086): the cut asks the tier itself, so advanceOnce, which asks the backups before a first tier, finds it +// asked and never takes them. A plan whose backups wait, failed or could not be worked out asks nothing; it is saved +// unasked and the walk's own step goes on from its record. +func askFirstTierAtCut(p *inventory.Plan, backups func(*inventory.Plan) (bool, error), ask func() error) error { + proceed, err := backups(p) + if err != nil { + p.Note = backupsNote + "not asked, the backups of the plan could not be worked out: " + err.Error() + + "; nothing is asked or sent, and the plan tries again" + return nil + } + if !proceed { + return nil + } + return ask() +} + // placedBy writes on each module of a plan the edges of its order that put it after another module of the plan, with the // rule that drew each (novox/hq ADR 0419 clause 5). func placedBy(plan *inventory.Plan, ordered []inventory.Edge) { diff --git a/cmd/mesh-controller/plan_backups_test.go b/cmd/mesh-controller/plan_backups_test.go index 61af531b..522611ee 100644 --- a/cmd/mesh-controller/plan_backups_test.go +++ b/cmd/mesh-controller/plan_backups_test.go @@ -237,3 +237,40 @@ func TestTheBackupHoldersAreEveryMachinesAndAFailedReadIsNotNoHolder(t *testing. t.Fatalf("a failed read of the holders must fail the step: %v", err) } } + +// Issue 1086: the cut of a plan asked its first tier itself, so gitea was sent to novox with no backup asked. +func giteaOnNovox(t *testing.T) inventory.Entry { + return aModuleWith(t, "gitea", []string{"novox"}, + `{"id":"data","path":"/var/lib/gitea/data","class":"valuable","why":"repositories"}`) +} + +func TestACutPlanAsksTheBackupOfGiteaOnNovoxBeforeItAsksItsFirstTier(t *testing.T) { + seat := &aFakeBackupSeat{} + seat.install(t) + facts := backupFactsFrom([]inventory.Entry{giteaOnNovox(t)}, holders("novox", "ace")) + p := aPlanOf("gitea", "mesh-delivery") + tierAsked := false + err := askFirstTierAtCut(p, + func(p *inventory.Plan) (bool, error) { return stepBackups(t.Context(), p, facts, backupT0) }, + func() error { tierAsked = true; return nil }) + if err != nil || tierAsked { + t.Fatalf("the first tier was asked before the backup: asked=%v err=%v", tierAsked, err) + } + if strings.Join(seat.asked, " ") != "novox/gitea" || p.Backups == nil || p.Backups.State != inventory.BackupPending { + t.Fatalf("asked %v, record %+v", seat.asked, p.Backups) + } + if !strings.HasPrefix(p.Note, backupsNote) { + t.Fatalf("note %q", p.Note) + } +} + +func TestACutPlanWhoseBackupsCouldNotBeWorkedOutAsksNothingAndSaysSo(t *testing.T) { + p := aPlanOf("gitea") + tierAsked := false + err := askFirstTierAtCut(p, + func(*inventory.Plan) (bool, error) { return false, fmt.Errorf("the catalogue could not be read") }, + func() error { tierAsked = true; return nil }) + if err != nil || tierAsked || !strings.Contains(p.Note, "could not be worked out") { + t.Fatalf("asked=%v err=%v note %q", tierAsked, err, p.Note) + } +}