[Gluster-devel] Glusterd daemon management code refactoring

[Kaushal M]

I had the exact same comment on this when KP showed this to me before.

I'm pasting the response I'd given then.

<paste>
It would also be good if have the service itself be a standard type.
This is along the lines of the xlator class we have. This way we could
have standard implementations of the various methods of a service. A
simple service in this case would just involve creating the standard
object with some customizations. People who want behaviours different
from the standard would implement their own methods.

I am thinking of something like this,

```c
struct glusterd_service_t {
    char name[];
    void *private_struct;
    struct conn_mgmt conn;
    struct proc_mgmt proc;
}
/* Default functions */
struct glusterd_service_t * glusterd_service_new (char *name, char *volid) {
    /* Build the service and the associated conn and proc objects
using the given service name and volid.
    * These are the only variables I see. Everything else could be
standardised to be based on these two.
   */
}
int glusterd_service_start (glusterd_service_t * service) {
    /* Will start the process and then establish connection  */
    service.proc.start();
    service.conn.connect();
}
int glusterd_service_stop (glusterd_service_t *) {
   /* Similar to above */
}
int glusterd_service_send_request (glusterd_service_t *, rpc_req_t*) {
}
// and so on

// Example Usage

rebalance_service = glusterd_service_new ("testvol-rebalance",
"testvol-rebalance");
/* Do customization */
rebalance_service.conn.notify = rebalance_notify_fn
/* Use*/
ret = glusterd_service_start(rebalance_service);

is_service_running(rebalance_service);

glusterd_service_send_request(rebalance_service, req);

// etc...

/* Advanced services */

adv_serv = glusterd_service_new("VeryAdvancedService", "advanced_vol");
adv_serv.private_struct = adv_serv_private;
adv_serv.proc.start = adv_serv_start_func();
// and so on..
adv_serv.conn.connect = adv_serv_conn()
adv_serv.conn.disconnect = adv_serv_disconn()
// and so on..

glusterd_service_start(adv_serv);

```
</paste>


~kaushal

On Tue, Dec 9, 2014 at 6:24 PM, Jeff Darcy <jdarcy at redhat.com> wrote:
>> I would like to propose refactoring of the code managing
>> various daemons in glusterd. Unlike other high(er) level proposals
>> about feature design, this one is at the implementation
>> level. Please go through the details of the proposal below
>> and share your thoughts/suggestions on the approach.
>
> I like this idea, as a way of reducing technical debt.  Can we take it a
> step further, and provide default *implementations* for some of these
> methods?  For example, connect/disconnect and is_running are likely to
> be the same for practically all daemons.
>
>> ### Introduction
>>
>> Glusterd manages GlusterFS daemons providing services like NFS, Proactive
>> self-heal, Quota, User servicable snapshots etc. Following are some of the
>> aspects that come under daemon management.
>>
>> - Connection Management
>>   - unix domain sockets based channel for internal communication
>>   - Methods - connect, disconnect, notify
>>
>> - Process Management
>>   - pidfile to detect if the daemon is running
>>   - Environment; run-dir, svc-dir, log-dir etc.
>>   - Methods - start, stop, status, kill
>>
>> - Daemon-specific Management
>>
>> Currently, the daemon management code is fragmented and doesn't elicit the
>> structure described above. This results in further fragmentation since new
>> developers may not identify common patterns, worse even, they won't be able
>> to
>> do anything about it.
>>
>> This proposal aims to do the following,
>>
>> - Provide an abstract data type that encapsulates what is common among
>> daemons
>>   that are managed by glusterd.
>>
>> - 'Port' existing code to make use of the abstract type. This would help in
>>   making this change self-documented to an extent.
>>
>> - Prescribe a way to maintain per-feature daemon code separate to glusterd's
>>   common code.
>>
>> ### Abstract data types
>>
>>         struct conn_mgmt {
>>             struct rpc_clnt *rpc;
>>             int (*connect) (struct conn_mgmt *self);
>>             int (*disconnect) (struct conn_mgmt self);
>>             int (*notify) (struct conn_mgmt *self, rpc_clnt_event_t
>>             *rpc_event);
>>         }
>>
>>         struct proc_mgmt {
>>             char svcdir[PATH_MAX];
>>             char rundir[PATH_MAX];
>>             char logdir[PATH_MAX];
>>             char pidfile[PATH_MAX];
>>             char logfile[PATH_MAX];
>>
>>             char volid[UUID_CANONICAL_FORM_LEN];
>
> How about a PID, so that default implementations (e.g. is_running
> or kill) can use it?
>
>>             int (*start) (struct proc_mgmt *self, int flags);
>>             int (*stop) (struct proc_mgmt *self, int flags);
>>             int (*is_running) (struct proc_mgmt *self);
>>             int (*kill) (struct proc_mgmt *self, int flags);
>>
>>         }
>>
>> Feature authors can define data type representing their service by
>> implementing
>> the above 'abstract' class. For e.g,
>>
>>         struct my_service {
>>             char name[PATH_MAX];
>>             /* my_service specific data members and methods */
>>
>>             /* The methods in the following structures should be implemented
>>             by
>>                respective feature authors */
>>
>>             struct conn_mgmt conn;
>>             struct proc_mgmt proc;
>>         }
>>
>> ### Code structure guidelines
>>
>> Each feature that introduces a daemon would implement the abstract data type.
>> The implementations should be in separate files named appropriately. The
>> intent
>> is to avoid feature specific code to leak into common glusterd codebase.
>> glusterd-utils.c is testament to such practices in the past.
>>
>> For e.g,
>> [kp at trantor glusterd]$ tree
>> .
>> └── src
>>     ├── glusterd-conn-mgmt.c
>>     ├── glusterd-conn-mgmt.h
>>     ├── glusterd-proc-mgmt.c
>>     ├── glusterd-proc-mgmt.h
>>     ├── my-feature-service.c
>>     └── my-feature-service.h
>>
>> [kp at trantor glusterd]$ cat src/my-feature-service.h
>>         #include "glusterd-conn-mgmt.h"
>>         #include "glusterd-proc-mgmt.h"
>>
>> ...
>> [rest of the code elided]
>>
>> ### Bibliography
>>
>> - Object-oriented design patterns in the kernel, part 1 -
>> http://lwn.net/Articles/444910/
>> - Object-oriented design patterns in the kernel, part 2 -
>> http://lwn.net/Articles/446317/
>>
>>
>> thanks,
>> kp
>> _______________________________________________
>> Gluster-devel mailing list
>> Gluster-devel at gluster.org
>> http://supercolony.gluster.org/mailman/listinfo/gluster-devel
>>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel