ACK
Fabio
On 01/21/2015 04:05 PM, Jan Friesse wrote:
> When config file is reloaded with removed UDPU member, internal icmap
> index of nodelist.node can change. This can result in removal and then
> adding back node. This, with UDPU alive filtering (where member is by
> default considered as not a member) makes corosync not sending messages
> to such members resulting in new membership creation.
>
> Solution is to properly test which members were really deleted and added
> (instead of relying on internal and dynamic naming of icmap hash table
> key name).
>
> Also trully dynamic add and remove node (via cmap) is now handled by
> same function so totem_config->interfaces is now updated properly.
>
> Signed-off-by: Jan Friesse <jfriesse@xxxxxxxxxx>
> ---
> exec/main.c | 67 -------------------------
> exec/totemconfig.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 134 insertions(+), 70 deletions(-)
>
> diff --git a/exec/main.c b/exec/main.c
> index 85c74ee..0ca5634 100644
> --- a/exec/main.c
> +++ b/exec/main.c
> @@ -583,71 +583,6 @@ static void corosync_totem_stats_updater (void *data)
> &corosync_stats_timer_handle);
> }
>
> -static void totem_dynamic_notify(
> - int32_t event,
> - const char *key_name,
> - struct icmap_notify_value new_val,
> - struct icmap_notify_value old_val,
> - void *user_data)
> -{
> - int res;
> - unsigned int ring_no;
> - unsigned int member_no;
> - struct totem_ip_address member;
> - int add_new_member = 0;
> - int remove_old_member = 0;
> - char tmp_str[ICMAP_KEYNAME_MAXLEN];
> -
> - res = sscanf(key_name, "nodelist.node.%u.ring%u%s", &member_no, &ring_no, tmp_str);
> - if (res != 3)
> - return ;
> -
> - if (strcmp(tmp_str, "_addr") != 0) {
> - return;
> - }
> -
> - if (event == ICMAP_TRACK_ADD && new_val.type == ICMAP_VALUETYPE_STRING) {
> - add_new_member = 1;
> - }
> -
> - if (event == ICMAP_TRACK_DELETE && old_val.type == ICMAP_VALUETYPE_STRING) {
> - remove_old_member = 1;
> - }
> -
> - if (event == ICMAP_TRACK_MODIFY && new_val.type == ICMAP_VALUETYPE_STRING &&
> - old_val.type == ICMAP_VALUETYPE_STRING) {
> - add_new_member = 1;
> - remove_old_member = 1;
> - }
> -
> - if (remove_old_member) {
> - log_printf(LOGSYS_LEVEL_DEBUG,
> - "removing dynamic member %s for ring %u", (char *)old_val.data, ring_no);
> - if (totemip_parse(&member, (char *)old_val.data, ip_version) == 0) {
> - totempg_member_remove (&member, ring_no);
> - }
> - }
> -
> - if (add_new_member) {
> - log_printf(LOGSYS_LEVEL_DEBUG,
> - "adding dynamic member %s for ring %u", (char *)new_val.data, ring_no);
> - if (totemip_parse(&member, (char *)new_val.data, ip_version) == 0) {
> - totempg_member_add (&member, ring_no);
> - }
> - }
> -}
> -
> -static void corosync_totem_dynamic_init (void)
> -{
> - icmap_track_t icmap_track = NULL;
> -
> - icmap_track_add("nodelist.node.",
> - ICMAP_TRACK_ADD | ICMAP_TRACK_DELETE | ICMAP_TRACK_MODIFY | ICMAP_TRACK_PREFIX,
> - totem_dynamic_notify,
> - NULL,
> - &icmap_track);
> -}
> -
> static void corosync_totem_stats_init (void)
> {
> icmap_set_uint32("runtime.totem.pg.mrp.srp.mtt_rx_token", 0);
> @@ -660,7 +595,6 @@ static void corosync_totem_stats_init (void)
> &corosync_stats_timer_handle);
> }
>
> -
> static void deliver_fn (
> unsigned int nodeid,
> const void *msg,
> @@ -1093,7 +1027,6 @@ static void main_service_ready (void)
> cs_ipcs_init();
> corosync_totem_stats_init ();
> corosync_fplay_control_init ();
> - corosync_totem_dynamic_init ();
> sync_init (
> corosync_sync_callbacks_retrieve,
> corosync_sync_completed);
> diff --git a/exec/totemconfig.c b/exec/totemconfig.c
> index 2acee2a..b678752 100644
> --- a/exec/totemconfig.c
> +++ b/exec/totemconfig.c
> @@ -532,7 +532,73 @@ static int find_local_node_in_nodelist(struct totem_config *totem_config)
> return (local_node_pos);
> }
>
> -static void put_nodelist_members_to_config(struct totem_config *totem_config)
> +/*
> + * Compute difference between two set of totem interface arrays. set1 and set2
> + * are changed so for same ring, ip existing in both set1 and set2 are cleared
> + * (set to 0), and ips which are only in set1 or set2 remains untouched.
> + * totempg_node_add/remove is called.
> + */
> +static void compute_interfaces_diff(int interface_count,
> + struct totem_interface *set1,
> + struct totem_interface *set2)
> +{
> + int ring_no, set1_pos, set2_pos;
> + struct totem_ip_address empty_ip_address;
> +
> + memset(&empty_ip_address, 0, sizeof(empty_ip_address));
> +
> + for (ring_no = 0; ring_no < interface_count; ring_no++) {
> + for (set1_pos = 0; set1_pos < set1[ring_no].member_count; set1_pos++) {
> + for (set2_pos = 0; set2_pos < set2[ring_no].member_count; set2_pos++) {
> + /*
> + * For current ring_no remove all set1 items existing
> + * in set2
> + */
> + if (memcmp(&set1[ring_no].member_list[set1_pos],
> + &set2[ring_no].member_list[set2_pos],
> + sizeof(struct totem_ip_address)) == 0) {
> + memset(&set1[ring_no].member_list[set1_pos], 0,
> + sizeof(struct totem_ip_address));
> + memset(&set2[ring_no].member_list[set2_pos], 0,
> + sizeof(struct totem_ip_address));
> + }
> + }
> + }
> + }
> +
> + for (ring_no = 0; ring_no < interface_count; ring_no++) {
> + for (set1_pos = 0; set1_pos < set1[ring_no].member_count; set1_pos++) {
> + /*
> + * All items which remained in set1 doesn't exists in set2 any longer so
> + * node has to be removed.
> + */
> + if (memcmp(&set1[ring_no].member_list[set1_pos], &empty_ip_address, sizeof(empty_ip_address)) != 0) {
> + log_printf(LOGSYS_LEVEL_DEBUG,
> + "removing dynamic member %s for ring %u",
> + totemip_print(&set1[ring_no].member_list[set1_pos]),
> + ring_no);
> +
> + totempg_member_remove(&set1[ring_no].member_list[set1_pos], ring_no);
> + }
> + }
> + for (set2_pos = 0; set2_pos < set2[ring_no].member_count; set2_pos++) {
> + /*
> + * All items which remained in set2 doesn't existed in set1 so this is no node
> + * and has to be added.
> + */
> + if (memcmp(&set2[ring_no].member_list[set2_pos], &empty_ip_address, sizeof(empty_ip_address)) != 0) {
> + log_printf(LOGSYS_LEVEL_DEBUG,
> + "adding dynamic member %s for ring %u",
> + totemip_print(&set2[ring_no].member_list[set2_pos]),
> + ring_no);
> +
> + totempg_member_add(&set2[ring_no].member_list[set2_pos], ring_no);
> + }
> + }
> + }
> +}
> +
> +static void put_nodelist_members_to_config(struct totem_config *totem_config, int reload)
> {
> icmap_iter_t iter, iter2;
> const char *iter_key, *iter_key2;
> @@ -544,6 +610,22 @@ static void put_nodelist_members_to_config(struct totem_config *totem_config)
> int member_count;
> unsigned int ringnumber = 0;
> int i, j;
> + struct totem_interface *orig_interfaces = NULL;
> + struct totem_interface *new_interfaces = NULL;
> +
> + if (reload) {
> + /*
> + * We need to compute diff only for reload. Also for initial configuration
> + * not all totem structures are initialized so corosync will crash during
> + * member_add/remove
> + */
> + orig_interfaces = malloc (sizeof (struct totem_interface) * INTERFACE_MAX);
> + assert(orig_interfaces != NULL);
> + new_interfaces = malloc (sizeof (struct totem_interface) * INTERFACE_MAX);
> + assert(new_interfaces != NULL);
> +
> + memcpy(orig_interfaces, totem_config->interfaces, sizeof (struct totem_interface) * INTERFACE_MAX);
> + }
>
> /* Clear out nodelist so we can put the new one in if needed */
> for (i = 0; i < totem_config->interface_count; i++) {
> @@ -590,8 +672,51 @@ static void put_nodelist_members_to_config(struct totem_config *totem_config)
> }
>
> icmap_iter_finalize(iter);
> +
> + if (reload) {
> + memcpy(new_interfaces, totem_config->interfaces, sizeof (struct totem_interface) * INTERFACE_MAX);
> +
> + compute_interfaces_diff(totem_config->interface_count, orig_interfaces, new_interfaces);
> +
> + free(new_interfaces);
> + free(orig_interfaces);
> + }
> +}
> +
> +static void nodelist_dynamic_notify(
> + int32_t event,
> + const char *key_name,
> + struct icmap_notify_value new_val,
> + struct icmap_notify_value old_val,
> + void *user_data)
> +{
> + int res;
> + unsigned int ring_no;
> + unsigned int member_no;
> + char tmp_str[ICMAP_KEYNAME_MAXLEN];
> + uint8_t reloading;
> + struct totem_config *totem_config = (struct totem_config *)user_data;
> +
> + /*
> + * If a full reload is in progress then don't do anything until it's done and
> + * can reconfigure it all atomically
> + */
> + if (icmap_get_uint8("config.totemconfig_reload_in_progress", &reloading) == CS_OK && reloading) {
> + return ;
> + }
> +
> + res = sscanf(key_name, "nodelist.node.%u.ring%u%s", &member_no, &ring_no, tmp_str);
> + if (res != 3)
> + return ;
> +
> + if (strcmp(tmp_str, "_addr") != 0) {
> + return;
> + }
> +
> + put_nodelist_members_to_config(totem_config, 1);
> }
>
> +
> /*
> * Tries to find node (node_pos) in config nodelist which address matches any
> * local interface. Address can be stored in ring0_addr or if ipaddr_key_prefix is not NULL
> @@ -999,7 +1124,7 @@ extern int totem_config_read (
> icmap_set_ro_access("nodelist.local_node_pos", 0, 1);
> }
>
> - put_nodelist_members_to_config(totem_config);
> + put_nodelist_members_to_config(totem_config, 0);
> }
>
> /*
> @@ -1362,7 +1487,7 @@ static void totem_reload_notify(
>
> /* Reload has completed */
> if (*(uint8_t *)new_val.data == 0) {
> - put_nodelist_members_to_config (totem_config);
> + put_nodelist_members_to_config (totem_config, 1);
> totem_volatile_config_read (totem_config, NULL);
> log_printf(LOGSYS_LEVEL_DEBUG, "Configuration reloaded. Dumping actual totem config.");
> debug_dump_totem_config(totem_config);
> @@ -1401,4 +1526,10 @@ static void add_totem_config_notification(struct totem_config *totem_config)
> totem_reload_notify,
> totem_config,
> &icmap_track);
> +
> + icmap_track_add("nodelist.node.",
> + ICMAP_TRACK_ADD | ICMAP_TRACK_DELETE | ICMAP_TRACK_MODIFY | ICMAP_TRACK_PREFIX,
> + nodelist_dynamic_notify,
> + (void *)totem_config,
> + &icmap_track);
> }
>
_______________________________________________
discuss mailing list
discuss@xxxxxxxxxxxx
http://lists.corosync.org/mailman/listinfo/discuss
