o/snapstate: refactor Install functions to use a singular implementation that operates on a Target #13949

andrewphelpsj · 2024-05-06T19:47:11Z

The Install family of functions in the snapstate has grown, and there are a handful of functions now that increase our API's surface area.

Since a lot of these functions look really similar, we can change up the public API a bit to allow for a single InstallTarget function that operates on a Target, which controls which snaps are installed, and where they come from. In this PR, I've implemented this for installing snaps from the store and local snaps.

I was able to implement Install, InstallMany, InstallPath, InstallWithDeviceContext, and InstallPathWithDeviceContext using the new InstallTarget function. There are two target implementations in this PR, StoreTarget and PathTarget. StoreTarget allows the installation of one or many snaps from the store. PathTarget allows the installation of one local snap. Note that PathTarget does not support installing many local snaps at once, since that looks much more like an update, and should be included in a potential refactor of the Update family.

There is very little change to the current public API of the package, as I was able to implement all of the functions mentioned in terms of the new functions. The only minor change needed, outside of the functions themselves, are the changes inside of functions called from the doPrerequisites task handler. This handler manually set up some lanes for the installed snaps, since InstallWithDeviceContext did not support lanes. This change enables InstallWithDeviceContext to support lanes.

Also something that I took note of while I was in here that may not need to be addressed. We don't currently care about validation sets right now when installing local snaps. Should that happen? If so, I could move that code from the StoreTarget implementation to the InstallTarget function so that all installs always care about validation sets.

…egatives in code

…s follow correct pattern when returning no error

…terfaces, which controls how snaps are found

InstallPathMany is omitted for now, since it is more of an update rather than an installation.

…ality in InstallWithDeviceContext

MiguelPires

Thanks for this. I left a few comments but this looks much better

overlord/snapstate/target.go

MiguelPires · 2024-05-09T13:14:56Z

overlord/snapstate/target.go

+// All of these fields are optional and can be left unset. The options in this
+// struct apply to all snaps that are part of an operation. Options that apply
+// to individual snaps can be found in RevisionOptions.
+type SnapstateOptions struct {


Since this is already in snapstate I don't think we need to repeat it in the name. From the outside it looks like snapstate.SnapstateOptions vs snapstate.Options

Good call, I had the same thought when I started using it to implement installing components from the store. Although, Samuele suggested that this type might not be something we want at all.

MiguelPires · 2024-05-09T13:38:47Z

overlord/snapstate/target.go

+	return nil
+}
+
+func InstallOne(ctx context.Context, st *state.State, target Target, opts SnapstateOptions) (*snap.Info, *state.TaskSet, error) {


Is the goal to fully replace the previous public functions with these later or to keep those as wrappers around these? Because if it's the latter, then these can be unexported

I think the goal will be to replace it eventually. For example, when installing components (the motivator for this change), the old API won't get that functionality.

overlord/snapstate/target.go

MiguelPires · 2024-05-09T14:39:27Z

overlord/snapstate/target.go

+// verify that StoreTarget implements the Target interface
+var _ Target = &StoreTarget{}


Do we need these explicit checks? My impression is that this goes against the point of implicit interface implementation. We have them in other places so other people may disagree but I'd say this check is done just by using StoreTarget and if it's not it's because we're not using the interface in which case it doesn't matter

We don't really need them, no. The main reason I added them in the first place was to verify that &PathTarget{} is an OptionInitializer, which wouldn't be compile-time checked anywhere else.

overlord/snapstate/target.go

MiguelPires · 2024-05-09T16:10:51Z

overlord/snapstate/target.go

+	return nil
+}
+
+func InstallOne(ctx context.Context, st *state.State, target Target, opts SnapstateOptions) (*snap.Info, *state.TaskSet, error) {


Comparing InstallOne with InstallTarget by their names, it's not clear which one does what. Especially since Target can be one or many snaps

Hrm, you're right. I'll think about names. In an ideal world, InstallTarget will become Install one day, once the old API stops being used.

overlord/snapstate/target.go

Co-authored-by: Miguel Pires <miguelpires94@gmail.com>

pedronis

thank you, did a pass, direction looks good, there is some cleaning up to do though

overlord/snapstate/handlers_prereq_test.go

pedronis · 2024-05-27T15:26:14Z

overlord/snapstate/handlers.go

+		flags.Lane = lanes[0]
+	} else {
+		flags.Transaction = client.TransactionPerSnap
+	}


this logic replaces the one below using joinLane?

Correct, now that InstallWithDeviceContext supports lanes, we can use that rather than manually doing it here.

overlord/snapstate/target.go

pedronis · 2024-05-27T15:50:54Z

overlord/snapstate/target.go

+
+// Installables returns the data needed to setup the snaps from the store for
+// installation.
+func (s *StoreTarget) Installables(ctx context.Context, st *state.State, installedSnaps map[string]*SnapState, opts Options) ([]Installable, error) {


this is mostly replacing installCandidates?

Yes, that is correct.

overlord/snapstate/target.go

overlord/hookstate/ctlcmd/services_test.go

…tErr when only one result is expected

…olean flag in Options that allows caller to skip seeded check

pedronis

thanks for the changes, did another quick pass with some comments on target.go

overlord/snapstate/target.go

pedronis · 2024-05-30T09:24:06Z

overlord/snapstate/target.go

+	if len(infos) != 1 || len(tasksets) != 1 {
+		return nil, nil, errors.New("internal error: expected exactly one snap and taskset")
+	}


this is a bit strange because one would expect that we could check whether goal return one target or not earlier

Yes, this is a bit annoying. I was trying to avoid a flag being passed into InstallWithGoal to check that. One option is some implementation of InstallGoal that wraps another InstallGoal that makes sure that we return a singular Target.

Might be a bit too complex though for something that really could just be an if.

This can also be handled with the RequireOneSnap option introduced in 938cd26.

pedronis · 2024-05-30T09:24:44Z

overlord/snapstate/target.go

+}
+
+// Target represents the data needed to setup a snap for installation.
+type Target struct {


as we discussed, should we make more things unexported ?

If this is unexported, then the interface should be unexported as well.

Done in bda59b7.

pedronis · 2024-05-30T09:26:34Z

overlord/snapstate/target.go

+
+	results, _, err := str.SnapAction(context.TODO(), curSnaps, actions, nil, user, refreshOpts)
+	if err != nil {
+		if len(actions) == 1 {


I'm not sure this is quite the right approach, passing 1 snap to a Many function previously has different error behavior than passing it to a single target function. We might need to have a Single flag in options or something to control this better

I was really trying to avoid having to do that, since it'd be really nice if the single case is just a single iteration of the many case. Are there more differences than this error string right here?

Added in 938cd26.

…assing in empty list of actions

…hen using snapstate.Install vs snapstate.InstallMany

… are only installing one snap AND allow us to choose how to generate errors

…lWithGroup

…Many (#14027) Install and InstallMany have subtle differences in how they handle the case where the store provides an empty response. Install -> returns a snapstate.ErrMissingExpectedResult InstallMany -> returns a store.SnapActionError Add tests to define this behavior, and thus requiring #13949 to conform to this behavior.

pedronis · 2024-05-31T07:56:04Z

@andrewphelpsj there is a conflict now

pedronis

thanks for the changes, couple quick comments

overlord/snapstate/target.go

…r than concrete types

pedronis

thank you, did another pass, some final minor things

overlord/snapstate/target.go

snap/revision.go

overlord/devicestate/firstboot.go

main issues are solved

…Goal

…efault to SideInfo.RealName

pedronis

thank you

pedronis · 2024-06-05T09:04:56Z

overlord/snapstate/target.go

+}
+
+// PathInstallGoal creates a new InstallGoal to install a snap from a given from
+// a path on disk. If instanceName is not provided, si.RealName will be used.


would "empty" be clearer than "is not provided"?

MiguelPires

Thank you, LGTM. Just some minor questions

overlord/snapstate/target.go

MiguelPires · 2024-06-05T11:16:48Z

overlord/snapstate/target.go

+//
+// TODO: rename this to Install once the API is settled, and we can rename or
+// remove the old Install function.
+func InstallWithGoal(ctx context.Context, st *state.State, goal InstallGoal, opts Options) ([]*snap.Info, []*state.TaskSet, error) {


Not really a problem but I'm just wondering if the plan is to keep the Install* functions in target.go? They feel a bit out of place. I expected target.go to have functions and interfaces that create installation goals but snapstate.go to have the functions actually install (handling tasksets and etc). Or is this more to split new functions from old for now?

I discussed with @andrewphelpsj to create an install.go file as well, maybe it could an immediate follow-up

Yeah, I'll move things around in a follow-up.

overlord/snapstate/target.go

andrewphelpsj requested review from pedronis, alfonsosanchezbeato and MiguelPires May 6, 2024 19:47

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label May 6, 2024

andrewphelpsj added 5 commits May 7, 2024 14:57

snap: add method for checking if a revision is set to reduce double n…

0b85cf8

…egatives in code

o/snapstate, o/hookstate: make mocked snapstate.EnforcedValidationSet…

39d4089

…s follow correct pattern when returning no error

o/snapstate: add new function InstallTarget that operates a Target in…

74dd241

…terfaces, which controls how snaps are found

o/snapstate: implement Install family of functions with InstallTarget

ebf6331

InstallPathMany is omitted for now, since it is more of an update rather than an installation.

o/snapstate: update perquisites task handler to use new lane function…

6a240ff

…ality in InstallWithDeviceContext

andrewphelpsj force-pushed the snapstate-refactor branch from ca87521 to 6a240ff Compare May 7, 2024 18:58

MiguelPires reviewed May 9, 2024

View reviewed changes

andrewphelpsj and others added 5 commits May 9, 2024 12:26

o/snapstate: doc comment fixes

2dc2c1b

Co-authored-by: Miguel Pires <miguelpires94@gmail.com>

o/snapstate: add license header to target.go

cfca07e

o/snapstate: rename SnapstateOptions to Options

6ffa306

o/snapstate: rename interface parameter to better indicate use

dfacb8d

o/snapstate: make laneGenerator a standalone function

ded554b

pedronis previously requested changes May 27, 2024

View reviewed changes

andrewphelpsj added 6 commits May 28, 2024 15:19

o/snapstate: fix redundant error message by calling singleActionResul…

5f645d4

…tErr when only one result is expected

o/snapstate: doc comments on new target functions/types

e366a4e

o/snapstate: rename StoreSnap.Name to StoreSnap.InstanceName

a3607df

o/snapstate: rename laneGenerator -> generateLane

1df8002

o/snapstate: test for error when installing an already installed snap

c00de38

o/snapstate: add test for invalid revision options

667d4f6

andrewphelpsj force-pushed the snapstate-refactor branch from 0851b53 to 667d4f6 Compare May 28, 2024 19:20

andrewphelpsj added 3 commits May 28, 2024 16:36

o/snapstate: remove awkward installed snaps param from Target interface

5086859

o/devicestate, o/snapstate: replace OptionInitializer with simpler bo…

58ae5b8

…olean flag in Options that allows caller to skip seeded check

o/devicestate, o/snapstate: rename new types introduced in refactor

b46b9cc

andrewphelpsj requested review from pedronis and MiguelPires May 29, 2024 15:28

pedronis reviewed May 30, 2024

View reviewed changes

o/snapstate: rename InstallGoal.Install to InstallGoal.ToInstall

5b27df5

andrewphelpsj mentioned this pull request May 30, 2024

o/snapstate: test for specific errors returned by Install and InstallMany #14027

Merged

andrewphelpsj added 4 commits May 30, 2024 12:05

o/snapstate: make fake store match behavior of real store impl when p…

efc0fef

…assing in empty list of actions

o/snapstate: add tests to define the difference in resulting errors w…

b70d4c2

…hen using snapstate.Install vs snapstate.InstallMany

o/snapstate: introduce Options.RequireOneSnap flag to require that we…

938cd26

… are only installing one snap AND allow us to choose how to generate errors

o/snapstate: reduce number of exported fields/types related to Instal…

bda59b7

…lWithGroup

andrewphelpsj requested a review from pedronis May 30, 2024 16:47

pedronis reviewed May 31, 2024

View reviewed changes

overlord/snapstate/target.go Outdated Show resolved Hide resolved

overlord/snapstate/target.go Outdated Show resolved Hide resolved

andrewphelpsj added 3 commits June 3, 2024 09:41

o/snapstate: re-export the InstallGoal interface

9101051

o/snapstate: make functions that create goals return interfaces rathe…

8705135

…r than concrete types

o/snapstate: rename StoreGoal to StoreInstallGoal

f9e1bba

andrewphelpsj requested a review from pedronis June 3, 2024 13:43

pedronis reviewed Jun 4, 2024

View reviewed changes

andrewphelpsj added 4 commits June 4, 2024 13:00

o/snapstate: rename RequireOneSnap to ExpectOneSnap

d9ab79f

o/snapstate: update doc commentsa on StoreInstallGoal and PathInstall…

63ab523

…Goal

snap, o/snapstate: remove snap.R.Set helper method

c331ad3

o/devicestate: replace unneeded param with empty string, which will d…

17af92f

…efault to SideInfo.RealName

andrewphelpsj requested a review from pedronis June 4, 2024 17:07

pedronis approved these changes Jun 5, 2024

View reviewed changes

MiguelPires approved these changes Jun 5, 2024

View reviewed changes

andrewphelpsj added 2 commits June 5, 2024 11:36

o/snapstate: manually re-lock lock earlier, avoid golangci-lint error

ff1c4e8

o/snapstate: replace manually building SnapSetup with method on target

f73a573

andrewphelpsj force-pushed the snapstate-refactor branch from ce46744 to f73a573 Compare June 5, 2024 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/snapstate: refactor Install functions to use a singular implementation that operates on a Target #13949

o/snapstate: refactor Install functions to use a singular implementation that operates on a Target #13949

andrewphelpsj commented May 6, 2024

MiguelPires left a comment

MiguelPires May 9, 2024

andrewphelpsj May 9, 2024

MiguelPires May 9, 2024

andrewphelpsj May 9, 2024

MiguelPires May 9, 2024

andrewphelpsj May 9, 2024

MiguelPires May 9, 2024

andrewphelpsj May 9, 2024

pedronis left a comment

pedronis May 27, 2024

andrewphelpsj May 28, 2024

pedronis May 27, 2024

andrewphelpsj May 28, 2024

pedronis left a comment

pedronis May 30, 2024

andrewphelpsj May 30, 2024

andrewphelpsj May 30, 2024

andrewphelpsj May 30, 2024

pedronis May 30, 2024

andrewphelpsj May 30, 2024

andrewphelpsj May 30, 2024

pedronis May 30, 2024

andrewphelpsj May 30, 2024

andrewphelpsj May 30, 2024

pedronis commented May 31, 2024

pedronis left a comment

pedronis left a comment

pedronis left a comment

pedronis Jun 5, 2024

MiguelPires left a comment

MiguelPires Jun 5, 2024

pedronis Jun 5, 2024

andrewphelpsj Jun 5, 2024

		// verify that StoreTarget implements the Target interface
		var _ Target = &StoreTarget{}

o/snapstate: refactor Install functions to use a singular implementation that operates on a Target #13949

Are you sure you want to change the base?

o/snapstate: refactor Install functions to use a singular implementation that operates on a Target #13949

Conversation

andrewphelpsj commented May 6, 2024

MiguelPires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented May 31, 2024

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelPires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment