Update modules: Implement NeedsArtifactReboot -> Automatic.

The motivation behind is change is to promote the use of `Automatic`
in all update modules. We want this because in order to support
multiple payloads, an update module should not reboot the host in its
`ArtifactReboot` script. Doing so means that `ArtifactReboot` scripts
in later payloads do not get to execute. Instead it should leave this
to Mender using the `Automatic` mechanism. Multiple payloads have not
been implemented yet, but since `Automatic` needs to be supported by
the update module itself, we implement this part now so that we don't
accumulate too many modules without it before we get multiple payload
support.

Note that the most important test of this feature is by changing the
`rootfs-image-v2` script, which is then used by the integration test
which already exists.

Changelog: Title

MEN-2011

Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech>

Documentation/update-modules-v3-file-api.md

@@ -105,14 +105,16 @@ The module should print one of the valid responses:
 
 * `No` - Mender will not run `ArtifactReboot`. This is the same as returning
   nothing, **and hence the default**.
-* `Yes` - Mender will run the update module with the `ArtifactReboot` argument
-* `Automatic` **[Unimplemented]** - Mender will not call the module with the
-  `ArtifactReboot` argument, but will instead perform one single reboot
-  itself. The intended use of this response is to group the reboots of several
-  update modules into one reboot. **This is usually the best choice** for all
-  modules that just require a normal reboot, but modules that reboot a
-  peripheral device may need to use `Yes` instead, and implement their own
-  method.
+* `Automatic` - Mender will not call the module with the `ArtifactReboot`
+  argument, but will instead perform one single reboot itself. The intended use
+  of this response is to group the reboots of several update modules into one
+  reboot. **This is usually the best choice** for all modules that just require
+  a normal reboot, but modules that reboot a peripheral device may need to use
+  `Yes` instead, and implement their own method.
+* `Yes` - Mender will run the update module with the `ArtifactReboot`
+  argument. Use this when you want to reboot a peripheral device that's
+  connected to the host. Don't use this if you want to reboot the host that
+  Mender runs on; use `Automatic` instead.
 
 **Note:** Even though the update module won't be called with the
 `ArtifactReboot` argument when using `Automatic`, it still counts as having

daemon.go

@@ -19,6 +19,7 @@ import (
 	"github.com/mendersoftware/log"
 	"github.com/mendersoftware/mender/datastore"
 	"github.com/mendersoftware/mender/store"
+	"github.com/mendersoftware/mender/system"
 	"github.com/pkg/errors"
 )
 
@@ -37,7 +38,8 @@ func NewDaemon(mender Controller, store store.Store) *menderDaemon {
 	daemon := menderDaemon{
 		mender: mender,
 		sctx: StateContext{
-			store: store,
+			store:    store,
+			rebooter: system.NewSystemRebootCmd(system.OsCalls{}),
 		},
 		store:       store,
 		updateCheck: make(chan bool, 1),

state.go

@@ -32,6 +32,7 @@ import (
 type StateContext struct {
 	// data store access
 	store                      store.Store
+	rebooter                   installer.Rebooter
 	lastUpdateCheckAttempt     time.Time
 	lastInventoryUpdateAttempt time.Time
 	lastAuthorizeAttempt       time.Time
@@ -878,14 +879,10 @@ func (is *UpdateInstallState) Handle(ctx *StateContext, c Controller) (State, bo
 		case datastore.RebootTypeNone:
 			// Do nothing.
 
-		case datastore.RebootTypeCustom:
+		case datastore.RebootTypeCustom, datastore.RebootTypeAutomatic:
 			// Go to reboot state if at least one payload requested it.
 			return NewUpdateRebootState(is.Update()), false
 
-		case datastore.RebootTypeAutomatic:
-			return is.HandleError(ctx, c, NewTransientError(errors.New(
-				"Update module automatic reboots are not supported by this client")))
-
 		default:
 			return is.HandleError(ctx, c, NewTransientError(errors.New(
 				"Unknown reboot type stored in database. Not continuing")))
@@ -1423,22 +1420,34 @@ func (e *UpdateRebootState) Handle(ctx *StateContext, c Controller) (State, bool
 		return NewUpdateRollbackState(e.Update()), false
 	}
 
-	log.Info("rebooting device")
+	log.Info("rebooting device(s)")
 
+	systemRebootRequested := false
 	for n, i := range c.GetInstallers() {
 		rebootRequested, err := e.Update().RebootRequested.Get(n)
 		if err != nil {
 			return e.HandleError(ctx, c, NewTransientError(errors.Wrap(
 				err, "Unable to get requested reboot type")))
 		}
-		if rebootRequested == datastore.RebootTypeCustom {
+		switch rebootRequested {
+		case datastore.RebootTypeCustom:
 			if err := i.Reboot(); err != nil {
 				log.Errorf("error rebooting device: %v", err)
 				return NewUpdateRollbackState(e.Update()), false
 			}
+
+		case datastore.RebootTypeAutomatic:
+			systemRebootRequested = true
 		}
 	}
 
+	if systemRebootRequested {
+		// Final system reboot after reboot scripts have run.
+		err := ctx.rebooter.Reboot()
+		// Should never return from Reboot().
+		return e.HandleError(ctx, c, NewTransientError(errors.Wrap(err, "Could not reboot host")))
+	}
+
 	// We may never get here, if the machine we're on rebooted. However, if
 	// we rebooted a peripheral device, we will get here.
 	return NewUpdateVerifyRebootState(e.Update()), false
@@ -1527,16 +1536,12 @@ func (rs *UpdateRollbackState) Handle(ctx *StateContext, c Controller) (State, b
 		case datastore.RebootTypeNone:
 			// Do nothing.
 
-		case datastore.RebootTypeCustom:
+		case datastore.RebootTypeCustom, datastore.RebootTypeAutomatic:
 			// Enter rollback reboot state if at least one payload
 			// asked for it.
 			log.Debug("will try to rollback reboot the device")
 			return NewUpdateRollbackRebootState(rs.Update()), false
 
-		case datastore.RebootTypeAutomatic:
-			return rs.HandleError(ctx, c, NewTransientError(errors.New(
-				"Update module automatic reboots are not supported by this client")))
-
 		default:
 			return rs.HandleError(ctx, c, NewTransientError(errors.New(
 				"Unknown reboot type stored in database. Not continuing")))
@@ -1572,23 +1577,35 @@ func (rs *UpdateRollbackRebootState) Handle(ctx *StateContext, c Controller) (St
 		log.Errorf("failed to enable deployment logger: %s", err)
 	}
 
-	log.Info("rebooting device after rollback")
+	log.Info("rebooting device(s) after rollback")
 
+	systemRebootRequested := false
 	for n, i := range c.GetInstallers() {
 		rebootRequested, err := rs.Update().RebootRequested.Get(n)
 		if err != nil {
 			return rs.HandleError(ctx, c, NewTransientError(errors.Wrap(
 				err, "Unable to get requested reboot type")))
 		}
-		if rebootRequested == datastore.RebootTypeCustom {
+		switch rebootRequested {
+		case datastore.RebootTypeCustom:
 			if err := i.RollbackReboot(); err != nil {
 				log.Errorf("error rebooting device: %v", err)
 				// Outcome is irrelevant, we will go to the
 				// VerifyRollbackReboot state regardless.
 			}
+
+		case datastore.RebootTypeAutomatic:
+			systemRebootRequested = true
 		}
 	}
 
+	if systemRebootRequested {
+		// Final system reboot after reboot scripts have run.
+		err := ctx.rebooter.Reboot()
+		// Should never return from Reboot().
+		return rs.HandleError(ctx, c, NewTransientError(errors.Wrap(err, "Could not reboot host")))
+	}
+
 	// We may never get here, if the machine we're on rebooted. However, if
 	// we rebooted a peripheral device, we will get here.
 	return NewUpdateVerifyRollbackRebootState(rs.Update()), false

state_test.go

@@ -35,6 +35,8 @@ import (
 	"github.com/mendersoftware/mender/installer"
 	"github.com/mendersoftware/mender/statescript"
 	"github.com/mendersoftware/mender/store"
+	"github.com/mendersoftware/mender/system"
+	stest "github.com/mendersoftware/mender/system/testing"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -4781,3 +4783,48 @@ func TestDBSchemaUpdate(t *testing.T) {
 	assert.Equal(t, datastore.StateDataVersion, sd.Version)
 	assert.False(t, sd.UpdateInfo.HasDBSchemaUpdate)
 }
+
+func TestAutomaticReboot(t *testing.T) {
+	tempDir, _ := ioutil.TempDir("", "logs")
+	defer os.RemoveAll(tempDir)
+
+	DeploymentLogger = NewDeploymentLogManager(tempDir)
+	defer func() {
+		DeploymentLogger.Disable()
+		DeploymentLogger = nil
+	}()
+
+	// This should not be necessary, but supposedly some other test is not
+	// cleaning up after itself.
+	log.Log = log.New()
+
+	log.AddHook(NewDeploymentLogHook(DeploymentLogger))
+	// We cannot remove hooks, so just clean up by resetting log.Log
+	// instead.
+	defer func() {
+		log.Log = log.New()
+	}()
+
+	ctx := &StateContext{
+		store:    store.NewMemStore(),
+		rebooter: system.NewSystemRebootCmd(stest.NewTestOSCalls("Called reboot", 99)),
+	}
+	u := &datastore.UpdateInfo{
+		Artifact: datastore.Artifact{
+			PayloadTypes: []string{"test-type"},
+		},
+		ID:              "abc",
+		RebootRequested: datastore.RebootRequestedType{datastore.RebootTypeAutomatic},
+	}
+	c := &stateTestController{}
+	rebootState := NewUpdateRebootState(u)
+
+	state, cancelled := rebootState.Handle(ctx, c)
+
+	assert.False(t, cancelled)
+	assert.IsType(t, &UpdateErrorState{}, state)
+
+	logs, err := DeploymentLogger.GetLogs("abc")
+	require.NoError(t, err)
+	assert.Contains(t, string(logs), "exit status 99")
+}

tests/rootfs-image-v2

@@ -34,13 +34,12 @@ bootcount 0
 EOF
         ;;
 
-    NeedsArtifactReboot|SupportsRollback)
-        echo "Yes"
+    NeedsArtifactReboot)
+        echo "Automatic"
         ;;
 
-    ArtifactReboot|ArtifactRollbackReboot)
-        reboot
-        sleep 600
+    SupportsRollback)
+        echo "Yes"
         ;;
 
     ArtifactVerifyReboot)