8000 Close all open files, clean up fsnotify watcher by matoval · Pull Request #1268 · ansible/receptor · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Close all open files, clean up fsnotify watcher #1268

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
Merged
merged 4 commits into from
Feb 17, 2025
Merged

Close all open files, clean up fsnotify watcher #1268

merged 4 commits into from
Feb 17, 2025

Conversation

matoval
Copy link
Collaborator
@matoval matoval commented Feb 13, 2025

Close all open files, clean up fsnotify watcher to fix fsnotify watcher: too many open files bug

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@codecov Codecov
Copy link
codecov bot commented Feb 13, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 20.68966% with 23 lines in your changes missing coverage. Please review.

Project coverage is 40.12%. Comparing base (9eb8256) to head (b738b77).
Report is 26 commits behind head on devel.

Files with missing lines Patch % Lines
pkg/workceptor/remote_work.go 0.00% 10 Missing ⚠️
pkg/workceptor/command.go 0.00% 8 Missing ⚠️
pkg/netceptor/conn.go 40.00% 2 Missing and 1 partial ⚠️
pkg/workceptor/workunitbase.go 66.66% 2 Missing ⚠️ 
@@            Coverage Diff             @@
##            devel    #1268      +/-   ##
==========================================
+ Coverage   39.80%   40.12%   +0.32%     
==========================================
  Files          45       48       +3     
  Lines        9376    10021     +645     
==========================================
+ Hits         3732     4021     +289     
- Misses       5354     5706     +352     
- Partials      290      294       +4
Files with missing lines Coverage Δ
pkg/workceptor/workunitbase.go 71.73% <66.66%> (-0.17%) ⬇️
pkg/netceptor/conn.go 3.80% <40.00%> (+0.46%) ⬆️
pkg/workceptor/command.go 44.81% <0.00%> (-0.88%) ⬇️
pkg/workceptor/remote_work.go 10.79% <0.00%> (-0.04%) ⬇️

... and 7 files with indirect coverage changes

Components Coverage Δ
Go 39.56% <20.68%> (-0.25%) ⬇️
Receptorctl 49.31% <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

AaronH88
AaronH88 reviewed Feb 14, 2025
View reviewed changes
pkg/workceptor/command.go
@@ -209,6 +209,14 @@ loop:
MainInstance.nc.GetLogger().Error("Error updating status file %s: %s", statusFilename, err)
}
}
err = stdin.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Im a bit worried about these two for longer running jobs,
Have you tested this with either
A job lasting longer than 5 min
Using defer
😄

@AaronH88
Copy link
Contributor

Mostly this looks great! And i think it is probably good as it currently is, but as the code around command is both complicated and critical I want to make sure longer jobs and kube have been tested.

AaronH88
AaronH88 approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor
@AaronH88 AaronH88 left a comment

Choose a reason for hiding this comment

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

lgtm

Ty for doing the longer tests

@AaronH88 AaronH88 merged commit 122aa3e into ansible:devel Feb 17, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronH88 AaronH88 AaronH88 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matoval @AaronH88
0