many: support adding local components to seeds #14149

alfonsosanchezbeato · 2024-07-01T21:28:20Z

This is a first PR that adds support for seeding components. The parts targeted by this PR are:

Add support for components in options.yaml
Add --comp option to snap prepare-image, for local component files of component names
Copy local component files to seed

Interactions with the store are not in this PR.

andrewphelpsj

Thank you, some initial comments.

seed/seedwriter/seed20.go

andrewphelpsj · 2024-07-02T16:05:02Z

seed/seedwriter/writer.go

@@ -655,13 +715,14 @@ func (w *Writer) InfoDerived() error {

 // SetInfo sets Info of the SeedSnap and possibly computes its
 // destination Path.
-func (w *Writer) SetInfo(sn *SeedSnap, info *snap.Info) error {
+func (w *Writer) SetInfo(sn *SeedSnap, info *snap.Info, seedComps []SeedComponent) error {


This is maybe a little strange? Passing in a param to just set a field on the SeedSnap.

In the end I decided to add a new method, Writer.AddComponentsToSnap to avoid mixing concerns with SetInfo. Although it just sets a variable.

andrewphelpsj · 2024-07-02T16:33:02Z

seed/seedwriter/writer.go

@@ -105,6 +128,16 @@ type SeedSnap struct {
 	local      bool
 	modelSnap  *asserts.ModelSnap
 	optionSnap *OptionsSnap
+
+	Components []SeedComponent


After reading the code some more, I see why this is you used SetInfo to initialize this field, since there is already this deferred initialization of parts of this type. Maybe a comment here on this field, and move this next to the Info field that is in the same boat.

I've moved and added a comment.

alfonsosanchezbeato

@andrewphelpsj thanks for the review, I've addressed your comments now.

seed/seedwriter/seed20.go

alfonsosanchezbeato · 2024-07-03T15:32:51Z

seed/seedwriter/writer.go

@@ -105,6 +128,16 @@ type SeedSnap struct {
 	local      bool
 	modelSnap  *asserts.ModelSnap
 	optionSnap *OptionsSnap
+
+	Components []SeedComponent


I've moved and added a comment.

alfonsosanchezbeato · 2024-07-03T15:34:47Z

seed/seedwriter/writer.go

@@ -655,13 +715,14 @@ func (w *Writer) InfoDerived() error {

 // SetInfo sets Info of the SeedSnap and possibly computes its
 // destination Path.
-func (w *Writer) SetInfo(sn *SeedSnap, info *snap.Info) error {
+func (w *Writer) SetInfo(sn *SeedSnap, info *snap.Info, seedComps []SeedComponent) error {


In the end I decided to add a new method, Writer.AddComponentsToSnap to avoid mixing concerns with SetInfo. Although it just sets a variable.

andrewphelpsj

Thanks for the changes, looks good to me.

pedronis

did a first pass, some questions

pedronis · 2024-07-05T12:16:31Z

seed/internal/options20.go

@@ -31,6 +31,14 @@ import (
 	"github.com/snapcore/snapd/snap/naming"
 )

+// ComponentOpt are the options for components for grade: dangerous.
+type ComponentOpt struct {


why Opt in the name?

Change to Component as I see that we have also internal.Snap20.

pedronis · 2024-07-05T12:23:29Z

image/image_linux.go

+			delete(pathToLocalComp, path)
+		}
+
+		s.w.AddComponentsToSnap(sn, seedComps)


I'm also a bit unsure about this API, shouldn't some of these checks be done by the writer instead?

Agreed, I've moved some checks to the writer.

pedronis · 2024-07-05T12:26:00Z

image/image_linux.go

@@ -451,14 +451,15 @@ func (s *imageSeeder) validateSnapArchs(snaps []*seedwriter.SeedSnap) error {

 type localSnapRefs map[*seedwriter.SeedSnap][]*asserts.Ref

-func (s *imageSeeder) deriveInfoForLocalSnaps(f seedwriter.SeedAssertionFetcher, db *asserts.Database) (localSnapRefs, error) {
+func (s *imageSeeder) deriveInfoForLocalSnaps(f seedwriter.SeedAssertionFetcher, db *asserts.Database, pathToLocalComp map[string]*snap.ComponentInfo) (localSnapRefs, error) {
 	localSnaps, err := s.w.LocalSnaps()


should there be something like LocalComponents() as well? we will symmetrically also be told what components to download to follow how the non-local case work? There's a symmetry between LocalSnaps() and SnapsToDownload that we should try to follow

I am trying to include information about the components in the snap structures, so LocalSnaps() should be enough as SeedSnap contains a slice of SeedComponent. The problem that I have here is that to fill that slice I have to do it in deriveInfoForLocalSnaps as in w.SetOptionsSnaps I cannot match local components to local snaps as I do not know the snap name until later, and I am trying to avoid a double read of the squashfs file.

This could be solved by some refactoring of the writer stages (running w.Start before w.SetOptionsSnap), but I did not want to put this in this PR as it was big enough.

The []*SeedSnap returned by SnapsToDownload would also include needed components inside as required.

my point is a bit different is not clear that we shouldn't let the writer manage something like pathToLocalComp internally? and let it do a bit more of the matching work as well, while letting the caller do a smaller part of the work. What I'm saying is that maybe SetOptionsSnaps should also take a list of OptionsComponent?

Ok, I've pushed now more things to the writer and passed down pathToLocalComp so it can be used when we know the names of the local snap files. I think that passing as OptionComponent would complicate things a bit as then we would need to get back the local components to image to read the files, and also, we have already consumed the non-local related component options so it would not be the full options actually.

alfonsosanchezbeato

@pedronis thanks for the review, I've addressed your comments now

alfonsosanchezbeato · 2024-07-08T14:35:37Z

seed/internal/options20.go

@@ -31,6 +31,14 @@ import (
 	"github.com/snapcore/snapd/snap/naming"
 )

+// ComponentOpt are the options for components for grade: dangerous.
+type ComponentOpt struct {


Change to Component as I see that we have also internal.Snap20.

alfonsosanchezbeato · 2024-07-08T15:18:46Z

image/image_linux.go

@@ -451,14 +451,15 @@ func (s *imageSeeder) validateSnapArchs(snaps []*seedwriter.SeedSnap) error {

 type localSnapRefs map[*seedwriter.SeedSnap][]*asserts.Ref

-func (s *imageSeeder) deriveInfoForLocalSnaps(f seedwriter.SeedAssertionFetcher, db *asserts.Database) (localSnapRefs, error) {
+func (s *imageSeeder) deriveInfoForLocalSnaps(f seedwriter.SeedAssertionFetcher, db *asserts.Database, pathToLocalComp map[string]*snap.ComponentInfo) (localSnapRefs, error) {
 	localSnaps, err := s.w.LocalSnaps()


I am trying to include information about the components in the snap structures, so LocalSnaps() should be enough as SeedSnap contains a slice of SeedComponent. The problem that I have here is that to fill that slice I have to do it in deriveInfoForLocalSnaps as in w.SetOptionsSnaps I cannot match local components to local snaps as I do not know the snap name until later, and I am trying to avoid a double read of the squashfs file.

This could be solved by some refactoring of the writer stages (running w.Start before w.SetOptionsSnap), but I did not want to put this in this PR as it was big enough.

The []*SeedSnap returned by SnapsToDownload would also include needed components inside as required.

alfonsosanchezbeato · 2024-07-08T15:32:13Z

image/image_linux.go

+			delete(pathToLocalComp, path)
+		}
+
+		s.w.AddComponentsToSnap(sn, seedComps)


Agreed, I've moved some checks to the writer.

alfonsosanchezbeato requested review from pedronis and andrewphelpsj July 1, 2024 21:28

alfonsosanchezbeato added 4 commits July 1, 2024 19:08

seed: add supports for local components

d18d2fb

o/devicestate: adapt to writer changes

fe89eb7

image: support for local components

2b1827e

cmd/cmd_prepare_image: support for components option

4cf73ca

alfonsosanchezbeato force-pushed the comp-add-to-seed branch from 1cc6121 to 4cf73ca Compare July 1, 2024 23:08

andrewphelpsj reviewed Jul 2, 2024

View reviewed changes

alfonsosanchezbeato added 3 commits July 3, 2024 11:30

fixup! seed: add supports for local components

4d99cb0

fixup! o/devicestate: adapt to writer changes

e60d12f

fixup! image: support for local components

d9e1096

alfonsosanchezbeato commented Jul 3, 2024

View reviewed changes

alfonsosanchezbeato requested a review from andrewphelpsj July 3, 2024 15:35

andrewphelpsj approved these changes Jul 3, 2024

View reviewed changes

pedronis reviewed Jul 5, 2024

View reviewed changes

alfonsosanchezbeato added 2 commits July 8, 2024 11:34

fixup! seed: add supports for local components

3702734

fixup! image: support for local components

619e807

alfonsosanchezbeato commented Jul 8, 2024

View reviewed changes

alfonsosanchezbeato requested a review from pedronis July 8, 2024 15:35

alfonsosanchezbeato added 4 commits July 9, 2024 16:28

fixup! image: support for local components

80dd045

fixup! o/devicestate: adapt to writer changes

8b3129f

fixup! seed: add supports for local components

bbcf04e

fixup! seed: add supports for local components

5cf88ed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: support adding local components to seeds #14149

many: support adding local components to seeds #14149

alfonsosanchezbeato commented Jul 1, 2024

andrewphelpsj left a comment

andrewphelpsj Jul 2, 2024

alfonsosanchezbeato Jul 3, 2024

andrewphelpsj Jul 2, 2024

alfonsosanchezbeato Jul 3, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jul 3, 2024

alfonsosanchezbeato Jul 3, 2024

andrewphelpsj left a comment

pedronis left a comment

pedronis Jul 5, 2024

alfonsosanchezbeato Jul 8, 2024

pedronis Jul 5, 2024

alfonsosanchezbeato Jul 8, 2024

pedronis Jul 5, 2024

alfonsosanchezbeato Jul 8, 2024

pedronis Jul 9, 2024

alfonsosanchezbeato Jul 9, 2024 •

edited

Loading

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jul 8, 2024

alfonsosanchezbeato Jul 8, 2024

alfonsosanchezbeato Jul 8, 2024

many: support adding local components to seeds #14149

Are you sure you want to change the base?

many: support adding local components to seeds #14149

Conversation

alfonsosanchezbeato commented Jul 1, 2024

andrewphelpsj 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

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj 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

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

alfonsosanchezbeato Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

alfonsosanchezbeato 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

alfonsosanchezbeato Jul 9, 2024 •

edited

Loading