[Cluster-devel] [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Cluster-devel] [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Jiaju Zhang]

Hi,

Since there is no proper way to enable SCTP when using pacemaker stack,
this patch is to auto-enabled SCTP when redundant rings have been configured
in corosync.
Review and comments are welcome :)

Thanks a lot,
Jiaju

diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
index 810c644..ed0f958 100644
--- a/group/dlm_controld/pacemaker.c
+++ b/group/dlm_controld/pacemaker.c
@@ -23,6 +23,7 @@
 #include <pacemaker/crm/cib.h>

 #define COMMS_DIR     "/sys/kernel/config/dlm/cluster/comms"
+#define PROTO_SCTP    1

 int setup_ccs(void)
 {
@@ -78,9 +79,11 @@ int setup_cluster(void)
     return ais_fd_async;
 }

+/*Force re-read of cluster membership */
 void update_cluster(void)
 {
     static uint64_t last_membership = 0;
+    ais_dispatch(ais_fd_async, NULL);
     cluster_quorate = crm_have_quorum;
     if(last_membership < crm_peer_seq) {
     log_debug("Processing membership %llu", crm_peer_seq);
@@ -91,7 +94,6 @@ void update_cluster(void)

 void process_cluster(int ci)
 {
-    ais_dispatch(ais_fd_async, NULL);
     update_cluster();
 }

@@ -141,6 +143,7 @@ void dlm_process_node(gpointer key, gpointer value,
gpointer user_data)
     char *addr_copy = strdup(node->addr);
     char *addr_top = addr_copy;
     char *addr = NULL;
+    int count = 0;

     if(do_remove) {
         action = "Re-added";
@@ -207,7 +210,13 @@ void dlm_process_node(gpointer key, gpointer value,
gpointer user_data)
         log_debug("Adding address %s to configfs for node %u/%s ", addr,
node->id, node->uname);
         add_configfs_node(node->id, ((char*)&cna_addr), cna_len, (node->id
== local_node_id));

+        count++;
+
     } while(addr != NULL);
+
+    if (count > 1)
+        cfgk_protocol = PROTO_SCTP;
+
     free(addr_top);
     }
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091014/ea122030/attachment.htm>

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Jiaju Zhang]

I'd like to resend the patch as attachment since it may have some format
problem
when I sent it last time.

Thanks,
Jiaju

On Wed, Oct 14, 2009 at 6:50 PM, Jiaju Zhang <jjzhang.linux@gmail.com>wrote:

> Hi,
>
> Since there is no proper way to enable SCTP when using pacemaker stack,
> this patch is to auto-enable SCTP when redundant rings have been configured
> in corosync.
> Review and comments are welcome :)
>
> Thanks a lot,
> Jiaju
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091014/56f1e2a3/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dlm-pcmk-enable-sctp.diff
Type: text/x-patch
Size: 1748 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091014/56f1e2a3/attachment.bin>

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[David Teigland]

On Wed, Oct 14, 2009 at 06:50:56PM +0800, Jiaju Zhang wrote:
> Hi,
> 
> Since there is no proper way to enable SCTP when using pacemaker stack,
> this patch is to auto-enabled SCTP when redundant rings have been configured
> in corosync.
> Review and comments are welcome :)

Hi,

Could we just make it a command line option?  Is it important to turn on
automatically?

Dave

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Jiaju Zhang]

On Thu, Oct 15, 2009 at 2:17 AM, David Teigland <teigland@redhat.com> wrote:

> On Wed, Oct 14, 2009 at 06:50:56PM +0800, Jiaju Zhang wrote:
> > Hi,
> >
> > Since there is no proper way to enable SCTP when using pacemaker stack,
> > this patch is to auto-enabled SCTP when redundant rings have been
> configured
> > in corosync.
> > Review and comments are welcome :)
>
> Hi,
>
> Could we just make it a command line option?  Is it important to turn on
> automatically?
>

Yes, making it a command line option also works fine and it is simpler than
that patch. Attached is a new patch which adding a command line option, I'll
be very appreciated for your review and comments.

The reason I wanted to turn on SCTP automatically is that it might have a
little better usability since the user won't care the detail "redundant
rings need SCTP to be configured" and even if the user hasn't read the
manual very carefully and hasn't turn on SCTP in command line, the software
will also help him to do this.
That was my original thought, I don't know which one is better at that time.

Thanks again for your review of the new patch :)

Signed-off-by: Jiaju Zhang <jjzhang.linux@gmail.com>
---
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091015/00a575be/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dlm-add-command-line-option.diff
Type: text/x-patch
Size: 1111 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091015/00a575be/attachment.bin>

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Andrew Beekhof]

Looks good to me :-)
Although it might be nice to have the automated detection in place too.

Less things for the admin to get wrong.  What do you think Dave?

On Thu, Oct 15, 2009 at 7:34 AM, Jiaju Zhang <jjzhang.linux@gmail.com> wrote:
>
>
> On Thu, Oct 15, 2009 at 2:17 AM, David Teigland <teigland@redhat.com> wrote:
>>
>> On Wed, Oct 14, 2009 at 06:50:56PM +0800, Jiaju Zhang wrote:
>> > Hi,
>> >
>> > Since there is no proper way to enable SCTP when using pacemaker stack,
>> > this patch is to auto-enabled SCTP when redundant rings have been
>> > configured
>> > in corosync.
>> > Review and comments are welcome :)
>>
>> Hi,
>>
>> Could we just make it a command line option? ?Is it important to turn on
>> automatically?
>>
>
> Yes, making it a command line option also works fine and it is simpler than
> that patch. Attached is a new patch which adding a command line option, I'll
> be very appreciated for your review and comments.
>
> The reason I wanted to turn on SCTP automatically is that it might have a
> little better usability since the user won't care the detail "redundant
> rings need SCTP to be configured" and even if the user hasn't read the
> manual very carefully and hasn't turn on SCTP in command line, the software
> will also help him to do this.
> That was my original thought, I don't know which one is better at that time.
>
> Thanks again for your review of the new patch :)
>
> Signed-off-by: Jiaju Zhang <jjzhang.linux@gmail.com>
> ---
>
>
>
>
>
>

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[David Teigland]

On Thu, Oct 15, 2009 at 09:43:56AM +0200, Andrew Beekhof wrote:
> Looks good to me :-)
> Although it might be nice to have the automated detection in place too.
> 
> Less things for the admin to get wrong.  What do you think Dave?

I don't mind, now that I've taken a moment to think how it should work...
There should be three protocol options, "tcp", "sctp", "detect".  Detect
should work by reading the totem/rrp_mode value in setup_ccs().

Dave

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Andrew Beekhof]

On Thu, Oct 15, 2009 at 4:51 PM, David Teigland <teigland@redhat.com> wrote:
> On Thu, Oct 15, 2009 at 09:43:56AM +0200, Andrew Beekhof wrote:
>> Looks good to me :-)
>> Although it might be nice to have the automated detection in place too.
>>
>> Less things for the admin to get wrong. ?What do you think Dave?
>
> I don't mind, now that I've taken a moment to think how it should work...
> There should be three protocol options, "tcp", "sctp", "detect". ?Detect
> should work by reading the totem/rrp_mode value in setup_ccs().

Agreed.
Now we just need someone to volunteer to write the patch :-)

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Jiaju Zhang]

On Thu, Oct 15, 2009 at 11:30 PM, Andrew Beekhof <andrew@beekhof.net> wrote:

> On Thu, Oct 15, 2009 at 4:51 PM, David Teigland <teigland@redhat.com>
> wrote:
> > On Thu, Oct 15, 2009 at 09:43:56AM +0200, Andrew Beekhof wrote:
> >> Looks good to me :-)
> >> Although it might be nice to have the automated detection in place too.
> >>
> >> Less things for the admin to get wrong.  What do you think Dave?
> >
> > I don't mind, now that I've taken a moment to think how it should work...
> > There should be three protocol options, "tcp", "sctp", "detect".  Detect
> > should work by reading the totem/rrp_mode value in setup_ccs().
>
> Agreed.
> Now we just need someone to volunteer to write the patch :-)
>

I would like to write the patch. Adding the detection function in
setup_ccs() is more reasonable. In fact, I have ever been thinking about
adding this function in setup_ccs(). But when I looked into the code, I
found another way could make less modification in code, so I wrote that
patch and sent out for comments.

Many thanks for your comments :)
Jiaju
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091016/8aa5531a/attachment.htm>

[Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld (pcmk) to be able to handle redundant rings

[Lars Marowsky-Bree]

On 2009-10-15T09:51:08, David Teigland <teigland@redhat.com> wrote:

> On Thu, Oct 15, 2009 at 09:43:56AM +0200, Andrew Beekhof wrote:
> > Looks good to me :-)
> > Although it might be nice to have the automated detection in place too.
> > 
> > Less things for the admin to get wrong.  What do you think Dave?
> 
> I don't mind, now that I've taken a moment to think how it should work...
> There should be three protocol options, "tcp", "sctp", "detect".  Detect
> should work by reading the totem/rrp_mode value in setup_ccs().

Can we then also agree on "detect" being the default?

(Experience shows that admins will misconfigure just about anything if
they have to deviate from the defaults.)


Thanks,
    Lars

-- 
Architect Storage/HA, OPS Engineering, Novell, Inc.
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG N?rnberg)
"Experience is the name everyone gives to their mistakes." -- Oscar Wilde