Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
2.3 direct models 1732384 #8128
Conversation
jameinel
and others
added some commits
Nov 15, 2017
anastasiamac
approved these changes
Nov 26, 2017
We are so close \o/
Thank you for under-taking this and completing it in such a short time :)
I've left some comments but this is a lot better than what we currently have and I'd love to see this in RC for users to take for a spin. We can follow up on any fall-outs/test gaps later...
| @@ -30,6 +30,8 @@ type ModelManagerBackend interface { | ||
| ModelUUID() string | ||
| ModelUUIDsForUser(names.UserTag) ([]string, error) | ||
| + ModelBasicInfoForUser(user names.UserTag) ([]state.ModelAccessInfo, error) |
anastasiamac
Nov 26, 2017
Member
Did we want to keep both ModelBasicInfoForUser and ModelSummariesForUser?
jameinel
Nov 27, 2017
Owner
I don't like the naming of ModelBasicInfoForUser but it is a necessary backing for "ListModels"
We might be able to get rid of ModelUUIDsForUser, though.
| @@ -160,6 +162,7 @@ func (st modelManagerStateShim) GetModel(modelUUID string) (Model, func() bool, | ||
| // Model implements ModelManagerBackend. | ||
| func (st modelManagerStateShim) Model() (Model, error) { | ||
| + st.State.Model() |
anastasiamac
Nov 26, 2017
Member
Maybe worth a comment why this line is needed?... Are we loading model into state here?
jameinel
Nov 27, 2017
Owner
I actually think it shouldn't be there. I'm a bit surprised that it is. (removed)
| @@ -0,0 +1,217 @@ | ||
| +// Copyright 2015 Canonical Ltd. |
| + attrs["agent-version"] = jujuversion.Current.String() | ||
| + cfg, err := config.New(config.UseDefaults, attrs) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + |
| -// ListModels returns the models that the specified user | ||
| -// has access to in the current server. Only that controller owner | ||
| +// ListModelSummaries returns models that the specified user | ||
| +// has access to in the current server. Only the controller owner |
anastasiamac
Nov 26, 2017
Member
Since we do not have the concept of 'controller owner', maybe we should say here 'controller admin'?
| + if err == nil { | ||
| + summary.UserAccess = access | ||
| + } | ||
| + // TODO (anastasiamac 2017-11-24) what happens here if there is no migration in progress? |
| +} | ||
| + | ||
| +// ListModels returns the models that the specified user | ||
| +// has access to in the current server. Only that controller owner |
jameinel
Nov 27, 2017
Owner
Replaced with:
// ListModels returns the models that the specified user
// has access to in the current server. Controller admins (superuser)
// can list models for any user. Other users
// can only ask about their own models.
good?
| - OwnerTag: model.Owner().String(), | ||
| + Name: mi.Name, | ||
| + UUID: mi.UUID, | ||
| + OwnerTag: names.NewUserTag(mi.Owner).String(), |
jameinel
Nov 27, 2017
Owner
we don't have a place to inject the error as a response.
It would be a bug in our output of ModelBasicInfoForUser (rather than a bug in user input).
But sure, we'll do as such.
Panic() in production code is bad mojo.
| + | ||
| + haveModels := false | ||
| + if modelmanagerAPI.BestAPIVersion() > 3 { | ||
| + // New code path |
| + w.Println("Access", "Last connection") | ||
| +} | ||
| + | ||
| +// formatTabular takes model summaries set to adhere to the cmd.Formatter interface |
| +// This test is only needed for older api versions as | ||
| +// whether the user has an access to a model will be checked on | ||
| +// the api side and the model data will not be sent. | ||
| +func (s *BaseModelsSuite) TestAllModelsWithOneUnauthorised(c *gc.C) { |
anastasiamac
Nov 26, 2017
Member
My thinking was - either the test comment is incorrect or this should reside on ModelSuiteV3 only. It looks like the test is passing for both v3 and v4 suites, so maybe get rid of the comment?
| @@ -99,12 +99,14 @@ func (s *cmdControllerSuite) TestCreateModelAdminUser(c *gc.C) { | ||
| func (s *cmdControllerSuite) TestAddModelNormalUser(c *gc.C) { | ||
| s.createModelNormalUser(c, "new-model", false) | ||
| context := s.run(c, "list-models", "--all") | ||
| + // TODO (anastasiamac 2017-11-23) currently logged in user does not have any right to the model, |
| + Status status.StatusInfo | ||
| + | ||
| + // Needs ModelUser, ModelUserLastConnection, and Permissions collections | ||
| + // This information will only be filled out if includeUsers is true and the user has at least Admin (write?) access |
| + | ||
| + cfg, err := config.New(config.NoDefaults, doc.Settings) | ||
| + if err != nil { | ||
| + // err on one model should kill all the other ones? |
jameinel
Nov 27, 2017
Owner
well, it currently does, what errors should be considered ok to just skip and go on to the next.
Is it just mgo.NotFound?
What errors can config.New give?
We have lots of potential validation errors.
It doesn't quite feel right that an invalid config on a model would cause that model to disappear from "juju models"
Should we be adding an 'err' field on ModelSummaries?
| + } | ||
| + if !remaining.IsEmpty() { | ||
| + // XXX: What error is appropriate? Do we need to care about models that its ok to be missing? | ||
| + return errors.Errorf("could not find settings/config for models: %v", remaining.SortedValues()) |
anastasiamac
Nov 26, 2017
Member
I'd prefer to log this as a Warning and return nil (i.e. continue processing up the stack)
jameinel
Nov 27, 2017
Owner
I'd like to discuss with you the general things around getting errors on a particular model.
We should be adding tests for it, and improve the behavior.
Should we just be adding cards around this?
| +} | ||
| + | ||
| +// removeExistingUsers takes a set of usernames, and removes any of them that are known-valid users. | ||
| +// it leaves behind names that could not be validated |
jameinel
Nov 27, 2017
Owner
actually, we can just drop this whole function because we don't include users anymore.
removed.
| + for i, uuid := range p.modelUUIDs { | ||
| + statusIds[i] = uuid + ":" + modelGlobalKey | ||
| + } | ||
| + // TODO: Track remaining and error if we're missing any |
anastasiamac
Nov 26, 2017
Member
If we plan to leave TODOs in this code, could we please follow the format TODO (name)...
Although there is 'git blame', the code could be shuffled and then it can become harder to track the person with the context :)
jameinel
Nov 27, 2017
Owner
updated all the TODO I found in this file.
We should probably just go through them all as potential cards and decide if we actually want to do them.
| + } | ||
| + modelIdx, ok := p.indexByUUID[modelUUID] | ||
| + if !ok { | ||
| + // ?? |
| + "gopkg.in/juju/names.v2" | ||
| + "gopkg.in/mgo.v2/bson" | ||
| + "github.com/juju/utils" | ||
| + //"gopkg.in/macaroon.v1" |
anastasiamac
Nov 26, 2017
Member
If we don't need these imports, let's delete them altogether rather than comment out :)
| +} | ||
| + | ||
| +func (s *ModelSummariesSuite) TestModelsForUser2(c *gc.C) { | ||
| + // User1 is only added to the model they own and the shared model |
| + c.Check(names, gc.DeepEquals, []string{"shared", "user2model"}) | ||
| +} | ||
| + | ||
| +func (s *ModelSummariesSuite) TestModelsForUser3(c *gc.C) { |
anastasiamac
Nov 26, 2017
Member
Does this test the same as TestModelForUser2? (just for a different user but the same in principal...)
jameinel
Nov 27, 2017
Owner
User 3 is admin on the shared model, user 2 is read access on the shared model, user 1 is write access.
I updated the comments to indicate as such.
| + // Since the new model is importing, when we do the list we shouldn't see it. | ||
| + names := s.modelNamesForUser(c, "user3admin") | ||
| + c.Check(names, gc.DeepEquals, []string{"shared", "user3model"}) | ||
| + // Superuser doesn't see importing models, either |
anastasiamac
Nov 26, 2017
Member
Completely off-side but .... I wonder why we have decided that this is how we want things? Would not the importing user, superuser in this case, want to know that there is a model that is about to come in because it is being imported?
I mean is it possible that the user will import, not see the model in the list and will try to import again?
jameinel
Nov 27, 2017
Owner
I have the feeling you're right. the reason we were stripping importing modules is because it caused "juju models" to fail, not because we didn't want them in the output.
We should put this as a card.
| + c.Check(names, gc.DeepEquals, []string{"shared", "testenv", "user1model", "user2model", "user3model"}) | ||
| +} | ||
| + | ||
| +func (s *ModelSummariesSuite) TestContainsConfigInformation(c *gc.C) { |
anastasiamac
Nov 26, 2017
Member
And we'd want a test where there is no config for a model. We can follow it up once this PR lands...
| +func (st *State) isUserSuperuser(user names.UserTag) (bool, error) { | ||
| + access, err := st.UserAccess(user, st.controllerTag) | ||
| + if err != nil { | ||
| + // We don't suppress NotFound becaus |
jameinel
Nov 27, 2017
Owner
actually, the change to --all probably means we have introduced a bug.
Namely, you can now ask for st.ModelSummariesForUser for users that don't exist at all.
Maybe that's not a problem, but it may be better to give a "no such user" rather than an empty list of models.
Thoughts?
I added a TODO so that we can decide how we want this to behave.
| @@ -43,7 +43,7 @@ type settingsDoc struct { | ||
| Settings settingsMap `bson:"settings"` | ||
| // Version is a version number for the settings, | ||
| - // and is increased every time the settings change. | ||
| + // and is increased every time the settingsechange. |
|
I didn't realize this was 2.5k lines patch. |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
|
$$merge$$ (I had committed the fixes, but push failed because I had updated the branch from another machine) |
|
Build failed: Tests failed |
jameinel
added some commits
Nov 27, 2017
|
$$merge$$ when responding to the review comments I broke ListModels (ParseUserTag takes a tag object, not a user string.) |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
jameinel commentedNov 26, 2017
Description of change
juju modelswas implemented very inefficiently. It spent a lot of time re-reading the same tables multiple times, and then all other tables it ended up reading one object at a time, rather than doing large scans over the various tables. We also were returning information that wasn't particularly useful (we computed all the information we knew about every model, only to show a very small summary.)While we have fixed a few of the issues around missing indexes in 2.3, this takes it all a step further and makes it much more efficient overall.
QA steps
In juju 2.2 we also had things like instantiating State objects rather than using the state pool, missing indexes, etc. However, with 200 models, the differences are:
If I do the sampling at the same time on all models, to help show what happens under load, the numbers change to:
Note also that 2.2 always reads all machines for every model (including their status and hardware characteristics), while the proposed 2.3 only computes the summary. So I would expect even better long term scaling on real models.
Documentation changes
I don't think this needs documentation changes. It does change the format of
juju models --format=yamlbut we believe it is not something that users depended upon. The information is still available injuju show-model, we just don't always compute all the details across all models.Bug reference
lp:1732353
lp:1732384