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

[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

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

[jjzhang]

<andrew@beekhof.net>, Lars Marowsky-Bree <lmb@suse.de>
Cc: cluster-devel at redhat.com
Bcc: 
Subject: Re: [Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld
	(pcmk) to be able to handle redundant rings
Reply-To: 
In-Reply-To: <20091016101722.GZ15975@suse.de>

On Fri, Oct 16, 2009 at 12:17:22PM +0200, Lars Marowsky-Bree wrote:
> 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.)

Hello,

Below is the improved patch. I have set the "default" as the same logic as 
"detect", but I'm not sure if this is OK.

Thanks a lot for your kindly review and comments :)

Signed-off-by: Jiaju Zhang <jjzhang.linux@gmail.com>
---

 configure.ac                   |    1 +
 group/dlm_controld/Makefile.am |    4 ++-
 group/dlm_controld/main.c      |    9 ++++++-
 group/dlm_controld/pacemaker.c |   48 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index d50de87..e0a1a0d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -131,6 +131,7 @@ if test "x${enable_pacemaker}" = xyes; then
 	PKG_CHECK_MODULES([totempg],[libtotem_pg])
 	PKG_CHECK_MODULES([xml],[libxml-2.0])
 	PKG_CHECK_MODULES([glib],[glib-2.0])
+	PKG_CHECK_MODULES([cfg],[libcfg])
 	check_lib_no_libs cib cib_new
 	check_lib_no_libs crmcluster crm_set_status_callback
 	check_lib_no_libs crmcommon init_server_ipc_comms
diff --git a/group/dlm_controld/Makefile.am b/group/dlm_controld/Makefile.am
index c14ab89..ce6e587 100644
--- a/group/dlm_controld/Makefile.am
+++ b/group/dlm_controld/Makefile.am
@@ -62,12 +62,14 @@ dlm_controld_pcmk_CPPFLAGS= $(shared_CPPFLAGS) \
 dlm_controld_pcmk_CFLAGS  = $(shared_CFLAGS) \
 			    $(glib_CFLAGS) \
 			    $(xml_CFLAGS) \
-			    $(totempg_CFLAGS)
+			    $(totempg_CFLAGS) \
+			    $(cfg_CFLAGS)
 
 dlm_controld_pcmk_LDFLAGS = $(shared_LIBS) \
 			    $(glib_LIBS) \
 			    $(xml_LIBS) \
 			    $(totempg_LIBS) \
+			    $(cfg_LIBS) \
 			    -lcib -lcrmcommon -lcrmcluster
 
 dlm_controld_pcmk_LDADD	  = $(shared_LDADD)
diff --git a/group/dlm_controld/main.c b/group/dlm_controld/main.c
index f90cd21..8949d11 100644
--- a/group/dlm_controld/main.c
+++ b/group/dlm_controld/main.c
@@ -1058,11 +1058,13 @@ static void print_usage(void)
 	printf("		Default is %u\n", DEFAULT_DROP_RESOURCES_COUNT);
 	printf("  -a <ms>	plock ownership drop resources age (milliseconds)\n");
 	printf("		Default is %u\n", DEFAULT_DROP_RESOURCES_AGE);
+	printf("  -r <num>	DLM in-kernel communication protocols: TCP(0), SCTP(1), DETECT(2)\n");
+	printf("		Default is auto-detect (2)");
 	printf("  -h		Print this help, then exit\n");
 	printf("  -V		Print program version information, then exit\n");
 }
 
-#define OPTION_STRING "LDKf:q:d:p:Pl:o:t:c:a:hV"
+#define OPTION_STRING "LDKf:q:d:p:Pl:o:t:c:a:r:hV"
 
 static void read_arguments(int argc, char **argv)
 {
@@ -1142,6 +1144,11 @@ static void read_arguments(int argc, char **argv)
 			cfgd_drop_resources_age = atoi(optarg);
 			break;
 
+		case 'r':
+			optk_protocol = 1;
+			cfgk_protocol = atoi(optarg);
+			break;
+
 		case 'h':
 			print_usage();
 			exit(EXIT_SUCCESS);
diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
index 810c644..b3b147a 100644
--- a/group/dlm_controld/pacemaker.c
+++ b/group/dlm_controld/pacemaker.c
@@ -22,7 +22,42 @@
 #include <pacemaker/crm/msg_xml.h>
 #include <pacemaker/crm/cib.h>
 
+#include <corosync/corotypes.h>
+#include <corosync/cfg.h>
+
 #define COMMS_DIR     "/sys/kernel/config/dlm/cluster/comms"
+#define PROTO_TCP     0
+#define PROTO_SCTP    1
+#define PROTO_DETECT  2
+
+static int get_ring_count(void)
+{
+    cs_error_t result;
+    corosync_cfg_handle_t handle;
+    unsigned int interface_count;
+    char **interface_names;
+    char **interface_status;
+
+    result = corosync_cfg_initialize(&handle, NULL);
+    if (result != CS_OK) {
+	log_error("Failed to initialize corosync configuration API (error=%d)", result);
+	(void)corosync_cfg_finalize(handle);
+	return -1;
+    }
+
+    result = corosync_cfg_ring_status_get(handle,
+					&interface_names,
+					&interface_status,
+					&interface_count);
+    if (result != CS_OK) {
+	log_error("Failed to get the ring status (error=%d)", result);
+	(void)corosync_cfg_finalize(handle);
+	return -1;
+    }
+
+    (void)corosync_cfg_finalize(handle);
+    return interface_count;
+}
 
 int setup_ccs(void)
 {
@@ -30,6 +65,19 @@ int setup_ccs(void)
      * only allow configuration from the command-line until CoroSync is stable
      * enough to be used with Pacemaker
      */
+    if (cfgk_protocol != -1 && cfgk_protocol != PROTO_TCP &&
+	cfgk_protocol != PROTO_SCTP && cfgk_protocol != PROTO_DETECT) {
+	log_error("ignore invalid value %d for dlm protocol", cfgk_protocol);
+	cfgk_protocol = -1;
+    }
+
+    if (cfgk_protocol == PROTO_DETECT || cfgk_protocol == -1) {
+	if (get_ring_count() > 1)
+	    cfgk_protocol = PROTO_SCTP;
+	else
+	    cfgk_protocol = PROTO_TCP;
+    }
+
     return 0;
 }
 

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

[Andrew Beekhof]

On Fri, Oct 23, 2009 at 12:38 PM, jjzhang <jjzhang.linux@gmail.com> wrote:
> <andrew@beekhof.net>, Lars Marowsky-Bree <lmb@suse.de>
> Cc: cluster-devel at redhat.com
> Bcc:
> Subject: Re: [Cluster-devel] Re: [RFC PATCH] dlm: enhancing dlm_controld
> ? ? ? ?(pcmk) to be able to handle redundant rings
> Reply-To:
> In-Reply-To: <20091016101722.GZ15975@suse.de>
>
> On Fri, Oct 16, 2009 at 12:17:22PM +0200, Lars Marowsky-Bree wrote:
>> 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.)
>
> Hello,
>
> Below is the improved patch. I have set the "default" as the same logic as
> "detect", but I'm not sure if this is OK.

I consider that to be the ideal default.

>
> Thanks a lot for your kindly review and comments :)

Looks good to me.
Unless anyone objects I'd like to apply this.

 (Can you resend as an attachment, inline patches have a habit of
getting mangled)

> Signed-off-by: Jiaju Zhang <jjzhang.linux@gmail.com>
> ---
>
> ?configure.ac ? ? ? ? ? ? ? ? ? | ? ?1 +
> ?group/dlm_controld/Makefile.am | ? ?4 ++-
> ?group/dlm_controld/main.c ? ? ?| ? ?9 ++++++-
> ?group/dlm_controld/pacemaker.c | ? 48 ++++++++++++++++++++++++++++++++++++++++
> ?4 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index d50de87..e0a1a0d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -131,6 +131,7 @@ if test "x${enable_pacemaker}" = xyes; then
> ? ? ? ?PKG_CHECK_MODULES([totempg],[libtotem_pg])
> ? ? ? ?PKG_CHECK_MODULES([xml],[libxml-2.0])
> ? ? ? ?PKG_CHECK_MODULES([glib],[glib-2.0])
> + ? ? ? PKG_CHECK_MODULES([cfg],[libcfg])
> ? ? ? ?check_lib_no_libs cib cib_new
> ? ? ? ?check_lib_no_libs crmcluster crm_set_status_callback
> ? ? ? ?check_lib_no_libs crmcommon init_server_ipc_comms
> diff --git a/group/dlm_controld/Makefile.am b/group/dlm_controld/Makefile.am
> index c14ab89..ce6e587 100644
> --- a/group/dlm_controld/Makefile.am
> +++ b/group/dlm_controld/Makefile.am
> @@ -62,12 +62,14 @@ dlm_controld_pcmk_CPPFLAGS= $(shared_CPPFLAGS) \
> ?dlm_controld_pcmk_CFLAGS ?= $(shared_CFLAGS) \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?$(glib_CFLAGS) \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?$(xml_CFLAGS) \
> - ? ? ? ? ? ? ? ? ? ? ? ? ? $(totempg_CFLAGS)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? $(totempg_CFLAGS) \
> + ? ? ? ? ? ? ? ? ? ? ? ? ? $(cfg_CFLAGS)
>
> ?dlm_controld_pcmk_LDFLAGS = $(shared_LIBS) \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?$(glib_LIBS) \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?$(xml_LIBS) \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?$(totempg_LIBS) \
> + ? ? ? ? ? ? ? ? ? ? ? ? ? $(cfg_LIBS) \
> ? ? ? ? ? ? ? ? ? ? ? ? ? ?-lcib -lcrmcommon -lcrmcluster
>
> ?dlm_controld_pcmk_LDADD ? ? ? ? ?= $(shared_LDADD)
> diff --git a/group/dlm_controld/main.c b/group/dlm_controld/main.c
> index f90cd21..8949d11 100644
> --- a/group/dlm_controld/main.c
> +++ b/group/dlm_controld/main.c
> @@ -1058,11 +1058,13 @@ static void print_usage(void)
> ? ? ? ?printf(" ? ? ? ? ? ? ? ?Default is %u\n", DEFAULT_DROP_RESOURCES_COUNT);
> ? ? ? ?printf(" ?-a <ms> ? ? ? plock ownership drop resources age (milliseconds)\n");
> ? ? ? ?printf(" ? ? ? ? ? ? ? ?Default is %u\n", DEFAULT_DROP_RESOURCES_AGE);
> + ? ? ? printf(" ?-r <num> ? ? ?DLM in-kernel communication protocols: TCP(0), SCTP(1), DETECT(2)\n");
> + ? ? ? printf(" ? ? ? ? ? ? ? ?Default is auto-detect (2)");
> ? ? ? ?printf(" ?-h ? ? ? ? ? ?Print this help, then exit\n");
> ? ? ? ?printf(" ?-V ? ? ? ? ? ?Print program version information, then exit\n");
> ?}
>
> -#define OPTION_STRING "LDKf:q:d:p:Pl:o:t:c:a:hV"
> +#define OPTION_STRING "LDKf:q:d:p:Pl:o:t:c:a:r:hV"
>
> ?static void read_arguments(int argc, char **argv)
> ?{
> @@ -1142,6 +1144,11 @@ static void read_arguments(int argc, char **argv)
> ? ? ? ? ? ? ? ? ? ? ? ?cfgd_drop_resources_age = atoi(optarg);
> ? ? ? ? ? ? ? ? ? ? ? ?break;
>
> + ? ? ? ? ? ? ? case 'r':
> + ? ? ? ? ? ? ? ? ? ? ? optk_protocol = 1;
> + ? ? ? ? ? ? ? ? ? ? ? cfgk_protocol = atoi(optarg);
> + ? ? ? ? ? ? ? ? ? ? ? break;
> +
> ? ? ? ? ? ? ? ?case 'h':
> ? ? ? ? ? ? ? ? ? ? ? ?print_usage();
> ? ? ? ? ? ? ? ? ? ? ? ?exit(EXIT_SUCCESS);
> diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
> index 810c644..b3b147a 100644
> --- a/group/dlm_controld/pacemaker.c
> +++ b/group/dlm_controld/pacemaker.c
> @@ -22,7 +22,42 @@
> ?#include <pacemaker/crm/msg_xml.h>
> ?#include <pacemaker/crm/cib.h>
>
> +#include <corosync/corotypes.h>
> +#include <corosync/cfg.h>
> +
> ?#define COMMS_DIR ? ? "/sys/kernel/config/dlm/cluster/comms"
> +#define PROTO_TCP ? ? 0
> +#define PROTO_SCTP ? ?1
> +#define PROTO_DETECT ?2
> +
> +static int get_ring_count(void)
> +{
> + ? ?cs_error_t result;
> + ? ?corosync_cfg_handle_t handle;
> + ? ?unsigned int interface_count;
> + ? ?char **interface_names;
> + ? ?char **interface_status;
> +
> + ? ?result = corosync_cfg_initialize(&handle, NULL);
> + ? ?if (result != CS_OK) {
> + ? ? ? log_error("Failed to initialize corosync configuration API (error=%d)", result);
> + ? ? ? (void)corosync_cfg_finalize(handle);
> + ? ? ? return -1;
> + ? ?}
> +
> + ? ?result = corosync_cfg_ring_status_get(handle,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &interface_names,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &interface_status,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &interface_count);
> + ? ?if (result != CS_OK) {
> + ? ? ? log_error("Failed to get the ring status (error=%d)", result);
> + ? ? ? (void)corosync_cfg_finalize(handle);
> + ? ? ? return -1;
> + ? ?}
> +
> + ? ?(void)corosync_cfg_finalize(handle);
> + ? ?return interface_count;
> +}
>
> ?int setup_ccs(void)
> ?{
> @@ -30,6 +65,19 @@ int setup_ccs(void)
> ? ? ?* only allow configuration from the command-line until CoroSync is stable
> ? ? ?* enough to be used with Pacemaker
> ? ? ?*/
> + ? ?if (cfgk_protocol != -1 && cfgk_protocol != PROTO_TCP &&
> + ? ? ? cfgk_protocol != PROTO_SCTP && cfgk_protocol != PROTO_DETECT) {
> + ? ? ? log_error("ignore invalid value %d for dlm protocol", cfgk_protocol);
> + ? ? ? cfgk_protocol = -1;
> + ? ?}
> +
> + ? ?if (cfgk_protocol == PROTO_DETECT || cfgk_protocol == -1) {
> + ? ? ? if (get_ring_count() > 1)
> + ? ? ? ? ? cfgk_protocol = PROTO_SCTP;
> + ? ? ? else
> + ? ? ? ? ? cfgk_protocol = PROTO_TCP;
> + ? ?}
> +
> ? ? return 0;
> ?}
>
>

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

[Jiaju Zhang]

On Fri, Oct 23, 2009 at 9:05 PM, Andrew Beekhof <andrew@beekhof.net> wrote:
>
> On Fri, Oct 23, 2009 at 12:38 PM, jjzhang <jjzhang.linux@gmail.com> wrote:
> > On Fri, Oct 16, 2009 at 12:17:22PM +0200, Lars Marowsky-Bree wrote:
> >> 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.)
> >
> > Hello,
> >
> > Below is the improved patch. I have set the "default" as the same logic as
> > "detect", but I'm not sure if this is OK.
>
> I consider that to be the ideal default.
>
> >
> > Thanks a lot for your kindly review and comments :)
>
> Looks good to me.
> Unless anyone objects I'd like to apply this.
>
> ?(Can you resend as an attachment, inline patches have a habit of
> getting mangled)
>

OK, Thanks :)
Please see the attachment.

Thanks,
Jiaju
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dlm-pcmk-config-protocol.diff
Type: text/x-diff
Size: 4397 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091023/3af24914/attachment.bin>

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

[Lars Marowsky-Bree]

On 2009-10-23T21:23:20, Jiaju Zhang <jjzhang.linux@gmail.com> wrote:

> OK, Thanks :)
> Please see the attachment.

Thanks for the work, this looks good.


Regards,
    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

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

[David Teigland]

On Fri, Oct 23, 2009 at 09:23:20PM +0800, Jiaju Zhang wrote:
+    result = corosync_cfg_ring_status_get(handle,
+                                       &interface_names,
+                                       &interface_status,
+                                       &interface_count);
+    if (result != CS_OK) {
+       log_error("Failed to get the ring status (error=%d)", result);
+       (void)corosync_cfg_finalize(handle);
+       return -1;
+    }
+
+    (void)corosync_cfg_finalize(handle);
+    return interface_count;

What is interface_count if there's one ring up but rrp is configured?
Could we read the totem/rrp config value from the objdb?

Dave

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

[Steven Dake]

On Fri, 2009-10-23 at 12:55 -0500, David Teigland wrote:
> On Fri, Oct 23, 2009 at 09:23:20PM +0800, Jiaju Zhang wrote:
> +    result = corosync_cfg_ring_status_get(handle,
> +                                       &interface_names,
> +                                       &interface_status,
> +                                       &interface_count);
> +    if (result != CS_OK) {
> +       log_error("Failed to get the ring status (error=%d)", result);
> +       (void)corosync_cfg_finalize(handle);
> +       return -1;
> +    }
> +
> +    (void)corosync_cfg_finalize(handle);
> +    return interface_count;
> 
> What is interface_count if there's one ring up but rrp is configured?
> Could we read the totem/rrp config value from the objdb?
> 
> Dave
> 
Dave,

The rrp mode can be read from the confdb currently.  The other values
cannot be read from the confdb currently.  interface_count is the number
of configured interfaces so that interface_status may be iterated.

Long term the plan is to deprecate all of the APIs related to
config/diag in favor of using confdb (or some successor to this api if
absolutely necessary).  This includes things like the above ring status
api.

We have alot of work to do to get there.  Corosync 2.0 material.

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

[David Teigland]

On Fri, Oct 23, 2009 at 12:18:09PM -0700, Steven Dake wrote:
> On Fri, 2009-10-23 at 12:55 -0500, David Teigland wrote:
> > On Fri, Oct 23, 2009 at 09:23:20PM +0800, Jiaju Zhang wrote:
> > +    result = corosync_cfg_ring_status_get(handle,
> > +                                       &interface_names,
> > +                                       &interface_status,
> > +                                       &interface_count);
> > +    if (result != CS_OK) {
> > +       log_error("Failed to get the ring status (error=%d)", result);
> > +       (void)corosync_cfg_finalize(handle);
> > +       return -1;
> > +    }
> > +
> > +    (void)corosync_cfg_finalize(handle);
> > +    return interface_count;
> > 
> > What is interface_count if there's one ring up but rrp is configured?
> > Could we read the totem/rrp config value from the objdb?


> Dave,
> 
> The rrp mode can be read from the confdb currently.  

Ah, yeah, that's what I meant.

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

[Jiaju Zhang]

On Sat, Oct 24, 2009 at 4:51 AM, David Teigland <teigland@redhat.com> wrote:
> On Fri, Oct 23, 2009 at 12:18:09PM -0700, Steven Dake wrote:
>> On Fri, 2009-10-23 at 12:55 -0500, David Teigland wrote:
>> > On Fri, Oct 23, 2009 at 09:23:20PM +0800, Jiaju Zhang wrote:
>> > + ? ?result = corosync_cfg_ring_status_get(handle,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &interface_names,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &interface_status,
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &interface_count);
>> > + ? ?if (result != CS_OK) {
>> > + ? ? ? log_error("Failed to get the ring status (error=%d)", result);
>> > + ? ? ? (void)corosync_cfg_finalize(handle);
>> > + ? ? ? return -1;
>> > + ? ?}
>> > +
>> > + ? ?(void)corosync_cfg_finalize(handle);
>> > + ? ?return interface_count;
>> >
>> > What is interface_count if there's one ring up but rrp is configured?
>> > Could we read the totem/rrp config value from the objdb?
>
>
>> Dave,
>>
>> The rrp mode can be read from the confdb currently.
>
> Ah, yeah, that's what I meant.

Hello,

Attached is the improved patch, which read the rrp mode from the confdb.
Thanks a lot for your comments :)

Thanks,
Jiaju
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dlm-pcmk-config-protocol.diff
Type: text/x-diff
Size: 5130 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20091026/877ad8d8/attachment.bin>