o/h/ctlcmd, t/m/component-snapctl: improve snapctl install consistency

* cmd, daemon, o/h/ctlcmd: exit code 0 and message when all components are already installed

* o/h/ctlcmd: exit with 0 and message on stderr when installing new and already installed components

* c/snapctl, daemon: avoid marshalling error directly in daemon and parse marshalled data with type checks in client

* c/snapctl, daemon: only include components in marshalled data

* o/h/ctlcmd: test install with already installed components

* c/snapctl: add test for already installed error in snapctl command

* c/snapctl, o/hctlcmd, t/m/component-snapctl: add spread test to check install end to end

* c/snapctl: use Fprintln instead of writing bytes

* o/h/ctlcmd: contain all the logic for handling already installed components in installCommand.Execute

* o/h/ctlcmd, t/m/component-snapctl: avoid passing managementCommand as a pointer and return affected components instead

* o/h/ctlcmd: change variable name from affected to affectedComponents

* o/h/ctlcmd: fix parallel install handling when checking if component is installed and add regression test for it

* o/h/ctlcmd: add comment for vset presence check

* o/h/ctlcmd, t/m/component-snapctl: fixups

Author: natibek

overlord/hookstate/ctlcmd/helpers.go

@@ -391,7 +391,7 @@ func changeIDIfNotEphemeral(hctx *hookstate.Context) string {
 	return ""
 }
 
-func createSnapctlInstallTasks(hctx *hookstate.Context, cmd managementCommand) (tss []*state.TaskSet, err error) {
+func createSnapctlInstallTasks(hctx *hookstate.Context, cmd managementCommand) (affectedComponents []string, tss []*state.TaskSet, err error) {
 	st := hctx.State()
 	st.Lock()
 	defer st.Unlock()
@@ -400,15 +400,45 @@ func createSnapctlInstallTasks(hctx *hookstate.Context, cmd managementCommand) (
 	// by the current change
 	vsets, err := hctx.PendingValidationSets()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	info, err := currentSnapInfo(st, hctx.InstanceName())
+	instanceName := hctx.InstanceName()
+	var snapst snapstate.SnapState
+	if err := snapstate.Get(st, instanceName, &snapst); err != nil {
+		return nil, nil, err
+	}
+
+	info, err := snapst.CurrentInfo()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
+	}
+
+	snapName := snap.InstanceSnap(instanceName)
+	if vsets != nil {
+		// When validation sets are provided, we install all components
+		// regardless of their current installation state. thus, all components
+		// will be affected.
+		affectedComponents = cmd.components
+	} else {
+		for _, comp := range cmd.components {
+			if snapst.CurrentComponentSideInfo(naming.NewComponentRef(snapName, comp)) == nil {
+				affectedComponents = append(affectedComponents, comp)
+			}
+		}
 	}
-	return snapstateInstallComponents(context.TODO(), st, cmd.components, info, vsets,
+
+	if len(affectedComponents) == 0 {
+		return nil, nil, snap.NewAlreadyInstalledComponentsError(instanceName, cmd.components)
+	}
+
+	tss, err = snapstateInstallComponents(context.TODO(), st, affectedComponents, info, vsets,
 		snapstate.Options{ExpectOneSnap: true, ConflictOptions: snapstate.ConflictOptions{FromChange: changeIDIfNotEphemeral(hctx)}})
+
+	if err != nil {
+		return nil, nil, err
+	}
+	return affectedComponents, tss, nil
 }
 
 func createSnapctlRemoveTasks(hctx *hookstate.Context, cmd managementCommand) (tss []*state.TaskSet, err error) {
@@ -421,21 +451,20 @@ func createSnapctlRemoveTasks(hctx *hookstate.Context, cmd managementCommand) (t
 			ConflictOptions: snapstate.ConflictOptions{FromChange: changeIDIfNotEphemeral(hctx)}})
 }
 
-func runSnapManagementCommand(hctx *hookstate.Context, cmd managementCommand) (id string, err error) {
-	st := hctx.State()
+func runSnapManagementCommand(hctx *hookstate.Context, cmd managementCommand) (id string, affectedComponents []string, err error) {
 	var tss []*state.TaskSet
 	var cmdStr, cmdVerb string
 	var changeKind string
 
 	// If the context is non-ephemeral, we don't support async because we are just queuing a change in the first place.
 	// In the future this could be made non-queuing, but for now we just return the error.
 	if cmd.async && !hctx.IsEphemeral() {
-		return "", fmt.Errorf("internal error: cannot run snap management command asynchronously from a non-ephemeral context")
+		return "", nil, fmt.Errorf("internal error: cannot run snap management command asynchronously from a non-ephemeral context")
 	}
 
 	switch cmd.operation {
 	case installManagementCommand:
-		tss, err = createSnapctlInstallTasks(hctx, cmd)
+		affectedComponents, tss, err = createSnapctlInstallTasks(hctx, cmd)
 		cmdStr = "install"
 		cmdVerb = "Installing"
 		changeKind = snapctlInstallChangeKind
@@ -448,15 +477,19 @@ func runSnapManagementCommand(hctx *hookstate.Context, cmd managementCommand) (i
 		err = fmt.Errorf("internal error: %q is not a valid snap management command", cmd.operation)
 	}
 	if err != nil {
-		return "", err
+		return "", nil, err
 	}
 
 	if !hctx.IsEphemeral() {
 		// Differently to service control commands, we always queue the
 		// management tasks if run from a hook.
-		return "", queueCommand(hctx, tss)
+		if err := queueCommand(hctx, tss); err != nil {
+			return "", nil, err
+		}
+		return "", affectedComponents, nil
 	}
 
+	st := hctx.State()
 	st.Lock()
 	chg := st.NewChange(changeKind,
 		fmt.Sprintf("%s components %v for snap %s",
@@ -469,16 +502,19 @@ func runSnapManagementCommand(hctx *hookstate.Context, cmd managementCommand) (i
 	st.Unlock()
 
 	if cmd.async {
-		return chg.ID(), nil
+		return chg.ID(), affectedComponents, nil
 	}
 
 	select {
 	case <-chg.Ready():
 		st.Lock()
 		defer st.Unlock()
-		return "", chg.Err()
+		if err := chg.Err(); err != nil {
+			return "", nil, err
+		}
+		return "", affectedComponents, nil
 	case <-time.After(10 * time.Minute):
-		return "", fmt.Errorf("snapctl %s command is taking too long", cmdStr)
+		return "", nil, fmt.Errorf("snapctl %s command is taking too long", cmdStr)
 	}
 }
 

overlord/hookstate/ctlcmd/install.go

@@ -23,6 +23,8 @@ import (
 	"fmt"
 
 	"github.com/snapcore/snapd/i18n"
+	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/strutil"
 )
 
 var (
@@ -55,10 +57,21 @@ func (c *installCommand) Execute([]string) error {
 		return err
 	}
 
-	id, err := runSnapManagementCommand(ctx, managementCommand{operation: installManagementCommand, components: comps, async: c.NoWait})
+	id, affectedComponents, err := runSnapManagementCommand(ctx, managementCommand{operation: installManagementCommand, components: comps, async: c.NoWait})
 
 	if err != nil {
-		return err
+		if _, ok := err.(*snap.AlreadyInstalledError); !ok {
+			return err
+		}
+	}
+
+	if len(affectedComponents) < len(comps) {
+		for _, comp := range comps {
+			if !strutil.ListContains(affectedComponents, comp) {
+				msg := fmt.Sprintf(i18n.G(`snapctl: component %q is already installed`), comp)
+				fmt.Fprintln(c.stderr, msg)
+			}
+		}
 	}
 
 	if c.NoWait {

overlord/hookstate/ctlcmd/remove.go

@@ -55,7 +55,7 @@ func (c *removeCommand) Execute([]string) error {
 		return err
 	}
 
-	id, err := runSnapManagementCommand(ctx, managementCommand{operation: removeManagementCommand, components: comps, async: c.NoWait})
+	id, _, err := runSnapManagementCommand(ctx, managementCommand{operation: removeManagementCommand, components: comps, async: c.NoWait})
 	if err != nil {
 		return err
 	}

overlord/hookstate/ctlcmd/snap_management_test.go

@@ -35,9 +35,11 @@ import (
 	"github.com/snapcore/snapd/overlord/hookstate/hooktest"
 	"github.com/snapcore/snapd/overlord/ifacestate/ifacerepo"
 	"github.com/snapcore/snapd/overlord/snapstate"
+	"github.com/snapcore/snapd/overlord/snapstate/sequence"
 	"github.com/snapcore/snapd/overlord/snapstate/snapstatetest"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/snap/naming"
 	"github.com/snapcore/snapd/snap/snaptest"
 	"github.com/snapcore/snapd/testutil"
 )
@@ -362,3 +364,141 @@ func (s *installSuite) TestNoWaitInstallAndRemoveCommands(c *C) {
 		s.st.Unlock()
 	}
 }
+
+func (s *installSuite) TestInstallWithParallelInstalledSnap(c *C) {
+	s.st.Lock()
+	s.chg = s.st.NewChange("install change", "install change")
+	task := s.st.NewTask("test-task", "my test task")
+	s.chg.AddTask(task)
+	setup := &hookstate.HookSetup{Snap: "test-snap_foo", Revision: snap.R(1), Hook: "test-hook"}
+
+	// create a context for the parallel installed snap
+	var err error
+	s.mockContext, err = hookstate.NewContext(task, task.State(), setup, s.mockHandler, "")
+	c.Assert(err, IsNil)
+
+	installTask := s.st.NewTask("queued", "queued task")
+	s.st.Unlock()
+
+	restore := ctlcmd.MockSnapstateInstallComponentsFunc(func(ctx context.Context, st *state.State, names []string, info *snap.Info, vsets *snapasserts.ValidationSets, opts snapstate.Options) ([]*state.TaskSet, error) {
+		c.Check(names, DeepEquals, []string{"two"})
+		c.Check(opts, DeepEquals, snapstate.Options{ExpectOneSnap: true,
+			ConflictOptions: snapstate.ConflictOptions{FromChange: s.mockContext.ChangeID()}})
+		var ts state.TaskSet
+		ts.AddTask(installTask)
+		return []*state.TaskSet{&ts}, nil
+	})
+	defer restore()
+
+	rev := snap.R(1)
+	si := &snap.SideInfo{
+		RealName: "test-snap",
+		Revision: rev,
+		SnapID:   "test-snap-id",
+	}
+
+	seq := snapstatetest.NewSequenceFromRevisionSideInfos([]*sequence.RevisionSideState{
+		sequence.NewRevisionSideState(si, nil),
+	})
+
+	// component +one is already installed
+	seq.AddComponentForRevision(snap.R(1), sequence.NewComponentState(&snap.ComponentSideInfo{
+		Component: naming.NewComponentRef("test-snap", "one"),
+		Revision:  snap.R(1),
+	}, snap.StandardComponent))
+
+	s.st.Lock()
+	snapstate.Set(s.st, "test-snap_foo", &snapstate.SnapState{
+		Active:   true,
+		Sequence: seq,
+		Current:  rev,
+	})
+	s.st.Unlock()
+
+	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"install", "+one", "+two"}, 0, nil)
+	c.Check(err, IsNil)
+	c.Check(stdout, HasLen, 0)
+	c.Check(string(stderr), Matches, `(?sm).*snapctl: component "one" is already installed`)
+}
+
+func (s *installSuite) TestInstallAllAlreadyInstalled(c *C) {
+	rev := snap.R(1)
+	si := &snap.SideInfo{
+		RealName: "test-snap",
+		Revision: rev,
+		SnapID:   "test-snap-id",
+	}
+
+	seq := snapstatetest.NewSequenceFromRevisionSideInfos([]*sequence.RevisionSideState{
+		sequence.NewRevisionSideState(si, nil),
+	})
+
+	seq.AddComponentForRevision(snap.R(1), sequence.NewComponentState(&snap.ComponentSideInfo{
+		Component: naming.NewComponentRef("test-snap", "one"),
+		Revision:  snap.R(1),
+	}, snap.StandardComponent))
+
+	seq.AddComponentForRevision(snap.R(1), sequence.NewComponentState(&snap.ComponentSideInfo{
+		Component: naming.NewComponentRef("test-snap", "two"),
+		Revision:  snap.R(1),
+	}, snap.StandardComponent))
+
+	s.st.Lock()
+	snapstate.Set(s.st, "test-snap", &snapstate.SnapState{
+		Active:   true,
+		Sequence: seq,
+		Current:  rev,
+	})
+	s.st.Unlock()
+
+	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"install", "+one", "+two"}, 0, nil)
+	c.Check(err, IsNil)
+	c.Check(stdout, HasLen, 0)
+	c.Check(string(stderr), Matches, `(?sm).*snapctl: component "one" is already installed`)
+	c.Check(string(stderr), Matches, `(?sm).*snapctl: component "two" is already installed`)
+}
+
+func (s *installSuite) TestInstallSomeAlreadyInstalled(c *C) {
+	s.st.Lock()
+	task := s.st.NewTask("queued", "queued task")
+	s.st.Unlock()
+
+	restore := ctlcmd.MockSnapstateInstallComponentsFunc(func(ctx context.Context, st *state.State, names []string, info *snap.Info, vsets *snapasserts.ValidationSets, opts snapstate.Options) ([]*state.TaskSet, error) {
+		c.Check(names, DeepEquals, []string{"two"})
+		c.Check(opts, DeepEquals, snapstate.Options{ExpectOneSnap: true,
+			ConflictOptions: snapstate.ConflictOptions{FromChange: s.mockContext.ChangeID()}})
+		var ts state.TaskSet
+		ts.AddTask(task)
+		return []*state.TaskSet{&ts}, nil
+	})
+	defer restore()
+
+	rev := snap.R(1)
+	si := &snap.SideInfo{
+		RealName: "test-snap",
+		Revision: rev,
+		SnapID:   "test-snap-id",
+	}
+
+	seq := snapstatetest.NewSequenceFromRevisionSideInfos([]*sequence.RevisionSideState{
+		sequence.NewRevisionSideState(si, nil),
+	})
+
+	seq.AddComponentForRevision(snap.R(1), sequence.NewComponentState(&snap.ComponentSideInfo{
+		Component: naming.NewComponentRef("test-snap", "one"),
+		Revision:  snap.R(1),
+	}, snap.StandardComponent))
+
+	s.st.Lock()
+	snapstate.Set(s.st, "test-snap", &snapstate.SnapState{
+		Active:   true,
+		Sequence: seq,
+		Current:  rev,
+	})
+
+	s.st.Unlock()
+	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"install", "+one", "+two"}, 0, nil)
+	c.Check(err, IsNil)
+	c.Check(stdout, HasLen, 0)
+	c.Check(string(stderr), Matches, `snapctl: component "one" is already installed\n`)
+}

tests/main/component-snapctl/task.yaml

@@ -22,3 +22,27 @@ execute: |
 
   snap set test-snap-components-snapctl command="remove +one+two"
   test ! -d "$SNAP_MOUNT_DIR"/test-snap-components-snapctl/components/
+
+  echo "Check already installed snap exits with code 0"
+  snap run --shell test-snap-components-snapctl -c "snapctl install +one" 1>stdout.out 2>stderr.out
+  test ! -s stdout.out
+  test ! -s stderr.out
+  snap components test-snap-components-snapctl | MATCH "test-snap-components-snapctl\+one\s+installed\s+test"
+  snap components test-snap-components-snapctl | MATCH "test-snap-components-snapctl\+two\s+available\s+test"
+
+  snap run --shell test-snap-components-snapctl -c "snapctl install +one" 1>stdout.out 2>stderr.out
+  test ! -s stdout.out
+  MATCH "snapctl: component \"one\" is already installed" < stderr.out
+
+  echo "Check new component can be installed when listed with already installed component"
+  snap run --shell test-snap-components-snapctl -c "snapctl install +one +two" 1>stdout.out 2>stderr.out
+  test ! -s stdout.out
+  MATCH "snapctl: component \"one\" is already installed" < stderr.out
+  snap components test-snap-components-snapctl | MATCH "test-snap-components-snapctl\+one\s+installed\s+test"
+  snap components test-snap-components-snapctl | MATCH "test-snap-components-snapctl\+two\s+installed\s+test"
+
+  snap run --shell test-snap-components-snapctl -c "snapctl install +one +two" 1>stdout.out 2>stderr.out
+  test ! -s stdout.out
+  MATCH "snapctl: component \"one\" is already installed" < stderr.out
+  MATCH "snapctl: component \"two\" is already installed" < stderr.out
+