Merge pull request #2571 from pqarmitage/updates

Enhance interface fault flags and resolve VRRP instances not leaving fault state and interface created

Author: pqarmitage

configure.ac

@@ -258,6 +258,8 @@ AC_ARG_ENABLE(malloc-check,
   [AS_HELP_STRING([--enable-malloc-check], [compile with malloc checking - i.e. malloc etc returning NULL])])
 AC_ARG_ENABLE(timer-check,
   [AS_HELP_STRING([--enable-timer-check], [compile with set time logging])])
+AC_ARG_ENABLE(fault-flags-check,
+  [AS_HELP_STRING([--enable-fault-flags-check], [compile with checking VRRP fault flags])])
 AC_ARG_ENABLE(debug,
   [AS_HELP_STRING([--enable-debug], [compile with most debugging options])])
 AC_ARG_ENABLE(netlink-timers,
@@ -621,6 +623,7 @@ AS_IF([test .$enable_debug = .yes],
 	AS_IF([test .$enable_checksum_debug = .], [enable_checksum_debug=yes])
 	AS_IF([test .$enable_netlink_timers = .], [enable_netlink_timers=yes])
 	AS_IF([test .$enable_network_timestamp = .], [enable_network_timestamp=yes])
+	AS_IF([test .$enable_fault_flags_check = .], [enable_fault_flags_check=yes])
       ])
 
     AS_IF([test .$enable_lvs != .no],
@@ -2961,6 +2964,17 @@ if test "${enable_timer_check}" = "yes"; then
   add_config_opt([TIMER_CHECK])
 fi
 
+AS_IF([test .$enable_vrrp != .no],
+  [
+    FAULT_FLAGS_CHECK=No
+    AS_IF([test .${enable_fault_flags_check} = .yes],
+      [
+	FAULT_FLAGS_CHECK=Yes
+	AC_DEFINE([_FAULT_FLAGS_CHECK_], [ 1 ], [Define to 1 to build with vrrp fault flag checking])
+	add_config_opt([FAULT_FLAGS_CHECK])
+     ])
+  ])
+
 dnl ----[ Debug in one process or not ? ]----
 if test "${enable_one_process_debug}" = yes; then
   AC_DEFINE([_ONE_PROCESS_DEBUG_], [ 1 ], [Define to 1 to build with debugging support])
@@ -3641,6 +3655,8 @@ AS_IF([test ${OPENSSL_MEM_CHECK} = Yes],
 if test ${TIMER_CHECK} = Yes; then
   echo "Set time logging         : Yes"
 fi
+AS_IF([test ${FAULT_FLAGS_CHECK} = Yes],
+  [echo "VRRP fault flags checks  : Yes"])
 if test ${ENABLE_DUMP_THREADS} = Yes; then
   echo "Thread debugging         : Yes"
 fi

keepalived/check/libipvs.c

@@ -385,11 +385,17 @@ static int ipvs_getinfo_parse_cb(struct nl_msg *msg, __attribute__((unused)) voi
 }
 #endif
 
-static int ipvs_getinfo(bool retry)
+static int ipvs_getinfo(
+#ifndef LIBIPVS_USE_NL
+		        __attribute__((unused))
+#endif
+						bool retry)
 {
 	socklen_t len;
 	struct ip_vs_getinfo ipvs_info;
+#ifdef LIBIPVS_USE_NL
 	unsigned retries = retry ? 1 : 0;
+#endif
 
 	/* It appears that if we need to install the ip_vs module
 	 * (via keepalived_modprobe), then the first ipvs_nl_send_message()
@@ -431,7 +437,11 @@ static int ipvs_getinfo(bool retry)
 	return getsockopt(sockfd, IPPROTO_IP, IP_VS_SO_GET_INFO, &ipvs_info, &len);
 }
 
-int ipvs_init(bool retry)
+int ipvs_init(
+#ifndef LIBIPVS_USE_NL
+	      __attribute__((unused))
+#endif
+				      bool retry)
 {
 	ipvs_func = ipvs_init;
 

keepalived/core/keepalived_netlink.c

@@ -1010,7 +1010,7 @@ netlink_if_address_filter(__attribute__((unused)) struct sockaddr_nl *snl, struc
 							    vrrp->family == AF_INET ? VRRP_CONFIGURED_IFP(vrrp) :
 #endif
 							    vrrp->ifp) &&
-						    (vrrp->num_track_fault || vrrp->flags_if_fault) &&
+						    vrrp->flags_if_fault &&
 						    vrrp->family == ifa->ifa_family &&
 						    vrrp->saddr.ss_family == AF_UNSPEC &&
 						    (!__test_bit(VRRP_FLAG_SADDR_FROM_CONFIG, &vrrp->flags) || is_tracking_saddr)) {
@@ -1019,7 +1019,7 @@ netlink_if_address_filter(__attribute__((unused)) struct sockaddr_nl *snl, struc
 								inet_ip4tosockaddr(addr.in, &vrrp->saddr);
 							else
 								inet_ip6tosockaddr(addr.in6, &vrrp->saddr);
-							try_up_instance(vrrp, false, false, VRRP_IF_FAULT_FLAG_NO_SOURCE_IP);
+							try_up_instance(vrrp, false, VRRP_FAULT_FL_NO_SOURCE_IP);
 						}
 #ifdef _HAVE_VRRP_VMAC_
 						/* If IPv6 link local and vmac doesn't have an address or we have
@@ -1041,9 +1041,9 @@ netlink_if_address_filter(__attribute__((unused)) struct sockaddr_nl *snl, struc
 									 * does not have one, then we will need the following code
 									 */
 									if (add_link_local_address(vrrp->ifp, addr.in6) &&
-								            (vrrp->num_track_fault || vrrp->flags_if_fault) &&
+								            vrrp->flags_if_fault &&
 									    (!__test_bit(VRRP_FLAG_SADDR_FROM_CONFIG, &vrrp->flags) || is_tracking_saddr))
-										try_up_instance(vrrp, false, false, VRRP_IF_FAULT_FLAG_NO_SOURCE_IP);
+										try_up_instance(vrrp, false, VRRP_FAULT_FL_NO_SOURCE_IP);
 								} else
 #endif
 									reset_link_local_address(&vrrp->ifp->sin6_addr, vrrp);
@@ -1167,7 +1167,7 @@ netlink_if_address_filter(__attribute__((unused)) struct sockaddr_nl *snl, struc
 						}
 						else if (IF_ISUP(ifp)) {
 							/* We failed to add an address, so down the instance */
-							down_instance(vrrp, false, VRRP_IF_FAULT_FLAG_NO_SOURCE_IP);
+							down_instance(vrrp, VRRP_FAULT_FL_NO_SOURCE_IP);
 							vrrp->saddr.ss_family = AF_UNSPEC;
 						}
 					}
@@ -1187,15 +1187,18 @@ netlink_if_address_filter(__attribute__((unused)) struct sockaddr_nl *snl, struc
 						  IF_ISUP(ifp)) &&
 #endif
 						 (!__test_bit(VRRP_FLAG_SADDR_FROM_CONFIG, &vrrp->flags) || is_tracking_saddr)) {
-						/* Don't attempt to send an IPv6 advert if no address on the interface */
+						/* Don't attempt to send an IPv6 advert if no address on the interface, but we can
+						 * for IPv4. So we want to clear the saddr before calling down_instance for IPv6,
+						 * but after for IPv4.
+						 */
 						if (vrrp->saddr.ss_family == AF_INET6
 #ifdef _HAVE_VRRP_VMAC_
 						    && !__test_bit(VRRP_VMAC_XMITBASE_BIT, &vrrp->flags)
 #endif
 											)
 							vrrp->saddr.ss_family = AF_UNSPEC;
 
-						down_instance(vrrp, false, VRRP_IF_FAULT_FLAG_NO_SOURCE_IP);
+						down_instance(vrrp, VRRP_FAULT_FL_NO_SOURCE_IP);
 						vrrp->saddr.ss_family = AF_UNSPEC;
 					}
 				}
@@ -1605,19 +1608,21 @@ process_if_status_change(interface_t *ifp)
 #ifdef _HAVE_VRRP_VMAC_
 			if (__test_bit(VRRP_VMAC_BIT, &vrrp->flags) &&
 					VRRP_CONFIGURED_IFP(vrrp) == ifp)
-				try_up_instance(vrrp, false, false, VRRP_IF_FAULT_FLAG_BASE_INTERFACE_DOWN);
+				try_up_instance(vrrp, false, VRRP_FAULT_FL_BASE_INTERFACE_DOWN);
 			else
 #endif
+			{
 				/* assuming there is only one tracked interface per vrrp : to be checked */
-				try_up_instance(vrrp, false, false, VRRP_IF_FAULT_FLAG_INTERFACE_DOWN);
+				try_up_instance(vrrp, false, VRRP_FAULT_FL_INTERFACE_DOWN);
+			}
 		} else {
 #ifdef _HAVE_VRRP_VMAC_
 			if (__test_bit(VRRP_VMAC_BIT, &vrrp->flags) &&
 					VRRP_CONFIGURED_IFP(vrrp) == ifp)
-				down_instance(vrrp, false, VRRP_IF_FAULT_FLAG_BASE_INTERFACE_DOWN);
+				down_instance(vrrp, VRRP_FAULT_FL_BASE_INTERFACE_DOWN);
 			else
 #endif
-				down_instance(vrrp, false, VRRP_IF_FAULT_FLAG_INTERFACE_DOWN);
+				down_instance(vrrp, VRRP_FAULT_FL_INTERFACE_DOWN);
 		}
 	}
 }
@@ -2214,7 +2219,6 @@ netlink_link_filter(__attribute__((unused)) struct sockaddr_nl *snl, struct nlms
 			if (strcmp(ifp->ifname, name)) {
 				/* The name can change, so handle that here */
 				log_message(LOG_INFO, "Interface name has changed from %s to %s", ifp->ifname, name);
-
 #ifndef _ONE_PROCESS_DEBUG_
 				if (prog_type != PROG_TYPE_VRRP) {
 					ifp->ifi_flags = 0;

keepalived/include/vrrp.h

@@ -271,23 +271,16 @@ typedef struct _unicast_peer_t {
 	list_head_t		e_list;
 } unicast_peer_t;
 
-
-enum vrrp_if_fault_flags_bits {
-	VRRP_IF_FAULT_FLAG_UNSPECIFIED = 0,
-	VRRP_IF_FAULT_FLAG_INTERFACE_DOWN,
+typedef enum vrrp_fault_fl {
+	VRRP_FAULT_FL_TRACKER = 0,
+	VRRP_FAULT_FL_INTERFACE_DOWN,
 #ifdef _HAVE_VRRP_VMAC_
-	VRRP_IF_FAULT_FLAG_BASE_INTERFACE_DOWN,
-	VRRP_IF_FAULT_FLAG_DUPLICATE_VRID,
+	VRRP_FAULT_FL_BASE_INTERFACE_DOWN,
 #endif /* _HAVE_VRRP_VMAC_ */
-	VRRP_IF_FAULT_FLAG_NO_SOURCE_IP,
-	VRRP_IF_FAULT_FLAG_CONFIG_ERROR,
-};
-
-#define VRRP_IF_FAULT_INTERFACE_DOWN 0x1
-#ifdef _HAVE_VRRP_VMAC_
-#define VRRP_IF_FAULT_BASE_INTERFACE_DOWN 0x2
-#endif
-#define VRRP_IF_FAULT_DUPLICATE_VRID 0x4
+	VRRP_FAULT_FL_DUPLICATE_VRID,
+	VRRP_FAULT_FL_NO_SOURCE_IP,
+	VRRP_FAULT_FL_CONFIG_ERROR,
+} vrrp_fault_fl_t;
 
 /* parameters per virtual router -- rfc2338.6.1.2 */
 typedef struct _vrrp_t {
@@ -502,7 +495,7 @@ enum vrrp_packet_status {
 #endif
 #define VRRP_PKT_SADDR(V) (((V)->saddr.ss_family) ? ((struct sockaddr_in *) &(V)->saddr)->sin_addr.s_addr : IF_ADDR(VRRP_CONFIGURED_IFP(V)))
 
-#define VRRP_ISUP(V)		(!(V)->num_track_fault && !(V)->flags_if_fault)
+#define VRRP_ISUP(V)		(!(V)->flags_if_fault)
 
 
 /* Configuration summary flags */

keepalived/include/vrrp_scheduler.h

@@ -72,7 +72,7 @@ extern void vrrp_gratuitous_arp_vmac_update_thread(thread_ref_t);
 #endif
 extern void vrrp_arp_thread(thread_ref_t);
 extern void vrrp_gna_thread(thread_ref_t);
-extern void try_up_instance(vrrp_t *, bool, bool, enum vrrp_if_fault_flags_bits);
+extern void try_up_instance(vrrp_t *, bool, vrrp_fault_fl_t);
 #ifdef _WITH_DUMP_THREADS_
 extern void dump_threads(void);
 #endif

keepalived/include/vrrp_track.h

@@ -204,7 +204,7 @@ extern void alloc_track_bfd(const char *, list_head_t *, const vector_t *);
 #endif
 extern vrrp_script_t *find_script_by_name(const char *) __attribute__ ((pure));
 extern void update_script_priorities(vrrp_script_t *, bool);
-extern void down_instance(struct _vrrp_t *, bool, unsigned);
+extern void down_instance(struct _vrrp_t *, unsigned); 	// last param should be vrrp_fault_fl_t);
 extern void vrrp_set_effective_priority(struct _vrrp_t *);
 extern void initialise_tracking_priorities(void);
 #ifdef _WITH_TRACK_PROCESS_

keepalived/trackers/track_file.c

@@ -709,7 +709,7 @@ process_update_vrrp_track_file_status(const tracked_file_t *tfile, int new_statu
 					    , vrrp->iname, tfile->fname);
 		if (top->weight)
 			vrrp->total_priority -= previous_status;
-		down_instance(vrrp, true, VRRP_IF_FAULT_FLAG_UNSPECIFIED);
+		down_instance(vrrp, VRRP_FAULT_FL_TRACKER);
 	} else if (previous_status == -254) {
 		if (top->weight) {
 			vrrp->total_priority += new_status;
@@ -722,7 +722,7 @@ process_update_vrrp_track_file_status(const tracked_file_t *tfile, int new_statu
 				log_message(LOG_INFO, "(%s) Setting effective priority to %d"
 						    , vrrp->iname, vrrp->effective_priority);
 		}
-		try_up_instance(vrrp, false, true, VRRP_IF_FAULT_FLAG_UNSPECIFIED);
+		try_up_instance(vrrp, false, VRRP_FAULT_FL_TRACKER);
 	} else {
 		vrrp->total_priority += new_status - previous_status;
 		vrrp_set_effective_priority(vrrp);

keepalived/vrrp/vrrp.c

@@ -1585,7 +1585,7 @@ vrrp_send_adv(vrrp_t * vrrp, uint8_t prio)
 {
 	unicast_peer_t *peer;
 
-	if (!vrrp->sockets)
+	if (!vrrp->sockets || vrrp->sockets->fd_out == -1)
 		return;
 
 #ifdef _HAVE_VRRP_VMAC_
@@ -2454,6 +2454,7 @@ add_vrrp_to_interface(vrrp_t *vrrp, interface_t *ifp, int weight, bool reverse,
 {
 	char addr_str[INET6_ADDRSTRLEN];
 	tracking_obj_t *top = NULL;
+	track_t old_type;
 
 	if (list_empty(&ifp->tracking_vrrp)) {
 		if (log_addr && __test_bit(LOG_DETAIL_BIT, &debug)) {
@@ -2468,11 +2469,11 @@ add_vrrp_to_interface(vrrp_t *vrrp, interface_t *ifp, int weight, bool reverse,
 						    , addr_str, ifp->ifname);
 			}
 		}
-	}
-	else if (type != TRACK_VRRP_DYNAMIC) {
+	} else {
 		/* Check if this is already in the list, and adjust the weight appropriately */
 		list_for_each_entry(top, &ifp->tracking_vrrp, e_list) {
 			if (top->obj.vrrp == vrrp) {
+				old_type = top->type;
 				if (top->type & (TRACK_VRRP | TRACK_IF | TRACK_SG) &&
 				    type & (TRACK_VRRP | TRACK_IF | TRACK_SG) &&
 				    top->weight != VRRP_NOT_TRACK_IF &&
@@ -2482,13 +2483,19 @@ add_vrrp_to_interface(vrrp_t *vrrp, interface_t *ifp, int weight, bool reverse,
 
 				/* Update the weight appropriately. We will use the sync group's
 				 * weight unless the vrrp setting is unweighted. */
-				if (top->weight && weight != VRRP_NOT_TRACK_IF) {
+				if (type != TRACK_VRRP_DYNAMIC && top->weight && weight != VRRP_NOT_TRACK_IF) {
 					top->weight = weight;
 					top->weight_multiplier = reverse ? -1 : 1;
 				}
 
 				top->type |= type;
 
+				/* If we have set the dynamic bit, move top to head of list */
+				if (!(old_type & TRACK_VRRP_DYNAMIC) && type == TRACK_VRRP_DYNAMIC) {
+					list_del_init(&top->e_list);
+					list_head_add(&top->e_list, &ifp->tracking_vrrp);
+				}
+
 				return;
 			}
 		}
@@ -2508,9 +2515,6 @@ add_vrrp_to_interface(vrrp_t *vrrp, interface_t *ifp, int weight, bool reverse,
 		list_head_add(&top->e_list, &ifp->tracking_vrrp);
 	else
 		list_add_tail(&top->e_list, &ifp->tracking_vrrp);
-
-	/* if vrrp->num_if_script_fault needs incrementing, it will be
-	 * done in initialise_tracking_priorities() */
 }
 
 void
@@ -2519,23 +2523,34 @@ del_vrrp_from_interface(vrrp_t *vrrp, interface_t *ifp)
 	tracking_obj_t *top, *top_tmp;
 
 	list_for_each_entry_safe(top, top_tmp, &ifp->tracking_vrrp, e_list) {
-		if (top->obj.vrrp == vrrp && top->type == TRACK_VRRP_DYNAMIC) {
+		if (top->obj.vrrp == vrrp && (top->type & TRACK_VRRP_DYNAMIC)) {
 			if (!IF_ISUP(ifp) && !__test_bit(VRRP_FLAG_DONT_TRACK_PRIMARY, &vrrp->flags)) {
 #ifdef _HAVE_VRRP_VMAC_
-					if (__test_bit(VRRP_VMAC_BIT, &vrrp->flags) && VRRP_CONFIGURED_IFP(vrrp) == ifp)
-							__clear_bit(VRRP_IF_FAULT_FLAG_BASE_INTERFACE_DOWN, &vrrp->flags_if_fault);
-					else
+				if (__test_bit(VRRP_VMAC_BIT, &vrrp->flags) && VRRP_CONFIGURED_IFP(vrrp) == ifp)
+					__clear_bit(VRRP_FAULT_FL_BASE_INTERFACE_DOWN, &vrrp->flags_if_fault);
+				else
 #endif
+				{
 					   /* assuming there is only one tracked interface per vrrp : to be checked */
-						__clear_bit(VRRP_IF_FAULT_FLAG_INTERFACE_DOWN, &vrrp->flags_if_fault);
+					__clear_bit(VRRP_FAULT_FL_INTERFACE_DOWN, &vrrp->flags_if_fault);
+				}
 			}
-			free_tracking_obj(top);
-			break;
+
+			top->type &= ~TRACK_VRRP_DYNAMIC;
+
+			if (!top->type)
+				free_tracking_obj(top);
+			else {
+				list_del_init(&top->e_list);
+				list_add_tail(&top->e_list, &ifp->tracking_vrrp);
+			}
+
+			return;
 		}
 
 		/* The dynamic entries are at the start of the list */
-		if (top->type != TRACK_VRRP_DYNAMIC)
-			break;
+		if (!(top->type & TRACK_VRRP_DYNAMIC))
+			return;
 	}
 }
 
@@ -2761,6 +2776,7 @@ open_sockpool_socket(sock_t *sock)
 	vrrp_t *vrrp;
 	sockaddr_t unicast_src;
 	const sockaddr_t *unicast_src_p = sock->unicast_src;
+	bool already_fault;
 
 	if (sock->unicast_src &&
 	    sock->unicast_src->ss_family == AF_INET6 &&
@@ -2783,8 +2799,9 @@ open_sockpool_socket(sock_t *sock)
 		rb_for_each_entry(vrrp, &sock->rb_vrid, rb_vrid) {
 			if (vrrp->state != VRRP_STATE_FAULT)
 				log_message(LOG_INFO, "(%s): entering FAULT state (src address not configured)", vrrp->iname);
-			down_instance(vrrp, false, VRRP_IF_FAULT_FLAG_NO_SOURCE_IP);
-			if ((__num_bit(&vrrp->flags_if_fault) + vrrp->num_track_fault) == 1)
+			already_fault = vrrp->flags_if_fault;
+			down_instance(vrrp, VRRP_FAULT_FL_NO_SOURCE_IP);
+			if (!already_fault)
 				send_instance_notifies(vrrp);
 		}
 		sock->fd_in = -1;
@@ -4904,7 +4921,7 @@ vrrp_complete_init(void)
 	 * We therefore need to clear num_track_fault and flags_if_fault here. */
 	list_for_each_entry(vrrp, &vrrp_data->vrrp, e_list) {
 		if (vrrp->num_config_faults)
-			__set_bit(VRRP_IF_FAULT_FLAG_CONFIG_ERROR, &vrrp->flags_if_fault);
+			__set_bit(VRRP_FAULT_FL_CONFIG_ERROR, &vrrp->flags_if_fault);
 		else {
 			vrrp->num_track_fault = 0;
 			vrrp->flags_if_fault = 0;