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

Attempt to kill bg process in tests #3894

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Oct 4, 2024

Describe your changes

Some e2e tests rely on background processes, especially the IBC ones with hermes and gaiad. There are cases in which the test fails and some of these tasks (especially gaiad) are not affected by the crash of the Namada node. In this case the runtime drops the NamadaBgCmd but unfortunately dropping a JoinHandle only detaches the target task. This is undesired cause if we run the test again the conflicting instances of the background process will cause another test failure.

This PR attempts to run some cleanup code when dropping NamadaBgCmd to kill the task.

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.24%. Comparing base (45e7e90) to head (1773a17).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3894   +/-   ##
=======================================
  Coverage   73.24%   73.24%           
=======================================
  Files         341      341           
  Lines      105240   105240           
=======================================
+ Hits        77085    77087    +2     
+ Misses      28155    28153    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@grarco grarco marked this pull request as ready for review October 4, 2024 13:53
@grarco grarco requested a review from sug0 October 4, 2024 14:58
@@ -897,17 +910,22 @@ impl NamadaCmd {
let mut cmd = self;
loop {
match abort_recv.try_recv() {
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR, but this try_recv might be contributing substantially to the resource usage of e2e tests :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the quick fix would be to call recv_timeout instead with a sane timeout. Probably a better solution would be to switch to async channels?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think async channels wouldn't help. there is no event here triggering a new channel read, so the only reasonable thing would be what you suggested first: a poll with a timer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I've opened #3897 to track that, I'll merge this one, thank you!

@grarco grarco added merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass backport-44 labels Oct 7, 2024
@mergify mergify bot merged commit 7ac415a into main Oct 7, 2024
37 of 38 checks passed
@mergify mergify bot deleted the grarco/kill-tests-bgprocs branch October 7, 2024 14:17
mergify bot added a commit that referenced this pull request Oct 10, 2024
Attempt to kill bg process in tests (backport #3894)
@tzemanovic tzemanovic mentioned this pull request Oct 10, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-44 merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants