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

undertaker: Enable force-destroying model with invalid cloud credential #10272

Merged
merged 3 commits into from Jun 4, 2019

Conversation

@babbageclunk
Copy link
Member

commented Jun 1, 2019

Description of change

If the model's cloud credential was invalid the model couldn't be destroyed, even with --force. This was because the undertaker was guarded by ifCredentialValid.

Removing the undertaker's guard wasn't sufficient, because the environ tracker is also guarded, so the undertaker's manifold is changed so it uses a custom environment destroyer if the environ tracker is unavailable (rather than just stopping). The custom destroyer always fails (because it can't talk to the environment at the moment) but the changes in #10245 mean that destroy-model --force will still be able to remove the model from the controller.

QA steps

  • Bootstrap a controller against a cloud where you can disable the credential used.
  • Add models m1 and m2.
  • Deploy an application in m1.
  • Disable the credential.
  • Add a unit to m1 to trigger suspension.
  • Destroying m2 fails because the credential is invalid.
  • Destroying m2 with --force succeeds, while logging an error that the cloud environment couldn't be torn down.

Documentation changes

@pmatulis it should be made clear in the destroy-model docs that if a model is force-destroyed then resources might not be cleaned up in the underlying cloud.

Bug reference

https://bugs.launchpad.net/juju/+bug/1827988

Remove pruner dependency on environ tracker
Neither the status history or action pruners need access to the cloud
to do their jobs. Looking at commit history it appears to have been a
thinko a while ago.
Remove the undertaker ifCredentialValid guard
We want the worker to run even if the cloud has no valid credentials,
so it can remove models if they're force-destroyed.
Let the undertaker run if the environ tracker is unavailable
In that case we start with a destroyer that always fails. This means
that the undertaker can remove models that are force-destroyed, while
non-forced destructions still fail (because the cloud environment
can't be cleaned up).

Because this manifold still depends on the environ tracker, the
undertaker will be restarted if the environ tracker becomes available
later on (eg. because the credentials have been fixed).
@babbageclunk

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

@anastasiamac

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

!!build!!

cmd/jujud/agent/model/manifolds.go Show resolved Hide resolved
cmd/jujud/agent/model/manifolds_test.go Show resolved Hide resolved
@@ -32,7 +33,12 @@ func (config ManifoldConfig) start(context dependency.Context) (worker.Worker, e
return nil, errors.Trace(err)
}
var destroyer environs.CloudDestroyer
if err := context.Get(config.CloudDestroyerName, &destroyer); err != nil {
if err := context.Get(config.CloudDestroyerName, &destroyer); isErrMissing(err) {

This comment has been minimized.

Copy link
@anastasiamac

anastasiamac Jun 3, 2019

Member

Nice \o/

worker/undertaker/manifold.go Show resolved Hide resolved
@babbageclunk

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

!!build!!

@babbageclunk

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

$$merge$$

@babbageclunk

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Timed out getting GPG key?
$$merge$$

@babbageclunk

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Failed to start mongo. I'm getting all the weirdos today!
$$merge$$

@jujubot jujubot merged commit 0b8134a into juju:2.6 Jun 4, 2019

1 of 2 checks passed

merge-multi-juju CI Run in progress
Details
check-multi-juju Build finished.
Details

@babbageclunk babbageclunk deleted the babbageclunk:undertaker-credential-guard branch Jun 4, 2019

jujubot added a commit that referenced this pull request Jun 4, 2019
Merge pull request #10283 from anastasiamac/2.6-merge-develop-0604
#10283

## Description of change


Merge pull request #10271 from babbageclunk/force-remove-machine-cleanup

Merge pull request #10272 from babbageclunk/undertaker-credential-guard

Merge pull request #10281 from anastasiamac/deprecate-add-cloud-replace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.