From e725dc4d5fa529c46c7218a1a0d8a3fafa88e51a Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 3 Dec 2024 16:20:11 -0500 Subject: [PATCH] feat: amend ResourceStoreFactory to take a ResourceStoreGetter The ResourceStoreFactory now uses a ResourceStoreGetter for TypeContainerImage. The ContainerImageResourceStore was pushed up to an individual domain. Supporting functionality for the old implemenation has been removed. ContainerImageResourceStore is now provided to NewResourceStoreFactory via a getter when a Resource domain is requested as a model service. --- core/resource/store/store.go | 3 + .../service/service.go | 5 +- domain/resource/service/resource.go | 12 -- domain/resource/service/resource_test.go | 1 - domain/services/model.go | 16 +- .../resource/store/object_store_mock_test.go | 7 +- internal/resource/store/package_test.go | 3 +- .../store/resource_store_mock_test.go | 159 ++++++++++++++++++ internal/resource/store/resourcestore.go | 44 ++--- internal/resource/store/resourcestore_test.go | 59 ++++--- 10 files changed, 229 insertions(+), 80 deletions(-) create mode 100644 internal/resource/store/resource_store_mock_test.go diff --git a/core/resource/store/store.go b/core/resource/store/store.go index b2b1366f420b..681be0bd7260 100644 --- a/core/resource/store/store.go +++ b/core/resource/store/store.go @@ -52,3 +52,6 @@ func NewFingerprint(f hash.Fingerprint) Fingerprint { // blob. This can be an object store metadata UUID or a container image metadata // storage key. type UUID string + +// ResourceStoreGetter is a function which returns a ResourceStore. +type ResourceStoreGetter func() ResourceStore diff --git a/domain/containerimageresourcestore/service/service.go b/domain/containerimageresourcestore/service/service.go index 6fc34b4dd0d6..bfa00a95db3e 100644 --- a/domain/containerimageresourcestore/service/service.go +++ b/domain/containerimageresourcestore/service/service.go @@ -11,7 +11,6 @@ import ( "github.com/juju/juju/core/resource/store" "github.com/juju/juju/domain/containerimageresourcestore" - charmresource "github.com/juju/juju/internal/charm/resource" "github.com/juju/juju/internal/docker" "github.com/juju/juju/internal/errors" ) @@ -83,8 +82,8 @@ func (s Service) Put( ctx context.Context, storageKey string, r io.Reader, - size int64, - fingerprint charmresource.Fingerprint, + _ int64, + _ store.Fingerprint, ) (store.UUID, error) { respBuf := new(bytes.Buffer) bytesRead, err := respBuf.ReadFrom(r) diff --git a/domain/resource/service/resource.go b/domain/resource/service/resource.go index a70bc4fd03d9..95a0c97c733d 100644 --- a/domain/resource/service/resource.go +++ b/domain/resource/service/resource.go @@ -50,9 +50,6 @@ type State interface { } type ResourceStoreGetter interface { - // AddStore injects a ResourceStore for the given type into the ResourceStoreFactory. - AddStore(t charmresource.Type, store store.ResourceStore) - // GetResourceStore returns the appropriate ResourceStore for the // given resource type. GetResourceStore(context.Context, charmresource.Type) (store.ResourceStore, error) @@ -72,15 +69,6 @@ func NewService( resourceStoreGetter ResourceStoreGetter, logger logger.Logger, ) *Service { - // Note: - // The store for container image resources is really a DQLite table today. - // Using AddStore is a compromise to avoid injecting one service into - // another, as would happen if NewResourceStoreFactory had a second - // argument to provide a containerImageResourceStore. - resourceStoreGetter.AddStore( - charmresource.TypeContainerImage, - nil, - ) return &Service{ st: st, logger: logger, diff --git a/domain/resource/service/resource_test.go b/domain/resource/service/resource_test.go index 2266c1bf2432..fe2d6b208adf 100644 --- a/domain/resource/service/resource_test.go +++ b/domain/resource/service/resource_test.go @@ -293,7 +293,6 @@ func (s *resourceServiceSuite) setupMocks(c *gc.C) *gomock.Controller { s.state = NewMockState(ctrl) s.resourceStoreGetter = NewMockResourceStoreGetter(ctrl) - s.resourceStoreGetter.EXPECT().AddStore(charmresource.TypeContainerImage, gomock.Any()) s.service = NewService(s.state, s.resourceStoreGetter, loggertesting.WrapCheckLog(c)) diff --git a/domain/services/model.go b/domain/services/model.go index 75752efd2ddf..53af0a5c34ca 100644 --- a/domain/services/model.go +++ b/domain/services/model.go @@ -15,6 +15,7 @@ import ( "github.com/juju/juju/core/model" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/core/providertracker" + coreresourcestore "github.com/juju/juju/core/resource/store" corestorage "github.com/juju/juju/core/storage" agentprovisionerservice "github.com/juju/juju/domain/agentprovisioner/service" agentprovisionerstate "github.com/juju/juju/domain/agentprovisioner/state" @@ -29,6 +30,8 @@ import ( blockdevicestate "github.com/juju/juju/domain/blockdevice/state" cloudimagemetadataservice "github.com/juju/juju/domain/cloudimagemetadata/service" cloudimagemetadatastate "github.com/juju/juju/domain/cloudimagemetadata/state" + containerimageresourcestoreservice "github.com/juju/juju/domain/containerimageresourcestore/service" + containerimageresourcestorestate "github.com/juju/juju/domain/containerimageresourcestore/state" keymanagerservice "github.com/juju/juju/domain/keymanager/service" keymanagerstate "github.com/juju/juju/domain/keymanager/state" keyupdaterservice "github.com/juju/juju/domain/keyupdater/service" @@ -345,13 +348,24 @@ func (s *ModelServices) BlockCommand() *blockcommandservice.Service { // Resource returns the service for persisting and retrieving application // resources for the current model. func (s *ModelServices) Resource() *resourceservice.Service { + containerImageResourceStoreGetter := func() coreresourcestore.ResourceStore { + return containerimageresourcestoreservice.NewService( + containerimageresourcestorestate.NewState( + changestream.NewTxnRunnerFactory(s.modelDB), + s.logger.Child("containerimageresourcestore.state"), + )) + } + resourceStoreFactory := store.NewResourceStoreFactory( + s.objectstore, + containerImageResourceStoreGetter, + ) return resourceservice.NewService( resourcestate.NewState( changestream.NewTxnRunnerFactory(s.modelDB), s.clock, s.logger.Child("resource.state"), ), - store.NewResourceStoreFactory(s.objectstore), + resourceStoreFactory, s.logger.Child("resource.service"), ) } diff --git a/internal/resource/store/object_store_mock_test.go b/internal/resource/store/object_store_mock_test.go index a438a2cc34eb..43c530088794 100644 --- a/internal/resource/store/object_store_mock_test.go +++ b/internal/resource/store/object_store_mock_test.go @@ -3,10 +3,10 @@ // // Generated by this command: // -// mockgen -typed -package resource -destination object_store_mock_test.go github.com/juju/juju/core/objectstore ObjectStore,ModelObjectStoreGetter +// mockgen -typed -package store -destination object_store_mock_test.go github.com/juju/juju/core/objectstore ObjectStore,ModelObjectStoreGetter // -// Package resource is a generated GoMock package. +// Package store is a generated GoMock package. package store import ( @@ -14,9 +14,8 @@ import ( io "io" reflect "reflect" - gomock "go.uber.org/mock/gomock" - objectstore "github.com/juju/juju/core/objectstore" + gomock "go.uber.org/mock/gomock" ) // MockObjectStore is a mock of ObjectStore interface. diff --git a/internal/resource/store/package_test.go b/internal/resource/store/package_test.go index 191011b0fdf8..648261391222 100644 --- a/internal/resource/store/package_test.go +++ b/internal/resource/store/package_test.go @@ -9,7 +9,8 @@ import ( gc "gopkg.in/check.v1" ) -//go:generate go run go.uber.org/mock/mockgen -typed -package resource -destination object_store_mock_test.go github.com/juju/juju/core/objectstore ObjectStore,ModelObjectStoreGetter +//go:generate go run go.uber.org/mock/mockgen -typed -package store -destination object_store_mock_test.go github.com/juju/juju/core/objectstore ObjectStore,ModelObjectStoreGetter +//go:generate go run go.uber.org/mock/mockgen -typed -package store -destination resource_store_mock_test.go github.com/juju/juju/core/resource/store ResourceStore func TestPackage(t *testing.T) { gc.TestingT(t) diff --git a/internal/resource/store/resource_store_mock_test.go b/internal/resource/store/resource_store_mock_test.go new file mode 100644 index 000000000000..95659f499e55 --- /dev/null +++ b/internal/resource/store/resource_store_mock_test.go @@ -0,0 +1,159 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/core/resource/store (interfaces: ResourceStore) +// +// Generated by this command: +// +// mockgen -typed -package store -destination resource_store_mock_test.go github.com/juju/juju/core/resource/store ResourceStore +// + +// Package store is a generated GoMock package. +package store + +import ( + context "context" + io "io" + reflect "reflect" + + store "github.com/juju/juju/core/resource/store" + gomock "go.uber.org/mock/gomock" +) + +// MockResourceStore is a mock of ResourceStore interface. +type MockResourceStore struct { + ctrl *gomock.Controller + recorder *MockResourceStoreMockRecorder +} + +// MockResourceStoreMockRecorder is the mock recorder for MockResourceStore. +type MockResourceStoreMockRecorder struct { + mock *MockResourceStore +} + +// NewMockResourceStore creates a new mock instance. +func NewMockResourceStore(ctrl *gomock.Controller) *MockResourceStore { + mock := &MockResourceStore{ctrl: ctrl} + mock.recorder = &MockResourceStoreMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockResourceStore) EXPECT() *MockResourceStoreMockRecorder { + return m.recorder +} + +// Get mocks base method. +func (m *MockResourceStore) Get(arg0 context.Context, arg1 string) (io.ReadCloser, int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Get", arg0, arg1) + ret0, _ := ret[0].(io.ReadCloser) + ret1, _ := ret[1].(int64) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// Get indicates an expected call of Get. +func (mr *MockResourceStoreMockRecorder) Get(arg0, arg1 any) *MockResourceStoreGetCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockResourceStore)(nil).Get), arg0, arg1) + return &MockResourceStoreGetCall{Call: call} +} + +// MockResourceStoreGetCall wrap *gomock.Call +type MockResourceStoreGetCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockResourceStoreGetCall) Return(arg0 io.ReadCloser, arg1 int64, arg2 error) *MockResourceStoreGetCall { + c.Call = c.Call.Return(arg0, arg1, arg2) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockResourceStoreGetCall) Do(f func(context.Context, string) (io.ReadCloser, int64, error)) *MockResourceStoreGetCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockResourceStoreGetCall) DoAndReturn(f func(context.Context, string) (io.ReadCloser, int64, error)) *MockResourceStoreGetCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// Put mocks base method. +func (m *MockResourceStore) Put(arg0 context.Context, arg1 string, arg2 io.Reader, arg3 int64, arg4 store.Fingerprint) (store.UUID, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Put", arg0, arg1, arg2, arg3, arg4) + ret0, _ := ret[0].(store.UUID) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Put indicates an expected call of Put. +func (mr *MockResourceStoreMockRecorder) Put(arg0, arg1, arg2, arg3, arg4 any) *MockResourceStorePutCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Put", reflect.TypeOf((*MockResourceStore)(nil).Put), arg0, arg1, arg2, arg3, arg4) + return &MockResourceStorePutCall{Call: call} +} + +// MockResourceStorePutCall wrap *gomock.Call +type MockResourceStorePutCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockResourceStorePutCall) Return(arg0 store.UUID, arg1 error) *MockResourceStorePutCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockResourceStorePutCall) Do(f func(context.Context, string, io.Reader, int64, store.Fingerprint) (store.UUID, error)) *MockResourceStorePutCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockResourceStorePutCall) DoAndReturn(f func(context.Context, string, io.Reader, int64, store.Fingerprint) (store.UUID, error)) *MockResourceStorePutCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// Remove mocks base method. +func (m *MockResourceStore) Remove(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Remove", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// Remove indicates an expected call of Remove. +func (mr *MockResourceStoreMockRecorder) Remove(arg0, arg1 any) *MockResourceStoreRemoveCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Remove", reflect.TypeOf((*MockResourceStore)(nil).Remove), arg0, arg1) + return &MockResourceStoreRemoveCall{Call: call} +} + +// MockResourceStoreRemoveCall wrap *gomock.Call +type MockResourceStoreRemoveCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockResourceStoreRemoveCall) Return(arg0 error) *MockResourceStoreRemoveCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockResourceStoreRemoveCall) Do(f func(context.Context, string) error) *MockResourceStoreRemoveCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockResourceStoreRemoveCall) DoAndReturn(f func(context.Context, string) error) *MockResourceStoreRemoveCall { + c.Call = c.Call.DoAndReturn(f) + return c +} diff --git a/internal/resource/store/resourcestore.go b/internal/resource/store/resourcestore.go index 82c0ba73cf5d..723ff1835949 100644 --- a/internal/resource/store/resourcestore.go +++ b/internal/resource/store/resourcestore.go @@ -5,7 +5,6 @@ package store import ( "context" - "sync" "github.com/juju/juju/core/objectstore" "github.com/juju/juju/core/resource/store" @@ -16,17 +15,19 @@ import ( // ResourceStoreFactory contains the information to provide the required // ResourceStore. type ResourceStoreFactory struct { - objectStore objectstore.ModelObjectStoreGetter - mu sync.Mutex - storeMap map[resource.Type]store.ResourceStore + objectStore objectstore.ModelObjectStoreGetter + containerImageStore store.ResourceStoreGetter } // NewResourceStoreFactory returns a factory which provides the appropriate // Resource Store for the resource type. -func NewResourceStoreFactory(objectStore objectstore.ModelObjectStoreGetter) *ResourceStoreFactory { +func NewResourceStoreFactory( + objectStore objectstore.ModelObjectStoreGetter, + containerImageStore store.ResourceStoreGetter, +) *ResourceStoreFactory { return &ResourceStoreFactory{ - objectStore: objectStore, - storeMap: make(map[resource.Type]store.ResourceStore), + objectStore: objectStore, + containerImageStore: containerImageStore, } } @@ -35,33 +36,14 @@ func NewResourceStoreFactory(objectStore objectstore.ModelObjectStoreGetter) *Re func (f *ResourceStoreFactory) GetResourceStore(ctx context.Context, t resource.Type) (store.ResourceStore, error) { switch t { case resource.TypeFile: - store, err := f.objectStore.GetObjectStore(ctx) + objectStore, err := f.objectStore.GetObjectStore(ctx) if err != nil { return nil, errors.Errorf("getting file resource store: %w", err) } - return fileResourceStore{objectStore: store}, nil + return fileResourceStore{objectStore: objectStore}, nil + case resource.TypeContainerImage: + return f.containerImageStore(), nil default: - f.mu.Lock() - defer f.mu.Unlock() - store, ok := f.storeMap[t] - if !ok { - return nil, UnknownResourceType - } - return store, nil + return nil, UnknownResourceType } } - -// AddStore injects a ResourceStore for the given type into the -// ResourceStoreFactory. -// -// Note: -// The store for container image resources is really a DQLite table today. -// This method is a compromise to avoid injecting one service into another -// if the ContainerImageResourceStore was provided as an argument to -// NewResourceStoreFactory. If we get a new implementation of a container image -// resource store this should be re-evaluated and hopefully removed. -func (f *ResourceStoreFactory) AddStore(t resource.Type, store store.ResourceStore) { - f.mu.Lock() - defer f.mu.Unlock() - f.storeMap[t] = store -} diff --git a/internal/resource/store/resourcestore_test.go b/internal/resource/store/resourcestore_test.go index 1de502c28a72..e14ae9188885 100644 --- a/internal/resource/store/resourcestore_test.go +++ b/internal/resource/store/resourcestore_test.go @@ -11,64 +11,69 @@ import ( "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" + "github.com/juju/juju/core/resource/store" charmresource "github.com/juju/juju/internal/charm/resource" ) type resourceStoreSuite struct { objectStore *MockObjectStore modelObjectStoreGetter *MockModelObjectStoreGetter + resourceStore *MockResourceStore } var _ = gc.Suite(&resourceStoreSuite{}) -func (s *resourceStoreSuite) setupMocks(c *gc.C) *gomock.Controller { - ctrl := gomock.NewController(c) - - s.modelObjectStoreGetter = NewMockModelObjectStoreGetter(ctrl) - s.objectStore = NewMockObjectStore(ctrl) - - return ctrl -} - -func (s *resourceStoreSuite) TestObjectStoreGetter(c *gc.C) { +func (s *resourceStoreSuite) TestGetResourceStoreTypeFile(c *gc.C) { defer s.setupMocks(c).Finish() - factory := NewResourceStoreFactory(s.modelObjectStoreGetter) s.modelObjectStoreGetter.EXPECT().GetObjectStore(gomock.Any()).Return(s.objectStore, nil) - store, err := factory.GetResourceStore(context.Background(), charmresource.TypeFile) + store, err := s.factory().GetResourceStore(context.Background(), charmresource.TypeFile) c.Assert(err, jc.ErrorIsNil) c.Assert(store, gc.Equals, fileResourceStore{s.objectStore}) } -func (s *resourceStoreSuite) TestObjectStoreGetterError(c *gc.C) { +func (s *resourceStoreSuite) TestGetResourceStoreTypeFileError(c *gc.C) { defer s.setupMocks(c).Finish() - factory := NewResourceStoreFactory(s.modelObjectStoreGetter) kaboom := errors.Errorf("kaboom") s.modelObjectStoreGetter.EXPECT().GetObjectStore(gomock.Any()).Return(nil, kaboom) - _, err := factory.GetResourceStore(context.Background(), charmresource.TypeFile) + _, err := s.factory().GetResourceStore(context.Background(), charmresource.TypeFile) c.Assert(err, jc.ErrorIs, kaboom) } -func (s *resourceStoreSuite) TestObjectStoreGetterAddStore(c *gc.C) { +func (s *resourceStoreSuite) TestGetResourceStoreNotFound(c *gc.C) { defer s.setupMocks(c).Finish() - factory := NewResourceStoreFactory(s.modelObjectStoreGetter) - newStore := fileResourceStore{objectStore: s.objectStore} - factory.AddStore(charmresource.TypeContainerImage, newStore) - - store, err := factory.GetResourceStore(context.Background(), charmresource.TypeContainerImage) - c.Assert(err, gc.IsNil) - c.Assert(store, gc.Equals, newStore) + _, err := s.factory().GetResourceStore(context.Background(), charmresource.Type(0)) + c.Assert(err, jc.ErrorIs, UnknownResourceType) } -func (s *resourceStoreSuite) TestObjectStoreGetterNotFound(c *gc.C) { +func (s *resourceStoreSuite) TestGetResourceStoreTypeContainerImage(c *gc.C) { defer s.setupMocks(c).Finish() - factory := NewResourceStoreFactory(s.modelObjectStoreGetter) + s.resourceStore.EXPECT().Remove(context.Background(), gomock.Any()).Return(nil) - _, err := factory.GetResourceStore(context.Background(), charmresource.Type(0)) - c.Assert(err, jc.ErrorIs, UnknownResourceType) + store, err := s.factory().GetResourceStore(context.Background(), charmresource.TypeContainerImage) + c.Assert(err, jc.ErrorIsNil) + err = store.Remove(context.Background(), "string") + c.Assert(err, jc.ErrorIsNil) +} + +func (s *resourceStoreSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) + + s.modelObjectStoreGetter = NewMockModelObjectStoreGetter(ctrl) + s.objectStore = NewMockObjectStore(ctrl) + s.resourceStore = NewMockResourceStore(ctrl) + + return ctrl +} + +func (s *resourceStoreSuite) factory() *ResourceStoreFactory { + getter := func() store.ResourceStore { + return s.resourceStore + } + return NewResourceStoreFactory(s.modelObjectStoreGetter, getter) }