Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isisd: Update IS-IS SR Label Manager #6254

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

odd22
Copy link
Member

@odd22 odd22 commented Apr 17, 2020

This change modify the way IS-IS is connected to the Label Manager:

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

This change modify the way IS-IS is connected to the Label Manager:
 - Add emission of Hello Message prior to the connection as per
   modification introduced by PR FRRouting#5925
 - Add 'session_id' as per modification introduced by PR FRRouting#6224
 - Add Doxygen documentation to Label Manager functions

Signed-off-by: Olivier Dugeon <olivier.dugeon@orange.com>
@odd22 odd22 added the isis label Apr 17, 2020
Copy link

@polychaeta polychaeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution to FRR!

Click for style suggestions

To apply these suggestions:

curl -s https://gist.githubusercontent.com/polychaeta/7cefef416650cc8ee5e266569aa83ebb/raw/1b6a7161731d8310b676703e6d0f4c2ca358faac/cr_6254_1587145468.diff | git apply

diff --git a/isisd/isis_zebra.c b/isisd/isis_zebra.c
index d4b9d2a62..dc8b64850 100644
--- a/isisd/isis_zebra.c
+++ b/isisd/isis_zebra.c
@@ -617,14 +617,16 @@ static void isis_zebra_label_manager_connect(void)
 
 	/* Connect to label manager */
 	while (lm_label_manager_connect(zclient_sync, 0) != 0) {
-		zlog_warn("%s: re-attempt connecting to label manager!", __func__);
+		zlog_warn("%s: re-attempt connecting to label manager!",
+			  __func__);
 		sleep(1);
 	}
 
 	label_chunk_list = list_new();
 	label_chunk_list->del = isis_zebra_del_label_chunk;
 	while (isis_zebra_get_label_chunk() != 0) {
-		zlog_warn("%s: re-attempt getting first label chunk!", __func__);
+		zlog_warn("%s: re-attempt getting first label chunk!",
+			  __func__);
 		sleep(1);
 	}
 }

If you are a new contributor to FRR, please see our contributing guidelines.

@odd22 odd22 requested review from rwestphal and mjstapp April 17, 2020 17:44
@odd22
Copy link
Member Author

odd22 commented Apr 17, 2020

Create a new PR because the dev_isis_sr branch had to be rebased first.
This PR replaces the #6200

zlog_warn(
"%s: re-attempt sending hello for synchronous zclient!",
__func__);
sleep(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this means we don't respond to vtysh or watchfrr pings, which can result in watchfrr assuming isisd has died, with bad consequences. A thread timer would be better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qlyoung Agree, but we need to also update ldpd and ospfd accordingly. Can you provide a example of thread timer ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened PR #6274 with an example for ldpd. But really, just because ldpd does a bad thing doesn't mean that isis can't do better, eh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odd22 I'm referring to thread_add_timer

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11932/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings
Report for isis_zebra.c | 12 issues
===============================================
< WARNING: please, no space before tabs
< #404: FILE: /tmp/f1-28858/isis_zebra.c:404:
< WARNING: please, no space before tabs
< #446: FILE: /tmp/f1-28858/isis_zebra.c:446:
< WARNING: please, no space before tabs
< #487: FILE: /tmp/f1-28858/isis_zebra.c:487:
< WARNING: line over 80 characters
< #620: FILE: /tmp/f1-28858/isis_zebra.c:620:
< WARNING: line over 80 characters
< #627: FILE: /tmp/f1-28858/isis_zebra.c:627:
< WARNING: Missing a blank line after declarations
< #652: FILE: /tmp/f1-28858/isis_zebra.c:652:

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-11932/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.4.1 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.4.1 (current is 4.3.0)
W: frr-doc: changelog-file-missing-explicit-entry 6.0-2 -> 7.4-dev-20200417-16-ga46b829ee-0 (missing) -> 7.4-dev-20200417-16-ga46b829ee-0~deb10u1
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 6.0-2 -> 7.4-dev-20200417-16-ga46b829ee-0 (missing) -> 7.4-dev-20200417-16-ga46b829ee-0~deb10u1
W: frr-snmp: changelog-file-missing-explicit-entry 6.0-2 -> 7.4-dev-20200417-16-ga46b829ee-0 (missing) -> 7.4-dev-20200417-16-ga46b829ee-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 6.0-2 -> 7.4-dev-20200417-16-ga46b829ee-0 (missing) -> 7.4-dev-20200417-16-ga46b829ee-0~deb10u1
W: frr: changelog-file-missing-explicit-entry 6.0-2 -> 7.4-dev-20200417-16-ga46b829ee-0 (missing) -> 7.4-dev-20200417-16-ga46b829ee-0~deb10u1

@odd22
Copy link
Member Author

odd22 commented Apr 30, 2020

Merge this PR as per decision during the last Telco meeting in order to prepare ISIS-SR inclusion in master. Label Manager will be definitely solved in another PR.

@odd22 odd22 merged commit f7e4b5d into FRRouting:dev_isis_sr Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants