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

tests: add support for running things under rr. #14920

Merged
merged 1 commit into from
Dec 5, 2023
Merged

tests: add support for running things under rr. #14920

merged 1 commit into from
Dec 5, 2023

Conversation

choppsv1
Copy link
Contributor

No description provided.

@frrbot frrbot bot added the tests Topotests, make check, etc label Nov 30, 2023
@qlyoung qlyoung self-requested a review November 30, 2023 18:32
qlyoung
qlyoung reviewed Nov 30, 2023
View reviewed changes
Copy link
Member

@qlyoung qlyoung left a comment

Choose a reason for hiding this comment

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

No issues with the code, just a couple docs nits. Otherwise looks amazing!

doc/developer/topotests.rst Outdated
""""""""""""""""""""""""""""""""""""""""""""""

Topotest can automatically launch any daemon under ``rr(1)`` to collect
execute state. The daemon is run in non-daemon mode with ``rr record``.
Copy link
Member

Choose a reason for hiding this comment

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

execution*, and maybe "in the foreground" would be clearer than "non daemon mode"

doc/developer/topotests.rst Outdated
(in a `rr` subdir that rr creates) under the test's run directoy.

Here's an example of collecting ``rr`` execution state from ``mgmtd`` on router
``r1`` during the config_timing test.
Copy link
Member

Choose a reason for hiding this comment

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

config_timing

Copy link
Contributor Author

@choppsv1 choppsv1 Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

config_timing

not sure what's being asked for here :) Oh.. I think you might mean add double ticks...

Copy link
Member

Choose a reason for hiding this comment

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

I put double backtics but github rendered them; I'm saying config_timing should be code-formatted in the docs.

@rgangam-PAN
Copy link

@choppsv1 There are instances where FRR drops the permissions to nobody:nobody , does rr supports recording in that case, where the recording is done.

@choppsv1
Copy link
Contributor Author

choppsv1 commented Dec 1, 2023

@choppsv1 There are instances where FRR drops the permissions to nobody:nobody , does rr supports recording in that case, where the recording is done.

@donaldsharp ?

@choppsv1
Copy link
Contributor Author

choppsv1 commented Dec 2, 2023

@choppsv1 There are instances where FRR drops the permissions to nobody:nobody , does rr supports recording in that case, where the recording is done.

I'm not sure; however, I don't see why not, rr is not dropping permissions.

@choppsv1
Copy link
Contributor Author

choppsv1 commented Dec 4, 2023

ci:rerun

@ton31337
Copy link
Member

ton31337 commented Dec 5, 2023

side note: very nice addition, just can't run it on AMD Ryzen :( rr-debugger/rr#2034

@donaldsharp donaldsharp merged commit ad80021 into FRRouting:master Dec 5, 2023
77 checks passed
@choppsv1 choppsv1 deleted the chopps/rr-support branch January 9, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qlyoung qlyoung qlyoung left review comments

Assignees
No one assigned
Labels
master size/M tests Topotests, make check, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@choppsv1 @rgangam-PAN @ton31337 @qlyoung @donaldsharp