[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4] virsh: Add option to undefine storage with domains



On 12/15/2011 08:20 AM, Peter Krempa wrote:
> Add an option for virsh undefine command, to remove associated storage
> volumes while undefining a domain. This patch allows the user to remove
> associated (libvirt managed ) storage volumes while undefining a domain.
> 
> The new option --storage for the undefine command takes a string
> argument that consists of comma separated list of target or source path
> of volumes to be undefined. Volumes are removed after the domain has
> been successfully undefined,
> 
> If a volume is not part of a storage pool, the user is warned to remove
> the volume in question himself.
> 
> Option --wipe-storage may be specified along with this, that ensures
> the image is wiped before removing.
> 
> Option --remove-all-storage enables the user to remove all storage. The
> name is chosen long as the users should be aware what they're about to
> do.
> ---
> Changes to v3:
>         - fix ton'o'typos (I should install a spell checker.)
>         - add a new variable to hold copy of argument to avoid confusion,
>           typecasting ...
>         - break volume string into tokens just once and store it in an array
>         - add both target and source as volume description
>           ( both are needed, as the user should know which volume he shoud

Dare I mention the typo here?  Nah, since it won't be part of the final
commit.  :)

> @@ -2027,6 +2066,20 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>          snapshots_safe = true;
>      }
> 
> +    /* Stash domain description for later use */
> +    if (remove_storage || remove_all_storage) {
> +        if (running) {
> +            vshError(ctl, _("Storage volume deletion is supported only on stopped domains"));
> +            goto cleanup;
> +        }

Doesn't hold up this patch, but I wonder if we should have a counterpart
virsh operation that allows deletion of volumes after destroying a
transient domain.  On the other hand, virsh isn't used as often with
transient domains, and any management app (I'm thinking things like
VDSM) that already prefers transient domains probably already knows how
to properly wipe storage after the fact, without needing syntactic sugar
in virsh.

Overall, the code looks nice now!

> 
>  =item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-metadata>]
> +[I<--storage> B<volumes> | I<--remove-all-storage> I<--wipe-storage>]

We want to make it obvious that --wipe-storage applies only if you use
either --storage or --remove-all-storage, so this has to be:

[ {I<--storage> B<volume> | I<--remove-all-storage>} [I<--wipe-storage]]

The outer [] says storage removal in general is optional, then the first
{} says that storage removal is an either-or between the two styles, and
the second [] says that --wipe-storage goes with either style.

> 
> +The I<--storage> flag takes a parameter B<volumes>, that is a comma separated

s/that/which/

> +list of volume target names or source paths of storage volumes to be removed
> +along with the undefined domain. Volumes can be undefined and thus removed only
> +on inactive domains. Volume deletion is only attempted after the domain is
> +undefined; if not all of the requested volumes could be deleted, the
> +error message indicates what still remains behind. If a volume path is not
> +found in the domain definition, it's treated as if the volume was successfully
> +deleteted.

s/deleteted/deleted/

> +(See B<domblklist> for list of target names associated to a domain).
> +Example: --storage vda,/path/to/storage.img
> +
> +The I<--remove-all-storage> flag specifies that all of the domain's storage
> +volumes should be deleted.
> +
> +The flag I<--wipe-storage> specifies that the storage volumes should be
> +wiped before removal.

ACK with nits in virsh.pod fixed; no need to send a v5.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]