Repair script status reports are honored even if the script errors

This means that even if the repair script errors the status (e.g. "done" or "skip") is honored if it was send over the status-fd.
canonical · Sep 1, 2017 · 7ef6dc1 · 7ef6dc1
1 parent b3fb1ce
commit 7ef6dc1
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 34 deletions.
diff --git a/cmd/snap-repair/runner.go b/cmd/snap-repair/runner.go
@@ -140,41 +140,47 @@ func (r *Repair) Run() error {
 	statusW.Close()
 
 	// wait for repair to finish or timeout
+	var scriptErr error
 	killTimerCh := time.After(defaultRepairTimeout)
 	doneCh := make(chan error)
 	go func() {
 		doneCh <- cmd.Wait()
 		close(doneCh)
 	}()
 	select {
-	case err = <-doneCh:
+	case scriptErr = <-doneCh:
 		// done
 	case <-killTimerCh:
 		if err := osutil.KillProcessGroup(cmd); err != nil {
 			logger.Noticef("cannot kill timed out repair %s: %s", r, err)
 		}
-		err = fmt.Errorf("repair did not finish within %s", defaultRepairTimeout)
+		scriptErr = fmt.Errorf("repair did not finish within %s", defaultRepairTimeout)
 	}
-	if err != nil {
-		err = fmt.Errorf("%q failed: %s", r.Ref(), err)
-
-		statusPath := filepath.Join(rundir, baseName+".retry")
-		if err := os.Rename(logPath, statusPath); err != nil {
-			logger.Noticef("cannot write stamp: %s", statusPath)
-		}
+	// read repair status pipe, use the last value
+	status := readStatus(statusR)
+	statusPath := filepath.Join(rundir, baseName+"."+status.String())
 
-		if err := r.errtrackerReport(err, statusPath); err != nil {
+	// if the script had an error exit status still honor what we
+	// read from the status-pipe, however report the error
+	if scriptErr != nil {
+		scriptErr = fmt.Errorf("%q failed: %s", r.Ref(), scriptErr)
+		if err := r.errtrackerReport(scriptErr, logPath); err != nil {
 			logger.Noticef("cannot report error to errtracker: %s", err)
 		}
 		// ensure the error is present in the output log
-		fmt.Fprintf(logf, "\n%s", err)
-
+		fmt.Fprintf(logf, "\n%s", scriptErr)
+	}
+	if err := os.Rename(logPath, statusPath); err != nil {
 		return err
 	}
+	r.SetStatus(status)
 
-	// read repair status pipe, use the last value
+	return nil
+}
+
+func readStatus(r io.Reader) RepairStatus {
 	var status RepairStatus
-	scanner := bufio.NewScanner(statusR)
+	scanner := bufio.NewScanner(r)
 	for scanner.Scan() {
 		switch strings.TrimSpace(scanner.Text()) {
 		case "done":
@@ -184,16 +190,9 @@ func (r *Repair) Run() error {
 		}
 	}
 	if scanner.Err() != nil {
-		return err
+		return RetryStatus
 	}
-
-	statusPath := filepath.Join(rundir, baseName+"."+status.String())
-	if err := os.Rename(logPath, statusPath); err != nil {
-		return err
-	}
-	r.SetStatus(status)
-
-	return nil
+	return status
 }
 
 // errtrackerReport reports an repairErr with the given logPath to the

diff --git a/cmd/snap-repair/runner_test.go b/cmd/snap-repair/runner_test.go
@@ -1490,7 +1490,7 @@ func (s *runScriptSuite) errtrackerReportRepair(repair, errMsg, dupSig string, e
 	return "some-oops-id", nil
 }
 
-func (s *runScriptSuite) testScriptRun(c *C, mockScript, expectedErr string) *repair.Repair {
+func (s *runScriptSuite) testScriptRun(c *C, mockScript string) *repair.Repair {
 	r1 := sysdb.InjectTrusted(s.storeSigning.Trusted)
 	defer r1()
 	r2 := repair.MockTrustedRepairRootKeys([]*asserts.AccountKey{s.repairRootAcctKey})
@@ -1502,11 +1502,7 @@ func (s *runScriptSuite) testScriptRun(c *C, mockScript, expectedErr string) *re
 	c.Assert(err, IsNil)
 
 	err = rpr.Run()
-	if expectedErr == "" {
-		c.Assert(err, IsNil)
-	} else {
-		c.Assert(err, ErrorMatches, expectedErr)
-	}
+	c.Assert(err, IsNil)
 
 	scrpt, err := ioutil.ReadFile(filepath.Join(s.runDir, "r0.script"))
 	c.Assert(err, IsNil)
@@ -1547,7 +1543,7 @@ echo "done" >&$SNAP_REPAIR_STATUS_FD
 exit 0
 `
 	s.seqRepairs = []string{makeMockRepair(script)}
-	s.testScriptRun(c, script, "")
+	s.testScriptRun(c, script)
 	// verify
 	s.verifyRundir(c, []string{
 		`^r0.done$`,
@@ -1564,7 +1560,7 @@ echo "unhappy output"
 exit 1
 `
 	s.seqRepairs = []string{makeMockRepair(script)}
-	s.testScriptRun(c, script, `"repair \(1; brand-id:canonical\)" failed: exit status 1`)
+	s.testScriptRun(c, script)
 	// verify
 	s.verifyRundir(c, []string{
 		`^r0.retry$`,
@@ -1596,7 +1592,7 @@ echo "skip" >&$SNAP_REPAIR_STATUS_FD
 exit 0
 `
 	s.seqRepairs = []string{makeMockRepair(script)}
-	s.testScriptRun(c, script, "")
+	s.testScriptRun(c, script)
 	// verify
 	s.verifyRundir(c, []string{
 		`^r0.script$`,
@@ -1619,7 +1615,7 @@ touch zzz-ran-once
 exit 1
 `
 	s.seqRepairs = []string{makeMockRepair(script)}
-	rpr := s.testScriptRun(c, script, `"repair \(1; brand-id:canonical\)" failed: exit status 1`)
+	rpr := s.testScriptRun(c, script)
 	s.verifyRundir(c, []string{
 		`^r0.retry$`,
 		`^r0.script$`,
@@ -1664,7 +1660,7 @@ sleep 100
 	c.Assert(err, IsNil)
 
 	err = rpr.Run()
-	c.Assert(err, ErrorMatches, `"repair \(1; brand-id:canonical\)" failed: repair did not finish within 10ms`)
+	c.Assert(err, IsNil)
 
 	s.verifyRundir(c, []string{
 		`^r0.retry$`,

diff --git a/errtracker/errtracker_test.go b/errtracker/errtracker_test.go
@@ -92,6 +92,7 @@ func (s *ErrtrackerTestSuite) TestReport(c *C) {
 
 	snapConfineProfile := filepath.Join(s.tmpdir, "/etc/apparmor.d/usr.lib.snapd.snap-confine")
 	err := os.MkdirAll(filepath.Dir(snapConfineProfile), 0755)
+	c.Assert(err, IsNil)
 	err = ioutil.WriteFile(snapConfineProfile, []byte("# fake profile of snap-confine"), 0644)
 	c.Assert(err, IsNil)