Patchwork [BUG:1517] mgmt/glusterd: add option to force kill gnfs process

login
register
Submitter Pranith K
Date 2010-09-24 10:08:36
Message ID <20100924100836.GA9885@dev.gluster.com>
Download mbox | patch
Permalink /patch/4956/
State Accepted
Headers show

Comments

Pranith K - 2010-09-24 10:08:36
Signed-off-by: Pranith Kumar K <pranithk@gluster.com>
---
 xlators/mgmt/glusterd/src/glusterd-utils.c |   37 +++++++++++++++-------------
 xlators/mgmt/glusterd/src/glusterd-utils.h |    2 +-
 2 files changed, 21 insertions(+), 18 deletions(-)
Csaba Henk - 2010-09-24 15:25:30
On Fri, Sep 24, 2010 at 3:38 PM, Pranith Kumar K <pranithk@gluster.com>wrote:

>
>
> Signed-off-by: Pranith Kumar K <pranithk@gluster.com>
> ---
>  xlators/mgmt/glusterd/src/glusterd-utils.c |   37
> +++++++++++++++-------------
>  xlators/mgmt/glusterd/src/glusterd-utils.h |    2 +-
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c
> b/xlators/mgmt/glusterd/src/glusterd-utils.c
> index a59a069..411d217 100644
> --- a/xlators/mgmt/glusterd/src/glusterd-utils.c
> +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
> @@ -769,8 +769,8 @@ glusterd_volinfo_find (char *volname,
> glusterd_volinfo_t **volinfo)
>
>
>  int32_t
> -glusterd_service_stop(const char *service, char *pidfile, int sig,
> -                      gf_boolean_t keep_pidfile)
> +glusterd_service_stop (const char *service, char *pidfile, int sig,
> +                       gf_boolean_t force_kill)
>

Pranith, here you are changing the semantics of the function parameters just
to advocate a special behavior for one of its users!
This is quite likely to effect the other users...

... as it does effect rb_kill_destination_brick(), which uses that parameter
in the "keep the pidfile" sense. That behavior was added in
32041afe<http://git.gluster.com/?p=glusterfs.git;a=commitdiff;h=32041afe>
by
Pavan. Have you checked with him that it's OK to remove the pidfile? (Well
sure yeah it can be that he kept the pidfile by mistake...).

Csaba
Pranith K - 2010-09-24 16:47:36
hi Csaba,
         This is the change I intended to do.
1) Remove keep_pid flag altogether, because when the process is killed 
the file is cleanedup any way.
2) add force_kill flag, because some actions need the process to be 
killed before carrying out subsequent action.
     nfs process restart and replace-brick operations need this option. 
so the flag is true for both of them and false in case of glusterfs 
process stopping.

It so happened that keep_pidfile was true for replace-brick and false 
for glusterfs process stop. So they were not showing up in the diff.
Thanks for keeping an eye on the commits :).

Thanks
Pranith.

On Friday 24 September 2010 08:55 PM, Csaba Henk wrote:
> On Fri, Sep 24, 2010 at 3:38 PM, Pranith Kumar K <pranithk@gluster.com 
> <mailto:pranithk@gluster.com>> wrote:
>
>
>
>     Signed-off-by: Pranith Kumar K <pranithk@gluster.com
>     <mailto:pranithk@gluster.com>>
>     ---
>      xlators/mgmt/glusterd/src/glusterd-utils.c |   37
>     +++++++++++++++-------------
>      xlators/mgmt/glusterd/src/glusterd-utils.h |    2 +-
>      2 files changed, 21 insertions(+), 18 deletions(-)
>
>     diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c
>     b/xlators/mgmt/glusterd/src/glusterd-utils.c
>     index a59a069..411d217 100644
>     --- a/xlators/mgmt/glusterd/src/glusterd-utils.c
>     +++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
>     @@ -769,8 +769,8 @@ glusterd_volinfo_find (char *volname,
>     glusterd_volinfo_t **volinfo)
>
>
>      int32_t
>     -glusterd_service_stop(const char *service, char *pidfile, int sig,
>     -                      gf_boolean_t keep_pidfile)
>     +glusterd_service_stop (const char *service, char *pidfile, int sig,
>     +                       gf_boolean_t force_kill)
>
>
> Pranith, here you are changing the semantics of the function 
> parameters just to advocate a special behavior for one of its users!
> This is quite likely to effect the other users...
>
> ... as it does effect rb_kill_destination_brick(), which uses that 
> parameter in the "keep the pidfile" sense. That behavior was added in 
> 32041afe 
> <http://git.gluster.com/?p=glusterfs.git;a=commitdiff;h=32041afe> by 
> Pavan. Have you checked with him that it's OK to remove the pidfile? 
> (Well sure yeah it can be that he kept the pidfile by mistake...).
>
> Csaba
>
Csaba Henk - 2010-09-24 19:03:14
On Fri, Sep 24, 2010 at 10:17 PM, Pranith Kumar K <pranithk@gluster.com>wrote:

>         This is the change I intended to do.
> 1) Remove keep_pid flag altogether, because when the process is killed the
> file is cleanedup any way.
> 2) add force_kill flag, because some actions need the process to be killed
> before carrying out subsequent action.
>     nfs process restart and replace-brick operations need this option. so
> the flag is true for both of them and false in case of glusterfs process
> stopping.
>
> It so happened that keep_pidfile was true for replace-brick and false for
> glusterfs process stop. So they were not showing up in the diff.
>

OK dude so I'm not disappointed in you ;)


> Thanks for keeping an eye on the commits :).
>

Rebase is what makes my day.

Csaba

Patch

diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index a59a069..411d217 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -769,8 +769,8 @@  glusterd_volinfo_find (char *volname, glusterd_volinfo_t **volinfo)
 
 
 int32_t
-glusterd_service_stop(const char *service, char *pidfile, int sig,
-                      gf_boolean_t keep_pidfile)
+glusterd_service_stop (const char *service, char *pidfile, int sig,
+                       gf_boolean_t force_kill)
 {
         int32_t  ret = -1;
         pid_t    pid = -1;
@@ -806,20 +806,23 @@  glusterd_service_stop(const char *service, char *pidfile, int sig,
 
         ret = kill (pid, sig);
 
-        if (ret) {
-                gf_log ("", GF_LOG_ERROR, "Unable to kill pid %d", pid);
-                goto out;
-        }
-
-        if (keep_pidfile)
-                goto out;
-
-        ret = unlink (pidfile);
-
-        if (ret && (ENOENT != errno)) {
-                gf_log ("", GF_LOG_ERROR, "Unable to unlink pidfile: %s",
-                                pidfile);
-                goto out;
+        if (force_kill) {
+                sleep (1);
+                ret = access (pidfile, F_OK);
+                if (!ret) {
+                        ret = kill (pid, SIGKILL);
+                        if (ret) {
+                                gf_log ("", GF_LOG_ERROR, "Unable to "
+                                        "kill pid %d", pid);
+                                goto out;
+                        }
+                        ret = unlink (pidfile);
+                        if (ret && (ENOENT != errno)) {
+                                gf_log ("", GF_LOG_ERROR, "Unable to "
+                                        "unlink pidfile: %s", pidfile);
+                                goto out;
+                        }
+                }
         }
 
         ret = 0;
@@ -1642,7 +1645,7 @@  glusterd_nfs_server_stop ()
         GLUSTERD_GET_NFS_DIR(path, priv);
         GLUSTERD_GET_NFS_PIDFILE(pidfile);
 
-        return glusterd_service_stop ("nfsd", pidfile, SIGTERM, _gf_false);
+        return glusterd_service_stop ("nfsd", pidfile, SIGTERM, _gf_true);
 }
 
 int
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.h b/xlators/mgmt/glusterd/src/glusterd-utils.h
index bc20c2d..3f49acf 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.h
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.h
@@ -92,7 +92,7 @@  glusterd_volinfo_find (char *volname, glusterd_volinfo_t **volinfo);
 
 int32_t
 glusterd_service_stop(const char *service, char *pidfile, int sig,
-                      gf_boolean_t keep_pidfile);
+                      gf_boolean_t force_kill);
 
 int32_t
 glusterd_resolve_brick (glusterd_brickinfo_t *brickinfo);