Bug 1462636 - Use of force with volume start, creates brick directory even it is not present
Use of force with volume start, creates brick directory even it is not present
Status: CLOSED CURRENTRELEASE
Product: GlusterFS
Classification: Community
Component: index (Show other bugs)
3.11
All Linux
urgent Severity urgent
: ---
: ---
Assigned To: Ravishankar N
: Triaged
Depends On: 1455022 1457202
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-19 02:07 EDT by Ravishankar N
Modified: 2017-06-28 14:32 EDT (History)
1 user (show)

See Also:
Fixed In Version: glusterfs-3.11.1
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: 1457202
Environment:
Last Closed: 2017-06-28 14:32:55 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ravishankar N 2017-06-19 02:07:21 EDT
+++ This bug was initially created as a clone of Bug #1457202 +++

+++ This bug was initially created as a clone of Bug #1455022 +++

Description of problem:

Use of force with volume create or volume start, creates brick directory even it is not present

Version-Release number of selected component (if applicable):

RHGS 3.2

How reproducible:

Everytime

Steps to Reproduce:
1. Run command gluster v create vol1 xx.xx.xx.xx:/test force
2. Even if /test is not present on xx.xx.xx.xx, it will create this path on root
3.Same thing is reproducible with gluster v start vol1

Actual results:

Use of force with volume create or volume start creates brick directory even it is not present

Expected results:

Use of force should not create brick directory if it is not present

Additional info:


--- Additional comment from Atin Mukherjee on 2017-05-24 01:28:47 EDT ---

force flag is being used in CLI/GlusterD to ensure we can bypass certain validations. So without a force option we don't allow the bricks to be created in root partition, but with force we do. Also please note, when volume create is executed, we always try to create the brickpath if it doesn't exist, if it do we just skip that part.

int                                                                                  
glusterd_validate_and_create_brickpath (glusterd_brickinfo_t *brickinfo,             
                                        uuid_t volume_id, char **op_errstr,     
                                        gf_boolean_t is_force,                       
                                        gf_boolean_t ignore_partition)               
{                                                                                    
        int          ret                 = -1;                                       
        char         parentdir[PATH_MAX] = {0,};                                     
        struct stat  parent_st           = {0,};                                     
        struct stat  brick_st            = {0,};                                     
        struct stat  root_st             = {0,};                                     
        char         msg[2048]           = {0,};                                     
        gf_boolean_t is_created          = _gf_false;                                
                                                                                     
        ret = sys_mkdir (brickinfo->path, 0777);                                     
        if (ret) {                                                                   
                if (errno != EEXIST) {                                               
                        snprintf (msg, sizeof (msg), "Failed to create brick "  
                                  "directory for brick %s:%s. Reason : %s ",    
                                  brickinfo->hostname, brickinfo->path,              
                                  strerror (errno));                                 
                        goto out;                                                    
                }                                                                    
        } else {                                                                     
                is_created = _gf_true;                                               
        }   
        ........
        ........

}

This is an expected behavior.

--- Additional comment from Atin Mukherjee on 2017-05-24 03:15:44 EDT ---

While the expectation of creating the directory (even in root partition) with volume create force is valid, but we do have a bug in volume start force codepath where the brick is getting recreated. GlusterD actually doesn't create this brick path. We have an issue in the index xlator here.

index_dir_create ()

int                                                                                 
index_dir_create (xlator_t *this, const char *subdir)                               
{                                                                                   
        int          ret = 0;                                                       
        struct       stat st = {0};                                                 
        char         fullpath[PATH_MAX] = {0};                                      
        char         path[PATH_MAX] = {0};                                          
        char         *dir = NULL;                                                   
        index_priv_t *priv = NULL;                                                  
        size_t       len = 0;                                                       
        size_t       pathlen = 0;                                                   
                                                                                    
        priv = this->private;                                                       
        make_index_dir_path (priv->index_basepath, subdir, fullpath,                
                             sizeof (fullpath));                                    
        ret = sys_stat (fullpath, &st);                                             
        if (!ret) {                                                                 
                if (!S_ISDIR (st.st_mode))                                          
                        ret = -2;                                                   
                goto out;                                                           
        }                                                                           
                                                                                    
        pathlen = strlen (fullpath);                                                
        if ((pathlen > 1) && fullpath[pathlen - 1] == '/')                          
                fullpath[pathlen - 1] = '\0';                                       
        dir = strchr (fullpath, '/');                                               
        while (dir) {                                                               
                dir = strchr (dir + 1, '/');                                        
                if (dir)                                                            
                        len = pathlen - strlen (dir);                               
                else                                                                
                        len = pathlen;                                              
                strncpy (path, fullpath, len);                                      
                path[len] = '\0';                                                   
                ret = sys_mkdir (path, 0600);  ====> this is actually creating the brickpath.                                      
                if (ret && (errno != EEXIST))                                                                                                                                                                                                 
                        goto out;                                                   
        }                                                                           
        ret = 0;                                                                    
out:                                                                                
        if (ret == -1) {                                                            
                gf_msg (this->name, GF_LOG_ERROR, errno,                            
                        INDEX_MSG_INDEX_DIR_CREATE_FAILED, "%s/%s: Failed to "  
                        "create", priv->index_basepath, subdir);                    
        } else if (ret == -2) {                                                     
                gf_msg (this->name, GF_LOG_ERROR, ENOTDIR,                          
                        INDEX_MSG_INDEX_DIR_CREATE_FAILED, "%s/%s: Failed to "  
                        "create, path exists, not a directory ",                    
                        priv->index_basepath, subdir);                              
        }                                                                           
        return ret;                                                                 
}

--- Additional comment from Worker Ant on 2017-05-31 06:13:15 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#1) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-08 09:35:42 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#2) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-08 21:27:23 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#3) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-15 06:25:33 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#4) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-15 06:59:15 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#5) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-15 07:27:18 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#6) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-15 09:09:49 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#7) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-18 06:02:51 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#8) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-18 12:13:48 EDT ---

REVIEW: https://review.gluster.org/17426 (index: Do not proceed with init if brick is not mounted) posted (#9) for review on master by Ravishankar N (ravishankar@redhat.com)

--- Additional comment from Worker Ant on 2017-06-19 01:16:57 EDT ---

COMMIT: https://review.gluster.org/17426 committed in master by Pranith Kumar Karampuri (pkarampu@redhat.com) 
------
commit b58a15948fb3fc37b6c0b70171482f50ed957f42
Author: Ravishankar N <ravishankar@redhat.com>
Date:   Thu Jun 15 15:36:07 2017 +0530

    index: Do not proceed with init if brick is not mounted
    
    ..or else when a volume start force is given, we end up creating
    /brick-path/.glusterfs/indices folder and various subdirs under it and
    eventually starting the brick process.
    
    As a part of this patch, glusterd_get_index_basepath() is added in
    glusterd, who will then use it to create the basepath during
    volume-create, add-brick, replace-brick and reset-brick. It also uses this
    function to set the 'index-base' xlator option for the index translator.
    
    Change-Id: Id018cf3cb6f1e2e35b5c4cf438d1e939025cb0fc
    BUG: 1457202
    Signed-off-by: Ravishankar N <ravishankar@redhat.com>
    Reviewed-on: https://review.gluster.org/17426
    Smoke: Gluster Build System <jenkins@build.gluster.org>
    NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
    CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
    Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
    Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Comment 1 Worker Ant 2017-06-19 02:09:47 EDT
REVIEW: https://review.gluster.org/17562 (index: Do not proceed with init if brick is not mounted) posted (#1) for review on release-3.11 by Ravishankar N (ravishankar@redhat.com)
Comment 2 Worker Ant 2017-06-19 11:48:14 EDT
COMMIT: https://review.gluster.org/17562 committed in release-3.11 by Shyamsundar Ranganathan (srangana@redhat.com) 
------
commit df807e0ae37e57dd1ffd7ac2ac8e177d44877da0
Author: Ravishankar N <ravishankar@redhat.com>
Date:   Thu Jun 15 15:36:07 2017 +0530

    index: Do not proceed with init if brick is not mounted
    
    ..or else when a volume start force is given, we end up creating
    /brick-path/.glusterfs/indices folder and various subdirs under it and
    eventually starting the brick process.
    
    As a part of this patch, glusterd_get_index_basepath() is added in
    glusterd, who will then use it to create the basepath during
    volume-create, add-brick, replace-brick and reset-brick. It also uses this
    function to set the 'index-base' xlator option for the index translator.
    
    > Reviewed-on: https://review.gluster.org/17426
    > Smoke: Gluster Build System <jenkins@build.gluster.org>
    > NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
    > CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
    > Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
    > Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
    (cherry picked from commit b58a15948fb3fc37b6c0b70171482f50ed957f42)
    
    Change-Id: Id018cf3cb6f1e2e35b5c4cf438d1e939025cb0fc
    BUG: 1462636
    Signed-off-by: Ravishankar N <ravishankar@redhat.com>
    Reviewed-on: https://review.gluster.org/17562
    Smoke: Gluster Build System <jenkins@build.gluster.org>
    Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
    NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
    CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
Comment 3 Shyamsundar 2017-06-28 14:32:55 EDT
This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-3.11.1, please open a new bug report.

glusterfs-3.11.1 has been announced on the Gluster mailinglists [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] http://lists.gluster.org/pipermail/announce/2017-June/000074.html
[2] https://www.gluster.org/pipermail/gluster-users/

Note You need to log in before you can comment on or make changes to this bug.