glusterd/snapshot: Fix snap delete cleanup

During commit the first thing that should happen is the
snaps should be marked as GD_SNAP_STATUS_DECOMMISSION.
So that if the node goes down the marked snap can be
cleaned up when the node is coming back. Also during snap
handshake while accepting snapshot from peer, we should
check if the snap in question is decomissioned. In that
case we shouldn't be accepting the peer data.

Change-Id: Ib4e38d1b6bf49411928623fbc9f72f2b37b72086
BUG: 1104714
Signed-off-by: Avra Sengupta <asengupt@redhat.com>
Reviewed-on: http://review.gluster.org/7996
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Rajesh Joseph <rjoseph@redhat.com>
Reviewed-by: Krishnan Parthasarathi <kparthas@redhat.com>
Tested-by: Krishnan Parthasarathi <kparthas@redhat.com>

5 files changed, 109 insertions, 113 deletions

diff --git a/cli/src/cli-rpc-ops.c b/cli/src/cli-rpc-ops.c
index 348fbe4e2e5..26b6152f2be 100644
--- a/cli/src/cli-rpc-ops.c
+++ b/cli/src/cli-rpc-ops.c
@@ -7648,20 +7648,19 @@ cli_snapshot_remove_reply (gf_cli_rsp *rsp, dict_t *dict)
         GF_ASSERT (rsp);
         GF_ASSERT (dict);
 
-        if (rsp->op_ret) {
-                cli_err("snapshot delete: failed: %s",
-                        rsp->op_errstr ? rsp->op_errstr :
-                        "Please check log file for details");
-                ret = rsp->op_ret;
-                goto out;
-        }
-
         ret = dict_get_str (dict, "snapname", &snap_name);
         if (ret) {
                 gf_log ("cli", GF_LOG_ERROR, "Failed to get snapname");
                 goto out;
         }
 
+        if (rsp->op_ret) {
+                cli_err("snapshot delete: failed: snap %s "
+                        "might not be in an usable state.", snap_name);
+                ret = rsp->op_ret;
+                goto out;
+        }
+
         cli_out ("snapshot delete: %s: snap removed successfully",
                  snap_name);
         ret = 0;
@@ -8303,6 +8302,8 @@ cli_get_snap_volume_status (dict_t *dict, char *key_prefix)
                         cli_out ("\t%-17s %s   %s", "LV Size", ":", buffer);
 
         }
+
+        ret = 0;
 out:
         return ret;
 }
diff --git a/xlators/mgmt/glusterd/src/glusterd-snapshot.c b/xlators/mgmt/glusterd/src/glusterd-snapshot.c
index 330e50c277d..8b3ffd5b760 100644
--- a/xlators/mgmt/glusterd/src/glusterd-snapshot.c
+++ b/xlators/mgmt/glusterd/src/glusterd-snapshot.c
@@ -2036,7 +2036,9 @@ glusterd_lvm_snapshot_remove (dict_t *rsp_dict, glusterd_volinfo_t *snap_vol)
         char                  buff[PATH_MAX]       = "";
         char                  brick_dir[PATH_MAX]  = "";
         char                 *tmp                  = NULL;
+        char                 *brick_mount_path     = NULL;
         gf_boolean_t          is_brick_dir_present = _gf_false;
+        struct stat           stbuf                = {0,};
 
         this = THIS;
         GF_ASSERT (this);
@@ -2060,6 +2062,15 @@ glusterd_lvm_snapshot_remove (dict_t *rsp_dict, glusterd_volinfo_t *snap_vol)
                         continue;
                 }
 
+                ret = lstat (brickinfo->path, &stbuf);
+                if (ret) {
+                        gf_log (this->name, GF_LOG_DEBUG,
+                                "Brick %s:%s already deleted.",
+                                brickinfo->hostname, brickinfo->path);
+                        ret = 0;
+                        continue;
+                }
+
                 if (brickinfo->snap_status == -1) {
                         gf_log (this->name, GF_LOG_INFO,
                                 "snapshot was pending. lvm not present "
@@ -2094,7 +2105,28 @@ glusterd_lvm_snapshot_remove (dict_t *rsp_dict, glusterd_volinfo_t *snap_vol)
                         gf_log (this->name, GF_LOG_WARNING, "getting the root "
                                "of the brick for volume %s (snap %s) failed ",
                                snap_vol->volname, snap_vol->snapshot->snapname);
-                        goto out;
+                        continue;
+                }
+
+                /* Fetch the brick mount path from the brickinfo->path */
+                ret = glusterd_find_brick_mount_path (brickinfo->path,
+                                                      brick_count + 1,
+                                                      &brick_mount_path);
+                if (ret) {
+                        gf_log (this->name, GF_LOG_ERROR,
+                                "Failed to find brick_mount_path for %s",
+                                brickinfo->path);
+                        GF_FREE (mnt_pt);
+                        mnt_pt = NULL;
+                        continue;
+                }
+
+                if (!strstr (mnt_pt, snap_vol->volname)) {
+                        gf_log (this->name, GF_LOG_DEBUG,
+                                "Lvm is not mounted for brick %s:%s. "
+                                "Removing the brick path.",
+                                brickinfo->hostname, brickinfo->path);
+                        goto remove_brick_path;
                 }
 
                 entry = glusterd_get_mnt_entry_info (mnt_pt, buff,
@@ -2106,7 +2138,7 @@ glusterd_lvm_snapshot_remove (dict_t *rsp_dict, glusterd_volinfo_t *snap_vol)
                                 brickinfo->path, snap_vol->snapshot->snapname,
                                 snap_vol->volname);
                         ret = -1;
-                        goto out;
+                        goto remove_brick_path;
                 }
                 ret = glusterd_do_lvm_snapshot_remove (snap_vol, brickinfo,
                                                        mnt_pt,
@@ -2115,45 +2147,43 @@ glusterd_lvm_snapshot_remove (dict_t *rsp_dict, glusterd_volinfo_t *snap_vol)
                         gf_log (this->name, GF_LOG_ERROR, "failed to "
                                 "remove the snapshot %s (%s)",
                                 brickinfo->path, entry->mnt_fsname);
-                        goto out;
-                }
-
-                ret = rmdir (mnt_pt);
-                if (ret) {
-                        gf_log (this->name, GF_LOG_ERROR,
-                                "Failed to rmdir: %s, err: %s",
-                                mnt_pt, strerror (errno));
-                        goto out;
                 }
 
+remove_brick_path:
                 /* After removing the brick dir fetch the parent path
                  * i.e /var/run/gluster/snaps/<snap-vol-id>/
                  */
                 if (is_brick_dir_present == _gf_false) {
-                        /* Peers not hosting bricks will have _gf_false */
-                        is_brick_dir_present = _gf_true;
-
                         /* Need to fetch brick_dir to be removed from
                          * brickinfo->path, as in a restored volume,
                          * snap_vol won't have the non-hyphenated snap_vol_id
                          */
-                        tmp = strstr (mnt_pt, "brick");
+                        tmp = strstr (brick_mount_path, "brick");
                         if (!tmp) {
                                 gf_log (this->name, GF_LOG_ERROR,
                                         "Invalid brick %s", brickinfo->path);
-                                ret = -1;
-                                goto out;
+                                GF_FREE (mnt_pt);
+                                GF_FREE (brick_mount_path);
+                                mnt_pt = NULL;
+                                brick_mount_path = NULL;
+                                continue;
                         }
 
-                        strncpy (brick_dir, mnt_pt, (size_t) (tmp - mnt_pt));
+                        strncpy (brick_dir, brick_mount_path,
+                                 (size_t) (tmp - brick_mount_path));
+
+                        /* Peers not hosting bricks will have _gf_false */
+                        is_brick_dir_present = _gf_true;
                 }
 
                 GF_FREE (mnt_pt);
+                GF_FREE (brick_mount_path);
                 mnt_pt = NULL;
+                brick_mount_path = NULL;
         }
 
         if (is_brick_dir_present == _gf_true) {
-                ret = rmdir (brick_dir);
+                ret = glusterd_recursive_rmdir (brick_dir);
                 if (ret) {
                         if (errno == ENOTEMPTY) {
                                 /* Will occur when multiple glusterds
@@ -2177,6 +2207,7 @@ glusterd_lvm_snapshot_remove (dict_t *rsp_dict, glusterd_volinfo_t *snap_vol)
         ret = 0;
 out:
         GF_FREE (mnt_pt);
+        GF_FREE (brick_mount_path);
         gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
         return ret;
 }
@@ -4779,6 +4810,19 @@ glusterd_snapshot_remove_commit (dict_t *dict, char **op_errstr,
                 goto out;
         }
 
+        /* Save the snap status as GD_SNAP_STATUS_DECOMMISSION so
+         * that if the node goes down the snap would be removed
+         */
+        snap->snap_status = GD_SNAP_STATUS_DECOMMISSION;
+        ret = glusterd_store_snap (snap);
+        if (ret) {
+                gf_log (this->name, GF_LOG_ERROR, "Failed to "
+                        "store snap object %s", snap->snapname);
+                goto out;
+        } else
+                gf_log (this->name, GF_LOG_INFO, "Successfully marked "
+                        "snap %s for decommission.", snap->snapname);
+
         if (is_origin_glusterd (dict) == _gf_true) {
                 /* TODO : As of now there is only volume in snapshot.
                  * Change this when multiple volume snapshot is introduced
@@ -6961,6 +7005,13 @@ glusterd_snapshot_postvalidate (dict_t *dict, int32_t op_ret, char **op_errstr,
                 }
                 break;
         case GF_SNAP_OPTION_TYPE_DELETE:
+                if (op_ret) {
+                        gf_log (this->name, GF_LOG_DEBUG,
+                                "op_ret = %d. Not performing delete "
+                                "post_validate", op_ret);
+                        ret = 0;
+                        goto out;
+                }
                 ret = glusterd_snapshot_update_snaps_post_validate (dict,
                                                                     op_errstr,
                                                                     rsp_dict);
diff --git a/xlators/mgmt/glusterd/src/glusterd-store.c b/xlators/mgmt/glusterd/src/glusterd-store.c
index a243a1365d3..839d77b7f8d 100644
--- a/xlators/mgmt/glusterd/src/glusterd-store.c
+++ b/xlators/mgmt/glusterd/src/glusterd-store.c
@@ -1498,12 +1498,8 @@ glusterd_store_delete_volume (glusterd_volinfo_t *volinfo)
         char             pathname[PATH_MAX]    = {0,};
         int32_t          ret                   = 0;
         glusterd_conf_t *priv                  = NULL;
-        DIR             *dir                   = NULL;
-        struct dirent   *entry                 = NULL;
-        char             path[PATH_MAX]        = {0,};
         char             delete_path[PATH_MAX] = {0,};
         char             trashdir[PATH_MAX]    = {0,};
-        struct stat      st                    = {0, };
         xlator_t        *this                  = NULL;
         gf_boolean_t     rename_fail           = _gf_false;
 
@@ -1540,61 +1536,7 @@ glusterd_store_delete_volume (glusterd_volinfo_t *volinfo)
                 goto out;
         }
 
-        dir = opendir (delete_path);
-        if (!dir) {
-                gf_log (this->name, GF_LOG_DEBUG, "Failed to open directory %s."
-                        " Reason : %s", delete_path, strerror (errno));
-                ret = 0;
-                goto out;
-        }
-        ret = glusterd_store_remove_bricks (volinfo, delete_path);
-
-        if (ret) {
-                gf_log (this->name, GF_LOG_DEBUG, "Remove bricks failed for %s",
-                        volinfo->volname);
-        }
-
-        glusterd_for_each_entry (entry, dir);
-        while (entry) {
-
-                snprintf (path, PATH_MAX, "%s/%s", delete_path, entry->d_name);
-                ret = stat (path, &st);
-                if (ret == -1) {
-                        gf_log (this->name, GF_LOG_DEBUG, "Failed to stat "
-                                "entry %s : %s", path, strerror (errno));
-                        goto stat_failed;
-                }
-
-                if (S_ISDIR (st.st_mode))
-                        ret = rmdir (path);
-                else
-                        ret = unlink (path);
-
-                if (ret) {
-                        gf_log (this->name, GF_LOG_DEBUG, " Failed to remove "
-                                "%s. Reason : %s", path, strerror (errno));
-                }
-
-                gf_log (this->name, GF_LOG_DEBUG, "%s %s",
-                                ret ? "Failed to remove":"Removed",
-                                entry->d_name);
-stat_failed:
-                memset (path, 0, sizeof(path));
-                glusterd_for_each_entry (entry, dir);
-        }
-
-        ret = closedir (dir);
-        if (ret) {
-                gf_log (this->name, GF_LOG_DEBUG, "Failed to close dir %s. "
-                        "Reason : %s",delete_path, strerror (errno));
-        }
-
-        ret = rmdir (delete_path);
-        if (ret) {
-                gf_log (this->name, GF_LOG_DEBUG, "Failed to rmdir: %s,err: %s",
-                        delete_path, strerror (errno));
-        }
-        ret = rmdir (trashdir);
+        ret = glusterd_recursive_rmdir (trashdir);
         if (ret) {
                 gf_log (this->name, GF_LOG_DEBUG, "Failed to rmdir: %s, Reason:"
                         " %s", trashdir, strerror (errno));
@@ -3059,7 +3001,6 @@ glusterd_recreate_vol_brick_mounts (xlator_t  *this,
                 if (ret) {
                         gf_log (this->name, GF_LOG_ERROR,
                                 "Failed to mount brick_mount_path");
-                        goto out;
                 }
 
                 if (brick_mount_path) {
@@ -3926,6 +3867,7 @@ glusterd_snap_cleanup (xlator_t  *this)
         int32_t               ret  = 0;
         glusterd_conf_t      *priv = NULL;
         glusterd_snap_t      *snap = NULL;
+        glusterd_snap_t      *tmp_snap = NULL;
 
         GF_ASSERT (this);
         priv = this->private;
@@ -3939,7 +3881,7 @@ glusterd_snap_cleanup (xlator_t  *this)
                 goto out;
         }
 
-        list_for_each_entry (snap, &priv->snapshots, snap_list) {
+        list_for_each_entry_safe (snap, tmp_snap, &priv->snapshots, snap_list) {
                 if (snap->snap_status == GD_SNAP_STATUS_RESTORED) {
                         ret = glusterd_snapshot_revert_restore_from_snap (snap);
                         if (ret) {
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index 776278ff6c3..f02b6197428 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -571,7 +571,7 @@ glusterd_volinfo_dup (glusterd_volinfo_t *volinfo,
 
         dict_copy (volinfo->dict, new_volinfo->dict);
         dict_copy (volinfo->gsync_slaves, new_volinfo->gsync_slaves);
-        new_volinfo->op_version = volinfo->op_version;
+        gd_update_volume_op_versions (new_volinfo);
 
         if (set_userauth) {
                 glusterd_auth_set_username (new_volinfo,
@@ -5195,29 +5195,6 @@ glusterd_gen_snap_volfiles (glusterd_volinfo_t *snap_vol, char *peer_snap_name)
 
         glusterd_list_add_snapvol (parent_volinfo, snap_vol);
 
-        list_for_each_entry (brickinfo, &snap_vol->bricks, brick_list) {
-                if (uuid_compare (brickinfo->uuid, MY_UUID))
-                        continue;
-
-                if (brickinfo->snap_status == -1) {
-                        gf_log (this->name, GF_LOG_INFO,
-                                "not starting snap brick %s:%s for "
-                                "for the snap %s (volume: %s)",
-                                brickinfo->hostname, brickinfo->path,
-                                peer_snap_name, parent_volinfo->volname);
-                        continue;
-                }
-
-                ret = glusterd_brick_start (snap_vol, brickinfo, _gf_true);
-                if (ret) {
-                        gf_log (this->name, GF_LOG_WARNING, "starting the "
-                                "brick %s:%s for the snap %s (volume: %s) "
-                                "failed", brickinfo->hostname, brickinfo->path,
-                                peer_snap_name, parent_volinfo->volname);
-                        goto out;
-                }
-        }
-
         snap_vol->status = GLUSTERD_STATUS_STARTED;
 
         ret = glusterd_store_volinfo (snap_vol, GLUSTERD_VOLINFO_VER_AC_NONE);
@@ -5264,6 +5241,14 @@ glusterd_import_friend_snap (dict_t *peer_data, int32_t snap_count,
                 goto out;
         }
 
+        dict = dict_new ();
+        if (!dict) {
+                gf_log (this->name, GF_LOG_ERROR,
+                        "Failed to create dict");
+                ret = -1;
+                goto out;
+        }
+
         strcpy (snap->snapname, peer_snap_name);
         uuid_parse (peer_snap_id, snap->snap_id);
 
@@ -5297,6 +5282,18 @@ glusterd_import_friend_snap (dict_t *peer_data, int32_t snap_count,
                 goto out;
         }
 
+        /* If the snap is scheduled to be decommissioned, then
+         * don't accept the snap */
+        if (snap->snap_status == GD_SNAP_STATUS_DECOMMISSION) {
+                gf_log (this->name, GF_LOG_DEBUG,
+                        "The snap(%s) is scheduled to be decommissioned "
+                        "Not accepting the snap.", peer_snap_name);
+                glusterd_snap_remove (dict, snap,
+                                      _gf_true, _gf_true);
+                ret = 0;
+                goto out;
+        }
+
         snprintf (buf, sizeof(buf), "%s.volcount", prefix);
         ret = dict_get_int32 (peer_data, buf, &volcount);
         if (ret) {
@@ -12562,7 +12559,7 @@ glusterd_recursive_rmdir (const char *delete_path)
         glusterd_for_each_entry (entry, dir);
         while (entry) {
                 snprintf (path, PATH_MAX, "%s/%s", delete_path, entry->d_name);
-                ret = stat (path, &st);
+                ret = lstat (path, &st);
                 if (ret == -1) {
                         gf_log (this->name, GF_LOG_DEBUG, "Failed to stat "
                                 "entry %s : %s", path, strerror (errno));
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.h b/xlators/mgmt/glusterd/src/glusterd-utils.h
index 9e0400cf86a..2a22831fe41 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.h
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.h
@@ -898,4 +898,9 @@ int32_t
 glusterd_is_snap_soft_limit_reached (glusterd_volinfo_t *volinfo,
                                      dict_t *dict);
 /* End snapd functions */
+
+int32_t
+glusterd_find_brick_mount_path (char *brick_path, int32_t brick_count,
+                                char **brick_mount_path);
+
 #endif