Merge pull request juju#16525 from SimonRichardson/objectstore-everyw…

…here

juju#16525

This PR uses the object store for resources and charm removals. This is a very mechanical, yet pervasive set of changes. The goal of this change and subsequent one to follow (agent tool binaries) is to make the object store an external dependency from the state package. Prior to this change, the `state/storage` package was just instantiated at the call site for when it wanted access to the gridfs store. Now that we have the object store as a dependency we need to weave it through everything to get access to it. This includes `Destroy` and `Remove` calls on certain entities. This kind of colours the function, so that you must always include the object store in these calls, but it does force you to recognise that when you destroy an entity there are blobs associated with those. 

There is an alternative; we could push the object store into state type when we pull it out of the state pool. The reason I went with the hard dependency model is it's explicit and it does help reduce the surface area of the state package. 

As per the API facade setup, you will get a shared object store per websocket connection that is tied to the model UUID. This will then be shared with other websocket connections that are associated with the same model UUID. This should reduce how many storage type instantiations we have per jujud request, as they're now loaned out. This will become more important when we implement file-backed storage. 


## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing

## QA steps

```sh
$ juju bootstrap lxd test --build-agent
$ juju add-model default
$ juju deploy ubuntu
$ juju deploy juju-qa-test
$ echo "boo" > foo-file.txt
$ juju attach-resource juju-qa-test foo-file=./foo-file.txt
$ juju show-status-log juju-qa-test/0
``` 

## Links

**Jira card:** JUJU-4889

apiserver/apiserver.go

@@ -789,7 +789,11 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) {
 			if err != nil {
 				return nil, nil, nil, errors.Trace(err)
 			}
-			rst := st.Resources()
+			store, err := httpCtxt.objectStoreForRequest(req)
+			if err != nil {
+				return nil, nil, nil, errors.Trace(err)
+			}
+			rst := st.Resources(store)
 			return rst, st, entity.Tag(), nil
 		},
 		ChangeAllowedFunc: func(req *http.Request) error {
@@ -810,12 +814,16 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) {
 			if err != nil {
 				return nil, nil, errors.Trace(err)
 			}
+			store, err := httpCtxt.objectStoreForRequest(req)
+			if err != nil {
+				return nil, nil, errors.Trace(err)
+			}
 			tagStr := req.URL.Query().Get(":unit")
 			tag, err := names.ParseUnitTag(tagStr)
 			if err != nil {
 				return nil, nil, errors.Trace(err)
 			}
-			opener, err := resource.NewResourceOpener(st.State, srv.getResourceDownloadLimiter, tag.Id())
+			opener, err := resource.NewResourceOpener(st.State, store, srv.getResourceDownloadLimiter, tag.Id())
 			if err != nil {
 				return nil, nil, errors.Trace(err)
 			}
@@ -844,6 +852,7 @@ func (srv *Server) endpoints() ([]apihttp.Endpoint, error) {
 	resourcesMigrationUploadHandler := &resourcesMigrationUploadHandler{
 		ctxt:          httpCtxt,
 		stateAuthFunc: httpCtxt.stateForMigrationImporting,
+		objectStore:   httpCtxt.objectStoreForRequest,
 	}
 	backupHandler := &backupHandler{ctxt: httpCtxt}
 	registerHandler := &registerUserHandler{ctxt: httpCtxt}

apiserver/charms_test.go

@@ -5,6 +5,7 @@ package apiserver_test
 
 import (
 	"bytes"
+	"context"
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
@@ -24,10 +25,10 @@ import (
 	"github.com/juju/juju/apiserver"
 	"github.com/juju/juju/apiserver/common"
 	apitesting "github.com/juju/juju/apiserver/testing"
+	"github.com/juju/juju/juju/testing"
 	jujutesting "github.com/juju/juju/juju/testing"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
-	"github.com/juju/juju/state/storage"
 	"github.com/juju/juju/testcharms"
 	"github.com/juju/juju/testing/factory"
 )
@@ -241,8 +242,8 @@ func (s *charmsSuite) TestUploadRespectsLocalRevision(c *gc.C) {
 	c.Assert(sch.IsUploaded(), jc.IsTrue)
 	c.Assert(sch.BundleSha256(), gc.Equals, expectedSHA256)
 
-	storage := storage.NewStorage(s.ControllerModelUUID(), s.ControllerModel(c).State().MongoSession())
-	reader, _, err := storage.Get(sch.StoragePath())
+	store := testing.NewObjectStore(c, s.ControllerModelUUID(), s.ControllerModel(c).State())
+	reader, _, err := store.Get(context.Background(), sch.StoragePath())
 	c.Assert(err, jc.ErrorIsNil)
 	defer reader.Close()
 	downloadedSHA256, _, err := utils.ReadSHA256(reader)
@@ -322,8 +323,8 @@ func (s *charmsSuite) TestUploadRepackagesNestedArchives(c *gc.C) {
 	// Get it from the storage and try to read it as a bundle - it
 	// should succeed, because it was repackaged during upload to
 	// strip nested dirs.
-	storage := storage.NewStorage(s.ControllerModelUUID(), s.ControllerModel(c).State().MongoSession())
-	reader, _, err := storage.Get(sch.StoragePath())
+	store := testing.NewObjectStore(c, s.ControllerModelUUID(), s.ControllerModel(c).State())
+	reader, _, err := store.Get(context.Background(), sch.StoragePath())
 	c.Assert(err, jc.ErrorIsNil)
 	defer reader.Close()
 

apiserver/common/crossmodel/crossmodel.go

@@ -76,7 +76,7 @@ func PublishRelationChange(auth authoriser, backend Backend, relationTag, applic
 			// If we are forcing cleanup, we can exit early here.
 			return errors.Trace(err)
 		}
-		if err := rel.Destroy(); err != nil {
+		if err := rel.Destroy(nil); err != nil {
 			return errors.Trace(err)
 		}
 	}

apiserver/common/crossmodel/interface.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/juju/juju/core/crossmodel"
 	"github.com/juju/juju/core/network"
+	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/permission"
 	coresecrets "github.com/juju/juju/core/secrets"
 	"github.com/juju/juju/core/status"
@@ -125,7 +126,7 @@ type Relation interface {
 	status.StatusSetter
 	// Destroy ensures that the relation will be removed at some point; if
 	// no units are currently in scope, it will be removed immediately.
-	Destroy() error
+	Destroy(objectstore.ObjectStore) error
 
 	// DestroyWithForce may force the destruction of the relation.
 	// In addition, this function also returns all non-fatal operational errors

apiserver/common/machine.go

@@ -11,6 +11,7 @@ import (
 	apiservererrors "github.com/juju/juju/apiserver/errors"
 	"github.com/juju/juju/core/instance"
 	"github.com/juju/juju/core/model"
+	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/status"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
@@ -72,16 +73,16 @@ type Machine interface {
 	HardwareCharacteristics() (*instance.HardwareCharacteristics, error)
 	Life() state.Life
 	ForceDestroy(time.Duration) error
-	Destroy() error
+	Destroy(objectstore.ObjectStore) error
 	IsManager() bool
 	IsLockedForSeriesUpgrade() (bool, error)
 }
 
-func DestroyMachines(st origStateInterface, force bool, maxWait time.Duration, ids ...string) error {
-	return destroyMachines(&stateShim{st}, force, maxWait, ids...)
+func DestroyMachines(st origStateInterface, store objectstore.ObjectStore, force bool, maxWait time.Duration, ids ...string) error {
+	return destroyMachines(&stateShim{st}, store, force, maxWait, ids...)
 }
 
-func destroyMachines(st stateInterface, force bool, maxWait time.Duration, ids ...string) error {
+func destroyMachines(st stateInterface, store objectstore.ObjectStore, force bool, maxWait time.Duration, ids ...string) error {
 	var errs []error
 	for _, id := range ids {
 		machine, err := st.Machine(id)
@@ -94,7 +95,7 @@ func destroyMachines(st stateInterface, force bool, maxWait time.Duration, ids .
 		case machine.Life() != state.Alive:
 			continue
 		default:
-			err = machine.Destroy()
+			err = machine.Destroy(store)
 		}
 		if err != nil {
 			errs = append(errs, err)

apiserver/common/machine_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/core/instance"
 	"github.com/juju/juju/core/model"
+	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/status"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
@@ -55,13 +56,14 @@ const (
 
 func (s *machineSuite) TestDestroyMachines(c *gc.C) {
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {},
 			"2": {destroyErr: errors.New("unit exists error")},
 			"3": {life: state.Dying},
 		},
 	}
-	err := common.MockableDestroyMachines(&st, false, dontWait, "1", "2", "3", "4")
+
+	err := common.MockableDestroyMachines(&st, &fakeObjectStore{}, false, dontWait, "1", "2", "3", "4")
 
 	c.Assert(st.machines["1"].Life(), gc.Equals, state.Dying)
 	c.Assert(st.machines["1"].forceDestroyCalled, jc.IsFalse)
@@ -77,12 +79,12 @@ func (s *machineSuite) TestDestroyMachines(c *gc.C) {
 
 func (s *machineSuite) TestForceDestroyMachines(c *gc.C) {
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {},
 			"2": {life: state.Dying},
 		},
 	}
-	err := common.MockableDestroyMachines(&st, true, dontWait, "1", "2")
+	err := common.MockableDestroyMachines(&st, &fakeObjectStore{}, true, dontWait, "1", "2")
 
 	c.Assert(st.machines["1"].Life(), gc.Equals, state.Dying)
 	c.Assert(st.machines["1"].forceDestroyCalled, jc.IsTrue)
@@ -96,7 +98,7 @@ func (s *machineSuite) TestMachineHardwareInfo(c *gc.C) {
 	amd64 := "amd64"
 	gig := uint64(1024)
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {id: "1", life: state.Alive, containerType: instance.NONE,
 				hw: &instance.HardwareCharacteristics{
 					Arch:     &amd64,
@@ -129,7 +131,7 @@ func (s *machineSuite) TestMachineHardwareInfo(c *gc.C) {
 
 func (s *machineSuite) TestMachineInstanceInfo(c *gc.C) {
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {
 				id:     "1",
 				instId: "123",
@@ -178,7 +180,7 @@ func (s *machineSuite) TestMachineInstanceInfo(c *gc.C) {
 
 func (s *machineSuite) TestMachineInstanceInfoWithEmptyDisplayName(c *gc.C) {
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {
 				id:          "1",
 				instId:      "123",
@@ -210,7 +212,7 @@ func (s *machineSuite) TestMachineInstanceInfoWithEmptyDisplayName(c *gc.C) {
 
 func (s *machineSuite) TestMachineInstanceInfoWithSetDisplayName(c *gc.C) {
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {
 				id:          "1",
 				instId:      "123",
@@ -242,7 +244,7 @@ func (s *machineSuite) TestMachineInstanceInfoWithSetDisplayName(c *gc.C) {
 
 func (s *machineSuite) TestMachineInstanceInfoWithHAPrimary(c *gc.C) {
 	st := mockState{
-		machines: map[string]*mockMachine{
+		machines: map[string]*fakeMachine{
 			"1": {
 				id:          "1",
 				instId:      "123",
@@ -284,7 +286,7 @@ func (s *machineSuite) TestMachineInstanceInfoWithHAPrimary(c *gc.C) {
 
 type mockState struct {
 	common.ModelManagerBackend
-	machines          map[string]*mockMachine
+	machines          map[string]*fakeMachine
 	controllerNodes   map[string]*mockControllerNode
 	haPrimaryMachineF func() (names.MachineTag, error)
 }
@@ -342,7 +344,7 @@ func (m *mockControllerNode) HasVote() bool {
 	return m.hasVote
 }
 
-type mockMachine struct {
+type fakeMachine struct {
 	state.Machine
 	id                 string
 	life               state.Life
@@ -358,34 +360,34 @@ type mockMachine struct {
 	destroyCalled      bool
 }
 
-func (m *mockMachine) Id() string {
+func (m *fakeMachine) Id() string {
 	return m.id
 }
 
-func (m *mockMachine) Life() state.Life {
+func (m *fakeMachine) Life() state.Life {
 	return m.life
 }
 
-func (m *mockMachine) InstanceId() (instance.Id, error) {
+func (m *fakeMachine) InstanceId() (instance.Id, error) {
 	return m.instId, nil
 }
 
-func (m *mockMachine) InstanceNames() (instance.Id, string, error) {
+func (m *fakeMachine) InstanceNames() (instance.Id, string, error) {
 	instId, err := m.InstanceId()
 	return instId, m.displayName, err
 }
 
-func (m *mockMachine) Status() (status.StatusInfo, error) {
+func (m *fakeMachine) Status() (status.StatusInfo, error) {
 	return status.StatusInfo{
 		Status: m.status,
 	}, m.statusErr
 }
 
-func (m *mockMachine) HardwareCharacteristics() (*instance.HardwareCharacteristics, error) {
+func (m *fakeMachine) HardwareCharacteristics() (*instance.HardwareCharacteristics, error) {
 	return m.hw, nil
 }
 
-func (m *mockMachine) ForceDestroy(time.Duration) error {
+func (m *fakeMachine) ForceDestroy(time.Duration) error {
 	m.forceDestroyCalled = true
 	if m.forceDestroyErr != nil {
 		return m.forceDestroyErr
@@ -394,7 +396,7 @@ func (m *mockMachine) ForceDestroy(time.Duration) error {
 	return nil
 }
 
-func (m *mockMachine) Destroy() error {
+func (m *fakeMachine) Destroy(_ objectstore.ObjectStore) error {
 	m.destroyCalled = true
 	if m.destroyErr != nil {
 		return m.destroyErr

apiserver/common/machinestatus_test.go

@@ -20,13 +20,13 @@ import (
 type MachineStatusSuite struct {
 	testing.IsolationSuite
 	ctx     common.ModelPresenceContext
-	machine *mockMachine
+	machine *fakeMachine
 }
 
 var _ = gc.Suite(&MachineStatusSuite{})
 
 func (s *MachineStatusSuite) SetUpTest(c *gc.C) {
-	s.machine = &mockMachine{
+	s.machine = &fakeMachine{
 		id:     "666",
 		status: status.Started,
 	}

apiserver/common/modelmanagerinterface.go

@@ -16,6 +16,7 @@ import (
 	"github.com/juju/juju/cloud"
 	"github.com/juju/juju/controller"
 	"github.com/juju/juju/core/network"
+	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/core/permission"
 	"github.com/juju/juju/core/secrets"
 	"github.com/juju/juju/core/status"
@@ -68,8 +69,8 @@ type ModelManagerBackend interface {
 	AllVolumes() ([]state.Volume, error)
 	ControllerUUID() string
 	ControllerTag() names.ControllerTag
-	Export(leaders map[string]string) (description.Model, error)
-	ExportPartial(state.ExportConfig) (description.Model, error)
+	Export(leaders map[string]string, store objectstore.ObjectStore) (description.Model, error)
+	ExportPartial(state.ExportConfig, objectstore.ObjectStore) (description.Model, error)
 	SetUserAccess(subject names.UserTag, target names.Tag, access permission.Access) (permission.UserAccess, error)
 	SetModelMeterStatus(string, string) error
 	AllSpaces() ([]*state.Space, error)

apiserver/common/remove.go

@@ -12,13 +12,15 @@ import (
 	"github.com/juju/names/v4"
 
 	apiservererrors "github.com/juju/juju/apiserver/errors"
+	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
 )
 
 // Remover implements a common Remove method for use by various facades.
 type Remover struct {
 	st             state.EntityFinder
+	store          objectstore.ObjectStore
 	afterDead      func(tag names.Tag)
 	callEnsureDead bool
 	getCanModify   GetAuthFunc
@@ -28,9 +30,10 @@ type Remover struct {
 // whether EnsureDead should be called on an entity before
 // removing. The GetAuthFunc will be used on each invocation of Remove
 // to determine current permissions.
-func NewRemover(st state.EntityFinder, afterDead func(tag names.Tag), callEnsureDead bool, getCanModify GetAuthFunc) *Remover {
+func NewRemover(st state.EntityFinder, store objectstore.ObjectStore, afterDead func(tag names.Tag), callEnsureDead bool, getCanModify GetAuthFunc) *Remover {
 	return &Remover{
 		st:             st,
+		store:          store,
 		afterDead:      afterDead,
 		callEnsureDead: callEnsureDead,
 		getCanModify:   getCanModify,
@@ -63,7 +66,7 @@ func (r *Remover) removeEntity(tag names.Tag) error {
 		}
 	}
 	// TODO (anastasiamac) this needs to work with force if needed
-	return remover.Remove()
+	return remover.Remove(r.store)
 }
 
 // Remove removes every given entity from state, calling EnsureDead