Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
juju destroy-model blocks #6405
Conversation
| + modelFound := newTimedModelFound(ctx, api, names.NewModelTag(modelDetails.ModelUUID), c.clock) | ||
| + found := modelFound(0) | ||
| + for found { | ||
| + ctx.Infof("Waiting for model to be removed...") |
axw
Oct 7, 2016
•
Edited 1 time
-
axw
Oct 7, 2016
Member
-
Oct 7, 2016
Can you please make this do the same thing as destroy-controller? i.e. like
"Waiting on %d machines, %d applications"
The output is not very useful as it is.
| + found := modelFound(0) | ||
| + for found { | ||
| + ctx.Infof("Waiting for model to be removed...") | ||
| + found = modelFound(2 * time.Second) |
| +func newTimedModelFound(ctx *cmd.Context, api DestroyModelAPI, tag names.ModelTag, clock clock.Clock) func(time.Duration) bool { | ||
| + return func(wait time.Duration) bool { | ||
| + if wait > 0 { | ||
| + <-clock.After(wait) |
axw
Oct 7, 2016
•
Edited 1 time
-
axw
Oct 7, 2016
Member
-
Oct 7, 2016
time.Sleep(wait)
EDIT: I guess you're doing this so you can use a mock clock. Perhaps just have a mock "sleep" function, rather than a whole clock.
| + if res[0].Error == nil { | ||
| + return true | ||
| + } | ||
| + return params.ErrCode(res[0].Error) != params.CodeUnauthorized |
axw
Oct 7, 2016
•
Edited 1 time
-
axw
Oct 7, 2016
Member
-
Oct 7, 2016
the intent would be a bit clearer if you used params.IsCodeNotFoundOrCodeUnauthorized
axw
approved these changes
Oct 7, 2016
LGTM with the Controller.ModelStatus permission changes as discussed.
| @@ -156,19 +181,78 @@ func (c *destroyCommand) Run(ctx *cmd.Context) error { | ||
| } | ||
| defer api.Close() | ||
| + // Attempt to connect to the controller API. If we can't, fail the destroy. | ||
| + controllerAPI, err := c.getControllerAPI() |
axw
Oct 7, 2016
Member
This will make a second API connection, which isn't great. It would be better to create a single, composite API from the two facades that we need.
wallyworld
Oct 9, 2016
Owner
We get a second API in several other CLI commands (sometimes even 3). How strongly do you feel about this?
axw
Oct 9, 2016
Member
Not strong enough to block this PR, but other CLI commands being crappy isn't a good argument to make another one crappy. It can be fixed later.
wallyworld
Oct 10, 2016
Owner
Ok, I've added a common.ModelStatus API which is embedded in the Controller and ModelManager facades
| + sleepFunc(wait) | ||
| + status, err := api.ModelStatus(tag) | ||
| + if err != nil { | ||
| + if params.ErrCode(err) != params.CodeNotFound { |
axw
Oct 7, 2016
•
Edited 1 time
-
axw
Oct 7, 2016
Member
-
Oct 7, 2016
you may need to change this to cater for CodeUnauthorized when you change Controller.ModelStatus permission handling? also in existing destroy/kill-controller usage
wallyworld
Oct 9, 2016
Owner
This is ok as is I think - if there would be a permission issue here, then the previous call to Destroy() would fail and we'd never get to this point.
axw
Oct 9, 2016
Member
I was thinking more that ErrPerm might be returned once the model is removed. Presumably you tested it, and it would have shown up if it was a problem.
wallyworld
Oct 9, 2016
Owner
Yeah, this ModelStatus api call returns not found if the model is not there. I did test it and that's how it behaved. I'll double check to be sure.
| +} | ||
| + | ||
| +func formatDestroyModelInfo(data *modelData) string { | ||
| + out := "Waititng on model to be removed" |
| @@ -60,15 +74,18 @@ func (s *DestroySuite) SetUpTest(c *gc.C) { | ||
| s.store.Accounts["test1"] = jujuclient.AccountDetails{ | ||
| User: "admin", | ||
| } | ||
| + s.sleep = func(time.Duration) { | ||
| + time.Sleep(1 * time.Millisecond) |
|
ty @wallyworld !!! |
| - if err := c.checkHasAdmin(); err != nil { | ||
| - return results, errors.Trace(err) | ||
| - } | ||
| + //if err := c.checkHasAdmin(); err != nil { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedOct 7, 2016
•
Edited 1 time
-
wallyworld
Oct 7, 2016
Fixes: https://bugs.launchpad.net/juju/+bug/1611111
juju destroy-model will block until the model is completely cleaned up.
As soon as the command returns, add-model can be called again with the same model name.
QA:
$ juju add-model amodel
Added 'amodel' model on lxd/localhost with credential 'default' for user 'admin'
$ juju destroy-model amodel -y
Destroying model
Waititng on model to be removed, 1 machine(s), 1 application(s)...
Waititng on model to be removed, 1 machine(s), 1 application(s)...
Waititng on model to be removed, 1 machine(s), 1 application(s)...
Waititng on model to be removed, 1 machine(s), 1 application(s)...
Waititng on model to be removed, 1 machine(s)...
Waititng on model to be removed, 1 machine(s)...
Waititng on model to be removed, 1 machine(s)...
Waititng on model to be removed, 1 machine(s)...
Waititng on model to be removed, 1 machine(s)...
Waititng on model to be removed...
Waititng on model to be removed...
Waititng on model to be removed...
$ juju add-model amodel
Added 'amodel' model on lxd/localhost with credential 'default' for user 'admin'
$