[Openais] A patch for configuration change of AMF

SAKAI MIYOTAKA sakai.miyotaka at nttcom.co.jp
Tue Sep 14 07:18:09 PDT 2004


Steve ,

Thank you for your comments .
I am replying inline .

>Sakai-san,
>
>Some comments inline on the patch.  Could you address them and post
>another patch?
>
Yes ,I will .

>On Sun, 2004-09-12 at 07:54, SAKAI MIYOTAKA wrote:
>  
>
>>Steave ,
>>
>>I  made a patch for configuration change .
>>This patch  is also corresponding to a certain node joining .
>>
>>Could you review the patch attached to this message ?
>>
>>I think this path works well .
>>
>>If you accept this patch ,next step is that each separate node has 
>>Active unit ,when joining each other.
>>For example ,
>>Node A has the unit that is Active .
>>Node B has the unit that is Active .
>>After that these nodes joining each other .
>>
>>In that case this patch doesn't work .
>>
>>SAKAI MIYOTAKA wrote:
>>    
>>
>>>Steve ,
>>>
>>>I made a patch for configuration change when a certain node leaves.
>>>
>>>Could you review the patch attached to this message ?
>>>
>>>I think review points are followings 
>>>
>>>- The basic frame of configuration change for AMF.
>>>- Adding source_addr to saAmfComponent as a member.
>>>
>>>thanks 
>>>- Miyotaka sakai
>>>      
>>>
>>______________________________________________________________________
>>diff -uNr openais.2004-09-09.21.30/exec/amf.c openais/exec/amf.c
>>--- openais.2004-09-09.21.30/exec/amf.c	2004-09-10 13:30:02.000000000 +0900
>>+++ openais/exec/amf.c	2004-09-12 22:49:09.000000000 +0900
>>@@ -36,6 +36,7 @@
>> #include <sys/socket.h>
>> #include <sys/un.h>
>> #include <netinet/in.h>
>>+#include <arpa/inet.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <stdlib.h>
>>@@ -142,6 +143,10 @@
>> static int activeServiceUnitsCount (
>> 	struct saAmfGroup *saAmfGroup);
>> 
>>+static void componentRegister( struct saAmfComponent *component );
>>+
>>    
>>
>^^ can you match the coding style in the rest of the code?
>
>try instead
>static void componentRegister (struct saAmfComponent *component);
>
>in fact, feel welcome to rename componentRegister to
>component_register.  I'd like to get rid of all of the uppercase
>function names in the executive.
>
OK .
I 'll be carefull to match the coding style in the rest of the code .

>>+static void componentUnregister ( struct saAmfComponent *component );
>>    
>>
>
>same as above
>
OK .

>>+
>> #ifdef COMPILE_OUT
>> static void enumerateComponents (
>> 	void (*function)(struct saAmfComponent *, void *data),
>>@@ -223,6 +228,18 @@
>> 
>> static int amfExecutiveInitialize (void);
>> 
>>+static void amf_confchg_nleave ( struct saAmfComponent *component ,void *data );
>>+
>>+static void amf_confchg_njoin ( struct saAmfComponent *component ,void *data );
>>+
>>+static void amf_confchg_nsync ( struct saAmfComponent *component ,void *data );
>>+
>>+static int amf_confchg_fn (
>>+	   struct sockaddr_in *member_list, int member_list_entries,
>>+	   struct sockaddr_in *left_list, int left_list_entries,
>>+	   struct sockaddr_in *joined_list, int joined_list_entries);
>>+
>>+
>> static int message_handler_req_exec_amf_componentregister (void *message, struct in_addr source_addr);
>> 
>> static int message_handler_req_exec_amf_componentunregister (void *message, struct in_addr source_addr);
>>@@ -371,7 +388,7 @@
>> 	.libais_handlers_count		= sizeof (amf_libais_handlers) / sizeof (struct libais_handler),
>> 	.aisexec_handler_fns		= amf_aisexec_handler_fns,
>> 	.aisexec_handler_fns_count	= sizeof (amf_aisexec_handler_fns) / sizeof (int (*)),
>>-	.confchg_fn					= 0,
>>+	.confchg_fn				= amf_confchg_fn,
>> 	.libais_init_fn				= message_handler_req_amf_init,
>> 	.libais_exit_fn				= amf_exit_fn,
>> 	.aisexec_init_fn			= amfExecutiveInitialize
>>@@ -488,8 +505,41 @@
>> 	gmi_mcast (&aisexec_groupname, iovecs, 1, GMI_PRIO_MED);
>> }
>> 
>>-#ifdef COMPILE_OUT
>>-This should be used for a partition I think
>>+void componentRegister (
>>+	struct saAmfComponent *component)
>>+{
>>+	struct req_exec_amf_componentregister req_exec_amf_componentregister;
>>+	struct iovec iovecs[2];
>>+
>>+	/*
>>+	 * This only works on local components
>>+	 */
>>+	if (component == 0 || component->local != 1) {
>>+		return;
>>+	}
>>+	log_printf (LOG_LEVEL_DEBUG, "componentRegister: registering component %s\n",
>>+		getSaNameT (&component->name));
>>+//	component->probableCause = SA_AMF_NOT_RESPONDING;
>>+
>>+	req_exec_amf_componentregister.header.size = sizeof (struct req_exec_amf_componentregister);
>>+	req_exec_amf_componentregister.header.id = MESSAGE_REQ_EXEC_AMF_COMPONENTREGISTER;
>>+
>>+	req_exec_amf_componentregister.source.conn_info = 0;
>>+	req_exec_amf_componentregister.source.in_addr.s_addr = 0;
>>+
>>+	memset (&req_exec_amf_componentregister.req_lib_amf_componentregister,
>>+		0, sizeof (struct req_lib_amf_componentregister));
>>+	memcpy (&req_exec_amf_componentregister.req_lib_amf_componentregister.compName,
>>+		&component->name,
>>+		sizeof (SaNameT));
>>+
>>+	iovecs[0].iov_base = (char *)&req_exec_amf_componentregister;
>>+	iovecs[0].iov_len = sizeof (req_exec_amf_componentregister);
>>+
>>+	gmi_mcast (&aisexec_groupname, iovecs, 1, GMI_PRIO_HIGH);
>>+}
>>    
>>
>
>In the case of a partition or merge, you want to use GMI_PRIO_RECOVERY. 
>You may have to pass a variable to component_register and
>component_unregister to specify the priority of th message.
>
OK. I use GMI_PRIO_RECOVERY .

>>+
>>+// This should be used for a partition I think
>> // This should be used for partition changes
>>    
>>
>This says the same thing twice.
>
>No C++ style comments please.  I know they are in there now, but they
>need to be removed as we go forward.
>
OK . I have to use C style comments .

>Stick with:
>/*
> * 
> */
>  
>
>> void enumerateComponents (
>> 	void (*function)(struct saAmfComponent *, void *data),
>>@@ -503,7 +553,6 @@
>> 	struct saAmfUnit *AmfUnit;
>> 	struct saAmfComponent *AmfComponent;
>> 
>>-
>> 	/*
>> 	 * Search all groups
>> 	 */
>>@@ -539,7 +588,6 @@
>> 		}
>> 	}
>> }
>>-#endif
>> 
>> int activeServiceUnitsCount (struct saAmfGroup *saAmfGroup) {
>> 	struct saAmfUnit *saAmfUnit;
>>@@ -568,8 +616,7 @@
>> 
>> 			saAmfComponent = list_entry (saAmfComponentList,
>> 				struct saAmfComponent, saAmfComponentList);
>>-
>>-			if (saAmfComponent->currentHAState != SA_AMF_ACTIVE) {
>>+			if (saAmfComponent->/*currentHAState*/newHAState != SA_AMF_ACTIVE) {
>> 				thisServiceUnitActive = 0;
>>    
>>
>
>this change looks really fishy.  Could you explain why you need to use
>the newhastate instead of the current ha state?  The new ha state
>shouldn't have a valid value for the case of enabledunlockedcompleted.
>  
>
For example ,
After MESSAGE_RES_AMF_CSISETCALLBACK is sent
and by the time message_handler_req_exec_amf_hastateset() is called ,
the currentHAstate (SA_AMF_QUIESED) is different from the
newHAState(SA_AMF_ACTIVE) .
In this sutiation if dsmEnabledUnlockedInServiceCompleted() is called ,
the program calls haStateSetApi( SA_AMF_ACTIVE ),
But in this case , I expect that component becomes SA_AMF_STANDBY.
that is the reason .

>> 		}
>>@@ -1623,6 +1670,79 @@
>> 	return (0);
>> }
>> 
>>+void amf_confchg_nleave( struct saAmfComponent *component ,void *data ) {
>>+
>>+	struct in_addr *source_addr;
>>+	source_addr = ( struct in_addr *) data;
>>+
>>+//	printf( "amf_confchg_nleave(%s)\n" ,(char *)inet_ntoa(*source_addr));
>>+//	printf( "                  (%s)\n" ,(char *)inet_ntoa(component->source_addr));
>>+
>>    
>>
>there is debug printing aparatus, perhaps you could use this instead of
>commenting out the printfs?
>
OK . I' ll get rid of these debug printf .

>>r) ,source_addr ,sizeof(*source_addr)) ) {
>>+		return;
>>+        }
>>+
>>    
>>
>coding style is (memcmp (c1, c2, sizeof (struct in_addr));
>
>  
>
>>+        component -> local = 1;
>>    
>>
>
>could you match the coding style in the code:
>component->local = 1;
>
OK .

>>+        componentUnregister( component );
>>+
>>+        return;
>>+}
>>+
>>+void amf_confchg_njoin ( struct saAmfComponent *component ,void *data ) {
>>+
>>+	if ( memcmp( &(component->source_addr),&(this_ip.sin_addr.s_addr),sizeof(component->source_addr)) ) {
>>+		return;
>>+ 	}
>>+
>>+	componentRegister( component );
>>+	return;
>>+}
>>+
>>+void amf_confchg_nsync ( struct saAmfComponent *component ,void *data ) {
>>+
>>+	if ( memcmp( &(component->source_addr),&(this_ip.sin_addr.s_addr),sizeof(component->source_addr)) ) {
>>+		return;
>>+ 	}
>>+
>>+	readinessStateSetCluster( component ,component->currentReadinessState );
>>+	haStateSetCluster ( component ,component->currentHAState );
>>    
>>
>
>I'm a little surprised this would work directly without additional
>state.  Have you tested several cases?  Ie: trying testamf5,6 then 1,2
>and 56, then 1,2 etc?
>  
>
Thank you for your advice .
I needed to explain more .
I didn't think this code work all of situation ,either .
But in the simple situation it works and it is easier to test .
I 'll change this code , but give me some time .

>>+
>>+	return;
>>+}
>>+
>>+int amf_confchg_fn (
>>+	struct sockaddr_in *member_list, int member_list_entries,
>>+	struct sockaddr_in *left_list, int left_list_entries,
>>+	struct sockaddr_in *joined_list, int joined_list_entries){
>>+
>>+	int i;
>>+	int size;
>>+	void *data;
>>+
>>+	/*
>>+	* If node join , component register 
>>+	*/
>>+	if ( joined_list_entries > 0 ) {
>>+		enumerateComponents ( amf_confchg_njoin ,NULL );
>>+		enumerateComponents ( amf_confchg_nsync ,NULL );
>>+	}
>>+
>>+        /*
>>+         * If node leave , component unregister 
>>+         */
>>+        size = sizeof((member_list->sin_addr));
>>+        if ( memcmp( &(member_list->sin_addr), &(this_ip.sin_addr.s_addr) ,size ) ){
>>+		return(0);
>>+        }
>>+
>>+        for ( i = 0; i<left_list_entries ; i++ ) {
>>+		data = ( void *)&(left_list->sin_addr); left_list ++;
>>+		enumerateComponents ( amf_confchg_nleave ,data );
>>+        }
>>+
>>    
>>
>the above construct is a little complicated with the pointer addition.
>could you use instead somethign like:
>enumerateComponent (amf_confchg_nleave, left_list[i].sin_addr);
>
OK .

>>+        return (0);
>>+}
>>+
>> int amf_exit_fn (struct conn_info *conn_info)
>> {
>> 	/*
>>@@ -1647,7 +1767,6 @@
>> 	return (0);
>> }
>> 
>>-
>> static int message_handler_req_exec_amf_componentregister (void *message, struct in_addr source_addr)
>> {
>> 	struct req_exec_amf_componentregister *req_exec_amf_componentregister = (struct req_exec_amf_componentregister *)message;
>>@@ -1700,6 +1819,7 @@
>> 		component->local = 0;
>> 		component->registered = 1;
>> 		component->conn_info = req_exec_amf_componentregister->source.conn_info;
>>+		component->source_addr = source_addr;
>> 		component->currentReadinessState = SA_AMF_OUT_OF_SERVICE;
>> 		component->newReadinessState = SA_AMF_OUT_OF_SERVICE;
>> 		component->currentHAState = SA_AMF_QUIESCED;
>>@@ -1916,6 +2036,7 @@
>> 			req_exec_amf_readinessstateset->readinessState);
>> 
>> 		component->currentReadinessState = req_exec_amf_readinessstateset->readinessState;
>>+		component->newReadinessState     = component->currentReadinessState;
>> 		dsm (component);
>> 	}
>> 	
>>@@ -1939,6 +2060,7 @@
>> 			getSaNameT (&component->name),
>> 			req_exec_amf_hastateset->haState);
>> 		component->currentHAState = req_exec_amf_hastateset->haState;
>>+		component->newHAState     = component->currentHAState;
>> 		dsm (component);
>> 	}
>> 	
>>diff -uNr openais.2004-09-09.21.30/exec/parse.h openais/exec/parse.h
>>--- openais.2004-09-09.21.30/exec/parse.h	2004-09-10 13:30:02.000000000 +0900
>>+++ openais/exec/parse.h	2004-09-11 10:42:04.000000000 +0900
>>@@ -130,6 +130,7 @@
>> 	int registered;
>> 	int local;
>> 	struct conn_info *conn_info;
>>+	struct in_addr source_addr;
>> 	SaNameT name;
>> 	SaAmfReadinessStateT currentReadinessState;
>> 	SaAmfReadinessStateT newReadinessState;
>>    
>>
>
>Sakai-san
>looks good.  If we can fix up the coding style issues and the one
>strange code segment, we are ready to merge.
>  
>
Thank you very much for your advice .
- Miyotaka sakai

>Thanks
>-steve
>  
>





More information about the Openais mailing list