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

ospfd: fixed disable mpls-te inter-as support #3059

Closed
wants to merge 3 commits into from
Closed

ospfd: fixed disable mpls-te inter-as support #3059

wants to merge 3 commits into from

Conversation

ak503
Copy link
Contributor

@ak503 ak503 commented Sep 20, 2018

Signed-off-by: Dmitrii Turlupov dturlupov@factor-ts.ru

ospf: fixed disable mpls-te inter-as support
006260e 
Signed-off-by: Dmitrii Turlupov <dturlupov@factor-ts.ru>
@ak503
Copy link
Contributor Author

ak503 commented Sep 20, 2018

ospf_mpls_te_unregister() always returned 0 if OspfMplsTE.inter_as == Off. Need disable OspfMplsTE.inter_as after ospf_mpls_te_unregister()

@LabN-CI
Copy link
Collaborator

LabN-CI commented Sep 20, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3059 006260e
Date 09/20/2018
Start 11:13:41
Finish 11:36:43
Run-Time 23:02
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-09-20-11:13:41.txt
Log autoscript-2018-09-20-11:14:19.log.bz2

For details, please contact louberger

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5372/

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.

Get source and apply patch from patchwork: Successful

Building Stage: Successful

Basic Tests: Failed

Ubuntu 12.04 deb pkg check: Successful
Static analyzer (clang): Successful
CentOS 6 rpm pkg check: Successful
Ubuntu 16.04 deb pkg check: Successful
IPv6 protocols on Ubuntu 14.04: Successful
Fedora 24 rpm pkg check: Successful
Addresssanitizer topotest: Successful
Debian 8 deb pkg check: Successful
IPv4 protocols on Ubuntu 14.04: Successful
CentOS 7 rpm pkg check: Successful
IPv4 ldp protocol on Ubuntu 16.04: Successful
Ubuntu 14.04 deb pkg check: Successful
Debian 9 deb pkg check: Successful
Topotest tests on Ubuntu 16.04 i386: Successful

Topology tests on Ubuntu 16.04 amd64: Failed

Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-5372/test

Topology Tests failed for Topology tests on Ubuntu 16.04 amd64:

2018-09-20 08:32:20,789 ERROR: ******************************************************************************
2018-09-20 08:32:20,790 ERROR: Test Target Summary                                                  Pass Fail
2018-09-20 08:32:20,790 ERROR: ******************************************************************************
2018-09-20 08:32:20,790 ERROR: FILE: scripts/adjacencies.py
2018-09-20 08:32:20,790 ERROR: 10   r2     Core adjacencies up +10.20 secs                          0    1
2018-09-20 08:32:20,790 ERROR: See /tmp/topotests/bgp_l3vpn_to_bgp_direct.test_bgp_l3vpn_to_bgp_direct/output.log for details of errors
2018-09-20 08:32:20,794 ERROR: assert failed at "bgp_l3vpn_to_bgp_direct.test_bgp_l3vpn_to_bgp_direct/test_adjacencies": 1 tests failed

see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5372/artifact/TOPOU1604/ErrorLog/log_topotests.txt

Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5372/artifact/TOPOU1604/MemoryLeaks/

CLANG Static Analyzer Summary

  • Github Pull Request 3059, comparing to Git base SHA dc790ba

No Changes in Static Analysis warnings compared to base

rwestphal
rwestphal reviewed Sep 24, 2018
View reviewed changes
ospfd/ospf_te.c Outdated
@@ -2412,6 +2411,7 @@ DEFUN (no_ospf_mpls_te_inter_as,

/* Deregister the Callbacks for Inter-AS suport */
ospf_mpls_te_unregister();
OspfMplsTE.inter_as = Off;
Copy link
Member

Choose a reason for hiding this comment

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

I think this makes sense. But maybe we could move these three lines into the if block above, to avoid calling ospf_mpls_te_unregister() when the mpls-te inter-as command is not previously configured.

Copy link
Member

Choose a reason for hiding this comment

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

Renato

You are right. Dimitrii, can you update your patch ?

@donaldsharp donaldsharp requested a review from odd22 October 9, 2018 15:26
@donaldsharp
Copy link
Member

Olivier says that this is probably insufficient to fix this issue

@eqvinox
Copy link
Contributor

eqvinox commented Oct 23, 2018

@odd22 can you comment on this? Is the fix insufficient?

odd22
odd22 requested changes Oct 30, 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.

As suggest by Renato, please move the ospf_mpls_te_unregister() and OspfMplsTE.inter_ass = Off inside the if block to avoid call unregister when not necessary.

ospfd/ospf_te.c Outdated
@@ -2412,6 +2411,7 @@ DEFUN (no_ospf_mpls_te_inter_as,

/* Deregister the Callbacks for Inter-AS suport */
ospf_mpls_te_unregister();
OspfMplsTE.inter_as = Off;
Copy link
Member

Choose a reason for hiding this comment

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

Renato

You are right. Dimitrii, can you update your patch ?

@eqvinox eqvinox added the submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.) label Oct 30, 2018
ospf: unregister mpls-te inter-as only if mpls-te is enabled
4fe240e 
Signed-off-by: Dmitrii Turlupov <dturlupov@factor-ts.ru>
@LabN-CI
Copy link
Collaborator

LabN-CI commented Nov 7, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git pull/3059 4fe240e (merge failed)
Date 11/07/2018
Start 09:40:16
Finish 10:03:50
Run-Time 23:34
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2018-11-07-09:40:16.txt
Log autoscript-2018-11-07-09:40:56.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-5848/

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.

CLANG Static Analyzer Summary

  • Github Pull Request 3059, comparing to Git base SHA dc790ba

No Changes in Static Analysis warnings compared to base

@LabN-CI
Copy link
Collaborator

LabN-CI commented Nov 7, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3059 d7ccc45
Date 11/07/2018
Start 10:50:17
Finish 11:14:01
Run-Time 23:44
Total 1816
Pass 1816
Fail 0
Valgrind-Errors
Valgrind-Loss
Details vncregress-2018-11-07-10:50:17.txt
Log autoscript-2018-11-07-10:50:59.log.bz2

For details, please contact louberger

@odd22
Copy link
Member

odd22 commented Nov 7, 2018

LGTM. But, before, merging this PR into master, can you fusion your 3 commits ? Use git rebase -i HEAD~2 (or HEAD~3) for that. Once done and CI OK, I will merge this PR.

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

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.

CLANG Static Analyzer Summary

  • Github Pull Request 3059, comparing to Git base SHA 8dfa7e4

No Changes in Static Analysis warnings compared to base

@ak503
Copy link
Contributor Author

ak503 commented Nov 9, 2018

rebase have some problem . i created new pr #3298

@ak503 ak503 closed this Nov 9, 2018
@ak503 ak503 deleted the mpls_te branch June 11, 2019 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwestphal rwestphal rwestphal left review comments

@odd22 odd22 odd22 approved these changes

Assignees
No one assigned
Labels
submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ak503 @LabN-CI @NetDEF-CI @donaldsharp @eqvinox @odd22 @rwestphal