Attempt every resource, and report every failure
Found in the lab while proving something else. A machine assigned a module declaring a package that does not exist applied NOTHING on every later push, for ever — the broker's queues were empty, so the declaration had been delivered and read; the machine stopped at the first failing resource and never reached the rest. A machine with one bad module and nine good ones ran none of the nine, and the mesh reported "failed" without saying the rest were never attempted. Nothing that re-pushes to machines that are behind could recover it either: it would retry a permanent failure for ever and make no progress on anything else. And which nine a broken module blocks is an accident of resolution order. The behaviour had a test asserting it, citing ADR 0010. That record does not decide this — it argues about pipelines against reconcilers, and says nothing about whether one resource failing should stop the next being attempted. The citation was doing more work than the record supports. So: everything is attempted, every failure is reported, and the first line says how many. The case for stopping was that a later resource may depend on an earlier one. It still may — and it then fails its own check and is reported, which is more information than skipping it. This host reads back after every write precisely so that is caught rather than assumed. Unchanged: a declaration that cannot be PARSED is still refused whole. That is a different thing — "this machine could not do it" against "this was never a declaration" — and they are fixed in different places. Recorded as novox/hq 04-ISSUES/011 with the evidence.
This commit is contained in:
+45
-3
@@ -76,11 +76,22 @@ type Error struct {
|
||||
Resource string
|
||||
Err error
|
||||
Done Report
|
||||
// Others is how many more resources also failed. Named rather than folded into the message,
|
||||
// because "one thing failed" and "eleven things failed" are different machines and the first
|
||||
// line is what somebody reads.
|
||||
Others int
|
||||
}
|
||||
|
||||
func (e *Error) Error() string {
|
||||
return fmt.Sprintf("applying %q: %v\n\n%d resource(s) were applied before this and remain; "+
|
||||
"the machine is in whatever state that left it.", e.Resource, e.Err, len(e.Done.Outcomes))
|
||||
also := ""
|
||||
if e.Others == 1 {
|
||||
also = ", and one other resource also failed"
|
||||
} else if e.Others > 1 {
|
||||
also = fmt.Sprintf(", and %d other resources also failed", e.Others)
|
||||
}
|
||||
return fmt.Sprintf("applying %q: %v%s\n\n%d resource(s) were applied and remain; "+
|
||||
"everything was attempted, so what is not listed as failed was done.",
|
||||
e.Resource, e.Err, also, len(e.Done.Outcomes))
|
||||
}
|
||||
|
||||
func (e *Error) Unwrap() error { return e.Err }
|
||||
@@ -129,11 +140,32 @@ func Apply(
|
||||
// restarting for it every time would make a steady machine restart its services for ever.
|
||||
changed := map[string]bool{}
|
||||
|
||||
// Everything is attempted, and every failure is reported.
|
||||
//
|
||||
// **It used to stop at the first one**, and that made one broken resource hold the whole
|
||||
// machine hostage: a module declaring a package that does not exist meant every module
|
||||
// ordered after it was never applied, for ever, and the mesh reported "failed" without
|
||||
// saying that the rest had not been tried. A machine with one bad module and nine good ones
|
||||
// ran none of the nine.
|
||||
//
|
||||
// The argument for stopping was that a resource may depend on an earlier one. It still may —
|
||||
// and it will then fail its own check and be reported, which is more information than not
|
||||
// attempting it. A service started against a file that was never written does not verify, and
|
||||
// this host reads back after every write precisely so that is caught rather than assumed.
|
||||
//
|
||||
// What does not change: **a declaration that cannot be parsed is still refused whole.** That
|
||||
// is a different thing — one is "this machine could not do it", the other is "this was never
|
||||
// a declaration", and they are fixed in different places.
|
||||
var failures []*Error
|
||||
for _, resource := range d.Resources {
|
||||
was, _ := known.Find(resource.Identity())
|
||||
outcome, err := applyOne(ctx, sys, resource, run, changed, was, unseal)
|
||||
if err != nil {
|
||||
return report, known, &Error{Resource: resource.Identity(), Err: err, Done: report}
|
||||
failures = append(failures, &Error{
|
||||
Resource: resource.Identity(), Err: err, Done: report,
|
||||
})
|
||||
log(fmt.Sprintf(" failed %s (%s): %v", resource.Identity(), outcome.Target, err))
|
||||
continue
|
||||
}
|
||||
|
||||
// Only now. The record follows the fact, never leads it.
|
||||
@@ -149,6 +181,16 @@ func Apply(
|
||||
log(fmt.Sprintf(" %s %s (%s)", outcome.Action, outcome.ID, outcome.Target))
|
||||
}
|
||||
}
|
||||
|
||||
if len(failures) > 0 {
|
||||
// The first, carrying everything that did happen. One error is what the caller reports
|
||||
// and what a person reads first; the rest are in the report, which is what the mesh
|
||||
// keeps.
|
||||
first := failures[0]
|
||||
first.Done = report
|
||||
first.Others = len(failures) - 1
|
||||
return report, known, first
|
||||
}
|
||||
return report, known, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -178,9 +178,16 @@ func TestARenameToTheSamePathDoesNotDeleteTheNewFile(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestAFailedStepFailsTheApply(t *testing.T) {
|
||||
// novox/hq ADR 0010. And the error carries what HAD been done, because the machine is in
|
||||
// whatever state the apply reached and the only honest thing to hand back is that list.
|
||||
func TestAFailedStepFailsTheApplyAndTheRestIsStillAttempted(t *testing.T) {
|
||||
// The apply fails, names the resource, and carries what did happen — because the machine is
|
||||
// in whatever state the apply reached and the only honest thing to hand back is that list.
|
||||
//
|
||||
// **And everything is attempted.** It used to stop at the first failure, which made one
|
||||
// broken resource hold the whole machine hostage: a module declaring a package that does not
|
||||
// exist meant every module after it was never applied, for ever
|
||||
// (novox/hq 04-ISSUES/011). The case for stopping was that a later resource may depend on an
|
||||
// earlier one — and it still may, and it then fails its own check and is reported, which is
|
||||
// more information than not attempting it.
|
||||
dir := t.TempDir()
|
||||
blocker := filepath.Join(dir, "blocker")
|
||||
if err := os.WriteFile(blocker, []byte("i am a file\n"), 0o644); err != nil {
|
||||
@@ -190,7 +197,7 @@ func TestAFailedStepFailsTheApply(t *testing.T) {
|
||||
d := parse(t, `{"declaration":1,"resources":[
|
||||
{"id":"fine","type":"file","path":"`+filepath.Join(dir, "fine.conf")+`","content":"a\n"},
|
||||
{"id":"doomed","type":"directory","path":"`+blocker+`"},
|
||||
{"id":"never","type":"file","path":"`+filepath.Join(dir, "never.conf")+`","content":"b\n"}
|
||||
{"id":"after","type":"file","path":"`+filepath.Join(dir, "after.conf")+`","content":"b\n"}
|
||||
]}`)
|
||||
|
||||
_, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
|
||||
@@ -205,12 +212,43 @@ func TestAFailedStepFailsTheApply(t *testing.T) {
|
||||
if applyErr.Resource != "doomed" {
|
||||
t.Errorf("the failure names %q, not the resource that failed", applyErr.Resource)
|
||||
}
|
||||
if len(applyErr.Done.Outcomes) != 1 {
|
||||
t.Errorf("the error does not carry what was already applied: %+v", applyErr.Done.Outcomes)
|
||||
// Everything that worked is in the report, before and after the failure.
|
||||
if len(applyErr.Done.Outcomes) != 2 {
|
||||
t.Errorf("the error does not carry what was applied: %+v", applyErr.Done.Outcomes)
|
||||
}
|
||||
// And nothing after the failure ran.
|
||||
if _, err := os.Stat(filepath.Join(dir, "never.conf")); !errors.Is(err, os.ErrNotExist) {
|
||||
t.Error("the apply continued past a failure")
|
||||
if _, err := os.Stat(filepath.Join(dir, "after.conf")); err != nil {
|
||||
t.Error("a resource after the failing one was never attempted, so one broken module " +
|
||||
"still blocks every module after it")
|
||||
}
|
||||
}
|
||||
|
||||
func TestEveryFailureIsCountedNotJustTheFirst(t *testing.T) {
|
||||
// "One thing failed" and "eleven things failed" are different machines, and the first line is
|
||||
// what somebody reads.
|
||||
dir := t.TempDir()
|
||||
for _, name := range []string{"one", "two"} {
|
||||
if err := os.WriteFile(filepath.Join(dir, name), []byte("a file\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
d := parse(t, `{"declaration":1,"resources":[
|
||||
{"id":"first","type":"directory","path":"`+filepath.Join(dir, "one")+`"},
|
||||
{"id":"second","type":"directory","path":"`+filepath.Join(dir, "two")+`"}
|
||||
]}`)
|
||||
|
||||
_, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, noServices, nil, nil)
|
||||
if err == nil {
|
||||
t.Fatal("two impossible resources did not fail the apply")
|
||||
}
|
||||
var applyErr *Error
|
||||
if !errors.As(err, &applyErr) {
|
||||
t.Fatalf("got %T", err)
|
||||
}
|
||||
if applyErr.Others != 1 {
|
||||
t.Errorf("the failure says %d others also failed, and one did", applyErr.Others)
|
||||
}
|
||||
if !strings.Contains(err.Error(), "one other resource also failed") {
|
||||
t.Errorf("the message does not say others failed: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user