isisd: Start Label Manager safer

Initial attempt to connect to the Label Manager used an infinite loop with
a sleep statement which block isisd until Label Manager connection fire up.

This commit changes the way Label Manager connection is established and uses
a `thread_add_timer()` call to re-attempt to establish the connection in case
of failure (zebra or label manager not ready).

New variables are added to the SRDB in order to control the request of SRGB
and SRLB to the Label Manager to start Segment Routing in a safe way.

Signed-off-by: Olivier Dugeon <olivier.dugeon@orange.com>

5 files changed, 174 insertions, 75 deletions

diff --git a/isisd/isis_nb_config.c b/isisd/isis_nb_config.c
index fafa22b49..8a37035c9 100644
--- a/isisd/isis_nb_config.c
+++ b/isisd/isis_nb_config.c
@@ -1433,14 +1433,12 @@ int isis_instance_segment_routing_enabled_modify(
 		if (IS_DEBUG_ISIS(DEBUG_EVENTS))
 			zlog_debug("SR: Segment Routing: OFF -> ON");
 
-		if (isis_sr_start(area) == 0)
-			area->srdb.enabled = true;
+		isis_sr_start(area);
 	} else {
 		if (IS_DEBUG_ISIS(DEBUG_EVENTS))
 			zlog_debug("SR: Segment Routing: ON -> OFF");
 
 		isis_sr_stop(area);
-		area->srdb.enabled = false;
 	}
 
 	return NB_OK;
@@ -1486,21 +1484,12 @@ void isis_instance_segment_routing_srgb_apply_finish(
 {
 	struct isis_area *area;
 	uint32_t lower_bound, upper_bound;
-	int ret;
 
 	area = nb_running_get_entry(args->dnode, NULL, true);
 	lower_bound = yang_dnode_get_uint32(args->dnode, "./lower-bound");
 	upper_bound = yang_dnode_get_uint32(args->dnode, "./upper-bound");
 
-	ret = isis_sr_cfg_srgb_update(area, lower_bound, upper_bound);
-	if (area->srdb.config.enabled) {
-		if (ret == 0)
-			area->srdb.enabled = true;
-		else {
-			isis_sr_stop(area);
-			area->srdb.enabled = false;
-		}
-	}
+	isis_sr_cfg_srgb_update(area, lower_bound, upper_bound);
 }
 
 /*
diff --git a/isisd/isis_sr.c b/isisd/isis_sr.c
index 32de71cca..f27aca991 100644
--- a/isisd/isis_sr.c
+++ b/isisd/isis_sr.c
@@ -142,10 +142,20 @@ int isis_sr_cfg_srgb_update(struct isis_area *area, uint32_t lower_bound,
 	sr_debug("ISIS-Sr (%s): Update SRGB with new range [%u/%u]",
 		 area->area_tag, lower_bound, upper_bound);
 
-	/* First release the old SRGB. */
-	if (srdb->config.enabled)
-		isis_zebra_release_label_range(srdb->config.srgb_lower_bound,
+	/* Just store new SRGB values if Label Manager is not available.
+	 * SRGB will be configured later when SR start */
+	if (!isis_zebra_label_manager_ready()) {
+		srdb->config.srgb_lower_bound = lower_bound;
+		srdb->config.srgb_upper_bound = upper_bound;
+		return 0;
+	}
+
+	/* Label Manager is ready, start by releasing the old SRGB. */
+	if (srdb->srgb_active) {
+	        isis_zebra_release_label_range(srdb->config.srgb_lower_bound,
 					       srdb->config.srgb_upper_bound);
+	        srdb->srgb_active = false;
+	}
 
 	srdb->config.srgb_lower_bound = lower_bound;
 	srdb->config.srgb_upper_bound = upper_bound;
@@ -157,8 +167,12 @@ int isis_sr_cfg_srgb_update(struct isis_area *area, uint32_t lower_bound,
 		if (isis_zebra_request_label_range(
 			    srdb->config.srgb_lower_bound,
 			    srdb->config.srgb_upper_bound
-				    - srdb->config.srgb_lower_bound + 1))
+				    - srdb->config.srgb_lower_bound + 1) < 0) {
+			srdb->srgb_active = false;
 			return -1;
+		} else
+			srdb->srgb_active = true;
+
 
 		sr_debug("  |- Got new SRGB [%u/%u]",
 			 srdb->config.srgb_lower_bound,
@@ -175,8 +189,7 @@ int isis_sr_cfg_srgb_update(struct isis_area *area, uint32_t lower_bound,
 		lsp_regenerate_schedule(area, area->is_type, 0);
 	} else if (srdb->config.enabled) {
 		/* Try to enable SR again using the new SRGB. */
-		if (isis_sr_start(area) == 0)
-			area->srdb.enabled = true;
+		isis_sr_start(area);
 	}
 
 	return 0;
@@ -199,33 +212,39 @@ int isis_sr_cfg_srlb_update(struct isis_area *area, uint32_t lower_bound,
 	struct isis_sr_db *srdb = &area->srdb;
 	struct listnode *node, *nnode;
 	struct sr_adjacency *sra;
-	int rc;
 
 	sr_debug("ISIS-Sr (%s): Update SRLB with new range [%u/%u]",
 		 area->area_tag, lower_bound, upper_bound);
 
-	/* First Delete SRLB */
+	/* Just store new SRLB values if Label Manager is not available.
+	 * SRLB will be configured later when SR start */
+	if (!isis_zebra_label_manager_ready()) {
+		srdb->config.srlb_lower_bound = lower_bound;
+		srdb->config.srlb_upper_bound = upper_bound;
+		return 0;
+	}
+
+	/* LM is ready, start by deleting the old SRLB */
 	sr_local_block_delete(area);
 
 	srdb->config.srlb_lower_bound = lower_bound;
 	srdb->config.srlb_upper_bound = upper_bound;
 
-	if (!srdb->enabled)
-		return 0;
-
-	/* Initialize new SRLB */
-	rc = sr_local_block_init(area);
-	if (rc !=0)
-		return rc;
-
-	/* Reinstall local Adjacency-SIDs with new labels. */
-	for (ALL_LIST_ELEMENTS(area->srdb.adj_sids, node, nnode, sra))
-		sr_adj_sid_update(sra, &srdb->srlb);
+	if (srdb->enabled) {
+		/* Initialize new SRLB */
+		if (sr_local_block_init(area) != 0)
+			return -1;
 
-	/* Update Router Capability */
+		/* Reinstall local Adjacency-SIDs with new labels. */
+		for (ALL_LIST_ELEMENTS(area->srdb.adj_sids, node, nnode, sra))
+			sr_adj_sid_update(sra, &srdb->srlb);
 
-	/* Update and Flood LSP */
-	lsp_regenerate_schedule(area, area->is_type, 0);
+		/* Update and Flood LSP */
+		lsp_regenerate_schedule(area, area->is_type, 0);
+	} else if (srdb->config.enabled) {
+		/* Try to enable SR again using the new SRLB. */
+		isis_sr_start(area);
+	}
 
 	return 0;
 }
@@ -1326,11 +1345,15 @@ static int sr_route_update(struct isis_area *area, struct prefix *prefix,
  *
  * @param area	IS-IS area
  */
-static void sr_local_block_init(struct isis_area *area)
+static int sr_local_block_init(struct isis_area *area)
 {
 	struct isis_sr_db *srdb = &area->srdb;
 	struct sr_local_block *srlb = &srdb->srlb;
 
+	/* Check if SRLB is not already configured */
+	if (srlb->active)
+		return 0;
+
 	/*
 	 * Request SRLB to the label manager. If the allocation fails, return
 	 * an error to disable SR until a new SRLB is successfully allocated.
@@ -1338,8 +1361,10 @@ static void sr_local_block_init(struct isis_area *area)
 	if (isis_zebra_request_label_range(
 		    srdb->config.srlb_lower_bound,
 		    srdb->config.srlb_upper_bound
-			    - srdb->config.srlb_lower_bound + 1))
-		return;
+			    - srdb->config.srlb_lower_bound + 1)) {
+		srlb->active = false;
+		return -1;
+	}
 
 	sr_debug("ISIS-Sr (%s): Got new SRLB [%u/%u]", area->area_tag,
 		 srdb->config.srlb_lower_bound, srdb->config.srlb_upper_bound);
@@ -1354,6 +1379,9 @@ static void sr_local_block_init(struct isis_area *area)
 		srlb->max_block++;
 	srlb->used_mark = XCALLOC(MTYPE_ISIS_SR_INFO,
 				  srlb->max_block * SRLB_BLOCK_SIZE);
+	srlb->active = true;
+
+	return 0;
 }
 
 /**
@@ -1366,6 +1394,10 @@ static void sr_local_block_delete(struct isis_area *area)
 	struct isis_sr_db *srdb = &area->srdb;
 	struct sr_local_block *srlb = &srdb->srlb;
 
+	/* Check if SRLB is not already delete */
+	if (!srlb->active)
+		return;
+
 	sr_debug("ISIS-Sr (%s): Remove SRLB [%u/%u]", area->area_tag,
 		 srlb->start, srlb->end);
 
@@ -1376,7 +1408,7 @@ static void sr_local_block_delete(struct isis_area *area)
 	/* Then reset SRLB structure */
 	if (srlb->used_mark != NULL)
 		XFREE(MTYPE_ISIS_SR_INFO, srlb->used_mark);
-	memset(srlb, 0, sizeof(struct sr_local_block));
+	srlb->active = false;
 }
 
 /**
@@ -2042,6 +2074,26 @@ DEFUN(show_sr_node, show_sr_node_cmd,
 /* --- IS-IS Segment Routing Management function ---------------------------- */
 
 /**
+ * Thread function to re-attempt connection to the Label Manager and thus be
+ * able to start Segment Routing.
+ *
+ * @param start		Thread structure that contains area as argument
+ *
+ * @return		1 on success
+ */
+static int sr_start_label_manager(struct thread *start)
+{
+	struct isis_area *area;
+
+	area = THREAD_ARG(start);
+
+	/* re-attempt to start SR & Label Manager connection */
+	isis_sr_start(area);
+
+	return 1;
+}
+
+/**
  * Enable SR on the given IS-IS area.
  *
  * @param area	IS-IS area
@@ -2054,19 +2106,35 @@ int isis_sr_start(struct isis_area *area)
 	struct isis_circuit *circuit;
 	struct listnode *node;
 
-	/* Initialize the SRLB */
-	if (sr_local_block_init(area) != 0)
+	/* First start Label Manager if not ready */
+	if (!isis_zebra_label_manager_ready())
+		if (isis_zebra_label_manager_connect() < 0) {
+			/* Re-attempt to connect to Label Manager in 1 sec. */
+			thread_add_timer(master, sr_start_label_manager, area,
+					 1, &srdb->t_start_lm);
+			return -1;
+		}
+
+	/* Label Manager is ready, initialize the SRLB */
+	if (sr_local_block_init(area) < 0)
 		return -1;
 
 	/*
-	 * Request SGRB to the label manager. If the allocation fails, return
-	 * an error to disable SR until a new SRGB is successfully allocated.
+	 * Request SGRB to the label manager if not already active. If the
+	 * allocation fails, return an error to disable SR until a new SRGB
+	 * is successfully allocated.
 	 */
-	if (isis_zebra_request_label_range(
-		    srdb->config.srgb_lower_bound,
-		    srdb->config.srgb_upper_bound
-			    - srdb->config.srgb_lower_bound + 1))
-		return -1;
+	if (!srdb->srgb_active) {
+		if (isis_zebra_request_label_range(
+			    srdb->config.srgb_lower_bound,
+			    srdb->config.srgb_upper_bound
+				    - srdb->config.srgb_lower_bound + 1)
+		    < 0) {
+			srdb->srgb_active = false;
+			return -1;
+		} else
+			srdb->srgb_active = true;
+	}
 
 	sr_debug("ISIS-Sr: Starting Segment Routing for area %s",
 		 area->area_tag);
@@ -2102,6 +2170,8 @@ int isis_sr_start(struct isis_area *area)
 		}
 	}
 
+	area->srdb.enabled = true;
+
 	/* Regenerate LSPs to advertise Segment Routing capabilities. */
 	lsp_regenerate_schedule(area, area->is_type, 0);
 
@@ -2122,6 +2192,9 @@ void isis_sr_stop(struct isis_area *area)
 	sr_debug("ISIS-Sr: Stopping Segment Routing for area %s",
 		 area->area_tag);
 
+	/* Disable any re-attempt to connect to Label Manager */
+	THREAD_TIMER_OFF(srdb->t_start_lm);
+
 	/* Uninstall all local Adjacency-SIDs. */
 	for (ALL_LIST_ELEMENTS(area->srdb.adj_sids, node, nnode, sra))
 		sr_adj_sid_del(sra);
@@ -2136,13 +2209,18 @@ void isis_sr_stop(struct isis_area *area)
 		}
 	}
 
-	/* Release SRGB. */
-	isis_zebra_release_label_range(srdb->config.srgb_lower_bound,
-				       srdb->config.srgb_upper_bound);
+	/* Release SRGB if active. */
+	if (srdb->srgb_active) {
+		isis_zebra_release_label_range(srdb->config.srgb_lower_bound,
+					       srdb->config.srgb_upper_bound);
+		srdb->srgb_active = false;
+	}
 
 	/* Delete SRLB */
 	sr_local_block_delete(area);
 
+	area->srdb.enabled = false;
+
 	/* Regenerate LSPs to advertise that the Node is no more SR enable. */
 	lsp_regenerate_schedule(area, area->is_type, 0);
 }
@@ -2161,7 +2239,6 @@ void isis_sr_area_init(struct isis_area *area)
 
 	/* Initialize Segment Routing Data Base */
 	memset(srdb, 0, sizeof(*srdb));
-	srdb->enabled = false;
 	srdb->adj_sids = list_new();
 
 	for (int level = ISIS_LEVEL1; level <= ISIS_LEVELS; level++) {
diff --git a/isisd/isis_sr.h b/isisd/isis_sr.h
index 45728f1eb..4379a1dcb 100644
--- a/isisd/isis_sr.h
+++ b/isisd/isis_sr.h
@@ -64,6 +64,7 @@ PREDECL_RBTREE_UNIQ(srdb_prefix_cfg)
 
 /* Segment Routing Local Block allocation */
 struct sr_local_block {
+	bool active;
 	uint32_t start;
 	uint32_t end;
 	uint32_t current;
@@ -223,6 +224,9 @@ struct isis_sr_db {
 	/* Global Operational status of Segment Routing. */
 	bool enabled;
 
+	/* Thread timer to start Label Manager */
+	struct thread *t_start_lm;
+
 	/* List of local Adjacency-SIDs. */
 	struct list *adj_sids;
 
@@ -232,8 +236,9 @@ struct isis_sr_db {
 	/* Segment Routing Prefix-SIDs per IS-IS level. */
 	struct srdb_area_prefix_head prefix_sids[ISIS_LEVELS];
 
-	/* Management of SRLB allocation */
+	/* Management of SRLB & SRGB allocation */
 	struct sr_local_block srlb;
+	bool srgb_active;
 
 	/* Area Segment Routing configuration. */
 	struct {
diff --git a/isisd/isis_zebra.c b/isisd/isis_zebra.c
index fa108c363..db0efcaf5 100644
--- a/isisd/isis_zebra.c
+++ b/isisd/isis_zebra.c
@@ -56,8 +56,6 @@
 struct zclient *zclient;
 static struct zclient *zclient_sync;
 
-static void isis_zebra_label_manager_connect(void);
-
 /* Router-id update message from zebra. */
 static int isis_router_id_update_zebra(ZAPI_CALLBACK_ARGS)
 {
@@ -464,6 +462,16 @@ void isis_zebra_redistribute_unset(afi_t afi, int type)
 /* Label Manager Functions */
 
 /**
+ * Check if Label Manager is Ready or not.
+ *
+ * @return	True if Label Manager is ready, False otherwise
+ */
+bool isis_zebra_label_manager_ready(void)
+{
+	return (zclient_sync->sock > 0);
+}
+
+/**
  * Request Label Range to the Label Manager.
  *
  * @param base		base label of the label range to request
@@ -476,8 +484,8 @@ int isis_zebra_request_label_range(uint32_t base, uint32_t chunk_size)
 	int ret;
 	uint32_t start, end;
 
-	if (zclient_sync->sock == -1)
-		isis_zebra_label_manager_connect();
+	if (zclient_sync->sock < 0)
+		return -1;
 
 	ret = lm_get_label_chunk(zclient_sync, 0, base, chunk_size, &start,
 				 &end);
@@ -494,47 +502,63 @@ int isis_zebra_request_label_range(uint32_t base, uint32_t chunk_size)
  *
  * @param start		start of label range to release
  * @param end		end of label range to release
+ *
+ * @return		0 on success, -1 otherwise
  */
-void isis_zebra_release_label_range(uint32_t start, uint32_t end)
+int isis_zebra_release_label_range(uint32_t start, uint32_t end)
 {
 	int ret;
 
-	if (zclient_sync->sock == -1)
-		isis_zebra_label_manager_connect();
+	if (zclient_sync->sock < 0)
+		return -1;
 
 	ret = lm_release_label_chunk(zclient_sync, start, end);
-	if (ret < 0)
+	if (ret < 0) {
 		zlog_warn("%s: error releasing label range!", __func__);
+		return -1;
+	}
+
+	return 0;
 }
 
 /**
  * Connect to the Label Manager.
+ *
+ * @return	0 on success, -1 otherwise
  */
-static void isis_zebra_label_manager_connect(void)
+int isis_zebra_label_manager_connect(void)
 {
 	/* Connect to label manager. */
-	while (zclient_socket_connect(zclient_sync) < 0) {
-		zlog_warn("%s: re-attempt connecting synchronous zclient!",
+	if (zclient_socket_connect(zclient_sync) < 0) {
+		zlog_warn("%s: failed connecting synchronous zclient!",
 			  __func__);
-		sleep(1);
+		return -1;
 	}
 	/* make socket non-blocking */
 	set_nonblocking(zclient_sync->sock);
 
 	/* Send hello to notify zebra this is a synchronous client */
-	while (zclient_send_hello(zclient_sync) < 0) {
-		zlog_warn(
-			"%s: re-attempt sending hello for synchronous zclient!",
-			__func__);
-		sleep(1);
+	if (zclient_send_hello(zclient_sync) < 0) {
+		zlog_warn("%s: failed sending hello for synchronous zclient!",
+			  __func__);
+		close(zclient_sync->sock);
+		zclient_sync->sock = -1;
+		return -1;
 	}
 
 	/* Connect to label manager */
-	while (lm_label_manager_connect(zclient_sync, 0) != 0) {
-		zlog_warn("%s: re-attempt connecting to label manager!",
-			  __func__);
-		sleep(1);
+	if (lm_label_manager_connect(zclient_sync, 0) != 0) {
+		zlog_warn("%s: failed connecting to label manager!", __func__);
+		if (zclient_sync->sock > 0) {
+			close(zclient_sync->sock);
+			zclient_sync->sock = -1;
+		}
+		return -1;
 	}
+
+	sr_debug("ISIS-Sr: Successfully connected to the Label Manager");
+
+	return 0;
 }
 
 static void isis_zebra_connected(struct zclient *zclient)
@@ -572,6 +596,8 @@ void isis_zebra_init(struct thread_master *master, int instance)
 
 void isis_zebra_stop(void)
 {
+	zclient_stop(zclient_sync);
+	zclient_free(zclient_sync);
 	zclient_stop(zclient);
 	zclient_free(zclient);
 	frr_fini();
diff --git a/isisd/isis_zebra.h b/isisd/isis_zebra.h
index e853ce34d..fd1c1df61 100644
--- a/isisd/isis_zebra.h
+++ b/isisd/isis_zebra.h
@@ -49,7 +49,9 @@ void isis_zebra_send_adjacency_sid(int cmd, const struct sr_adjacency *sra);
 int isis_distribute_list_update(int routetype);
 void isis_zebra_redistribute_set(afi_t afi, int type);
 void isis_zebra_redistribute_unset(afi_t afi, int type);
+bool isis_zebra_label_manager_ready(void);
+int isis_zebra_label_manager_connect(void);
 int isis_zebra_request_label_range(uint32_t base, uint32_t chunk_size);
-void isis_zebra_release_label_range(uint32_t start, uint32_t end);
+int isis_zebra_release_label_range(uint32_t start, uint32_t end);
 
 #endif /* _ZEBRA_ISIS_ZEBRA_H */