From 10960035984b3d843e012d348d4b9ad412eb92d0 Mon Sep 17 00:00:00 2001 From: "Brian C. Lane" Date: Tue, 18 Oct 2022 14:23:54 -0700 Subject: [PATCH] store: Fix loading cross distro compose results When the store is written to disk it simplifies the ImageBuild details into a simple image type string. This works fine for composes that match the host's distro but isn't enough detail to load composes made for other distros, especially if the image type name isn't supported on the host. This results in cross distro compose results being lost after a reboot. This fix uses the distro information from the compose's blueprint to determine which distro the image type should be loaded from. It assumes that the architecture matches the hosts' arch -- this is currently always true but in the future if cross-arch builds are added it will need to be addressed in a different way. newComposeFromV0, newComposesFromV0, and newStoreFromV0 now take a pointer to the full distro registry instead of an Arch, this allows them to access the correct image types for the distro selected by the blueprint. When loading the composes from disk the blueprint distro is loaded from the registry before checking the image type string. This means that we do not have to change the store version or on disk format, the only thing changing is how it decides to populate the ImageBuild when reloading the store. A number of tests use a fake test distro using fake architecture names. These tests have been adjusted to use a fake distro registry with overridden host architecture that matches the fake one. --- cmd/osbuild-store-dump/main.go | 4 +- internal/distro/test_distro/distro.go | 13 +++++ internal/store/fixtures.go | 26 ++++++---- internal/store/json.go | 31 +++++++++--- internal/store/json_test.go | 72 +++++++++++++++------------ internal/store/store.go | 5 +- internal/store/store_test.go | 5 +- internal/weldr/api.go | 5 +- 8 files changed, 106 insertions(+), 55 deletions(-) diff --git a/cmd/osbuild-store-dump/main.go b/cmd/osbuild-store-dump/main.go index 239058598..3019b3701 100644 --- a/cmd/osbuild-store-dump/main.go +++ b/cmd/osbuild-store-dump/main.go @@ -12,6 +12,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/distro/fedora" + "github.com/osbuild/osbuild-composer/internal/distroregistry" "github.com/osbuild/osbuild-composer/internal/dnfjson" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/store" @@ -142,7 +143,8 @@ func main() { } rpmmdCache := path.Join(homeDir, ".cache/osbuild-composer/rpmmd") - s := store.New(&cwd, a, nil) + dr, _ := distroregistry.New(d, nil) + s := store.New(&cwd, dr, nil) if s == nil { panic("could not create store") } diff --git a/internal/distro/test_distro/distro.go b/internal/distro/test_distro/distro.go index 3fbc35db0..029d4f13b 100644 --- a/internal/distro/test_distro/distro.go +++ b/internal/distro/test_distro/distro.go @@ -9,6 +9,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/container" "github.com/osbuild/osbuild-composer/internal/distro" + "github.com/osbuild/osbuild-composer/internal/distroregistry" "github.com/osbuild/osbuild-composer/internal/osbuild" "github.com/osbuild/osbuild-composer/internal/rpmmd" ) @@ -344,6 +345,18 @@ func New() *TestDistro { return newTestDistro(TestDistroName, TestDistroModulePlatformID, TestDistroReleasever) } +func NewRegistry() *distroregistry.Registry { + td := New() + registry, err := distroregistry.New(td, td) + if err != nil { + panic(err) + } + + // Override the host's architecture name with the test's name + registry.SetHostArchName(TestArchName) + return registry +} + // New2 returns new instance of TestDistro named "test-distro-2". func New2() *TestDistro { return newTestDistro(TestDistro2Name, TestDistro2ModulePlatformID, TestDistro2Releasever) diff --git a/internal/store/fixtures.go b/internal/store/fixtures.go index dff422659..93d936a66 100644 --- a/internal/store/fixtures.go +++ b/internal/store/fixtures.go @@ -41,7 +41,8 @@ func FixtureBase() *Store { }, } - d := test_distro.New() + dr := test_distro.NewRegistry() + d := dr.FromHost() arch, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err)) @@ -54,7 +55,8 @@ func FixtureBase() *Store { if err != nil { panic(fmt.Sprintf("failed to create a manifest: %v", err)) } - s := New(nil, arch, nil) + + s := New(nil, dr, nil) pkgs := []rpmmd.PackageSpec{ { @@ -176,7 +178,8 @@ func FixtureFinished() *Store { }, } - d := test_distro.New() + dr := test_distro.NewRegistry() + d := dr.FromHost() arch, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err)) @@ -189,7 +192,8 @@ func FixtureFinished() *Store { if err != nil { panic(fmt.Sprintf("failed to create a manifest: %v", err)) } - s := New(nil, arch, nil) + + s := New(nil, dr, nil) pkgs := []rpmmd.PackageSpec{ { @@ -272,13 +276,14 @@ func FixtureEmpty() *Store { Customizations: nil, } - d := test_distro.New() - arch, err := d.GetArch(test_distro.TestArchName) + dr := test_distro.NewRegistry() + d := dr.FromHost() + _, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err)) } - s := New(nil, arch, nil) + s := New(nil, dr, nil) s.blueprints[bName] = b @@ -310,13 +315,14 @@ func FixtureOldChanges() *Store { Customizations: nil, } - d := test_distro.New() - arch, err := d.GetArch(test_distro.TestArchName) + dr := test_distro.NewRegistry() + d := dr.FromHost() + _, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err)) } - s := New(nil, arch, nil) + s := New(nil, dr, nil) s.PushBlueprint(b, "Initial commit") b.Version = "0.0.1" diff --git a/internal/store/json.go b/internal/store/json.go index 424a2ac57..096d73334 100644 --- a/internal/store/json.go +++ b/internal/store/json.go @@ -2,6 +2,7 @@ package store import ( "errors" + "fmt" "log" "sort" "time" @@ -10,6 +11,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/common" "github.com/osbuild/osbuild-composer/internal/distro" + "github.com/osbuild/osbuild-composer/internal/distroregistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/target" ) @@ -96,11 +98,11 @@ func newWorkspaceFromV0(workspaceStruct workspaceV0) map[string]blueprint.Bluepr return workspace } -func newComposesFromV0(composesStruct composesV0, arch distro.Arch, log *log.Logger) map[uuid.UUID]Compose { +func newComposesFromV0(composesStruct composesV0, dr *distroregistry.Registry, log *log.Logger) map[uuid.UUID]Compose { composes := make(map[uuid.UUID]Compose) for composeID, composeStruct := range composesStruct { - c, err := newComposeFromV0(composeStruct, arch) + c, err := newComposeFromV0(composeStruct, dr) if err != nil { if log != nil { log.Printf("ignoring compose: %v", err) @@ -143,15 +145,32 @@ func newImageBuildFromV0(imageBuildStruct imageBuildV0, arch distro.Arch) (Image }, nil } -func newComposeFromV0(composeStruct composeV0, arch distro.Arch) (Compose, error) { +func newComposeFromV0(composeStruct composeV0, dr *distroregistry.Registry) (Compose, error) { if len(composeStruct.ImageBuilds) != 1 { return Compose{}, errors.New("compose with unsupported number of image builds") } + + // Get the distro from the blueprint (empty means use host distro) + bp := composeStruct.Blueprint.DeepCopy() + distroName := bp.Distro + if len(distroName) == 0 { + distroName = dr.FromHost().Name() + } + distro := dr.GetDistro(distroName) + if distro == nil { + return Compose{}, fmt.Errorf("Unknown distro - %s", distroName) + } + + // Get the host distro's architecture. This contains the distro+arch specific image types + arch, err := distro.GetArch(dr.HostArchName()) + if err != nil { + return Compose{}, err + } + ib, err := newImageBuildFromV0(composeStruct.ImageBuilds[0], arch) if err != nil { return Compose{}, err } - bp := composeStruct.Blueprint.DeepCopy() pkgs := make([]rpmmd.PackageSpec, len(composeStruct.Packages)) copy(pkgs, composeStruct.Packages) @@ -234,11 +253,11 @@ func newCommitsFromV0(commitsMapStruct commitsV0, changesMapStruct changesV0) ma return commitsMap } -func newStoreFromV0(storeStruct storeV0, arch distro.Arch, log *log.Logger) *Store { +func newStoreFromV0(storeStruct storeV0, dr *distroregistry.Registry, log *log.Logger) *Store { return &Store{ blueprints: newBlueprintsFromV0(storeStruct.Blueprints), workspace: newWorkspaceFromV0(storeStruct.Workspace), - composes: newComposesFromV0(storeStruct.Composes, arch, log), + composes: newComposesFromV0(storeStruct.Composes, dr, log), sources: newSourceConfigsFromV0(storeStruct.Sources), blueprintsChanges: newChangesFromV0(storeStruct.Changes), blueprintsCommits: newCommitsFromV0(storeStruct.Commits, storeStruct.Changes), diff --git a/internal/store/json_test.go b/internal/store/json_test.go index bec573da9..89846a101 100644 --- a/internal/store/json_test.go +++ b/internal/store/json_test.go @@ -15,6 +15,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/distro/fedora" "github.com/osbuild/osbuild-composer/internal/distro/test_distro" + "github.com/osbuild/osbuild-composer/internal/distroregistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/target" "github.com/stretchr/testify/assert" @@ -114,13 +115,14 @@ func Test_imageTypeFromCompatString(t *testing.T) { func TestMarshalEmpty(t *testing.T) { d := test_distro.New() - arch, err := d.GetArch(test_distro.TestArchName) + _, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err)) } store1 := FixtureEmpty() storeV0 := store1.toStoreV0() - store2 := newStoreFromV0(*storeV0, arch, nil) + dr := test_distro.NewRegistry() + store2 := newStoreFromV0(*storeV0, dr, nil) if !reflect.DeepEqual(store1, store2) { t.Errorf("marshal/unmarshal roundtrip not a noop for empty store:\n got: %#v\n want: %#v", store1, store2) } @@ -128,13 +130,14 @@ func TestMarshalEmpty(t *testing.T) { func TestMarshalFinished(t *testing.T) { d := test_distro.New() - arch, err := d.GetArch(test_distro.TestArchName) + _, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err)) } store1 := FixtureFinished() storeV0 := store1.toStoreV0() - store2 := newStoreFromV0(*storeV0, arch, nil) + dr := test_distro.NewRegistry() + store2 := newStoreFromV0(*storeV0, dr, nil) if !reflect.DeepEqual(store1, store2) { t.Errorf("marshal/unmarshal roundtrip not a noop for base store:\n got: %#v\n want: %#v", store2, store1) } @@ -187,11 +190,12 @@ func TestStore_toStoreV0(t *testing.T) { func Test_newStoreFromV0(t *testing.T) { type args struct { storeStruct storeV0 - arch distro.Arch + registry *distroregistry.Registry } - testDistro := test_distro.New() - testArch, _ := testDistro.GetArch(test_distro.TestArchName) + //testDistro := test_distro.New() + // testArch, _ := testDistro.GetArch(test_distro.TestArchName) + dr := test_distro.NewRegistry() tests := []struct { name string @@ -202,14 +206,14 @@ func Test_newStoreFromV0(t *testing.T) { name: "empty", args: args{ storeStruct: storeV0{}, - arch: testArch, + registry: dr, }, - want: New(nil, testArch, nil), + want: New(nil, dr, nil), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := newStoreFromV0(tt.args.storeStruct, tt.args.arch, nil); !reflect.DeepEqual(got, tt.want) { + if got := newStoreFromV0(tt.args.storeStruct, tt.args.registry, nil); !reflect.DeepEqual(got, tt.want) { t.Errorf("newStoreFromV0() =\n got: %#v\n want: %#v", got, tt.want) } }) @@ -303,9 +307,13 @@ func Test_upgrade(t *testing.T) { assert.NoErrorf(err, "Could not read test-store '%s': %v", fileName, err) err = json.Unmarshal([]byte(file), &storeStruct) assert.NoErrorf(err, "Could not parse test-store '%s': %v", fileName, err) - arch, err := fedora.NewF35().GetArch(distro.X86_64ArchName) + f35 := fedora.NewF35() + registry, err := distroregistry.New(f35, f35) assert.NoError(err) - store := newStoreFromV0(storeStruct, arch, nil) + + // The test data has image types only supported on Fedora X86_64 + registry.SetHostArchName(distro.X86_64ArchName) + store := newStoreFromV0(storeStruct, registry, nil) assert.Equal(1, len(store.blueprints)) assert.Equal(1, len(store.blueprintsChanges)) assert.Equal(1, len(store.blueprintsCommits)) @@ -1094,25 +1102,26 @@ func Test_newComposeFromV0(t *testing.T) { testDistro := test_distro.New() testArch, _ := testDistro.GetArch(test_distro.TestArchName) testImageType, _ := testArch.GetImageType(test_distro.TestImageTypeName) + dr := test_distro.NewRegistry() tests := []struct { - name string - compose composeV0 - arch distro.Arch - want Compose - errOk bool + name string + compose composeV0 + registry *distroregistry.Registry + want Compose + errOk bool }{ { - name: "empty", - compose: composeV0{}, - arch: testArch, - want: Compose{}, - errOk: true, + name: "empty", + compose: composeV0{}, + registry: dr, + want: Compose{}, + errOk: true, }, { - name: "qcow2 compose", - arch: testArch, - errOk: false, + name: "qcow2 compose", + registry: dr, + errOk: false, compose: composeV0{ Blueprint: &bp, ImageBuilds: []imageBuildV0{ @@ -1182,7 +1191,7 @@ func Test_newComposeFromV0(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := newComposeFromV0(tt.compose, tt.arch) + got, err := newComposeFromV0(tt.compose, tt.registry) if err != nil { if !tt.errOk { t.Errorf("newComposeFromV0() error = %v", err) @@ -1371,22 +1380,23 @@ func Test_newComposesFromV0(t *testing.T) { testDistro := test_distro.New() testArch, _ := testDistro.GetArch(test_distro.TestArchName) testImageType, _ := testArch.GetImageType(test_distro.TestImageTypeName) + dr := test_distro.NewRegistry() tests := []struct { name string - arch distro.Arch + registry *distroregistry.Registry composes composesV0 want map[uuid.UUID]Compose }{ { name: "empty", - arch: testArch, + registry: dr, composes: composesV0{}, want: make(map[uuid.UUID]Compose), }, { - name: "two composes", - arch: testArch, + name: "two composes", + registry: dr, composes: composesV0{ uuid.MustParse("f53b49c0-d321-447e-8ab8-6e827891e3f0"): { Blueprint: &bp, @@ -1525,7 +1535,7 @@ func Test_newComposesFromV0(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := newComposesFromV0(tt.composes, tt.arch, nil); !reflect.DeepEqual(got, tt.want) { + if got := newComposesFromV0(tt.composes, tt.registry, nil); !reflect.DeepEqual(got, tt.want) { t.Errorf("newComposesFromV0() =\n got: %#v\n want: %#v", got, tt.want) } }) diff --git a/internal/store/store.go b/internal/store/store.go index bcc37dfee..71158e5b8 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -20,6 +20,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/blueprint" "github.com/osbuild/osbuild-composer/internal/common" + "github.com/osbuild/osbuild-composer/internal/distroregistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/target" @@ -79,7 +80,7 @@ func (e *NoLocalTargetError) Error() string { return e.message } -func New(stateDir *string, arch distro.Arch, log *log.Logger) *Store { +func New(stateDir *string, dr *distroregistry.Registry, log *log.Logger) *Store { var storeStruct storeV0 var db *jsondb.JSONDatabase @@ -91,7 +92,7 @@ func New(stateDir *string, arch distro.Arch, log *log.Logger) *Store { } } - store := newStoreFromV0(storeStruct, arch, log) + store := newStoreFromV0(storeStruct, dr, log) store.stateDir = stateDir store.db = db diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 96cd2fceb..6cc489e80 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -133,10 +133,11 @@ func (suite *storeTest) SetupSuite() { //setup before each test func (suite *storeTest) SetupTest() { distro := test_distro.New() - arch, err := distro.GetArch(test_distro.TestArchName) + _, err := distro.GetArch(test_distro.TestArchName) suite.NoError(err) suite.dir = suite.T().TempDir() - suite.myStore = New(&suite.dir, arch, nil) + dr := test_distro.NewRegistry() + suite.myStore = New(&suite.dir, dr, nil) } func (suite *storeTest) TestRandomSHA1String() { diff --git a/internal/weldr/api.go b/internal/weldr/api.go index 64e604d2d..032f786c2 100644 --- a/internal/weldr/api.go +++ b/internal/weldr/api.go @@ -164,13 +164,12 @@ func New(repoPaths []string, stateDir string, solver *dnfjson.BaseSolver, dr *di return nil, fmt.Errorf("error loading repository definitions: %v", err) } - var hostArch distro.Arch hostDistro := dr.GetDistro(hostDistroName) if hostDistro != nil { // get canonical distro name if the host distro is supported hostDistroName = hostDistro.Name() - hostArch, err = hostDistro.GetArch(archName) + _, err = hostDistro.GetArch(archName) if err != nil { return nil, fmt.Errorf("Host distro does not support host architecture: %v", err) } @@ -185,7 +184,7 @@ func New(repoPaths []string, stateDir string, solver *dnfjson.BaseSolver, dr *di log.Printf("host distro %q is not supported: only cross-distro builds are available", hostDistroName) } - store := store.New(&stateDir, hostArch, logger) + store := store.New(&stateDir, dr, logger) compatOutputDir := path.Join(stateDir, "outputs") api := &API{