Skip to content

Commit

Permalink
Merge pull request juju#17684 from ca-scribner/prompt-user-on-model-s…
Browse files Browse the repository at this point in the history 
…torage-destroy

juju#17684

This adds a check during `juju destroy-model` calls to proactively alert the user when they have storage but have not invoked a `--destroy-storage` or `--release-storage` flag. Previously, the user would not be alerted to this until after they've confirmed deletion, leading to an annoying UX.

Because this fix invokes a `ModelStatus` query earlier in the destroy command, many tests needed modifications to pass. This was typically for one of two reasons:
* the default test case includes detachable volumes/filesystems. Some `TestDestroy*` tests previously would "pass" because their assertions were satisfied before the storage mattered, but now it is tested proactively and the tests would fail. These were updated to test without storage.
* some tests have additional `nil` entries in `s.api.modelInfoErr` because `ModelStatus` is callewd an extra time

Tests were updated to handle this.

During QA for this fix, a few driveby fixes were done on the `destroyModelMsg` and `destroyModelMsgDetails` templates. 
 The template only worked for machine models. For example, machine models would get this message:

```
juju destory-model lxd:test
WARNING This command will destroy the "test" model and all its resources. It cannot be stopped.

 - 2 machines will be destroyed
 - machine list: "0 (juju-03f299-0)" "1 (juju-03f299-1)"
 - 2 applications will be removed
 - application list: "lxd" "postgresql"
 - 1 filesystem and 1 volume will be destroyed

To continue, enter the name of the model to be destroyed: 
```

whereas container models see:

```
juju destroy-model container-model:test
WARNING This command will destroy the "test" model and all its resources. It cannot be stopped.


To continue, enter the name of the model to be destroyed:
```

This was likely due to a typo in the `destroyModelMsgDetails` template and has been fixed here.

We also see the same "WARNING This command will destroy X model and all its resources" even when `--release-storage` is specified. The wording has been updated to avoid confusion. 

The message now renders, both for container and machine models, as:

```
juju destroy-model container-model:test
WARNING This command will destroy the "test" model and all its resources. It cannot be stopped.

 - 1 application will be removed
 - application list: "minio"
 - 0 filesystem and 1 volume will be destroyed

To continue, enter the name of the model to be destroyed:
```

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [ ] ~Code style: imports ordered, good names, simple structure, etc~
- [ ] ~Comments saying why design decisions were made~
- [x] Go unit tests, with comments saying what you're testing
- [ ] ~[Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [ ] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

### Machine model

setup:
```
juju bootstrap lxd lxd
juju add-model -c lxd test
# volume storage
juju deploy ch:lxd --model lxd:test --storage local=10G,1
# filesystem storage
juju deploy postgresql --model lxd:test --storage pgdata=lxd,2G
```

without `--destroy-storage` or `--release-storage`:
```
juju destroy-model lxd:test
ERROR cannot destroy model "test"

The model has persistent storage remaining:
 1 filesystem(s)

To destroy the storage, run the destroy-model
command again with the "--destroy-storage" option.

To release the storage from Juju's management
without destroying it, use the "--release-storage"
option instead. The storage can then be imported
into another Juju model.
```

with `--destroy-storage` or `--release-storage`:
```
juju destroy-model lxd:test --destroy-storage
WARNING This command will destroy the "test" model and affect the following resources. It cannot be stopped.

 - 2 machines will be destroyed
 - machine list: "0 (juju-43ffa3-0)" "1 (juju-43ffa3-1)"
 - 2 applications will be removed
 - application list: "lxd" "postgresql"
 - 1 filesystem and 1 volume will be destroyed

To continue, enter the name of the model to be destroyed: test
Destroying model
```

### Kubernetes model

setup:
```
juju add-model test -c microk8s

# volume storage
juju deploy minio --model microk8s:test
```

without flag:
```
juju destroy-model microk8s:test
ERROR cannot destroy model "test"

The model has persistent storage remaining:
 1 volume(s)
 1 filesystem(s)

To destroy the storage, run the destroy-model
command again with the "--destroy-storage" option.

To release the storage from Juju's management
without destroying it, use the "--release-storage"
option instead. The storage can then be imported
into another Juju model.
```

with flag:
```
juju destroy-model microk8s:test --destroy-storage
WARNING This command will destroy the "test" model and affect the following resources. It cannot be stopped.

 - 1 application will be removed
 - application list: "minio"
 - 1 filesystem and 1 volume will be destroyed

To continue, enter the name of the model to be destroyed: test
Destroying model
```

## Documentation changes

## Links

**Launchpad bug:** https://bugs.launchpad.net/juju/+bug/2012593/
  • Loading branch information
@jujubot
jujubot authored Aug 12, 2024
2 parents 016b7e3 + 96933f2 commit 5e07f2c
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 83 deletions.
154 changes: 77 additions & 77 deletions cmd/juju/model/destroy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
package model

import (
"bytes"
"fmt"
"io"
"os"
"strings"
"text/template"
"time"

Expand Down Expand Up @@ -101,18 +101,32 @@ const destroyExamples = `
`

var destroyModelMsg = `
This command will destroy the %q model and all its resources. It cannot be stopped.`[1:]
This command will destroy the %q model and affect the following resources. It cannot be stopped.`[1:]

var destroyModelMsgDetails = `
{{- if gt .MachineCount 0}}
- {{.MachineCount}} {{if .IsCaaS}}container{{else}}machine{{end}}{{if gt .MachineCount 1}}s{{end}} will be destroyed
- {{if .IsCaaS}}container{{else}}machine{{end}} list:{{range .MachineIds}} "{{.}}"{{end}}
- {{.MachineCount}} machine{{if gt .MachineCount 1}}s{{end}} will be destroyed
- machine list:{{range .MachineIds}} "{{.}}"{{end}}
{{- end}}
- {{.ApplicationCount}} application{{if gt .ApplicationCount 1}}s{{end}} will be removed
{{- if gt (len .ApplicationNames) 0}}
- application list:{{range .ApplicationNames}} "{{.}}"{{end}}
{{- end}}
- {{.FilesystemCount}} filesystem{{if gt .FilesystemCount 1}}s{{end}} and {{.VolumeCount}} volume{{if gt .VolumeCount 1}}s{{end}} will be {{if .ReleaseStorage}}released{{else}}destroyed{{end}}
{{- end}}
`

var persistentStorageErrorMsg = `cannot destroy model "%s"
The model has persistent storage remaining:%s
To destroy the storage, run the destroy-model
command again with the "--destroy-storage" option.
To release the storage from Juju's management
without destroying it, use the "--release-storage"
option instead. The storage can then be imported
into another Juju model.
`

// DestroyModelAPI defines the methods on the modelmanager
Expand Down Expand Up @@ -199,18 +213,17 @@ func getApplicationNames(data base.ModelStatus) []string {
}

// printDestroyWarningDetails prints to stderr the warning with additional info about destroying model.
func printDestroyWarningDetails(ctx *cmd.Context, modelStatus base.ModelStatus, modelName string, modelType model.ModelType, releaseStorage bool) error {
func printDestroyWarningDetails(ctx *cmd.Context, modelStatus *base.ModelStatus, modelName string, modelType model.ModelType, releaseStorage bool) error {
destroyMsgDetailsTmpl := template.New("destroyMsdDetails")
destroyMsgDetailsTmpl, err := destroyMsgDetailsTmpl.Parse(destroyModelMsgDetails)
if err != nil {
return errors.Annotate(err, "Destroy controller message template parsing error.")
}
_ = destroyMsgDetailsTmpl.Execute(ctx.Stderr, map[string]any{
"IsCaaS": modelType == model.CAAS,
"MachineCount": modelStatus.HostedMachineCount,
"MachineIds": getMachineIds(modelStatus),
"MachineIds": getMachineIds(*modelStatus),
"ApplicationCount": modelStatus.ApplicationCount,
"ApplicationNames": getApplicationNames(modelStatus),
"ApplicationNames": getApplicationNames(*modelStatus),
"FilesystemCount": len(modelStatus.Filesystems),
"VolumeCount": len(modelStatus.Volumes),
"ReleaseStorage": releaseStorage,
Expand Down Expand Up @@ -258,13 +271,21 @@ func (c *destroyCommand) Run(ctx *cmd.Context) error {
}
defer func() { _ = api.Close() }()

modelTag := names.NewModelTag(modelDetails.ModelUUID)
modelStatus, err := getModelStatus(modelTag, api)
if err != nil {
return err
}

// Error out if the model has detachable storage and is not authorized to destroy or release it.
detachableVolumes, detachableFilesystems := countDetachableStorage(modelStatus)
if (detachableVolumes > 0 || detachableFilesystems > 0) && !(c.destroyStorage || c.releaseStorage) {
return generatePersistentStorageErrorMsg(modelName, detachableVolumes, detachableFilesystems)
}

if c.DestroyConfirmationCommandBase.NeedsConfirmation() {
modelStatuses, err := api.ModelStatus(names.NewModelTag(modelDetails.ModelUUID))
if err != nil {
return errors.Annotate(err, "getting model status")
}
ctx.Warningf(destroyModelMsg, modelName)
if err := printDestroyWarningDetails(ctx, modelStatuses[0], modelName, modelDetails.ModelType, c.releaseStorage); err != nil {
if err := printDestroyWarningDetails(ctx, modelStatus, modelName, modelDetails.ModelType, c.releaseStorage); err != nil {
return errors.Trace(err)
}
if err := jujucmd.UserConfirmName(modelName, "model", ctx); err != nil {
Expand All @@ -287,16 +308,28 @@ func (c *destroyCommand) Run(ctx *cmd.Context) error {
maxWait = &zeroSec
}
}
modelTag := names.NewModelTag(modelDetails.ModelUUID)

var timeout *time.Duration
if c.timeout >= 0 {
timeout = &c.timeout
}
if err := api.DestroyModel(modelTag, destroyStorage, force, maxWait, timeout); err != nil {
return c.handleError(
modelTag, modelName, api,
errors.Annotate(err, "cannot destroy model"),
)
err = errors.Annotate(err, "cannot destroy model")

if params.IsCodeOperationBlocked(err) {
return block.ProcessBlockedError(err, block.BlockDestroy)
}
if params.IsCodeHasPersistentStorage(err) {
modelStatus, err := getModelStatus(modelTag, api)
if err != nil {
return err
}

persistentVolumes, persistentFilesystems := countDetachableStorage(modelStatus)
return generatePersistentStorageErrorMsg(modelName, persistentVolumes, persistentFilesystems)
}
logger.Errorf(`failed to destroy model %q`, modelName)
return err
}

// Wait for model to be destroyed.
Expand Down Expand Up @@ -354,7 +387,7 @@ func waitForModelDestroyed(
fmt.Fprintln(ctx.Stderr, msg)
return cmd.ErrSilent
case <-clock.After(intervalSeconds):
data, erroredStatuses = getModelStatus(ctx, api, tag)
data, erroredStatuses = summarizeModelStatus(ctx, api, tag)
if data == nil {
// model has been destroyed successfully.
return nil
Expand Down Expand Up @@ -421,7 +454,7 @@ You can fix the problem causing the errors and run destroy-model again.
return nil
}

func getModelStatus(ctx *cmd.Context, api DestroyModelAPI, tag names.ModelTag) (*modelData, modelResourceErrorStatusSummary) {
func summarizeModelStatus(ctx *cmd.Context, api DestroyModelAPI, tag names.ModelTag) (*modelData, modelResourceErrorStatusSummary) {
var erroredStatuses modelResourceErrorStatusSummary

status, err := api.ModelStatus(tag)
Expand Down Expand Up @@ -529,45 +562,41 @@ func formatDestroyModelAbortInfo(data *modelData, timeout, force bool) string {
return out
}

func (c *destroyCommand) handleError(
modelTag names.ModelTag,
modelName string,
api DestroyModelAPI,
err error,
) error {
if params.IsCodeOperationBlocked(err) {
return block.ProcessBlockedError(err, block.BlockDestroy)
}
if params.IsCodeHasPersistentStorage(err) {
return handlePersistentStorageError(modelTag, modelName, api)
}
logger.Errorf(`failed to destroy model %q`, modelName)
return err
}

func handlePersistentStorageError(
modelTag names.ModelTag,
modelName string,
api DestroyModelAPI,
) error {
func getModelStatus(modelTag names.ModelTag, api DestroyModelAPI) (*base.ModelStatus, error) {
modelStatuses, err := api.ModelStatus(modelTag)
if err != nil {
return errors.Annotate(err, "getting model status")
return nil, errors.Annotate(err, "getting model status")
}
if l := len(modelStatuses); l != 1 {
return errors.Errorf("error finding model status: expected one result, got %d", l)
return nil, errors.Errorf("error finding model status: expected one result, got %d", l)
}
modelStatus := modelStatuses[0]
if modelStatus.Error != nil {
if errors.IsNotFound(modelStatus.Error) {
// This most likely occurred because a model was
// destroyed half-way through the call.
return nil
return nil, errors.Errorf("model not found, it may have been destroyed during this operation")
}
return errors.Annotate(err, "getting model status")
return nil, errors.Annotate(err, "getting model status")
}
return &modelStatus, nil
}

func generatePersistentStorageErrorMsg(modelName string, detachableVolumeCount, detachableFilesystemCount int) error {
var storageBuilder strings.Builder

if detachableVolumeCount > 0 {
storageBuilder.WriteString(fmt.Sprintf("\n %d volume(s)", detachableVolumeCount))
}

if detachableFilesystemCount > 0 {
storageBuilder.WriteString(fmt.Sprintf("\n %d filesystem(s)", detachableFilesystemCount))
}

var buf bytes.Buffer
return errors.Errorf(persistentStorageErrorMsg, modelName, storageBuilder.String())
}

func countDetachableStorage(modelStatus *base.ModelStatus) (int, int) {
var persistentVolumes, persistentFilesystems int
for _, v := range modelStatus.Volumes {
if v.Detachable {
Expand All @@ -579,34 +608,5 @@ func handlePersistentStorageError(
persistentFilesystems++
}
}
if n := persistentVolumes; n > 0 {
fmt.Fprintf(&buf, "%d volume", n)
if n > 1 {
buf.WriteRune('s')
}
if persistentFilesystems > 0 {
buf.WriteString(" and ")
}
}
if n := persistentFilesystems; n > 0 {
fmt.Fprintf(&buf, "%d filesystem", n)
if n > 1 {
buf.WriteRune('s')
}
}

return errors.Errorf(`cannot destroy model %q
The model has persistent storage remaining:
%s
To destroy the storage, run the destroy-model
command again with the "--destroy-storage" option.
To release the storage from Juju's management
without destroying it, use the "--release-storage"
option instead. The storage can then be imported
into another Juju model.
`, modelName, buf.String())
return persistentVolumes, persistentFilesystems
}
Loading

0 comments on commit 5e07f2c

Please sign in to comment.