Remove various state.Model constructors #7710

Merged
merged 15 commits into from Aug 9, 2017

Conversation

Projects
None yet
3 participants
Contributor

mjs commented Aug 8, 2017

Description of change

This change makes most of the steps so that state.Model instances can only be created from state.State associated to that model. There were many ways to create a state.Model from any State which meant that the Model referred to a state.State associated with the wrong model. This is dangerous - requiring great care - and limits what can be done by Model instances.

Much functionality will soon be moved onto state.Model, requiring that the state.State it references is the correct one.

Change highlights:

  • State.ControllerModel() has been replaced by State.ControllerModelTag() and State.ControllerModelUUID()
  • State.AllModels() has been replaced by State.AllModelUUIDs()
  • State.ModelsForUser() now returns UUIDs instead of UserModels. state.UserModel no longer exists.
  • Fixed a bug in State.ModelsForUser() where dead or importing models could be reported for admin users.
  • Access to Model instances is now done via state.State in most cases. Added a StatePool.GetModel() helper as a convenience.
  • Many changes in the apiserver package resulted as fallout from the above. This resulted in most calls to State.ForModel() being removed (bug 1698702)

There is still one state.Model constructor to remove: state.State.GetModel(). This will be done in the next PR - this one was getting big enough.

QA steps

Confirmed that core model, controller and migration functionality worked:

$ juju bootstrap lxd source
# wait
$ juju bootstrap lxd target
# wait
$ juju switch source
$ juju add-model foo
$ juju deploy ubuntu
# wait
$ juju migrate foo target
# wait
$ juju debug-log -m source:controller -l ERROR # check controller logs for errors
$ juju debug-log -m target:controller -l ERROR # check controller logs for errors
$ juju destroy-controller -y --destroy-all-models source
$ juju destroy-controller -y --destroy-all-models target

Documentation changes

N.A. - internal only

Bug reference

Drive-by changes fixed https://bugs.launchpad.net/juju/+bug/1698702

@@ -99,21 +101,13 @@ var _ ModelManagerBackend = (*modelManagerStateShim)(nil)
type modelManagerStateShim struct {
*state.State
+ pool *state.StatePool
@wallyworld

wallyworld Aug 8, 2017

Owner

Not sure i agree with this - the state shim is supposed to only wrap state to provide access to its methods.
I rather stick to keeping state and state pool separate in these shim objects to reflect the semantics of those structs

@mjs

mjs Aug 8, 2017

Contributor

I had them separate initially but it worked out much nicer to wrap them together. The backend interface then just represents everything to do with "state". It make the tests cleaner too.

@@ -91,7 +91,7 @@ func NewOffersAPI(ctx facade.Context) (*OffersAPI, error) {
return createOffersAPI(
GetApplicationOffers,
environFromModel,
- GetStateAccess(ctx.State()),
+ GetStateAccess(ctx.State(), ctx.StatePool()),
@wallyworld

wallyworld Aug 8, 2017

Owner

we have a state pool below, prefer to keep them seperate

@mjs

mjs Aug 8, 2017

Contributor

I had missed that a pool was being used separately here. I would actually like to update the PR to extend the Backend interface to have a GetBackend method and not pass the pool directly to createOffersAPI at all.

Happy to discuss further of course.

@wallyworld

wallyworld Aug 8, 2017

Owner

as in GetBackend(uuid)? Like is done for model manager backend I'm guessing

state/modeluser.go
- }
- return result, nil
-}
-
// ModelsForUser returns a list of models that the user
@wallyworld

wallyworld Aug 8, 2017

Owner

It really returns the UUIDs right?
SHould these methods that still say "Models" be renamed to "ModelUUIDs" for better clarity?

@mjs

mjs Aug 9, 2017

Contributor

Done

Please add a todo (or two) to alter the backends to just take a state pool

mjs added some commits Aug 1, 2017

state: Simplify Model refreshing
There is no need for Model.refresh to take a query. It just needs a
model UUID.
state: Remove state.ControllerModel()
This is part of a series of changes to clean up how Model instances
are created. The ControllerModel method has been replaced with
ControlModelUUID and ControllerModelTag methods. To get a
controller Model you must now go via a State for the controller.

To simplify one common use case, a new ControllerOwner method has been
added to State.
state: Removed State.AllModels
Removed yet another dodgy way that Models were being
instantiated. This had a lot of follow-on effects.
state: Added StatePool.GetModel
This is a convenience helper for a common use case.
state: Removed UserModel
State.ModelsForUser now returns UUIDs instead of UserModels. The
caller can retrieve the Model separately if required.

Also fixed a bug where Dead or importing models were reported for the
admin user.
apiserver/facades: Remove AllModels and ForModels usage
AllModels is now replaced with AllModelUUIDs combined with
GetModel. Exposing an AllModels backend interface was problematic in
terms releasing States. Separating this out means we're not using
State instances for Models which have already been released.

Various uses for ForModel were also removed as part of this. This is
fixes https://bugs.launchpad.net/juju/+bug/1698702.
apiserver: Remove explicit pool usage around ModelManagerAPI
ModelManagerAPI no longer takes a StatePool, instead relying on the
GetModel and GetBackend methods on the state backends it takes. This
cleans up the implementation and tests signficantly.
apiserver/facades/client/applicationoffers: Normalise pool usage
A StatePool was being passed in as well as being used in the state
backend. The state backend no longer uses a pool and the separate pool
is used consistently.
state: Model listing methods always sort by (name, owner)
Document the methods that already sorted and make it so ModelsForUser
sorts in the same way too.
state: Don't exclude dead models from ModelsForUser
It turns out that visibility of these is required.
Contributor

mjs commented Aug 9, 2017

$$merge$$

Contributor

jujubot commented Aug 9, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 4108b27 into juju:develop Aug 9, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@mjs mjs deleted the mjs:model-refactor branch Aug 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment