Fail a backup whose holder cannot be asked at the bound, not retry forever (issue 1066)

cmd/mesh-controller/plan_backups.go

@@ -131,8 +131,8 @@ func loadBackupFacts(ctx context.Context, inv *inventory.Inventory) (backupFacts
 // stepBackups takes one step of a plan's backups and says whether the plan may go on to ask its first tier. It
 // asks what is not asked, reads how the asked ones went, and records it all in p.Backups; it waits (false, nil)
 // while any is pending, and stays stopped (false, nil) once one failed, which part B holds and says. A plan whose
-// moved modules have no backed-up item keeps no record and goes on at once. An error is an ask that could not be
+// moved modules have no backed-up item keeps no record and goes on at once. A holder that cannot be asked is tried again
-// made or a read the store refused: the step is tried again, the asks already made kept.
+// within the bound and then failed; an error is left for what the step itself cannot do.
 func stepBackups(ctx context.Context, p *inventory.Plan, facts backupFacts, now time.Time) (bool, error) {
 	proceed, err := stepBackupsOnce(ctx, p, facts, now)
 	if err == nil {
@@ -164,16 +164,25 @@ func stepBackupsOnce(ctx context.Context, p *inventory.Plan, facts backupFacts,
 		}
 		if a.AskedAt == nil {
 			asked := now
+			if a.TriedAt == nil {
+				a.TriedAt = &asked
+			}
 			if err := askBackupNow(ctx, a.Machine, a.Module); err != nil {
 				var unasked *backupNotAsked
 				if errors.As(err, &unasked) {
-					return false, err
+					// A holder off the bus is tried again within the bound, by the look and never by the
+					// "tried again" error path; past it the backup is failed and the plan held.
+					a.Words = "could not be asked: " + err.Error()
+					if now.Sub(*a.TriedAt) >= backupsWithin {
+						a.State = inventory.BackupFailed
+					}
+					continue
 				}
 				a.State, a.Words = inventory.BackupFailed, err.Error()
-				a.AskedAt = &asked
+				a.AskedAt, a.TriedAt = &asked, nil
 				continue
 			}
-			a.AskedAt = &asked
+			a.AskedAt, a.TriedAt, a.Words = &asked, nil, ""
 			continue
 		}
 		if _, read := nights[a.Machine]; !read {
@@ -355,7 +364,7 @@ func askBackupsAgain(p *inventory.Plan, by, why string) ([]inventory.PlanBackup,
 			continue
 		}
 		again = append(again, *a)
-		a.State, a.AskedAt, a.Words = inventory.BackupPending, nil, ""
+		a.State, a.AskedAt, a.TriedAt, a.Words = inventory.BackupPending, nil, nil, ""
 	}
 	p.Backups.State = backupsStateOf(p.Backups.Asked)
 	p.Note = backupsNote + "asked again by " + by + ": " + why

cmd/mesh-controller/plan_backups_hold_test.go

@@ -1,6 +1,8 @@
 package main
 
 import (
+	"context"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -115,3 +117,38 @@ func TestAHeldPlanRaisesOneConditionWithTheBackupsWords(t *testing.T) {
 		t.Fatalf("%+v", o)
 	}
 }
+
+// A holder off the bus cannot be asked: the plan waits within the bound, retried by the look and not by the error
+// path, and at the bound the backup is failed with the reason, so the hold and the condition take over.
+func TestABackupHolderThatCannotBeAskedIsFailedAtTheBoundWithTheReason(t *testing.T) {
+	seat := &aFakeBackupSeat{}
+	seat.install(t)
+	was := askBackupNow
+	t.Cleanup(func() { askBackupNow = was })
+	calls := 0
+	askBackupNow = func(_ context.Context, machine, module string) error {
+		calls++
+		return &backupNotAsked{fmt.Errorf("%s's backup holder was not asked: no responders", machine)}
+	}
+	entries := []inventory.Entry{aModuleWith(t, "mail", []string{"ace"}, anItem("a", ""))}
+	facts := backupFactsFrom(entries, holders("ace"))
+	p := aPlanOf("mail")
+	if proceed, err := stepBackups(t.Context(), p, facts, backupT0); proceed || err != nil || p.Backups.State != inventory.BackupPending {
+		t.Fatalf("proceed=%v err=%v %+v", proceed, err, p.Backups)
+	}
+	if proceed, err := stepBackups(t.Context(), p, facts, backupT0.Add(backupsWithin/2)); proceed || err != nil ||
+		p.Backups.State != inventory.BackupPending || calls != 2 || p.Backups.Asked[0].AskedAt != nil {
+		t.Fatalf("within the bound: proceed=%v err=%v calls=%d %+v", proceed, err, calls, p.Backups)
+	}
+	if proceed, err := stepBackups(t.Context(), p, facts, backupT0.Add(backupsWithin)); proceed || err != nil {
+		t.Fatalf("proceed=%v err=%v", proceed, err)
+	}
+	a := p.Backups.Asked[0]
+	if p.Backups.State != inventory.BackupFailed || a.State != inventory.BackupFailed ||
+		!strings.Contains(a.Words, "could not be asked: ") || !strings.Contains(a.Words, "no responders") {
+		t.Fatalf("%+v", p.Backups)
+	}
+	if strings.Contains(p.Note, "tried again") || !strings.Contains(p.Note, "held") {
+		t.Fatalf("note %q", p.Note)
+	}
+}

internal/inventory/plans.go

@@ -100,6 +100,9 @@ type PlanBackup struct {
 	State string `json:"state"`
 	// AskedAt is when the holder was asked; nil while it is not asked yet.
 	AskedAt *time.Time `json:"asked_at,omitempty"`
+	// TriedAt is when the controller first tried to ask, kept while the holder cannot be reached: the bound of a
+	// backup that is never asked counts from it. Nil once asked.
+	TriedAt *time.Time `json:"tried_at,omitempty"`
 	// Words are the backup's own: the holder's error for a failed one, "did not answer in ten minutes" for a silent one.
 	Words string `json:"words,omitempty"`
 }