Skip to content

Commit

Permalink
Merge pull request juju#13028 from wallyworld/merge-2.8-20210527
Browse files Browse the repository at this point in the history 
juju#13028

Merge 2.8

juju#13026 Move all time consuming steps to timeout block;
juju#13027 Lazily create storage registry in client storage facade

```
# Conflicts:
# apiserver/facades/client/storage/base_test.go
```

## QA steps

See PRs
  • Loading branch information
@jujubot
jujubot authored May 27, 2021
2 parents a996d8c + eda1284 commit fd9ccb8
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 67 deletions.
11 changes: 8 additions & 3 deletions apiserver/facades/client/storage/base_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/juju/juju/environs/context"
"github.com/juju/juju/state"
jujustorage "github.com/juju/juju/storage"
"github.com/juju/juju/storage/poolmanager"
coretesting "github.com/juju/juju/testing"
)

Expand Down Expand Up @@ -69,9 +70,9 @@ func (s *baseStorageSuite) SetUpTest(c *gc.C) {
s.poolsInUse = []string{}

s.callContext = context.NewEmptyCloudCallContext()
s.api = storage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.registry, s.poolManager, s.authorizer, s.callContext)
s.apiCaas = storage.NewStorageAPIForTest(s.state, state.ModelTypeCAAS, s.storageAccessor, s.registry, s.poolManager, s.authorizer, s.callContext)
newAPI := storage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.registry, s.poolManager, s.authorizer, s.callContext)
s.api = storage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.storageMetadata, s.authorizer, s.callContext)
s.apiCaas = storage.NewStorageAPIForTest(s.state, state.ModelTypeCAAS, s.storageAccessor, s.storageMetadata, s.authorizer, s.callContext)
newAPI := storage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.storageMetadata, s.authorizer, s.callContext)
s.apiv3 = &storage.StorageAPIv3{
StorageAPIv4: storage.StorageAPIv4{
StorageAPIv5: storage.StorageAPIv5{
Expand All @@ -81,6 +82,10 @@ func (s *baseStorageSuite) SetUpTest(c *gc.C) {
}
}

func (s *baseStorageSuite) storageMetadata() (poolmanager.PoolManager, jujustorage.ProviderRegistry, error) {
return s.poolManager, s.registry, nil
}

// TODO(axw) get rid of assertCalls, use stub directly everywhere.
func (s *baseStorageSuite) assertCalls(c *gc.C, expectedCalls []string) {
s.stub.CheckCallNames(c, expectedCalls...)
Expand Down
9 changes: 8 additions & 1 deletion apiserver/facades/client/storage/poollist_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func (s *poolSuite) TestListNoPools(c *gc.C) {
}

func (s *poolSuite) TestListFilterEmpty(c *gc.C) {
err := apiserverstorage.ValidatePoolListFilter(s.api, params.StoragePoolFilter{})
err := apiserverstorage.ValidatePoolListFilter(s.api, s.registry, params.StoragePoolFilter{})
c.Assert(err, jc.ErrorIsNil)
}

Expand All @@ -188,13 +188,15 @@ func (s *poolSuite) TestListFilterValidProviders(c *gc.C) {
s.registerProviders(c)
err := apiserverstorage.ValidateProviderCriteria(
s.api,
s.registry,
[]string{validProvider})
c.Assert(err, jc.ErrorIsNil)
}

func (s *poolSuite) TestListFilterUnregisteredProvider(c *gc.C) {
err := apiserverstorage.ValidateProviderCriteria(
s.api,
s.registry,
[]string{validProvider})
c.Assert(err, gc.ErrorMatches, `storage provider "loop" not found`)
}
Expand All @@ -203,6 +205,7 @@ func (s *poolSuite) TestListFilterUnknownProvider(c *gc.C) {
s.registerProviders(c)
err := apiserverstorage.ValidateProviderCriteria(
s.api,
s.registry,
[]string{invalidProvider})
c.Assert(err, gc.ErrorMatches, `storage provider "invalid" not found`)
}
Expand All @@ -225,6 +228,7 @@ func (s *poolSuite) TestListFilterValidProvidersAndNames(c *gc.C) {
s.registerProviders(c)
err := apiserverstorage.ValidatePoolListFilter(
s.api,
s.registry,
params.StoragePoolFilter{
Providers: []string{validProvider},
Names: []string{validName}})
Expand All @@ -235,6 +239,7 @@ func (s *poolSuite) TestListFilterValidProvidersAndInvalidNames(c *gc.C) {
s.registerProviders(c)
err := apiserverstorage.ValidatePoolListFilter(
s.api,
s.registry,
params.StoragePoolFilter{
Providers: []string{validProvider},
Names: []string{invalidName}})
Expand All @@ -244,6 +249,7 @@ func (s *poolSuite) TestListFilterValidProvidersAndInvalidNames(c *gc.C) {
func (s *poolSuite) TestListFilterInvalidProvidersAndValidNames(c *gc.C) {
err := apiserverstorage.ValidatePoolListFilter(
s.api,
s.registry,
params.StoragePoolFilter{
Providers: []string{invalidProvider},
Names: []string{validName}})
Expand All @@ -253,6 +259,7 @@ func (s *poolSuite) TestListFilterInvalidProvidersAndValidNames(c *gc.C) {
func (s *poolSuite) TestListFilterInvalidProvidersAndNames(c *gc.C) {
err := apiserverstorage.ValidatePoolListFilter(
s.api,
s.registry,
params.StoragePoolFilter{
Providers: []string{invalidProvider},
Names: []string{invalidName}})
Expand Down
121 changes: 73 additions & 48 deletions apiserver/facades/client/storage/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,29 @@ import (
"github.com/juju/juju/storage/poolmanager"
)

type storageMetadataFunc func() (poolmanager.PoolManager, storage.ProviderRegistry, error)

// StorageAPI implements the latest version (v6) of the Storage API.
type StorageAPI struct {
backend backend
storageAccess storageAccess
registry storage.ProviderRegistry
poolManager poolmanager.PoolManager
authorizer facade.Authorizer
callContext context.ProviderCallContext
modelType state.ModelType
backend backend
storageAccess storageAccess
storageMetadata storageMetadataFunc
authorizer facade.Authorizer
callContext context.ProviderCallContext
modelType state.ModelType
}

// APIv5 implements the storage v5 API.
// StorageAPIv5 implements the storage v5 API.
type StorageAPIv5 struct {
StorageAPI
}

// APIv4 implements the storage v4 API adding AddToUnit, Import and Remove (replacing Destroy)
// StorageAPIv4 implements the storage v4 API adding AddToUnit, Import and Remove (replacing Destroy)
type StorageAPIv4 struct {
StorageAPIv5
}

// APIv3 implements the storage v3 API.
// StorageAPIv3 implements the storage v3 API.
type StorageAPIv3 struct {
StorageAPIv4
}
Expand All @@ -64,15 +65,21 @@ func NewStorageAPI(ctx facade.Context) (*StorageAPI, error) {
if err != nil {
return nil, errors.Trace(err)
}
registry, err := stateenvirons.NewStorageProviderRegistryForModel(
model,
stateenvirons.GetNewEnvironFunc(environs.New),
stateenvirons.GetNewCAASBrokerFunc(caas.New))
if err != nil {
return nil, errors.Trace(err)
storageMetadata := func() (poolmanager.PoolManager, storage.ProviderRegistry, error) {
model, err := st.Model()
if err != nil {
return nil, nil, errors.Trace(err)
}
registry, err := stateenvirons.NewStorageProviderRegistryForModel(
model,
stateenvirons.GetNewEnvironFunc(environs.New),
stateenvirons.GetNewCAASBrokerFunc(caas.New))
if err != nil {
return nil, nil, errors.Trace(err)
}
pm := poolmanager.New(state.NewStateSettings(st), registry)
return pm, registry, nil
}
pm := poolmanager.New(state.NewStateSettings(st), registry)

storageAccessor, err := getStorageAccessor(st)
if err != nil {
return nil, errors.Annotate(err, "getting backend")
Expand All @@ -82,26 +89,24 @@ func NewStorageAPI(ctx facade.Context) (*StorageAPI, error) {
if !authorizer.AuthClient() {
return nil, apiservererrors.ErrPerm
}
return newStorageAPI(stateShim{st}, model.Type(), storageAccessor, registry, pm, authorizer, context.CallContext(st)), nil
return newStorageAPI(stateShim{st}, model.Type(), storageAccessor, storageMetadata, authorizer, context.CallContext(st)), nil
}

func newStorageAPI(
backend backend,
modelType state.ModelType,
storageAccess storageAccess,
registry storage.ProviderRegistry,
pm poolmanager.PoolManager,
storageMetadata storageMetadataFunc,
authorizer facade.Authorizer,
callContext context.ProviderCallContext,
) *StorageAPI {
return &StorageAPI{
backend: backend,
modelType: modelType,
storageAccess: storageAccess,
registry: registry,
poolManager: pm,
authorizer: authorizer,
callContext: callContext,
backend: backend,
modelType: modelType,
storageAccess: storageAccess,
storageMetadata: storageMetadata,
authorizer: authorizer,
callContext: callContext,
}
}

Expand Down Expand Up @@ -380,14 +385,19 @@ func (a *StorageAPI) ensureStoragePoolFilter(filter params.StoragePoolFilter) pa
}

func (a *StorageAPI) listPools(filter params.StoragePoolFilter) ([]params.StoragePool, error) {
if err := a.validatePoolListFilter(filter); err != nil {
pm, registry, err := a.storageMetadata()
if err != nil {
return nil, errors.Trace(err)
}
pools, err := a.poolManager.List()
if err := a.validatePoolListFilter(registry, filter); err != nil {
return nil, errors.Trace(err)
}

pools, err := pm.List()
if err != nil {
return nil, errors.Trace(err)
}
providers, err := a.registry.StorageProviderTypes()
providers, err := registry.StorageProviderTypes()
if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -455,8 +465,8 @@ func filterPools(
return all
}

func (a *StorageAPI) validatePoolListFilter(filter params.StoragePoolFilter) error {
if err := a.validateProviderCriteria(filter.Providers); err != nil {
func (a *StorageAPI) validatePoolListFilter(registry storage.ProviderRegistry, filter params.StoragePoolFilter) error {
if err := a.validateProviderCriteria(registry, filter.Providers); err != nil {
return errors.Trace(err)
}
if err := a.validateNameCriteria(filter.Names); err != nil {
Expand All @@ -474,9 +484,9 @@ func (a *StorageAPI) validateNameCriteria(names []string) error {
return nil
}

func (a *StorageAPI) validateProviderCriteria(providers []string) error {
func (a *StorageAPI) validateProviderCriteria(registry storage.ProviderRegistry, providers []string) error {
for _, p := range providers {
_, err := a.registry.StorageProvider(storage.ProviderType(p))
_, err := registry.StorageProvider(storage.ProviderType(p))
if err != nil {
return errors.Trace(err)
}
Expand All @@ -486,20 +496,26 @@ func (a *StorageAPI) validateProviderCriteria(providers []string) error {

// CreatePool creates a new pool with specified parameters.
func (a *StorageAPIv4) CreatePool(p params.StoragePool) error {
_, err := a.poolManager.Create(
p.Name,
storage.ProviderType(p.Provider),
p.Attrs)
return err
result, err := a.StorageAPIv5.CreatePool(params.StoragePoolArgs{
Pools: []params.StoragePool{p},
})
if err != nil {
return apiservererrors.ServerError(err)
}
return result.OneError()
}

// CreatePool creates a new pool with specified parameters.
func (a *StorageAPI) CreatePool(p params.StoragePoolArgs) (params.ErrorResults, error) {
results := params.ErrorResults{
Results: make([]params.ErrorResult, len(p.Pools)),
}
pm, _, err := a.storageMetadata()
if err != nil {
return params.ErrorResults{}, errors.Trace(err)
}
for i, pool := range p.Pools {
_, err := a.poolManager.Create(
_, err := pm.Create(
pool.Name,
storage.ProviderType(pool.Provider),
pool.Attrs)
Expand Down Expand Up @@ -1088,7 +1104,12 @@ func (a *StorageAPI) importStorage(arg params.ImportStorageParams) (*params.Impo
return nil, errors.NotValidf("pool name %q", arg.Pool)
}

cfg, err := a.poolManager.Get(arg.Pool)
pm, registry, err := a.storageMetadata()
if err != nil {
return nil, errors.Trace(err)
}

cfg, err := pm.Get(arg.Pool)
if errors.IsNotFound(err) {
cfg, err = storage.NewConfig(
arg.Pool,
Expand All @@ -1101,7 +1122,7 @@ func (a *StorageAPI) importStorage(arg params.ImportStorageParams) (*params.Impo
} else if err != nil {
return nil, errors.Trace(err)
}
provider, err := a.registry.StorageProvider(cfg.Provider())
provider, err := registry.StorageProvider(cfg.Provider())
if err != nil {
return nil, errors.Trace(err)
}
Expand Down Expand Up @@ -1201,8 +1222,13 @@ func (a *StorageAPI) UpdatePool(p params.StoragePoolArgs) (params.ErrorResults,
if err := a.checkCanWrite(); err != nil {
return results, errors.Trace(err)
}
pm, _, err := a.storageMetadata()
if err != nil {
return results, errors.Trace(err)
}

for i, pool := range p.Pools {
err := a.poolManager.Replace(pool.Name, pool.Provider, pool.Attrs)
err := pm.Replace(pool.Name, pool.Provider, pool.Attrs)
if err != nil {
results.Results[i].Error = apiservererrors.ServerError(err)
}
Expand All @@ -1214,15 +1240,14 @@ func (a *StorageAPI) UpdatePool(p params.StoragePoolArgs) (params.ErrorResults,
// code in rpc/rpcreflect/type.go:newMethod skips 2-argument methods,
// so this removes the method as far as the RPC machinery is concerned.

// Added in v6 api version
// DetachStorage added in v6 api version
func (*StorageAPIv5) DetachStorage(_, _ struct{}) {}

// Added in v5 api version
// RemovePool etc added in v5 api version
func (*StorageAPIv4) RemovePool(_, _ struct{}) {}
func (*StorageAPIv4) UpdatePool(_, _ struct{}) {}

// Added in v4
// Destroy was dropped in V4, replaced with Remove.
// Remove etc added in v4 api version.
func (*StorageAPIv3) Remove(_, _ struct{}) {}
func (*StorageAPIv3) Import(_, _ struct{}) {}
func (*StorageAPIv3) importStorage(_, _ struct{}) {}
Expand Down
4 changes: 2 additions & 2 deletions apiserver/facades/client/storage/storage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ func (s *storageSuite) TestListStorageAsAdminOnNotOwnedModel(c *gc.C) {
s.authorizer = apiservertesting.FakeAuthorizer{
Tag: names.NewUserTag("superuserfoo"),
}
s.api = facadestorage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.registry, s.poolManager, s.authorizer, s.callContext)
s.api = facadestorage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.storageMetadata, s.authorizer, s.callContext)

// Sanity check before running test:
// Ensure that the user has NO read access to the model but SuperuserAccess
Expand All @@ -860,7 +860,7 @@ func (s *storageSuite) TestListStorageAsNonAdminOnNotOwnedModel(c *gc.C) {
s.authorizer = apiservertesting.FakeAuthorizer{
Tag: names.NewUserTag("userfoo"),
}
s.api = facadestorage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.registry, s.poolManager, s.authorizer, s.callContext)
s.api = facadestorage.NewStorageAPIForTest(s.state, state.ModelTypeIAAS, s.storageAccessor, s.storageMetadata, s.authorizer, s.callContext)

// Sanity check before running test:
// Ensure that the user has NO read access to the model and NO SuperuserAccess
Expand Down
7 changes: 4 additions & 3 deletions snap/local/wrappers/fetch-oci
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ if ! which $container_cmd; then
fi
fi

# We will give up if below steps can not be finished in 110s because snapd will kill us in 120s.
# It's not an install/refresh blocker if we can't get the images.
timeout 110s sh <<EOT || echo "Timed out waiting to install microk8s image."
echo "Wait for microk8s to be ready if needed."
wait_result=$(microk8s.status --wait-ready --timeout 30 2>&1)
if [ $? -ne 0 ]; then
Expand All @@ -36,9 +40,6 @@ oci_image="docker.io/jujusolutions/jujud-operator:$juju_version"
mongo_image="docker.io/jujusolutions/juju-db:4.0"
echo "Going to cache images: $oci_image and $mongo_image."

# It's not an install/refresh blocker if we can't get the images.
timeout 90s sh <<EOT || echo "Timed out waiting to install microk8s image."
echo "Pulling: $oci_image."
$container_cmd $image_arg list | grep $oci_image || $container_cmd $image_arg pull $oci_image || true
echo "Pulling: $mongo_image."
Expand Down
Loading

0 comments on commit fd9ccb8

Please sign in to comment.