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

zebra: FPM should have a way of shutting down #5361

Merged
merged 1 commit into from
Nov 19, 2019

Conversation

donaldsharp
Copy link
Member

When we shut down zebra, we were not doing anything to shut
down the FPM. Perform the necessary occult rituals and
stop the threads from running during early shutdown.

Signed-off-by: Donald Sharp sharpd@cumulusnetworks.com

Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

just had one small question

@@ -2029,11 +2031,21 @@ static int zfpm_init(struct thread_master *master)
return 0;
}

static int zfpm_fini(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do stop_stats_timer also, or is that one ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

When we shut down zebra, we were not doing anything to shut
down the FPM.  Perform the necessary occult rituals and
stop the threads from running during early shutdown.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

looking good to me

@@ -2029,11 +2031,24 @@ static int zfpm_init(struct thread_master *master)
return 0;
}

static int zfpm_fini(void)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we take care of the below two or these are taken care else where for closing the FPM connection ?

  1. close the zfpm_g->sock socket ?
  2. reset zfpm_g->ibuf and zfpm_g->obuf ?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are all sorts of memory leaks in the fpm code upon shutdown. I made no attempt to actually resolve those as that I believe a crash is worse than a memory leak on shutdown. imo someone needs to take ownership of the fpm code and fix this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

@donaldsharp I will investigate to find such leaks and will take care of fixing. Could you please list if you have any thing on top of you mind ?

I will go ahead and merge after CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

the socket is not closed, ibuf and obuf are not freed there is a mac hash table that is not freed. That is just off the top of my head

@LabN-CI
Copy link
Collaborator

LabN-CI commented Nov 18, 2019

🚧 Basic BGPD CI results: Partial FAILURE, 1 tests failed

Results table
_ _
Result SUCCESS git merge/5361 f0c459f
Date 11/18/2019
Start 15:21:16
Finish 15:47:03
Run-Time 25:47
Total 1815
Pass 1814
Fail 1
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-11-18-15:21:16.txt
Log autoscript-2019-11-18-15:22:07.log.bz2
Memory 432 430 360

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-9751/

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.

<TITLE>clang_check</TITLE>

clang_check

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

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.

<TITLE>clang_check</TITLE>

clang_check

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

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.

<TITLE>clang_check</TITLE>

clang_check

@riw777 riw777 merged commit 9546c1b into FRRouting:master Nov 19, 2019
@donaldsharp donaldsharp deleted the fpm_crash branch December 9, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants