Skip to content

snapshot: remove a snapshot's brick pid directory on deactivate and delete (bug-1597662.t on any localstatedir) - #4819

Draft
ThalesBarretto wants to merge 2 commits into
gluster:develfrom
ThalesBarretto:fix/tests-snapshot-rundir-from-pidfiledir
Draft

ThalesBarretto wants to merge 2 commits into
gluster:develfrom
ThalesBarretto:fix/tests-snapshot-rundir-from-pidfiledir

Conversation

@ThalesBarretto

Copy link
Copy Markdown
Contributor

snapshot: remove a snapshot's brick pid directory on deactivate and delete

glusterd keeps a snapshot volume's brick pid files under <run-directory>/snaps/<snap-name>/<snap-volume> (priv->rundir,
${localstatedir}/run/gluster by default) but aims the deactivate/delete clean-ups of that directory at snap_mount_dir,
the literal system run directory + /gluster/snaps, where the snapshot bricks are mounted. The two are one directory only
when localstatedir is /var (plain ./configure, %configure, upstream CI). Anywhere else the pid tree is never removed
(one snaps/<snap-name>/<snap-volume> per snapshot; 200 of them were found on one test node after a day of snapshot test
runs), and tests/bugs/snapshot/bug-1597662.t - the test of the 2018 change that added those clean-ups - hardcodes the
literal root and fails at its EXPECT "1" steps for the wrong reason. Details in #4818. Two commits:

1. tests: derive the snapshot run-dir path from GLUSTERD_PIDFILEDIR (4 files, +18/-4)

  • bug-1597662.t, bug-1597662-zfs.t: snap_path=$GLUSTERD_PIDFILEDIR/snaps (env.rc exports
    GLUSTERD_PIDFILEDIR=@localstatedir@/run/gluster, the value priv->rundir defaults to; volume.rc already builds brick
    pid paths from it); comments say what is checked.
  • snapshot.rc (_cleanup_lvm_again), snapshot_zfs.rc (cleanup_zfs): also rm -rf ${GLUSTERD_PIDFILEDIR}/snaps/*,
    guarded on the variable being set, next to the literal-root removal (kept: the brick mounts live there after the
    findmnt/umount step). The -mmin -2 scans in _cleanup_zfs_again() are left alone: it runs only from cleanup_zfs(),
    right before the wholesale removal of both roots.

On a localstatedir=/var build nothing changes. On e.g. --prefix=/usr/local, this commit alone makes the twins fail at the
deactivate/delete EXPECT "0" checks (subtests 11 and 15) instead of the EXPECT "1" ones - the leak, seen directly - and
stops the harness from accumulating the pid tree.

2. glusterd: remove the snapshot pid directory on deactivate and delete (glusterd-snapshot.c +55, glusterd.h +10)

snap_mount_dir cannot follow the run directory: glusterd_do_snap_vol() computes every brick's path on every node with
snap_ops->brick_path(snap_mount_dir, ...), the paths go into the volume's info file, and
glusterd_compare_friend_volume() checksums that file across peers - with a per-node root (tests/cluster.rc gives each
node its own management.run-directory) peers reject each other's snapshot and clone volumes
(tests/basic/volume-snapshot-clone.t fails). The literal root is a cluster-wide constant and stays exactly as it is.

So the pid directory is removed where it is: a new best-effort glusterd_snap_volume_pid_dir_remove() recursively removes
<run-directory>/snaps/<snapname>/<volname> (GLUSTERD_GET_VOLUME_PID_DIR) and then rmdir()s the <snapname> directory
above it (ENOENT/ENOTEMPTY expected; a GLUSTERD_GET_SNAP_PID_DIR macro beside the pid-dir macro builds that path). It never
fails the operation and is called from glusterd_snapshot_deactivate_commit() next to the existing mount-dir removal, and
from glusterd_snap_volume_remove() once the loop that stops and removes this node's bricks has finished - not from the
per-brick glusterd_snapshot_remove(), where a sibling brick of the same volume may still be running with its pid file in
that directory, the file glusterd looks the process up by. Clones (is_snap_volume false, pid dir under vols/) are left
alone.

  • With localstatedir=/var: the pid directory is the same directory the code next to the new call has just removed,
    so the new removal is an idempotent no-op (recursive_rmdir() returns 0 for a directory it cannot open; the parent's
    ENOENT is tolerated) - plain syscalls on paths, no shared state. Bricks are stopped at both call sites (pid files
    unlinked; brick sockets live in GLUSTERD_SOCK_DIR, not here) and glusterd_brick_start() recreates the directory on
    activate. No mount path, brick path or snap_mount_dir logic changes anywhere.

Test

tests/bugs/snapshot/bug-1597662.t and bug-1597662-zfs.t on a ./configure --prefix=/usr/local build
(localstatedir=/usr/local/var): devel fails subtests 9 and 13 ('1 is_snap_path' -> Got "0"); with commit 1 alone they
fail 11 and 15 ('0 is_snap_path' -> Got "1") and the harness leaves no pid tree behind; with both commits they pass
17/17 and both ${localstatedir}/run/gluster/snaps/ and /run/gluster/snaps/ are empty after the run. The whole
tests/bugs/snapshot/ + tests/basic/volume-snapshot* area on that build: 66 run, 66 pass, 0 cores (volume-snapshot-clone.t
50/50, volume-snapshot-clone-zfs.t 59/59, volume-snapshot.t 49/49), with the bricks still mounted under the literal
/run/gluster/snaps/<snapvol-id>/brickN and no "Failed to remove" warning from the new code in any glusterd log. On a
localstatedir=/var build the new removal targets the directory the existing code has just removed and is a no-op by
construction (recursive_rmdir() returns 0 for a missing directory, the parent's ENOENT is tolerated); such a build was
not run here - upstream CI is that configuration. No new test: the two existing tests are the test.

Fixes: #4818

tests/bugs/snapshot/bug-1597662.t and its ZFS twin hardcode
snap_path=/var/run/gluster/snaps and count the entries matching snap1
there: 1 after activate, 0 after deactivate, 1 after re-activate and 0
after delete. The entry they count is the pid directory of the snapshot
volume's bricks: GLUSTERD_GET_VOLUME_PID_DIR() places it at
<run-directory>/snaps/<snap-name>/<snap-volume>, where <run-directory>
is priv->rundir, glusterd's pid-file directory (management.run-directory
if set, else DEFAULT_VAR_RUN_DIRECTORY = DATADIR "/run/gluster" =
${localstatedir}/run/gluster), and glusterd_brick_start() creates it
through _mk_rundir_p(). The removal the test was written for -
43f5bbe ("snapshot : remove stale entry", 2018-07-03) added the test
together with recursive_rmdir(snap_mount_dir/<snap-name>) in
glusterd_snapshot_deactivate_commit() and on delete - uses another
root: snap_mount_dir is the literal system run directory (/run when
/var/run is a symlink to it, else /var/run) + "/gluster/snaps", as
glusterd_find_correct_var_run_dir() picks it, the root the snapshot
brick mounts live under.

The two roots are one directory only when localstatedir is /var: a
plain ./configure (configure.ac forces localstatedir=/var when no prefix
is given), the packaged builds (%configure and run-tests-in-vagrant.sh
pass --localstatedir=/var) and therefore upstream CI. Any other
localstatedir - an explicit --prefix without --localstatedir, e.g.
--prefix=/usr/local - puts the pid tree under
${localstatedir}/run/gluster/snaps while the test reads the literal
root: subtests 9 and 13 fail with 'Got "0" instead of "1"', the "0"
expectations pass vacuously, and glusterd's rmdirs, aimed at the
literal root, never remove the pid tree, which accumulates as
snaps/<snap-name>/<snap-volume> directories under the run directory.

The harness clean-ups have the same blind spot: snapshot.rc
(_cleanup_lvm_again) and snapshot_zfs.rc (cleanup_zfs) rm -rf only
/var/run/gluster/snaps/*, so nothing removes the leaked pid tree
between tests either.

Derive snap_path from $GLUSTERD_PIDFILEDIR/snaps in both tests - env.rc
exports GLUSTERD_PIDFILEDIR=@localstatedir@/run/gluster, the value
priv->rundir defaults to, and volume.rc already builds brick pid paths
from it - and make the two clean-ups also remove
${GLUSTERD_PIDFILEDIR}/snaps/*, guarded so an unset variable never
expands to /snaps/*. The literal-root removal stays: the snapshot brick
mounts live there, after the findmnt/umount step. The -mmin -2 scans of
/run/gluster/snaps in _cleanup_zfs_again() are left as they are: that
function runs only from cleanup_zfs(), right before the wholesale
removal of both roots, which covers whatever they would have picked
from the pid tree.

Where the roots coincide nothing changes. Where they do not, the tests
now look at the directory they were written to check: the "1"
expectations pass, and the "0" ones report that glusterd left the pid
tree behind - the stale entry of the 2018 fix, unremoved - instead of a
false "never created". Keying the snapshot pid directories and the
snapshot mount directory off one root in glusterd is a separate change.

Updates: gluster#4818
Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
glusterd keeps two roots for a snapshot volume. The brick pid files go
under priv->rundir - GLUSTERD_GET_VOLUME_PID_DIR() gives
<run-directory>/snaps/<snapname>/<volname>, created by
glusterd_brick_start() through _mk_rundir_p() - where <run-directory>
is the pid-file directory, ${localstatedir}/run/gluster unless
management.run-directory says otherwise. The snapshot bricks are
mounted under snap_mount_dir, the literal system run directory (/run
when /var/run is a symlink to it, else /var/run) + "/gluster/snaps".

The clean-ups aimed at the pid directory are written against
snap_mount_dir: glusterd_snapshot_deactivate_commit() does
recursive_rmdir(snap_mount_dir/<snapname>), glusterd_snapshot_remove()
rmdir()s snap_mount_dir/<snapname>/<volname> and its parent ("Cleanup
of Snapshot pid directory"). They reach the directory brick start
created only where the two roots are one directory, i.e. with
localstatedir=/var and no management.run-directory. Anywhere else the
pid tree is never removed - one snaps/<snapname>/<volname> directory
per snapshot stays behind under the run directory; 200 of them, from
15 snapshot names, were found on one regression node after a day of
snapshot test runs - and tests/bugs/snapshot/bug-1597662.t, the test
of the 2018 change that added those rmdirs (43f5bbe, "snapshot :
remove stale entry"), fails at its deactivate and delete checks as
soon as it reads the pid directory where it is (the preceding commit).

snap_mount_dir cannot follow the run directory: glusterd_do_snap_vol()
computes every brick's path on every node with
snap_ops->brick_path(snap_mount_dir, ...), the paths go into the
volume's info file, and glusterd_compare_friend_volume() checksums
that file across peers - with a per-node root (tests/cluster.rc gives
each node its own run directory) peers reject each other's snapshot
and clone volumes. The literal root is a cluster-wide constant and
stays as it is.

Remove the pid directory where it is instead. A new
glusterd_snap_volume_pid_dir_remove() recursively removes
<run-directory>/snaps/<snapname>/<volname> and then rmdir()s the
<snapname> directory above it (ENOENT and ENOTEMPTY expected), best
effort, never failing the operation. It is called from
glusterd_snapshot_deactivate_commit() next to the existing removal of
the mount-dir tree, and from glusterd_snap_volume_remove() once the
loop that stops and removes this node's bricks has finished - not from
the per-brick glusterd_snapshot_remove(), where a sibling brick of the
same volume may still be running with its pid file in that directory,
the file glusterd looks the brick process up by. Clone volumes
(is_snap_volume false) keep their pid directory under vols/ and are
left alone.

With localstatedir=/var the pid directory is the same directory as
<snap_mount_dir>/<snapname>/<volname>, already removed by the code
next to the new call, so the new removal is an idempotent no-op:
recursive_rmdir() returns 0 for a directory it cannot open and the
parent rmdir()'s ENOENT is tolerated - plain syscalls on paths, no
shared state, no double-free hazard. The bricks are stopped at both
call sites (pid files unlinked; brick sockets live in GLUSTERD_SOCK_DIR,
not in this directory) and glusterd_brick_start() recreates the
directory on activate.

With the preceding tests commit, bug-1597662.t and its ZFS twin pass on
every localstatedir.

Fixes: gluster#4818
Signed-off-by: Thales Antunes de Oliveira Barretto <thales.barretto.git@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant