[JUJU-3226] Fix destroy-model/destroy-controller to handle --force better. #15328

hpidcock · 2023-03-23T08:24:11Z

--force and --timeout/--model-timeout now properly control the destruction of models so that --force with --timeout properly progresses despite the status of the model/environment.

--force without --timeout now only propagates force to the cleanup of model entities, rather than the model removal itself.

Since model destruction cannot be stopped once it has started, --timeout no longer makes sense to be passed for non-forceful model destruction.

Graceful model destroys now wait indefinitely
for the entities to cleanup and the cloud resources to cleanup before removing the model from state. Due to this change it is now required to be able to change the model destruction parameters while a model destroy is being processed by the undertaker. The undertaker will now restart itself when ForceDestroy or Timeout changes.

QA steps

General bootstrap and model-destroy/controller-destroy, with and without --force/--timeout.

Specifically test the following:

lxc config set core.trust_password <mypassword>
lxc config set core.https_address '[::]'
lxc remote add lxd2 <myip> --password <mypassword>
juju bootstrap localhost
juju add-cloud lxd2 on controller
juju autoload-credentials for lxd2 cloud on controller
juju add-model a lxd2
juju deploy ubuntu
wait for clean deploy
lxc config trust list
lxc config trust remove <lxd2 trust>
juju destroy-model a
watch that it never finishes
new terminal window
juju destroy-model a --force
watch that it also never finishes
juju destroy-model a --force --timeout 1m
watch that it destroys in around 2m and all the other terminals exit gracefully

Documentation changes

Command documentation and possibly something else.
Upgrading a controller with broken models (environ with broken/no credentials) will require some hacky steps due to controller upgrade prechecks requiring functioning environ.

Bug reference

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

wallyworld

Some initial feedback - I am concerned about the shift away from CloudDestroyer and think it will break on caas.

apiserver/facades/controller/undertaker/register.go

cmd/juju/controller/destroy.go

cmd/juju/model/destroy.go

wallyworld · 2023-03-24T02:29:07Z

cmd/jujud/agent/model/manifolds.go

@@ -472,15 +482,15 @@ func CAASManifolds(config ManifoldsConfig) dependency.Manifolds {
 	modelTag := agentConfig.Model()
 	manifolds := dependency.Manifolds{
 		// The undertaker is currently the only ifNotAlive worker.
-		undertakerName: ifNotUpgrading(ifNotAlive(undertaker.Manifold(undertaker.ManifoldConfig{
+		undertakerName: ifNotAlive(undertaker.Manifold(undertaker.ManifoldConfig{


We don't want the undertaker to run if the controllers are upgrading

this is the model upgrading not the controller.

right but we still don't want it running

The model upgrading is for the environ only, we don't run the environ upgrader when the model is dying. Otherwise we could never destroy the model if the environ was dying and not upgradable.

version/version.go

worker/undertaker/manifold.go

wallyworld

Thanks for fixing. I'd like to request a change to be very strict with what param combinations are accepted, ie --force with no timeout does a clean destroy should not be allowed - error in facade and caught in the CLI

wallyworld · 2023-03-25T05:46:30Z

cmd/jujud/agent/model/manifolds.go

@@ -472,15 +482,15 @@ func CAASManifolds(config ManifoldsConfig) dependency.Manifolds {
 	modelTag := agentConfig.Model()
 	manifolds := dependency.Manifolds{
 		// The undertaker is currently the only ifNotAlive worker.
-		undertakerName: ifNotUpgrading(ifNotAlive(undertaker.Manifold(undertaker.ManifoldConfig{
+		undertakerName: ifNotAlive(undertaker.Manifold(undertaker.ManifoldConfig{


right but we still don't want it running

worker/undertaker/undertaker.go

wallyworld · 2023-03-25T06:06:55Z

worker/undertaker/undertaker.go

+	// Even if ForceDestroyed is true, if we don't have a timeout, we treat them the same
+	// as a non-force destroyed model.


Shouldn't we catch this and raise an error. Allowing client code / CLI to submit what they believe is --force but then does a clean destroy is rather disingenuous. We should be very strict about ensuring only valid param combinations are passed given the number of permutations and real potential for confusion.

--force without --timeout is a perfectly valid default, it does still force, but it waits for cleanup ops/tear down dance to finish first.

The undertaker is just there to ensure the cloud resources are indeed destroyed + the model is removed from state.

wallyworld · 2023-03-25T06:07:56Z

worker/undertaker/undertaker.go

 	"fmt"
 	"time"

 	"github.com/juju/clock"
 	"github.com/juju/errors"
 	"github.com/juju/worker/v3/catacomb"
+	"gopkg.in/retry.v1"


FWIW we're trying to move away from this and juju/retry is the preferred lib.

Happy to loop back and use juju/retry. I feel like "gopkg.in/retry.v1" works better here, but will propose a seperate PR to drop it.

apiserver/facades/client/modelupgrader/upgrader.go

--force and --timeout/--model-timeout now properly control the destruction of models so that --force with --timeout properly progresses despite the status of the model/environment. --force without --timeout now only propagates force to the cleanup of model entities, rather than the model removal itself. Since model destruction cannot be stopped once it has started, --timeout no longer makes sense to be passed for non-forceful model destruction. Graceful model destroys now wait indefinitely for the entities to cleanup and the cloud resources to cleanup before removing the model from state. Due to this change it is now required to be able to change the model destruction parameters while a model destroy is being processed by the undertaker. The undertaker will now restart itself when ForceDestroy or Timeout changes.

hpidcock · 2023-06-19T04:50:15Z

/build

hpidcock · 2023-06-19T06:50:17Z

/merge

hpidcock · 2023-06-19T21:44:00Z

/merge

hpidcock · 2023-06-19T22:54:49Z

/merge

#15831 Forward ports: - #15731 - #15755 - #15770 - #15328 - #15762 - #15783 - #15797 - #15827 - #15828 Conflicts: - api/client/modelmanager/modelmanager.go - api/client/modelmanager/modelmanager_test.go - apiserver/facades/client/modelupgrader/upgrader.go - apiserver/facades/client/modelupgrader/upgrader_test.go - apiserver/facades/controller/undertaker/register.go - apiserver/facades/controller/undertaker/undertaker.go - apiserver/facades/controller/undertaker/undertaker_test.go - cmd/juju/controller/destroy.go - cmd/juju/controller/destroy_test.go - cmd/juju/model/destroy.go - cmd/juju/model/destroy_test.go - tests/includes/juju.sh

#15834 Forward ports: - #15731 - #15755 - #15770 - #15328 - #15762 - #15783 - #15797 - #15827 - #15828 - #15831 Conflicts: - api/client/modelmanager/modelmanager_test.go - provider/openstack/firewaller.go

#15835 Forward ports: - #15731 - #15755 - #15770 - #15328 - #15762 - #15783 - #15797 - #15793 - #15815 - #15816 - #15827 - #15828 - #15398 - #15823 - #15831 - #15834 Conflicts: - cmd/juju/controller/destroy.go - cmd/juju/model/destroy.go - state/applicationoffers.go

#15847 Merge 3.3 -> main: - #15731 - #15755 - #15770 - #15328 - #15762 - #15783 - #15797 - #15793 - #15815 - #15816 - #15766 - #15821 - #15827 - #15828 - #15398 - #15823 - #15831 - #15834 - #15835 - #15818 - #15837 - #15839 - #15842 - #15830 - #15844 - #15846 Conflicts: - cmd/juju/application/deploy_test.go - cmd/juju/status/status_internal_test.go

#15864 #15328 made a miss-step in making the upgrader only run when the model is alive, since many things hang off the upgraded flag, including the cleaner worker, which needs to run during model destruction. ## QA steps `./main.sh -v -s '"test_block_commands,test_display_clouds,test_model_config,test_model_defaults,test_unregister"' cli test_local_charms` ## Documentation changes N/A ## Bug reference https://jenkins.juju.canonical.com/job/test-cli-test-local-charms-lxd/1336/consoleText

hpidcock added the 2.9 label Mar 23, 2023

hpidcock changed the title ~~Fix destroy-model/destroy-controller to handle --force better.~~ [JUJU-3225] Fix destroy-model/destroy-controller to handle --force better. Mar 23, 2023

hpidcock force-pushed the fix-undertaker branch from bb6da29 to 30a1d78 Compare March 23, 2023 23:56

wallyworld reviewed Mar 24, 2023

View reviewed changes

hpidcock force-pushed the fix-undertaker branch 2 times, most recently from d872120 to 671ca75 Compare March 24, 2023 07:42

wallyworld approved these changes Mar 25, 2023

View reviewed changes

hpidcock force-pushed the fix-undertaker branch from 671ca75 to 3a8f071 Compare March 27, 2023 05:18

hpidcock added the has merge conflicts label Mar 27, 2023

hpidcock force-pushed the fix-undertaker branch from 3a8f071 to 6b6db0f Compare March 27, 2023 06:46

hpidcock removed the has merge conflicts label Mar 27, 2023

hpidcock force-pushed the fix-undertaker branch from 6b6db0f to 77e03c1 Compare March 27, 2023 07:17

manadart reviewed Mar 28, 2023

View reviewed changes

apiserver/facades/client/modelupgrader/upgrader.go Outdated Show resolved Hide resolved

hpidcock force-pushed the fix-undertaker branch 2 times, most recently from 4d8add3 to 1899c46 Compare April 6, 2023 05:57

hpidcock added the has merge conflicts label May 30, 2023

hpidcock force-pushed the fix-undertaker branch from 1899c46 to 6d5f009 Compare June 5, 2023 03:07

hpidcock removed the has merge conflicts label Jun 5, 2023

hpidcock force-pushed the fix-undertaker branch from 6d5f009 to d7d37c8 Compare June 5, 2023 03:31

hpidcock force-pushed the fix-undertaker branch from d7d37c8 to 69548bc Compare June 5, 2023 22:15

Use timeout tool for clienside timeout

874fd8e

hpidcock changed the title ~~[JUJU-3225] Fix destroy-model/destroy-controller to handle --force better.~~ [JUJU-3226] Fix destroy-model/destroy-controller to handle --force better. Jun 19, 2023

Add destroy-(controller|model) tests for error messages.

b048634

jujubot merged commit c6783de into juju:2.9 Jun 19, 2023
17 of 19 checks passed

hpidcock mentioned this pull request Jun 29, 2023

Merge 2.9 to 3.1 #15831

Merged

hpidcock mentioned this pull request Jun 30, 2023

Merge 3.1 to 3.2 #15834

Merged

hpidcock mentioned this pull request Jun 30, 2023

Merge 3.2 to 3.3 #15835

Merged

ycliuhw mentioned this pull request Jul 3, 2023

Merge 3.3 #15847

Merged

hpidcock mentioned this pull request Jul 6, 2023

Fix cleaner not running when the model is dying. #15864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JUJU-3226] Fix destroy-model/destroy-controller to handle --force better. #15328

[JUJU-3226] Fix destroy-model/destroy-controller to handle --force better. #15328

hpidcock commented Mar 23, 2023

wallyworld left a comment

wallyworld Mar 24, 2023

hpidcock Mar 24, 2023

wallyworld Mar 25, 2023

hpidcock Mar 28, 2023

wallyworld left a comment

wallyworld Mar 25, 2023

wallyworld Mar 25, 2023

hpidcock Apr 6, 2023

wallyworld Mar 25, 2023

hpidcock Apr 6, 2023

hpidcock commented Jun 19, 2023

hpidcock commented Jun 19, 2023

hpidcock commented Jun 19, 2023

hpidcock commented Jun 19, 2023

		// Even if ForceDestroyed is true, if we don't have a timeout, we treat them the same
		// as a non-force destroyed model.

[JUJU-3226] Fix destroy-model/destroy-controller to handle --force better. #15328

[JUJU-3226] Fix destroy-model/destroy-controller to handle --force better. #15328

Conversation

hpidcock commented Mar 23, 2023

QA steps

Documentation changes

Bug reference

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpidcock commented Jun 19, 2023

hpidcock commented Jun 19, 2023

hpidcock commented Jun 19, 2023

hpidcock commented Jun 19, 2023