A plan's cut takes the backups before asking its first tier, and a machine it cannot ask holds it (issue 1086)

The cut asked tier 0 itself, so the step before a first tier never ran and gitea was sent to novox with no restore point. A machine with no backup holder is now a failed backup that holds the plan.

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) {

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})

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.
+// A machine with no backup holder cannot be asked: the plan holds and says so, and asks nothing (issue 1086).
-func TestANodeWithNoBackupHolderAsksNothingAndThePlanSaysSo(t *testing.T) {
+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" {
+	if len(seat.asked) != 0 {
-		t.Fatalf("asked %v", seat.asked)
+		t.Fatalf("a held plan asks nothing: %v", seat.asked)
 	}
-	if p.Backups == nil || len(p.Backups.NoHolder) != 1 || p.Backups.NoHolder[0] != "bare" {
+	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)
+		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)
+	}
+}