All, We have merged the "Daemon management refactoring" changes[1] successfully. Thanks you for all those who reviewed it. As part of this patch we have moved gluster-NFS, gluster self-heal daemon, quota daemon and snapshot daemon (for serving User servicable snapshots) into the new framework. This is great progress! Thanks Atin for working tirelessly on the comments. This change could cause some inconvenience for those who have begun working on their respective daemon management, copy-pasting code from existing daemons for lack of an alternative. To address this, we have attached a document that provides broad guidelines on how one could approach 'porting' their code into the new 'framework' [2]. Feel free to reach out on gluster-devel for any further questions. cheers, kp [1] - http://review.gluster.com/9428 [2] - https://github.com/gluster/glusterfs/blob/master/doc/daemon-management-framework.md ----- Original Message ----- > [Apologies for sending an incomplete message] > > While implementing daemon management code as proposed here, I found it wasn't > possible to embed structures that are abstract data types (ADT). In short, > ADT's 'real' size is unknown to the containing type and compilation fails. > This > brings us to which one of the two is important. > > Embedded structures allows us to model daemons in a compositional style > lending > clarity of purpose to each component. For e.g any daemon management could be > decomposed into, > - managing the daemon process' lifecycle > - maintaining IPC with the daemon > - managing daemon's file system environment > With embedded structures, we don't have to allocate memory individually. This > simplifies the implementation. > > While ADT would protect consumers from changes to the type, like adding a > member, it could lead to a lot of getters and setters (boiler-plate). With > these observations, I choose to go with embedded structures approach, leaving > ADTs for a different purpose elsewhere. Thoughts? > > ~kp > > ----- Original Message ----- > > While modelling daemons as proposed here, I noticed that I didn't > > foresee thathow abstract data types and embedded structures > > (think struct list_head) > > > > ----- Original Message ----- > > > Here is the first patch, http://review.gluster.org/9278, which marks the > > > beginning > > > of the implementation phase of this refactoring effort. Reviews/comments > > > for > > > the (design) proposal > > > helped us to correct a few things and kick start coding. Look forward to > > > see > > > the same enthusiasm > > > with the code review(s) :-) > > > > > > > > > ----- Original Message ----- > > > > > > > > > > > > On 12/09/2014 06:24 PM, Jeff Darcy 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. > > > > We do have a plan to implement the default implementations. > > > > > > > > > > > > > >> ### 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? > > > > pidfile will have the details for 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@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@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@xxxxxxxxxxx > > > > >> http://supercolony.gluster.org/mailman/listinfo/gluster-devel > > > > >> > > > > > _______________________________________________ > > > > > Gluster-devel mailing list > > > > > Gluster-devel@xxxxxxxxxxx > > > > > http://supercolony.gluster.org/mailman/listinfo/gluster-devel > > > > > > > > > ~Atin > > > > > > > _______________________________________________ > > > Gluster-devel mailing list > > > Gluster-devel@xxxxxxxxxxx > > > http://supercolony.gluster.org/mailman/listinfo/gluster-devel > > > > > _______________________________________________ > > Gluster-devel mailing list > > Gluster-devel@xxxxxxxxxxx > > http://www.gluster.org/mailman/listinfo/gluster-devel > > > _______________________________________________ > Gluster-devel mailing list > Gluster-devel@xxxxxxxxxxx > http://www.gluster.org/mailman/listinfo/gluster-devel > _______________________________________________ Gluster-devel mailing list Gluster-devel@xxxxxxxxxxx http://www.gluster.org/mailman/listinfo/gluster-devel
