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

Improved graphtrace #396

Merged
merged 5 commits into from
Sep 28, 2023
Merged

Improved graphtrace #396

merged 5 commits into from
Sep 28, 2023

Conversation

PlagueCZ
Copy link
Contributor

@PlagueCZ PlagueCZ commented Sep 25, 2023

As we started using it in operations, I did some improvements on the graphtrace tool:

  1. Better argument handling (added version, etc)
  2. Fixed some wrong states in stopping
  3. Handle more signals (because of dp_graphtrace | head or dp_graphtrace | grep)
  4. Print Graphtrace successfully disabled in dp-service to stderr (because stdout will be grepped often)
  5. Added --stop to at least have a way to stop manually (e.g. for when the above message is not confirmed)
  6. Log only PORT ## >> rx and tx >> PORT ## by default
  7. Added --drops to log also dropped packets (to match them to PORT ## >> rx when there is no tx >> PORT
  8. Added --nodes to log the full graph traversal like the original tool did
  9. Renamed the tool to dpservice-dump as discussed. The rest of the renaming is part of Update DPDK to 22.11 #385
  10. Greatly improved dp_conf_generate.py that can now easily serve multiple tools and (non-generated) code duplication is minimized.

Stuff left for the followup PR:

  1. PCAP output
  2. node-name filtering?
  3. BPF filtering?

Unfortunately the dp_conf_generate.py change is the most verbose in diffs, but going by commits you can just skim that part.

@github-actions github-actions bot added enhancement New feature or request size/XL labels Sep 25, 2023
// try to also handle most of the rest (some are not possible, like KILL)
for (int i = 1; i < _NSIG; ++i)
signal(i, signal_handler);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guvenc Can you take a look at this block? We at least need to add SIGPIPE for dp_graphtrace|head etc., but it would be safer to handle all signals.
But I was unable to find a better solution than to just iterate like this. But then some signals cannot be handled.
So I could either exhaustively add them with a check, or just make sure the essential ones are setup and leave the rest to chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah actually not that good an idea, as SIGSEGV will get caught in there too. I guess we just need to add exhaustively those that can be encountered in operations...

@PlagueCZ PlagueCZ force-pushed the feature/improved_graphtrace branch 3 times, most recently from ff8e4b3 to 2d4da81 Compare September 26, 2023 16:13
@github-actions github-actions bot added size/XXL documentation Improvements or additions to documentation and removed size/XL labels Sep 27, 2023
@PlagueCZ PlagueCZ force-pushed the feature/improved_graphtrace branch 4 times, most recently from 6ae80b5 to 8f4eaa5 Compare September 28, 2023 13:49
@PlagueCZ PlagueCZ marked this pull request as ready for review September 28, 2023 14:09
Copy link
Collaborator

@guvenc guvenc left a comment

Choose a reason for hiding this comment

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

Thanks

@guvenc guvenc merged commit c2137af into main Sep 28, 2023
7 checks passed
@guvenc guvenc deleted the feature/improved_graphtrace branch September 28, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants