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.

Sign up for GitHub

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

Jump to bottom

Feature: IS-IS triggered hello #3174

Merged
merged 8 commits into from
Dec 4, 2018
Merged

Feature: IS-IS triggered hello #3174

merged 8 commits into from
Dec 4, 2018

Conversation

cfra
Copy link
Member

@cfra cfra commented Oct 14, 2018

isisd would only send hellos after each hello interval (modulo jitter). When running with a high hello interval, this leads to significant time until adjancencies are established. For example, with three-way handshake enabled and an IIH of 60 seconds, it can take up to three minutes for IS-IS to declare an adjacency up.

This is addressed by triggering transmission of hellos whenever an adjacency state change is observed. While in line with the spec, this leads to almost immediate (sub second) adjacency establishment after the first transmitted/received packet.

donaldsharp
donaldsharp reviewed Oct 14, 2018
View reviewed changes
isisd/isis_pdu.c
}

if (circuit->circ_type != CIRCUIT_T_BROADCAST) {
zlog_warn("%s: encountered unknown circuit type %d on %s",
Copy link
Member

Choose a reason for hiding this comment

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

Question to broader audience, while isis has not been converted to using the new flog_XXX code, should we insist on new warnings/errors be converted now?

Copy link
Member

Choose a reason for hiding this comment

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

It might make more sense to do a request to change all the zlog's to flog's at once (?)... I'm not certain about mixing these changes into other fixes, probably better to separate them (?).

@NetDEF-CI

This comment has been minimized.

Sign in to view
@LabN-CI
Copy link
Collaborator

LabN-CI commented Oct 14, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3174 017b1db
Date 10/14/2018
Start 19:10:23
Finish 19:33:32
Run-Time 23:09
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-10-14-19:10:23.txt
Log autoscript-2018-10-14-19:11:01.log.bz2

For details, please contact louberger

@cfra
Copy link
Member Author

cfra commented Oct 15, 2018 via email

@cfra
Copy link
Member Author

cfra commented Oct 15, 2018 via email

@cfra
Copy link
Member Author

cfra commented Oct 15, 2018 via email

@donaldsharp
Copy link
Member

I missread the diff. My apologies about the lib/thread.c changes.

@eqvinox eqvinox added the submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.) label Oct 23, 2018
@NetDEF-CI

This comment has been minimized.

Sign in to view
@donaldsharp donaldsharp requested a review from riw777 October 30, 2018 15:51
riw777
riw777 approved these changes Nov 1, 2018
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

This is a good change. The code looks good to me, other than @donaldsharp 's comment on the timer. Commented on the zlog issue in a separate comment.

odd22
odd22 reviewed Nov 6, 2018
View reviewed changes
Copy link
Member

@odd22 odd22 left a comment

Choose a reason for hiding this comment

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

Just a pending question regarding isis_dr_commence() function.

Otherwise, the patch looks good to me. The CI error must be corrected before merging.

isisd/isis_dr.c
thread_add_timer(master, isis_run_dr_l2, circuit,
2 * circuit->hello_interval[1],
&circuit->u.bc.t_run_dr[1]);

thread_add_timer(master, send_l2_csnp, circuit,
Copy link
Member

Choose a reason for hiding this comment

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

Why do you not add:

THREAD_TIMER_OFF(circuit->u.bc.t_run_dr[level -1];
thread_add_timer(master, isis_run_dr, circuit, 2 * circuit->hello_interval[level - 1], &circuit->u.bc.t_run_dr[level - 1]);

Like in line 258 ?
This is no more necessary at this step ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had some trouble recalling why I did that. Looking at a unified diff with more context made it clearer to me again. Maybe it also helps you to verify whether my change is correct, so here it is:

 int isis_dr_commence(struct isis_circuit *circuit, int level)
 {
 	uint8_t old_dr[ISIS_SYS_ID_LEN + 2];
 
 	if (isis->debugs & DEBUG_EVENTS)
 		zlog_debug("isis_dr_commence l%d", level);
 
 	/* Lets keep a pause in DR election */
 	circuit->u.bc.run_dr_elect[level - 1] = 0;
-	if (level == 1)
-		thread_add_timer(master, isis_run_dr_l1, circuit,
-				 2 * circuit->hello_interval[0],
-				 &circuit->u.bc.t_run_dr[0]);
-	else
-		thread_add_timer(master, isis_run_dr_l2, circuit,
-				 2 * circuit->hello_interval[1],
-				 &circuit->u.bc.t_run_dr[1]);
+	thread_add_timer(master, isis_run_dr,
+			 &circuit->level_arg[level - 1],
+			 2 * circuit->hello_interval[level - 1],
+			 &circuit->u.bc.t_run_dr[level - 1]);
 	circuit->u.bc.is_dr[level - 1] = 1;
 
 	if (level == 1) {
 		memcpy(old_dr, circuit->u.bc.l1_desig_is, ISIS_SYS_ID_LEN + 1);
 		LSP_FRAGMENT(old_dr) = 0;
 		if (LSP_PSEUDO_ID(old_dr)) {
 			/* there was a dr elected, purge its LSPs from the db */
 			lsp_purge_pseudo(old_dr, circuit, level);
 		}
 		memcpy(circuit->u.bc.l1_desig_is, isis->sysid, ISIS_SYS_ID_LEN);
 		*(circuit->u.bc.l1_desig_is + ISIS_SYS_ID_LEN) =
 			circuit->circuit_id;
 
 		assert(circuit->circuit_id); /* must be non-zero */
 		/*    if (circuit->t_send_l1_psnp)
 		   thread_cancel (circuit->t_send_l1_psnp); */
 		lsp_generate_pseudo(circuit, 1);
 
-		THREAD_TIMER_OFF(circuit->u.bc.t_run_dr[0]);
-		thread_add_timer(master, isis_run_dr_l1, circuit,
-				 2 * circuit->hello_interval[0],
-				 &circuit->u.bc.t_run_dr[0]);
-
 		thread_add_timer(master, send_l1_csnp, circuit,
 				 isis_jitter(circuit->csnp_interval[level - 1],
 					     CSNP_JITTER),
 				 &circuit->t_send_csnp[0]);
 
 	} else {
 		memcpy(old_dr, circuit->u.bc.l2_desig_is, ISIS_SYS_ID_LEN + 1);
 		LSP_FRAGMENT(old_dr) = 0;
 		if (LSP_PSEUDO_ID(old_dr)) {
 			/* there was a dr elected, purge its LSPs from the db */
 			lsp_purge_pseudo(old_dr, circuit, level);
 		}
 		memcpy(circuit->u.bc.l2_desig_is, isis->sysid, ISIS_SYS_ID_LEN);
 		*(circuit->u.bc.l2_desig_is + ISIS_SYS_ID_LEN) =
 			circuit->circuit_id;
 
 		assert(circuit->circuit_id); /* must be non-zero */
 		/*    if (circuit->t_send_l1_psnp)
 		   thread_cancel (circuit->t_send_l1_psnp); */
 		lsp_generate_pseudo(circuit, 2);
 
-		THREAD_TIMER_OFF(circuit->u.bc.t_run_dr[1]);
-		thread_add_timer(master, isis_run_dr_l2, circuit,
-				 2 * circuit->hello_interval[1],
-				 &circuit->u.bc.t_run_dr[1]);
-
 		thread_add_timer(master, send_l2_csnp, circuit,
 				 isis_jitter(circuit->csnp_interval[level - 1],
 					     CSNP_JITTER),
 				 &circuit->t_send_csnp[1]);
 	}
 
 	thread_add_event(master, isis_event_dis_status_change, circuit, 0,
 			 NULL);
 
 	return ISIS_OK;
 }

It seemed to me like the old code would:

  1. schedule the timeout
  2. cancel it immediately
  3. schedule the exact timeout again

So I decided to drop Steps 2 and 3.

Copy link
Member

@odd22 odd22 Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

OK. That's clear now.

So, I have one more suggestion: perhaps it is safer to drop step 1 & 2 and keep step 3 instead. i.e. move the call to thread_add_timer() after the if (level == 1) { ... } else { ... } ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

cfra added 8 commits December 4, 2018 12:49
@cfra
isisd: Add level&circuit thread argument to circuit
7da4aa3 
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
isisd: reduce code duplication for levels
7c4f7aa 
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
isisd: Move dr election timer to the end of isis_dr_commence
0a5e562 
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
lib: add thread_timer_remain_msec function
78ca034 
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
isisd: add send_hello_sched function
a0a707e 
Add a function send_hello_sched so that the logic for scheduling a
hello is not replicated inconsistently into different locations.

Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
isisd: trigger hellos on adjacency state change
5346ace 
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
topotests/isis-topo1: Set useful IS-IS debugs
32bc1ea 
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra
topotests: Update json_cmp_result to return readable result
7fe06d5 
Before this update, json_cmp_results which were formatted as strings
would not show the error mesage but just an object reference.

Signed-off-by: Christian Franke <chris@opensourcerouting.org>
@cfra cfra force-pushed the feature/isis-triggered-hello branch from 017b1db to 7fe06d5 Compare December 4, 2018 15:14
@cfra
Copy link
Member Author

cfra commented Dec 4, 2018

I have found and fixed a bug in this PR which caused CI to fail.

@cfra cfra added review & merge me look at me! and removed submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.) labels Dec 4, 2018
@LabN-CI
Copy link
Collaborator

LabN-CI commented Dec 4, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3174 7fe06d5
Date 12/04/2018
Start 10:43:58
Finish 11:07:32
Run-Time 23:34
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-12-04-10:43:58.txt
Log autoscript-2018-12-04-10:44:39.log.bz2

For details, please contact louberger

@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-6064/

This is a comment from an EXPERIMENTAL 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:

<stdin>:406: trailing whitespace.
	
<stdin>:408: trailing whitespace.
	
warning: 2 lines add whitespace errors.
Report for isis_adjacency.c | 4 issues
===============================================
< WARNING: braces {} are not necessary for single statement blocks
< #218: FILE: /tmp/f1-19956/isis_adjacency.c:218:
< WARNING: braces {} are not necessary for single statement blocks
< #233: FILE: /tmp/f1-19956/isis_adjacency.c:233:
Report for isis_dr.c | 4 issues
===============================================
< WARNING: line over 80 characters
< #83: FILE: /tmp/f1-19956/isis_dr.c:83:
< WARNING: Prefer using '"%s...", __func__' to using 'isis_run_dr', this function's name, in a string
< #83: FILE: /tmp/f1-19956/isis_dr.c:83:
Report for isis_pdu.c | 8 issues
===============================================
< ERROR: trailing whitespace
< #1748: FILE: /tmp/f1-19956/isis_pdu.c:1748:
< ERROR: trailing whitespace
< #1750: FILE: /tmp/f1-19956/isis_pdu.c:1750:
< WARNING: line over 80 characters
< #1783: FILE: /tmp/f1-19956/isis_pdu.c:1783:
< WARNING: line over 80 characters
< #1819: FILE: /tmp/f1-19956/isis_pdu.c:1819:
Report for thread.h | 2 issues
===============================================
< WARNING: function definition argument 'struct thread *' should also have an identifier name
< #221: FILE: /tmp/f1-19956/thread.h:221:

CLANG Static Analyzer Summary

  • Github Pull Request 3174, comparing to Git base SHA b59d17b
  • Base image data for Git b59d17b does not exist - compare skipped

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-6064/artifact/shared/static_analysis/index.html

@odd22 odd22 merged commit 5898044 into FRRouting:master Dec 4, 2018
@eqvinox eqvinox deleted the feature/isis-triggered-hello branch April 18, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donaldsharp donaldsharp donaldsharp left review comments

@riw777 riw777 riw777 approved these changes

@odd22 odd22 odd22 approved these changes

Assignees
No one assigned
Labels
review & merge me look at me!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cfra @NetDEF-CI @LabN-CI @donaldsharp @riw777 @odd22 @eqvinox