ospfd: remove commands for broken GR helper mode

Issue #9983 explains what is wrong with the GR helper mode.

To unblock the CI that fails almost all the time on the ospf_gr_topo1
test, remove the commands and disable the test. Also add a reminder to
completely remove the helper mode if no one fixes the code in a month.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>

ospfd/ospf_vty.c

@@ -9863,161 +9863,10 @@ DEFUN (no_ospf_proactive_arp,
 	return CMD_SUCCESS;
 }
 
-/* Graceful Restart HELPER Commands */
+#if CONFDATE > 20211209
-DEFPY(ospf_gr_helper_enable, ospf_gr_helper_enable_cmd,
+CPP_NOTICE("Time to remove broken OSPF GR helper")
-      "graceful-restart helper enable [A.B.C.D$address]",
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Enable Helper support\n"
-      "Advertising Router-ID\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	if (address_str) {
-		ospf_gr_helper_support_set_per_routerid(ospf, &address,
-							OSPF_GR_TRUE);
-		return CMD_SUCCESS;
-	}
-
-	ospf_gr_helper_support_set(ospf, OSPF_GR_TRUE);
-
-	return CMD_SUCCESS;
-}
-
-DEFPY(no_ospf_gr_helper_enable,
-      no_ospf_gr_helper_enable_cmd,
-      "no graceful-restart helper enable [A.B.C.D$address]",
-      NO_STR
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Enable Helper support\n"
-      "Advertising Router-ID\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	if (address_str) {
-		ospf_gr_helper_support_set_per_routerid(ospf, &address,
-							OSPF_GR_FALSE);
-		return CMD_SUCCESS;
-	}
-
-	ospf_gr_helper_support_set(ospf, OSPF_GR_FALSE);
-	return CMD_SUCCESS;
-}
-
-#if CONFDATE > 20220921
-CPP_NOTICE(
-	"Time to remove the deprecated \"[no] graceful-restart helper-only\" commands")
 #endif
 
-DEFPY_HIDDEN(ospf_gr_helper_only, ospf_gr_helper_only_cmd,
-      "graceful-restart helper-only [A.B.C.D]",
-      "OSPF Graceful Restart\n"
-      "Enable Helper support\n"
-      "Advertising router id\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-	struct in_addr addr;
-	int ret;
-
-	vty_out(vty,
-		"%% This command is deprecated. Please, use `graceful-restart helper enable` instead.\n");
-
-	if (argc == 3) {
-		ret = inet_aton(argv[2]->arg, &addr);
-		if (!ret) {
-			vty_out(vty,
-				"Please specify the valid routerid address.\n");
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-
-		ospf_gr_helper_support_set_per_routerid(ospf, &addr, OSPF_GR_TRUE);
-		return CMD_SUCCESS;
-	}
-
-	ospf_gr_helper_support_set(ospf, OSPF_GR_TRUE);
-
-	return CMD_SUCCESS;
-}
-
-ALIAS_HIDDEN(no_ospf_gr_helper_enable,
-      no_ospf_gr_helper_only_cmd,
-      "no graceful-restart helper-only [A.B.C.D]",
-      NO_STR
-      "OSPF Graceful Restart\n"
-      "Disable Helper support\n"
-      "Advertising router id\n")
-
-DEFPY(ospf_gr_helper_enable_lsacheck,
-      ospf_gr_helper_enable_lsacheck_cmd,
-      "graceful-restart helper strict-lsa-checking",
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Enable strict LSA check\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	ospf_gr_helper_lsa_check_set(ospf, OSPF_GR_TRUE);
-	return CMD_SUCCESS;
-}
-
-DEFPY(no_ospf_gr_helper_enable_lsacheck,
-      no_ospf_gr_helper_enable_lsacheck_cmd,
-      "no graceful-restart helper strict-lsa-checking",
-      NO_STR
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Disable strict LSA check\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	ospf_gr_helper_lsa_check_set(ospf, OSPF_GR_FALSE);
-	return CMD_SUCCESS;
-}
-
-DEFPY(ospf_gr_helper_supported_grace_time,
-      ospf_gr_helper_supported_grace_time_cmd,
-      "graceful-restart helper supported-grace-time (10-1800)$interval",
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Supported grace timer\n"
-      "Grace interval(in seconds)\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	ospf_gr_helper_supported_gracetime_set(ospf, interval);
-	return CMD_SUCCESS;
-}
-
-DEFPY(no_ospf_gr_helper_supported_grace_time,
-      no_ospf_gr_helper_supported_grace_time_cmd,
-      "no graceful-restart helper supported-grace-time (10-1800)$interval",
-      NO_STR
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Supported grace timer\n"
-      "Grace interval(in seconds)\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	ospf_gr_helper_supported_gracetime_set(ospf, OSPF_MAX_GRACE_INTERVAL);
-	return CMD_SUCCESS;
-}
-
-DEFPY(ospf_gr_helper_planned_only,
-      ospf_gr_helper_planned_only_cmd,
-      "graceful-restart helper planned-only",
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Supported only planned restart\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	ospf_gr_helper_set_supported_planned_only_restart(ospf, OSPF_GR_TRUE);
-
-	return CMD_SUCCESS;
-}
-
 /* External Route Aggregation */
 DEFUN (ospf_external_route_aggregation,
        ospf_external_route_aggregation_cmd,
@@ -10100,256 +9949,6 @@ DEFUN (no_ospf_external_route_aggregation,
 	return CMD_SUCCESS;
 }
 
-DEFPY(no_ospf_gr_helper_planned_only,
-      no_ospf_gr_helper_planned_only_cmd,
-      "no graceful-restart helper planned-only",
-      NO_STR
-      "OSPF Graceful Restart\n"
-      "OSPF GR Helper\n"
-      "Supported only for planned restart\n")
-{
-	VTY_DECLVAR_INSTANCE_CONTEXT(ospf, ospf);
-
-	ospf_gr_helper_set_supported_planned_only_restart(ospf, OSPF_GR_FALSE);
-
-	return CMD_SUCCESS;
-}
-
-static int ospf_print_vty_helper_dis_rtr_walkcb(struct hash_bucket *bucket,
-						void *arg)
-{
-	struct advRtr *rtr = bucket->data;
-	struct vty *vty = (struct vty *)arg;
-	static unsigned int count;
-
-	vty_out(vty, "%-6pI4,", &rtr->advRtrAddr);
-	count++;
-
-	if (count % 5 == 0)
-		vty_out(vty, "\n");
-
-	return HASHWALK_CONTINUE;
-}
-
-static int ospf_show_gr_helper_details(struct vty *vty, struct ospf *ospf,
-				       uint8_t use_vrf, json_object *json,
-				       bool uj, bool detail)
-{
-	struct listnode *node;
-	struct ospf_interface *oi;
-	char buf[PREFIX_STRLEN];
-	json_object *json_vrf = NULL;
-
-	if (uj) {
-		if (use_vrf)
-			json_vrf = json_object_new_object();
-		else
-			json_vrf = json;
-	}
-
-	if (ospf->instance) {
-		if (uj)
-			json_object_int_add(json, "ospfInstance",
-					    ospf->instance);
-		else
-			vty_out(vty, "\nOSPF Instance: %d\n\n", ospf->instance);
-	}
-
-	ospf_show_vrf_name(ospf, vty, json_vrf, use_vrf);
-
-	if (uj) {
-		if (use_vrf)
-			json_object_object_add(json, ospf_get_name(ospf),
-					       json_vrf);
-	} else
-		vty_out(vty, "\n");
-
-	/* Show Router ID. */
-	if (uj) {
-		json_object_string_add(json_vrf, "routerId",
-				       inet_ntop(AF_INET, &ospf->router_id,
-						 buf, sizeof(buf)));
-	} else {
-		vty_out(vty, "\n       OSPF Router with ID (%pI4)\n\n",
-			&ospf->router_id);
-	}
-
-	if (!uj) {
-
-		if (ospf->is_helper_supported)
-			vty_out(vty,
-				" Graceful restart helper support enabled.\n");
-		else
-			vty_out(vty,
-				" Graceful restart helper support disabled.\n");
-
-		if (ospf->strict_lsa_check)
-			vty_out(vty, " Strict LSA check is enabled.\n");
-		else
-			vty_out(vty, " Strict LSA check is disabled.\n");
-
-		if (ospf->only_planned_restart)
-			vty_out(vty,
-				" Helper supported for planned restarts only.\n");
-		else
-			vty_out(vty,
-				" Helper supported for Planned and Unplanned Restarts.\n");
-
-		vty_out(vty,
-			" Supported Graceful restart interval: %d(in seconds).\n",
-			ospf->supported_grace_time);
-
-		if (OSPF_HELPER_ENABLE_RTR_COUNT(ospf)) {
-			vty_out(vty, " Enable Router list:\n");
-			vty_out(vty, "   ");
-			hash_walk(ospf->enable_rtr_list,
-				  ospf_print_vty_helper_dis_rtr_walkcb, vty);
-			vty_out(vty, "\n\n");
-		}
-
-		if (ospf->last_exit_reason != OSPF_GR_HELPER_EXIT_NONE) {
-			vty_out(vty, " Last Helper exit Reason :%s\n",
-				ospf_exit_reason2str(ospf->last_exit_reason));
-		}
-
-		if (ospf->active_restarter_cnt)
-			vty_out(vty,
-				" Number of Active neighbours in graceful restart: %d\n",
-				ospf->active_restarter_cnt);
-		else
-			vty_out(vty, "\n");
-
-	} else {
-		json_object_string_add(
-			json_vrf, "helperSupport",
-			(ospf->is_helper_supported) ? "Enabled" : "Disabled");
-		json_object_string_add(json_vrf, "strictLsaCheck",
-				       (ospf->strict_lsa_check) ? "Enabled"
-								: "Disabled");
-		json_object_string_add(
-			json_vrf, "restartSupoort",
-			(ospf->only_planned_restart)
-				? "Planned Restart only"
-				: "Planned and Unplanned Restarts");
-
-		json_object_int_add(json_vrf, "supportedGracePeriod",
-				    ospf->supported_grace_time);
-
-		if (ospf->last_exit_reason != OSPF_GR_HELPER_EXIT_NONE)
-			json_object_string_add(
-				json_vrf, "LastExitReason",
-				ospf_exit_reason2str(ospf->last_exit_reason));
-
-		if (ospf->active_restarter_cnt)
-			json_object_int_add(json_vrf, "activeRestarterCnt",
-					    ospf->active_restarter_cnt);
-	}
-
-
-	if (detail) {
-		int cnt = 1;
-		json_object *json_neighbors = NULL;
-
-		for (ALL_LIST_ELEMENTS_RO(ospf->oiflist, node, oi)) {
-			struct route_node *rn;
-			struct ospf_neighbor *nbr;
-			json_object *json_neigh;
-
-			if (ospf_interface_neighbor_count(oi) == 0)
-				continue;
-
-			if (uj) {
-				json_object_object_get_ex(json_vrf, "Neighbors",
-							  &json_neighbors);
-				if (!json_neighbors) {
-					json_neighbors =
-						json_object_new_object();
-					json_object_object_add(json_vrf,
-							       "Neighbors",
-							       json_neighbors);
-				}
-			}
-
-			for (rn = route_top(oi->nbrs); rn;
-			     rn = route_next(rn)) {
-
-				if (!rn->info)
-					continue;
-
-				nbr = rn->info;
-
-				if (!OSPF_GR_IS_ACTIVE_HELPER(nbr))
-					continue;
-
-				if (!uj) {
-					vty_out(vty, " Neighbour %d :\n", cnt);
-					vty_out(vty, "   Address  : %pI4\n",
-						&nbr->address.u.prefix4);
-					vty_out(vty, "   Routerid : %pI4\n",
-						&nbr->router_id);
-					vty_out(vty,
-						"   Received Grace period : %d(in seconds).\n",
-						nbr->gr_helper_info
-							.recvd_grace_period);
-					vty_out(vty,
-						"   Actual Grace period : %d(in seconds)\n",
-						nbr->gr_helper_info
-							.actual_grace_period);
-					vty_out(vty,
-						"   Remaining GraceTime:%ld(in seconds).\n",
-						thread_timer_remain_second(
-							nbr->gr_helper_info
-							.t_grace_timer));
-					vty_out(vty,
-						"   Graceful Restart reason: %s.\n\n",
-						ospf_restart_reason2str(
-							nbr->gr_helper_info
-							.gr_restart_reason));
-					cnt++;
-				} else {
-					json_neigh = json_object_new_object();
-					json_object_string_add(
-						json_neigh, "srcAddr",
-						inet_ntop(AF_INET, &nbr->src,
-							  buf, sizeof(buf)));
-
-					json_object_string_add(
-						json_neigh, "routerid",
-						inet_ntop(AF_INET,
-							  &nbr->router_id,
-							  buf, sizeof(buf)));
-					json_object_int_add(
-						json_neigh,
-						"recvdGraceInterval",
-						nbr->gr_helper_info
-							.recvd_grace_period);
-					json_object_int_add(
-						json_neigh,
-						"actualGraceInterval",
-						nbr->gr_helper_info
-							.actual_grace_period);
-					json_object_int_add(
-						json_neigh, "remainGracetime",
-						thread_timer_remain_second(
-							nbr->gr_helper_info
-							.t_grace_timer));
-					json_object_string_add(
-						json_neigh, "restartReason",
-						ospf_restart_reason2str(
-							nbr->gr_helper_info
-							.gr_restart_reason));
-					json_object_object_add(
-						json_neighbors,
-						inet_ntop(AF_INET, &nbr->src,
-							  buf, sizeof(buf)),
-						json_neigh);
-				}
-			}
-		}
-	}
-	return CMD_SUCCESS;
-}
-
 DEFUN (ospf_external_route_aggregation_no_adrvertise,
        ospf_external_route_aggregation_no_adrvertise_cmd,
        "summary-address A.B.C.D/M no-advertise",
@@ -10438,108 +10037,6 @@ DEFUN (ospf_route_aggregation_timer,
 	return CMD_SUCCESS;
 }
 
-DEFPY (show_ip_ospf_gr_helper,
-       show_ip_ospf_gr_helper_cmd,
-       "show ip ospf [vrf <NAME|all>] graceful-restart helper [detail] [json]",
-       SHOW_STR
-       IP_STR
-       "OSPF information\n"
-       VRF_CMD_HELP_STR
-       "All VRFs\n"
-       "OSPF Graceful Restart\n"
-       "Helper details in the router\n"
-       "Detailed informtion\n"
-       JSON_STR)
-{
-	char *vrf_name = NULL;
-	bool all_vrf = false;
-	int ret = CMD_SUCCESS;
-	int idx_vrf = 0;
-	int idx = 0;
-	uint8_t use_vrf = 0;
-	bool uj = use_json(argc, argv);
-	struct ospf *ospf = NULL;
-	json_object *json = NULL;
-	struct listnode *node = NULL;
-	int inst = 0;
-	bool detail = false;
-
-	OSPF_FIND_VRF_ARGS(argv, argc, idx_vrf, vrf_name, all_vrf);
-
-	if (argv_find(argv, argc, "detail", &idx))
-		detail = true;
-
-	if (uj)
-		json = json_object_new_object();
-
-	/* vrf input is provided */
-	if (vrf_name) {
-		use_vrf = 1;
-
-		if (all_vrf) {
-			for (ALL_LIST_ELEMENTS_RO(om->ospf, node, ospf)) {
-				if (!ospf->oi_running)
-					continue;
-
-				ret = ospf_show_gr_helper_details(
-					vty, ospf, use_vrf, json, uj, detail);
-			}
-
-			if (uj) {
-				vty_out(vty, "%s\n",
-					json_object_to_json_string_ext(
-						json, JSON_C_TO_STRING_PRETTY));
-				json_object_free(json);
-			}
-
-			return ret;
-		}
-
-		ospf = ospf_lookup_by_inst_name(inst, vrf_name);
-
-		if (ospf == NULL || !ospf->oi_running) {
-
-			if (uj) {
-				vty_out(vty, "%s\n",
-					json_object_to_json_string_ext(
-						json, JSON_C_TO_STRING_PRETTY));
-				json_object_free(json);
-			} else
-				vty_out(vty, "%% OSPF instance not found\n");
-
-			return CMD_SUCCESS;
-		}
-
-	} else {
-		/* Default Vrf */
-		ospf = ospf_lookup_by_vrf_id(VRF_DEFAULT);
-
-		if (ospf == NULL || !ospf->oi_running) {
-
-			if (uj) {
-				vty_out(vty, "%s\n",
-					json_object_to_json_string_ext(
-						json, JSON_C_TO_STRING_PRETTY));
-				json_object_free(json);
-			} else
-				vty_out(vty, "%% OSPF instance not found\n");
-
-			return CMD_SUCCESS;
-		}
-
-		ospf_show_gr_helper_details(vty, ospf, use_vrf, json, uj,
-					    detail);
-	}
-
-	if (uj) {
-		vty_out(vty, "%s\n", json_object_to_json_string_ext(
-					     json, JSON_C_TO_STRING_PRETTY));
-		json_object_free(json);
-	}
-
-	return CMD_SUCCESS;
-}
-/* Graceful Restart HELPER commands end */
 DEFUN (no_ospf_route_aggregation_timer,
        no_ospf_route_aggregation_timer_cmd,
        "no aggregation timer",
@@ -12608,9 +12105,6 @@ void ospf_vty_show_init(void)
 	/* "show ip ospf vrfs" commands. */
 	install_element(VIEW_NODE, &show_ip_ospf_vrfs_cmd);
 
-	/* "show ip ospf gr-helper details" command */
-	install_element(VIEW_NODE, &show_ip_ospf_gr_helper_cmd);
-
 	/* "show ip ospf summary-address" command */
 	install_element(VIEW_NODE, &show_ip_ospf_external_aggregator_cmd);
 }
@@ -12721,18 +12215,6 @@ static void ospf_vty_zebra_init(void)
 	install_element(OSPF_NODE, &no_ospf_distance_ospf_cmd);
 	install_element(OSPF_NODE, &ospf_distance_ospf_cmd);
 
-	/*Ospf garcefull restart helper configurations */
-	install_element(OSPF_NODE, &ospf_gr_helper_enable_cmd);
-	install_element(OSPF_NODE, &no_ospf_gr_helper_enable_cmd);
-	install_element(OSPF_NODE, &ospf_gr_helper_only_cmd);
-	install_element(OSPF_NODE, &no_ospf_gr_helper_only_cmd);
-	install_element(OSPF_NODE, &ospf_gr_helper_enable_lsacheck_cmd);
-	install_element(OSPF_NODE, &no_ospf_gr_helper_enable_lsacheck_cmd);
-	install_element(OSPF_NODE, &ospf_gr_helper_supported_grace_time_cmd);
-	install_element(OSPF_NODE, &no_ospf_gr_helper_supported_grace_time_cmd);
-	install_element(OSPF_NODE, &ospf_gr_helper_planned_only_cmd);
-	install_element(OSPF_NODE, &no_ospf_gr_helper_planned_only_cmd);
-
 	/* External LSA summarisation config commands.*/
 	install_element(OSPF_NODE, &ospf_external_route_aggregation_cmd);
 	install_element(OSPF_NODE, &no_ospf_external_route_aggregation_cmd);

tests/topotests/ospf_gr_helper/test_ospf_gr_helper.py

@@ -100,6 +100,8 @@ TC8.    Verify helper functionality when dut is helping RR and new grace lsa
 
 
 def setup_module(mod):
+    return pytest.skip("OSPF GR helper mode is currently broken")
+
     """
     Sets up the pytest environment
 

tests/topotests/ospf_gr_topo1/test_ospf_gr_topo1.py

@@ -139,6 +139,8 @@ def build_topo(tgen):
 
 
 def setup_module(mod):
+    return pytest.skip("OSPF GR helper mode is currently broken")
+
     "Sets up the pytest environment"
     tgen = Topogen(build_topo, mod.__name__)
     tgen.start_topology()