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

call out to fio for host filesystem performance #1275

Merged
merged 38 commits into from
Oct 3, 2023

Conversation

adamancini
Copy link
Member

@adamancini adamancini commented Jul 20, 2023

Description, Motivation and Context

Shell out to the fio utility to collect filesystem write latency info instead of using the old method of writing to disk directly. Work in some additional types to support using any of the metrics that fio returns in future analyzers.

#1215

Checklist

Does this PR introduce a breaking change?

  • Yes
  • No

the collector will require the installation of the fio binary. If fio is not installed, the collector will fail to run and print an error, similarly to our other collectors that require userspace utilities to be installed.

@adamancini adamancini changed the title Host filesystem collector call out to fio for host filesystem performance Jul 20, 2023
@adamancini adamancini linked an issue Jul 20, 2023 that may be closed by this pull request
@adamancini adamancini added type::feature New feature or request echoes/effort: M Medium effort changes by the team's standards (default) labels Jul 20, 2023
bin/watch.js Outdated Show resolved Hide resolved
@adamancini adamancini marked this pull request as ready for review July 27, 2023 20:57
@adamancini adamancini requested a review from a team as a code owner July 27, 2023 20:57
@xavpaice
Copy link
Member

can we get a description in the PR for what this is doing exactly, and fix up the merge conflicts? I'm hoping that'll trigger the tests to run.

We are going to need to get some testing into this PR to be able to approve it.

I like the approach, makes sense to me and will produce much more accurate results.

@adamancini adamancini force-pushed the host-filesystem-collector branch from 7fa54be to 817bf74 Compare August 9, 2023 13:58
@adamancini
Copy link
Member Author

ok, tests should finally run, fixed up go.mod problems, @xavpaice ack on the test coverage

@adamancini adamancini requested a review from banjoh August 10, 2023 18:50
@adamancini
Copy link
Member Author

@adamancini
Copy link
Member Author

added some code to handle permission errors gracefully:

ada@ada-kurl:~$ ./support-bundle sample-troubleshoot.yaml

 * failed to run host collector: filesystem-latency-two-minute-benchmark-background-iops-off: open /var/lib/etcd/fsperf permission denied.  Please run this collector as root.: open /var/lib/etcd/fsperf: permission denied


support-bundle-2023-09-06T20_09_26.tar.gz

Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

Changes look good. I ran some tests using both preflight and support-bundle binaries.

I made a few minor comments to improve the code

pkg/collect/host_filesystem_performance_linux.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
@adamancini adamancini requested a review from banjoh September 18, 2023 21:01
@adamancini
Copy link
Member Author

adamancini commented Sep 18, 2023

merged main, updated collector tests, ready for re-re-review

mm tests are failing, have to fix something

Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

Changes look good overall. I added a few comments, but nothing that would affect the structure of the PR too much.

pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance_linux.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance_test.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance_test.go Outdated Show resolved Hide resolved
pkg/collect/host_filesystem_performance.go Outdated Show resolved Hide resolved
@arcolife arcolife self-requested a review October 3, 2023 05:29
banjoh
banjoh previously approved these changes Oct 3, 2023
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

Ship it 🚀

@adamancini adamancini merged commit e3adc1c into replicatedhq:main Oct 3, 2023
22 checks passed
@adamancini adamancini deleted the host-filesystem-collector branch October 3, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/effort: M Medium effort changes by the team's standards (default) type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filesystemPerformance check false alarms
4 participants