/juju

Wait for machines on controller kill and destroy commands #6674

Merged
merged 1 commit into from Dec 8, 2016

Conversation

Reviewers

@bz2 bz2

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@macgreagoir @mitechie @axw @jujubot @bz2
@macgreagoir
Contributor

macgreagoir commented Dec 8, 2016

kill-controller and destroy-controller now also wait on machines yet
to be removed as well as the previous behaviour of waiting on models yet
to be destroyed. This fixes a bug in which machines in the controller
model were not given time to complete their removal before the
controller itself was destroyed.

Fixes lp:1642295

QA steps:

  • Best seen in a manual environment, as per the bug
  • bootstrap, then add-machine to the controller model
  • destroy-controller and observe the destroy procedure continue until the machine in the controller model is removed, which will happen after the default model is destroyed
$ juju destroy-controller -y manual
Destroying controller
Waiting for hosted model resources to be reclaimed
Waiting on 0 model, 1 machine
Waiting on 0 model, 1 machine
Waiting on 0 model, 1 machine
All hosted models reclaimed, cleaning up controller machines
  • Repeat testing kill-controller
@mitechie
Owner

mitechie commented Dec 8, 2016

ping @bz2 for peeking at this please and thank you
@axw
Member

axw commented Dec 8, 2016

Noice, turned out to be much simpler than I expected. LGTM, but please wait for @bz2's comments.
@bz2

bz2 suggested changes Dec 8, 2016
View changes

Code change looks good, but I think it's time to come up with nicer spelling.

cmd/juju/controller/destroy.go
ctx.Infof("Waiting for hosted model resources to be reclaimed")
- for ; hasUnDeadModels(modelsStatus); ctrStatus, modelsStatus = updateStatus(2 * time.Second) {
+ for ; hasUnDeadModels(modelsStatus) || ctrStatus.HostedMachineCount > 0; ctrStatus, modelsStatus = updateStatus(2 * time.Second) {
@bz2

bz2 Dec 8, 2016

Contributor

With the loop getting this complicated (and sort of repeated twice), I think it warrants some refactoring.

Maybe updateStatus should return a single encapsulating object, which a (renamed) hasUnDeadModels should take and return a bool?

@bz2

bz2 approved these changes Dec 8, 2016
View changes

Thanks! I think this is much easier to follow now.

@macgreagoir
Wait for machines on controller kill and destroy commands 
`kill-controller` and `destroy-controller` now also wait on machines yet
to be removed as well as the previous behaviour of waiting on models yet
to be destroyed. This fixes a bug in which machines in the controller
model were not given time to complete their removal before the
controller itself was destroyed.

Fixes lp:1642295
daeff6e
@macgreagoir
Contributor

macgreagoir commented Dec 8, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Dec 8, 2016

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

jujubot commented Dec 8, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9830
@macgreagoir
Contributor

macgreagoir commented Dec 8, 2016

!!build!!
@macgreagoir
Contributor

macgreagoir commented Dec 8, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Dec 8, 2016

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

@jujubot jujubot merged commit e9e1a46 into juju:develop Dec 8, 2016
1 check passed

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

macgreagoir added a commit to macgreagoir/juju that referenced this pull request Dec 9, 2016

@jujubot @macgreagoir
Merge pull request #6674 from macgreagoir/destroy-wait-on-machines 
Wait for machines on controller kill and destroy commands

`kill-controller` and `destroy-controller` now also wait on machines yet
to be removed as well as the previous behaviour of waiting on models yet
to be destroyed. This fixes a bug in which machines in the controller
model were not given time to complete their removal before the
controller itself was destroyed.

Fixes lp:1642295

QA steps:
 * Best seen in a manual environment, as per the bug
 * `bootstrap`, then `add-machine` to the controller model
 * `destroy-controller` and observe the destroy procedure continue until the machine in the controller model is removed, which will happen after the default model is destroyed
```
$ juju destroy-controller -y manual
Destroying controller
Waiting for hosted model resources to be reclaimed
Waiting on 0 model, 1 machine
Waiting on 0 model, 1 machine
Waiting on 0 model, 1 machine
All hosted models reclaimed, cleaning up controller machines
```
 * Repeat testing `kill-controller`
94f2012

macgreagoir added a commit to macgreagoir/juju that referenced this pull request Dec 13, 2016

@jujubot @macgreagoir
Merge pull request #6674 from macgreagoir/destroy-wait-on-machines 
Wait for machines on controller kill and destroy commands

`kill-controller` and `destroy-controller` now also wait on machines yet
to be removed as well as the previous behaviour of waiting on models yet
to be destroyed. This fixes a bug in which machines in the controller
model were not given time to complete their removal before the
controller itself was destroyed.

Fixes lp:1642295

QA steps:
 * Best seen in a manual environment, as per the bug
 * `bootstrap`, then `add-machine` to the controller model
 * `destroy-controller` and observe the destroy procedure continue until the machine in the controller model is removed, which will happen after the default model is destroyed
```
$ juju destroy-controller -y manual
Destroying controller
Waiting for hosted model resources to be reclaimed
Waiting on 0 model, 1 machine
Waiting on 0 model, 1 machine
Waiting on 0 model, 1 machine
All hosted models reclaimed, cleaning up controller machines
```
 * Repeat testing `kill-controller`
13a0556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment