Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a simplifed dump-model #7471

Merged
merged 3 commits into from Jun 8, 2017
Merged

Conversation

howbazaar
Copy link
Contributor

Description of change

juju status is shown to be terribly slow on very large systems. This is mostly due to how complexity of status has grown over time and the O(n²) operations it has. This code adds an arg struct to state.Export to allow a simplified export which could be used in the status generation. This allows us to test the timing of the information gathering aspect of what status could be in order to determine if the work is worthwhile doing.

QA steps

juju bootstrap lxd testing
juju deploy ubuntu
JUJU_DEV_FEATURE_FLAGS=developer-mode juju dump-model

observe that status history, ssh host keys, model config is in the output

JUJU_DEV_FEATURE_FLAGS=developer-mode juju dump-model --simplified

observe that those values are empty.

Documentation changes

None at this stage.

@howbazaar
Copy link
Contributor Author

The output format has changed to deal with a bug in dump-model where all the integers in the model description came out in the YAML as floats.

This was because the model was deserialized in the apiserver into map[string]interface{} and then passed over the JSON RPC methods, which only knows floats. When the map on the other side was converted to YAML, the numbers stayed floats.

Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things, including a missing test.
If you use the facade context form of the api facade New() method, you get the state pool right there and it's trivial to then use it instead of ForModel()

@@ -353,6 +382,7 @@ func (m *ModelManagerAPI) dumpModel(args params.Entity) (map[string]interface{},

st := m.state
if st.ModelTag() != modelTag {
// XXX state pool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's stuff all work to use the state pool - could we do it as a drive by?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I intended to. Done now.

@@ -54,6 +55,7 @@ func (c *dumpCommand) Info() *cmd.Info {
func (c *dumpCommand) SetFlags(f *gnuflag.FlagSet) {
c.ControllerCommandBase.SetFlags(f)
c.out.AddFlags(f, "yaml", output.DefaultFormatters)
f.BoolVar(&c.simplified, "simplified", false, "Dump a simplified partial model")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that this defaults to false, but we should print a warning if true and only v2 api is available, lest the user expects they will get a simplified model and then they don't get one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@@ -32,14 +32,15 @@ func (f *fakeDumpClient) Close() error {
return f.NextErr()
}

func (f *fakeDumpClient) DumpModel(model names.ModelTag) (map[string]interface{}, error) {
func (f *fakeDumpClient) DumpModel(model names.ModelTag, simplified bool) (map[string]interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should record the value of simplified and in TestDumpSimple verify that it was indeed as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encode it in the response map and assert in the tests.


// ExportPartial the current model for the State optionally skipping
// aspects as defined by the ExportConfig.
func (st *State) ExportPartial(cfg ExportConfig) (description.Model, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some tests

@@ -412,10 +467,26 @@ func (m *ModelManagerAPI) dumpModelDB(args params.Entity) (map[string]interface{
return st.DumpAll()
}

func (m *ModelManagerAPI) DumpModels(args params.DumpModelRequest) params.StringResults {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a comment similar to the V2 method below which does have a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@howbazaar
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Jun 8, 2017

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

@jujubot jujubot merged commit 2ff4965 into juju:2.2 Jun 8, 2017
@howbazaar howbazaar deleted the 2.2-state-export-tweaks branch June 8, 2017 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants