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

Reorganize Iperf error reporting #349

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

Kuba314
Copy link
Contributor

@Kuba314 Kuba314 commented Nov 21, 2023

Description

Improve iperf reporting and reorganize the code a bit.

Tests

J:8583736 - second recipeset contains debug logs.

Reviews

@jtluka

Closes: #201

@Kuba314 Kuba314 force-pushed the report-iperf-errors branch 2 times, most recently from 30807cb to 18b2509 Compare November 21, 2023 13:46
Copy link
Collaborator

@jtluka jtluka left a comment

Choose a reason for hiding this comment

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

Overall looks fine. Please address my comments. Also, do few test runs on the internal failing iperf test cases where we would benefit from the added logging.

lnst/Tests/Iperf.py Show resolved Hide resolved
lnst/Tests/Iperf.py Outdated Show resolved Hide resolved
lnst/Tests/Iperf.py Outdated Show resolved Hide resolved
@Kuba314 Kuba314 force-pushed the report-iperf-errors branch 2 times, most recently from 8ec12dd to 3b9391b Compare November 22, 2023 13:02
jtluka
jtluka previously approved these changes Nov 27, 2023
Copy link
Collaborator

@jtluka jtluka left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Collaborator

@olichtne olichtne left a comment

Choose a reason for hiding this comment

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

just some minor comments to clear up

lnst/Tests/Iperf.py Show resolved Hide resolved
lnst/Tests/Iperf.py Outdated Show resolved Hide resolved
@Kuba314
Copy link
Contributor Author

Kuba314 commented Dec 13, 2023

Currently I can't test this properly because of #354

EDIT: Planning to test this PR.

SIG_DFL seems to exit the process, but we want the default python
handler that raises KeyboardInterrupt instead.
olichtne
olichtne previously approved these changes Jan 18, 2024
Copy link
Collaborator

@olichtne olichtne left a comment

Choose a reason for hiding this comment

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

looks good now.

@olichtne
Copy link
Collaborator

Currently I can't test this properly because of #354

EDIT: Planning to test this PR.

still planning to do some tests or is that also done now?

@Kuba314
Copy link
Contributor Author

Kuba314 commented Jan 18, 2024

@olichtne So... I tested this PR but I'm not sure what the behaviour should be exactly...

I SSHed into the client agent and tried sending INT via kill:

  • INTing iperf process directly makes the IperfClient module FAIL and even if there's data, after all iterations are done, there's a ZeroDivisionError when trying to get time_aligned_results.
  • INTing the lnst-agent process which starts iperf does nothing (because KeyboardInterrupt is caught and there's a pass followed by another .communicate() call). I tried changing pass to server.send_signal(signal.SIG_INT), which results in the same behaviour as the previous point -> iperf gets SIGINT, finishes, but for some reason we get ZeroDivisionError.
  • INTing main lnst-agent process kills the agent (Caught signal 2 -> dying on agent, host1 hard disconnected on controller side)

@Kuba314
Copy link
Contributor Author

Kuba314 commented Jan 18, 2024

I added back the signal sending from the KeyboardInterrupt handler to forward it to the forked lnst-agent process and the iperf process. Should be good now. Ready to be merged.

@olichtne olichtne merged commit b16fce1 into LNST-project:master Jan 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests/Iperf.py: res_data['msg'] is assigned twice discarding the previous content
3 participants