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.

Sign up for GitHub

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

Jump to bottom

Ability to specify type to 'zfs destroy` #9522

Open
mailinglists35 opened this issue Oct 28, 2019 · 4 comments · May be fixed by #15959
Open

Ability to specify type to 'zfs destroy` #9522

mailinglists35 opened this issue Oct 28, 2019 · 4 comments · May be fixed by #15959
Labels
good first issue Indicates a good issue for first-time contributors Type: Feature Feature request or new feature

Comments

@mailinglists35
Copy link

System information

Type Version/Name
Distribution Name all
Distribution Version all
Linux Kernel all
Architecture all
ZFS Version all
SPL Version all

Describe the problem you're observing

There are no safeguards when destroying. Whatever is the argument of zfs destroy, it will get destroyed. Issuers of the destroy command are humans and humans can make mistakes: for example in shell scripts that generate destroy commands, typos etc.

A minimal protection mechanism could reject a destroy action if the type of the dataset to be destroyed does not match the dataset type specified in command line, similar to how zfs list -t works, so for example zfs destroy -t filesystem pool/volumename or zfs destroy -t snapshot pool/filesystem would fail.

Describe how to reproduce the problem

Include any warning/errors/backtraces from the system logs
@behlendorf behlendorf added the Type: Feature Feature request or new feature label Oct 28, 2019
@paranoidi
Copy link

paranoidi commented Nov 29, 2019 •
edited

This was my pretty much first thing I noticed about ZFS. I'm currently checking it out since I fear the day I accidentally delete my files and snapshots provide excellent guard against that. However the destroy command looks a bit like ACME product ...

Edit: After some thinking, one possible solution that does not require backward incompatible CLI changes and does not rely user remembering to specify -t type would be to add "destroy guard" option flag on the pool.
Edit 2: Disregard first edit

@mailinglists35
Copy link
Author

mailinglists35 commented Dec 1, 2019 •
edited

changing behaviour of defaults and/or existing options IS a backwards compatibility breaking, adding a new option is not.

adding new options is how evolution works while keeping backwards compatibility

you don't need a feature flag for pool, because the change i'm requesting is a behaviour enhancement that has nothing to do with how the on-disk format is layed out.

in other words, this can be implemented in zfs command without needing to alter the pool attributes. altering the pool is unneeded for this particular case and will only complicate the administration of zfs.

a script that wants to use this feature would have to simply invoke zfs --version and determine if the -t option is available or not by knowing in what release this option was made available. a human would have to do the same, or look up the option in the manual page of the target installed system.

@behlendorf behlendorf changed the title feature request: able to specify type of destroy (snapshot, filesystem, volume, bookmark) similar to "zfs list -t" Ability to specify type to 'zfs destroy` Dec 6, 2019
@behlendorf behlendorf added the good first issue Indicates a good issue for first-time contributors label Dec 6, 2019
grahamc added a commit to grahamc/zfs that referenced this issue Jan 24, 2020
@grahamc
zfs-destroy: Allow callers to limit destruction for safer automation
99e1c9d 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

At this time it also supports `-t bookmark`.

I considered trying to support additional types like volume and
filesystem, however my little bit of C experience made that a fairly
spooky proposition.

In particular, any combination of `-rR` and `-t volume|filesystem`
seemed like a potentially complicated set of expected behavior. ("What
should happen if I `zfs destroy -t volume -r rpool`?")

Closes openzfs#9522.

Signed-off-by: Graham Christensen <graham@grahamc.com>
@grahamc grahamc mentioned this issue Jan 24, 2020
Closed
12 tasks
grahamc added a commit to grahamc/zfs that referenced this issue Jan 24, 2020
@grahamc
zfs-destroy: Allow callers to limit destruction for safer automation
8906ed6 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

At this time it also supports `-t bookmark`.

I considered trying to support additional types like volume and
filesystem, however my little bit of C experience made that a fairly
spooky proposition.

In particular, any combination of `-rR` and `-t volume|filesystem`
seemed like a potentially complicated set of expected behavior. ("What
should happen if I `zfs destroy -t volume -r rpool`?")

Closes openzfs#9522.

Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Jan 25, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
d3e1f4e 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 7, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
c71a950 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 7, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
7ebf508 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
4454224 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
ee531a2 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
130c4cc 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
107087c 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
ce62d4b 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
d337022 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
grahamc added a commit to grahamc/zfs that referenced this issue Feb 8, 2020
@grahamc @InsanePrawn
zfs-destroy: Allow callers to limit destruction for safer automation
16fe9fe 
After creating recursive snapshots with `zfs snapshot -r`, it is
tempting to use `zfs destroy -r` to clean up. However, as the man page
correctly points out, it is extremely dangerous:

    Extreme care should be taken when applying either the -r or the
    -R options, as they can destroy large portions of a pool and
    cause unexpected behavior for mounted file systems in use.

As an author of script-based automation, this gives me great
heart-burn. This becomes a pretty tricky validation problem with
fairly dire failure modes.

Instead, this patch makes it possible for automation authors and
fumbling typists to tell ZFS what they *intend* to destroy *before*
they pass `-r`, and *before* they start typing in any sort of dataset
names:

    # zfs destroy -t snapshot -r tank@initial-installation

'zfs destroy -t` also supports "filesystem", "volume", "snap",
"snapshot", "bookmark", "all" or a comma-separated list of these

Closes openzfs#9522.

Co-authored-by: InsanePrawn <insane.prawny@gmail.com>
Signed-off-by: Graham Christensen <graham@grahamc.com>
@mailinglists35 mailinglists35 mentioned this issue Feb 17, 2023
Closed
@mailinglists35
Copy link
Author

mailinglists35 commented Feb 17, 2023 •
edited

hey @grahamc, is your patch still working on main branch?

@grahamc
Copy link
Contributor

grahamc commented Feb 17, 2023

I sent a patch in #9882 and I don't know if it is working anymore or not. After I took it to the mailing list for discussion I got feedback that was useful and I think good. However, implementing the patch in the first place was stretching my knowledge of C. I didn't make much effort to update the patch for the feedback I got. It seemed quite a bit more complicated to implement. With my limited experience with C and the ZFS codebase I decided to leave it as-is in case someone else wanted to take it on. Sorry for not saying so sooner!

simons-public added a commit to simons-public/zfs that referenced this issue Mar 4, 2024
@simons-public
Add -t option to zfs destroy
9735aa9 
Resolves openzfs#9522 by adding an optional [-t type] option gate to the
`zfs destroy` command. Checks the type of the supplied argument,
then checks if the supplied type is the same or one of the aliases
that are also used in `zfs list`. If the type doesn't match, it
prints an error to stderr and exits.

Signed-off-by: Chris Simons <chris@simons.network>
simons-public added a commit to simons-public/zfs that referenced this issue Mar 4, 2024
@simons-public
Add -t option to zfs destroy
f1647ac 
Resolves openzfs#9522 by adding an optional [-t type] option gate to the
`zfs destroy` command. Checks the type of the supplied argument,
then checks if the supplied type is the same or one of the aliases
that are also used in `zfs list`. If the type doesn't match, it
prints an error to stderr and exits.

Signed-off-by: Chris Simons <chris@simons.network>
@simons-public simons-public linked a pull request Mar 4, 2024 that will close this issue
Open
13 tasks
simons-public added a commit to simons-public/zfs that referenced this issue Mar 4, 2024
@simons-public
Add -t option to zfs destroy
c3a968c 
Resolves openzfs#9522 by adding an optional [-t type] option gate to the
`zfs destroy` command. Checks the type of the supplied argument,
then checks if the supplied type is the same or one of the aliases
that are also used in `zfs list`. If the type doesn't match, it
prints an error to stderr and exits.

Signed-off-by: Chris Simons <chris@simons.network>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Indicates a good issue for first-time contributors Type: Feature Feature request or new feature
Projects
None yet
Milestone
No milestone
4 participants
@grahamc @behlendorf @paranoidi @mailinglists35