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.go b/cmd/mesh-controller/plan_backups.go index f741e126..32377e19 100644 --- a/cmd/mesh-controller/plan_backups.go +++ b/cmd/mesh-controller/plan_backups.go @@ -240,7 +240,7 @@ func stepBackupsOnce(ctx context.Context, p *inventory.Plan, facts backupFacts, } // wantedBackups is the record of backups a plan has to take, or nil for none: one entry per machine with a holder -// running a moved module that has a backed-up item, and the machines with none. +// running a moved module that has a backed-up item; a machine with no holder is a failed entry that holds the plan. func wantedBackups(p inventory.Plan, facts backupFacts) *inventory.PlanBackups { var asked []inventory.PlanBackup noHolder := map[string]bool{} @@ -248,7 +248,12 @@ func wantedBackups(p inventory.Plan, facts backupFacts) *inventory.PlanBackups { for _, module := range tier { for _, machine := range facts.backedUp[module] { if !facts.holder[machine] { + // A machine the plan sends data to and cannot ask holds the plan (issue 1086): a send never + // goes on without the restore point it promised. noHolder[machine] = true + asked = append(asked, inventory.PlanBackup{Machine: machine, Module: module, + State: inventory.BackupFailed, Words: "could not be asked: " + machine + " holds no " + + catalogue.BackupSeat}) continue } asked = append(asked, inventory.PlanBackup{Machine: machine, Module: module, State: inventory.BackupPending}) diff --git a/cmd/mesh-controller/plan_backups_test.go b/cmd/mesh-controller/plan_backups_test.go index 61af531b..941fbda7 100644 --- a/cmd/mesh-controller/plan_backups_test.go +++ b/cmd/mesh-controller/plan_backups_test.go @@ -111,8 +111,8 @@ func TestOnlyModulesWithAnItemProtectedByABackupAreAsked(t *testing.T) { } } -// A machine with no backup holder is asked nothing, the plan says so, and it does not hold the plan. -func TestANodeWithNoBackupHolderAsksNothingAndThePlanSaysSo(t *testing.T) { +// A machine with no backup holder cannot be asked: the plan holds and says so, and asks nothing (issue 1086). +func TestANodeWithNoBackupHolderHoldsThePlanAndSaysSo(t *testing.T) { seat := &aFakeBackupSeat{} seat.install(t) entries := []inventory.Entry{aModuleWith(t, "copied", []string{"ace", "bare"}, anItem("a", ""))} @@ -121,17 +121,17 @@ func TestANodeWithNoBackupHolderAsksNothingAndThePlanSaysSo(t *testing.T) { if err != nil || proceed { t.Fatalf("proceed=%v err=%v", proceed, err) } - if strings.Join(seat.asked, " ") != "ace/copied" { - t.Fatalf("asked %v", seat.asked) + if len(seat.asked) != 0 { + t.Fatalf("a held plan asks nothing: %v", seat.asked) } - if p.Backups == nil || len(p.Backups.NoHolder) != 1 || p.Backups.NoHolder[0] != "bare" { - t.Fatalf("the plan must say bare has no holder: %+v", p.Backups) + if p.Backups == nil || len(p.Backups.NoHolder) != 1 || p.Backups.NoHolder[0] != "bare" || + p.Backups.State != inventory.BackupFailed || !strings.Contains(p.Note, "copied on bare") { + t.Fatalf("the plan must hold and say bare has no holder: %+v note %q", p.Backups, p.Note) } - // Only the no-holder machine: nothing to wait for, the plan goes on, and still says it. only := []inventory.Entry{aModuleWith(t, "copied", []string{"bare"}, anItem("a", ""))} q := aPlanOf("copied") proceed, err = stepBackups(t.Context(), q, backupFactsFrom(only, holders("ace")), backupT0) - if err != nil || !proceed || q.Backups == nil || q.Backups.NoHolder[0] != "bare" || q.Backups.State != inventory.BackupDone { + if err != nil || proceed || q.Backups.State != inventory.BackupFailed { t.Fatalf("proceed=%v err=%v record=%+v", proceed, err, q.Backups) } } @@ -237,3 +237,58 @@ 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 TestACutPlanHoldsWhenTheMachineItSendsDataToHasNoBackupHolder(t *testing.T) { + seat := &aFakeBackupSeat{} + seat.install(t) + facts := backupFactsFrom([]inventory.Entry{giteaOnNovox(t)}, holders("ace")) + p := aPlanOf("gitea") + 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("asked=%v err=%v", tierAsked, err) + } + if p.Backups == nil || p.Backups.State != inventory.BackupFailed || !strings.Contains(p.Note, "novox") || + !strings.Contains(p.Note, "held") { + t.Fatalf("the plan must hold and say so: %+v note %q", p.Backups, 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) + } +}