From 12f53821de1983b78017caee52a9136439f8e9f3 Mon Sep 17 00:00:00 2001 From: Thales Antunes de Oliveira Barretto Date: Mon, 21 Sep 2026 10:40:44 -0300 Subject: [PATCH 1/2] tests: derive the snapshot run-dir path from GLUSTERD_PIDFILEDIR 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 /snaps//, where 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 - 43f5bbe19e ("snapshot : remove stale entry", 2018-07-03) added the test together with recursive_rmdir(snap_mount_dir/) 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// 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: #4818 Signed-off-by: Thales Antunes de Oliveira Barretto --- tests/bugs/snapshot/bug-1597662-zfs.t | 7 +++++-- tests/bugs/snapshot/bug-1597662.t | 7 +++++-- tests/snapshot.rc | 4 ++++ tests/snapshot_zfs.rc | 4 ++++ 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/bugs/snapshot/bug-1597662-zfs.t b/tests/bugs/snapshot/bug-1597662-zfs.t index 2988a3b9d80..7c3f8349fe2 100644 --- a/tests/bugs/snapshot/bug-1597662-zfs.t +++ b/tests/bugs/snapshot/bug-1597662-zfs.t @@ -20,7 +20,10 @@ TEST pidof glusterd; TEST $CLI volume create $V0 $H0:$L1 $H0:$L2 $H0:$L3; TEST $CLI volume start $V0; -snap_path=/var/run/gluster/snaps +# glusterd keeps the pid files of a snapshot volume's bricks under +# /snaps/; env.rc exports that directory +# as GLUSTERD_PIDFILEDIR. +snap_path=$GLUSTERD_PIDFILEDIR/snaps TEST $CLI snapshot create snap1 $V0 no-timestamp; @@ -28,7 +31,7 @@ $CLI snapshot activate snap1; EXPECT 'Started' snapshot_status snap1; -# This Function will check for entry /var/run/gluster/snaps/ +# This Function will check for entry $snap_path/ # against snap-name function is_snap_path diff --git a/tests/bugs/snapshot/bug-1597662.t b/tests/bugs/snapshot/bug-1597662.t index f582930476a..3d68ac3e4f4 100644 --- a/tests/bugs/snapshot/bug-1597662.t +++ b/tests/bugs/snapshot/bug-1597662.t @@ -14,7 +14,10 @@ TEST pidof glusterd; TEST $CLI volume create $V0 $H0:$L1 $H0:$L2 $H0:$L3; TEST $CLI volume start $V0; -snap_path=/var/run/gluster/snaps +# glusterd keeps the pid files of a snapshot volume's bricks under +# /snaps/; env.rc exports that directory +# as GLUSTERD_PIDFILEDIR. +snap_path=$GLUSTERD_PIDFILEDIR/snaps TEST $CLI snapshot create snap1 $V0 no-timestamp; @@ -22,7 +25,7 @@ $CLI snapshot activate snap1; EXPECT 'Started' snapshot_status snap1; -# This Function will check for entry /var/run/gluster/snaps/ +# This Function will check for entry $snap_path/ # against snap-name function is_snap_path diff --git a/tests/snapshot.rc b/tests/snapshot.rc index c298e78e26c..4012ba692d8 100644 --- a/tests/snapshot.rc +++ b/tests/snapshot.rc @@ -112,6 +112,10 @@ function _cleanup_lvm_again() { findmnt -nRlT "${B0}" -o TARGET,SOURCE | grep "${LVM_PREFIX}" | awk '{print $2}' | xargs -r ${UMOUNT_F} findmnt -nRlo TARGET,SOURCE | grep "run/gluster/snaps" | awk '{print $2}' | xargs -r ${UMOUNT_F} \rm -rf /var/run/gluster/snaps/* + # the pid files of snapshot bricks live under glusterd's run directory + if [ -n "${GLUSTERD_PIDFILEDIR}" ]; then + \rm -rf ${GLUSTERD_PIDFILEDIR}/snaps/* + fi vgremove -fyS "vg_name=~^${LVM_PREFIX}_vg" diff --git a/tests/snapshot_zfs.rc b/tests/snapshot_zfs.rc index 88f3fa54499..478d0ca01b3 100644 --- a/tests/snapshot_zfs.rc +++ b/tests/snapshot_zfs.rc @@ -64,6 +64,10 @@ function cleanup_zfs() { _cleanup_zfs_again >/dev/null 2>&1 \rm -rf /var/run/gluster/snaps/* + # the pid files of snapshot bricks live under glusterd's run directory + if [ -n "${GLUSTERD_PIDFILEDIR}" ]; then + \rm -rf ${GLUSTERD_PIDFILEDIR}/snaps/* + fi zfs list | grep "${ZFS_PREFIX}" | awk '{print $1}'| xargs -L 1 -r zpool destroy -f 2>/dev/null return 0 } From aa3de018f4faef611367e1ba5a053d8821fd261e Mon Sep 17 00:00:00 2001 From: Thales Antunes de Oliveira Barretto Date: Mon, 21 Sep 2026 11:50:36 -0300 Subject: [PATCH 2/2] glusterd: remove the snapshot pid directory on deactivate and delete glusterd keeps two roots for a snapshot volume. The brick pid files go under priv->rundir - GLUSTERD_GET_VOLUME_PID_DIR() gives /snaps//, created by glusterd_brick_start() through _mk_rundir_p() - where 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/), glusterd_snapshot_remove() rmdir()s snap_mount_dir// 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// 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 (43f5bbe19e, "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 /snaps// and then rmdir()s the 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 //, 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: #4818 Signed-off-by: Thales Antunes de Oliveira Barretto --- xlators/mgmt/glusterd/src/glusterd-snapshot.c | 55 +++++++++++++++++++ xlators/mgmt/glusterd/src/glusterd.h | 10 ++++ 2 files changed, 65 insertions(+) diff --git a/xlators/mgmt/glusterd/src/glusterd-snapshot.c b/xlators/mgmt/glusterd/src/glusterd-snapshot.c index fbf510642ef..1a11f322a62 100644 --- a/xlators/mgmt/glusterd/src/glusterd-snapshot.c +++ b/xlators/mgmt/glusterd/src/glusterd-snapshot.c @@ -2651,6 +2651,52 @@ glusterd_snapshot_remove(dict_t *rsp_dict, glusterd_volinfo_t *snap_vol, return ret; } +/* Remove the directory glusterd keeps the pid files of a snapshot volume's + * bricks in, /snaps// (see + * GLUSTERD_GET_VOLUME_PID_DIR), and the directory above it once + * that is empty. It is the same directory as + * // only when localstatedir is /var; + * there the callers have already removed the mount-dir tree and this is a + * no-op (recursive_rmdir() returns 0 for a missing directory). Call it only + * once every brick of the volume on this node is stopped: the directory + * holds the pid files brick stop looks the processes up by. + * glusterd_brick_start() recreates it on activate. Best effort - it never + * fails the caller. Clones keep their pid directory under vols/ and are + * left alone. + */ +static void +glusterd_snap_volume_pid_dir_remove(glusterd_volinfo_t *snap_vol) +{ + xlator_t *this = THIS; + glusterd_conf_t *priv = this->private; + char pid_dir[PATH_MAX] = ""; + int ret = -1; + + if (!snap_vol->is_snap_volume || !snap_vol->snapshot) + return; + + GLUSTERD_GET_VOLUME_PID_DIR(pid_dir, snap_vol, priv); + if (!pid_dir[0]) + return; + + ret = recursive_rmdir(pid_dir); + if (ret) { + gf_msg(this->name, GF_LOG_WARNING, errno, GD_MSG_DIR_OP_FAILED, + "Failed to remove %s directory", pid_dir); + return; + } + + GLUSTERD_GET_SNAP_PID_DIR(pid_dir, snap_vol->snapshot->snapname, priv); + if (!pid_dir[0]) + return; + + ret = sys_rmdir(pid_dir); + if (ret && (errno != ENOENT) && (errno != ENOTEMPTY)) { + gf_msg(this->name, GF_LOG_WARNING, errno, GD_MSG_DIR_OP_FAILED, + "Failed to remove %s directory", pid_dir); + } +} + int32_t glusterd_snap_volume_remove(dict_t *rsp_dict, glusterd_volinfo_t *snap_vol, gf_boolean_t remove_snapshot, gf_boolean_t force) @@ -2708,6 +2754,11 @@ glusterd_snap_volume_remove(dict_t *rsp_dict, glusterd_volinfo_t *snap_vol, } } + /* Every brick of this node is stopped now (or a stop failed under + * force, in which case leave the pid files alone). */ + if (!save_ret) + glusterd_snap_volume_pid_dir_remove(snap_vol); + ret = glusterd_store_delete_volume(snap_vol); if (ret) { gf_msg(this->name, GF_LOG_WARNING, 0, GD_MSG_VOL_DELETE_FAIL, @@ -5799,6 +5850,10 @@ glusterd_snapshot_deactivate_commit(dict_t *dict, char **op_errstr, goto out; } + /* The bricks' pid files live under the run directory, which is that + * same tree only when localstatedir is /var. */ + glusterd_snap_volume_pid_dir_remove(snap_volinfo); + ret = dict_set_dynstr_with_alloc(rsp_dict, "snapuuid", uuid_utoa(snap->snap_id)); if (ret) { diff --git a/xlators/mgmt/glusterd/src/glusterd.h b/xlators/mgmt/glusterd/src/glusterd.h index b5382734cc6..824b6ba81d4 100644 --- a/xlators/mgmt/glusterd/src/glusterd.h +++ b/xlators/mgmt/glusterd/src/glusterd.h @@ -574,6 +574,16 @@ enum glusterd_op_ret { } \ } while (0) +#define GLUSTERD_GET_SNAP_PID_DIR(path, snapname, priv) \ + do { \ + int32_t _snap_pid_len; \ + _snap_pid_len = snprintf(path, PATH_MAX, "%s/snaps/%s", priv->rundir, \ + snapname); \ + if ((_snap_pid_len < 0) || (_snap_pid_len >= PATH_MAX)) { \ + path[0] = 0; \ + } \ + } while (0) + #define GLUSTERD_GET_SNAP_GEO_REP_DIR(path, snap, priv) \ do { \ int32_t _snap_geo_len; \