Bug 236043 - [patch] bectl can't destroy origin snapshot
Summary: [patch] bectl can't destroy origin snapshot
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Kyle Evans
URL:
Keywords: patch
: 236006 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-02-26 00:00 UTC by Rob
Modified: 2019-04-08 17:42 UTC (History)
2 users (show)

See Also:


Attachments
don't check if a dataset is mounted when destroying a snapshot (1.12 KB, patch)
2019-02-26 00:00 UTC, Rob
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob 2019-02-26 00:00:05 UTC
Created attachment 202370 [details]
don't check if a dataset is mounted when destroying a snapshot

How to reproduce:

$ bectl create bootenv

$ bectl list -s | grep -A1 bootenv
bootenv
  zroot/ROOT/bootenv                       -      -          8K    2019-02-25 12:45
    zroot/ROOT/solid@2019-02-25-12:45:25   -      -          0     2019-02-25 12:45

$ bectl destroy -o bootenv
cannot destroy mounted boot env unless forced

$ bectl list | grep bootenv
/* no output*/

$ zfs list -t snapshot | grep solid@2019-02-25-12:45:25
zroot/ROOT/solid@2019-02-25-12:45:25                       0      -  21.7G  -

The bug is in the libbe library, specifically the 'be_destroy' function. 'be_destroy' can destroy
a boot environment by name or snapshot, in either case 'be_destroy' acquires a zfs handle internally. When
'be_destroy' is given a boot environment name, the zfs handle will be the boot environment dataset (zroot/ROOT/bootenv).
When 'be_destroy' is given a snapshot name, the zfs handle will be the dataset that the snapshot was
taken from (zroot/ROOT/solid). After the zfs handle is acquired, a check is made to see if the dataset
is mounted and bombs if it is (unless forced). In the above case, 'zroot/ROOT/solid' is the current boot environment.

In short - when destroying a snapshot, 'be_destroy' tries to unmount the dataset that the snapshot was
taken from. The attached fix checks if the dataset is mounted only when that dataset (i.e. boot environment) will
be destroyed. In other words, the patch doesn't check if a dataset is mounted when destroying a snapshot.
Comment 1 Sergey 2019-02-26 06:44:16 UTC
*** Bug 236006 has been marked as a duplicate of this bug. ***
Comment 2 Kyle Evans freebsd_committer 2019-03-19 18:46:04 UTC
Take
Comment 3 commit-hook freebsd_committer 2019-04-01 17:44:40 UTC
A commit references this bug:

Author: kevans
Date: Mon Apr  1 17:44:21 UTC 2019
New revision: 345769
URL: https://svnweb.freebsd.org/changeset/base/345769

Log:
  libbe: Fix zfs_is_mounted check w/ snapshots

  'be_destroy' can destroy a boot environment (by name) or a given snapshot.
  If the target to be destroyed is a dataset, check if it's mounted. We don't
  want to check if the origin dataset is mounted when destroying a snapshot.

  PR:		236043
  Submitted by:	Rob Fairbanks <rob.fx907 gmail com>
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D19650

Changes:
  head/lib/libbe/be.c
  head/sbin/bectl/tests/bectl_test.sh
Comment 4 Kyle Evans freebsd_committer 2019-04-01 17:48:08 UTC
Committed, thanks!
Comment 5 commit-hook freebsd_committer 2019-04-08 17:42:12 UTC
A commit references this bug:

Author: kevans
Date: Mon Apr  8 17:41:40 UTC 2019
New revision: 346034
URL: https://svnweb.freebsd.org/changeset/base/346034

Log:
  MFC r343335, r343977, r343993-r343994, r344034, r344084, r345302, r345769

  r343335:
  libbe(3): simplify import, allow replication streams

  Previously, we directly used libzfs_core's lzc_receive to import to a
  temporary snapshot, then cloned the snapshot and setup the properties. This
  failed when attempting to import replication streams with questionable
  error.

  libzfs's zfs_receive is a much better fit here, so we now use it instead
  with the destination dataset and let libzfs take care of the dirty details.
  be_import is greatly simplified as a result.

  r343977:
  libbe(3): Add a destroy option for removing the origin

  Currently origin snapshots are left behind when a BE is destroyed, whether
  it was an auto-created snapshot or explicitly specified via, for example,
  `bectl create -e be@mysnap ...`.

  Removing it automatically could be argued as a POLA violation in some
  circumstances, so provide a flag to be_destroy for it. An accompanying
  option will be added to bectl(8) to utilize this.

  Some minor style/consistency nits in the affected areas also addressed.

  r343993:
  bectl(8): Add -o flag to destroy to clean up the origin snapshot of BE

  We can't predict when destruction of origin is needed, and currently we have
  a precedent for not prompting for things. Leave the decision up to the user
  of bectl(8) if they want the origin snapshot to be destroyed or not.

  Emits a warning when -o isn't used and an origin snapshot is left to be
  cleaned up, for the time being. This is handy when one drops the -o flag but
  really did want to clean up the origin.

  A couple of -e ignore's have been sprinkled around the test suite for places
  that we don't care that the origin's not been cleaned up. -o functionality
  tests will be added in the future, but are omitted for now to reduce
  conflicts with work in flight to fix bits of the tests.

  r343994:
  bectl(8): commit missing test modifications from r343993

  r344034:
  libbe(3): Belatedly note the BE_DESTROY_ORIGIN option added in r343977

  r344084:
  libbe(3): Fix be_destroy behavior w.r.t. deep BE snapshots and -o

  be_destroy is documented to recursively destroy a boot environment.  In the
  case of snapshots, one would take this to mean that these are also
  recursively destroyed.  However, this was previously not the case.
  be_destroy would descend into the be_destroy callback and attempt to
  zfs_iter_children on the top-level snapshot, which is bogus.

  Our alternative approach is to take note of the snapshot name and iterate
  through all of fs children of the BE to try destruction in the children.

  The -o option is also fixed to work properly with deep BEs.  If the BE was
  created with `bectl create -e otherDeepBE newDeepBE`, for instance, then a
  recursive snapshot of otherDeepBE would have been taken for construction of
  newDeepBE but a subsequent destroy with BE_DESTROY_ORIGIN set would only
  clean up the snapshot at the root of otherDeepBE: ${BEROOT}/otherDeepBE@...

  The most recent iteration instead pretends not to know how these things
  work, verifies that the origin is another BE and then passes that back
  through be_destroy to DTRT when snapshots and deep BEs may be in play.

  r345302:
  bectl(8): change jail command to execute jail(8)

  The jail(8) command provides a variety of jail pseudo-parameters that are
  useful to consumers of bectl, mount.devfs being the most-often-requested
  paramater by bectl users.

  command, exec.start, nopersist, and persist may not be specified via -o to
  bectl. The command/exec.start remains passed as it always has at the end of
  bectl, and persistence is dictated by -b/-U bectl jail arguments.

  r345769:
  libbe: Fix zfs_is_mounted check w/ snapshots

  'be_destroy' can destroy a boot environment (by name) or a given snapshot.
  If the target to be destroyed is a dataset, check if it's mounted. We don't
  want to check if the origin dataset is mounted when destroying a snapshot.

  PR:		236043

Changes:
_U  stable/12/
  stable/12/lib/libbe/be.c
  stable/12/lib/libbe/be.h
  stable/12/lib/libbe/be_error.c
  stable/12/lib/libbe/libbe.3
  stable/12/sbin/bectl/bectl.8
  stable/12/sbin/bectl/bectl.c
  stable/12/sbin/bectl/bectl_jail.c
  stable/12/sbin/bectl/tests/bectl_test.sh