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

feat(inputs.lustre2): Skip brw_stats in case of insufficient permissions #15045

Merged

Conversation

lukeyeager
Copy link
Contributor

@lukeyeager lukeyeager commented Mar 22, 2024

Before this commit, the plugin entirely fails when run as anyone other than root, because brw_stats files are symlinks to /sys/kernel/debug/:

# readlink -f /proc/fs/lustre/osd-ldiskfs/MGS/brw_stats
/sys/kernel/debug/lustre/osd-ldiskfs/MGS/brw_stats

Which was causing errors like this:

2024-03-22T02:40:40Z E! [inputs.lustre2] Error in plugin: failed to read file /proc/fs/lustre/osd-ldiskfs/MGS/brw_stats: open /proc/fs/lus
tre/osd-ldiskfs/MGS/brw_stats: permission denied

After this change, the plugin continues to run and gather whatever metrics it can, even if brw_stats files are unreadable.

I'll rebase after #15042 is merged.

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 22, 2024
@lukeyeager lukeyeager force-pushed the lustre2-skip-brw-when-not-root branch 2 times, most recently from df4f9b6 to bed31ce Compare March 22, 2024 16:04
@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Mar 22, 2024
@srebhan srebhan changed the title feat(inputs.lustre2): skip brw_stats if not readable feat(inputs.lustre2): Skip brw_stats for insufficient permissions Mar 25, 2024
@srebhan srebhan changed the title feat(inputs.lustre2): Skip brw_stats for insufficient permissions feat(inputs.lustre2): Skip brw_stats in case of insufficient permissions Mar 25, 2024
@srebhan
Copy link
Member

srebhan commented Mar 25, 2024

@lukeyeager thanks for your contribution. It would be nice if you can resolve the merge conflict...

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

One more comment regarding error checking... It might be necessary to import errors if that's not done already...

plugins/inputs/lustre2/lustre2.go Outdated Show resolved Hide resolved
Before this commit, the plugin entirely fails when run as anyone other
than root, because brw_stats files are symlinks to /sys/kernel/debug/.

Which was causing errors like this:
```
2024-03-22T02:40:40Z E! [inputs.lustre2] Error in plugin: failed to read file /proc/fs/lustre/osd-ldiskfs/MGS/brw_stats: open /proc/fs/lustre/osd-ldiskfs/MGS/brw_stats: permission denied
```

After this change, the plugin continues to run and gather whatever
metrics it can, even if brw_stats files are unreadable.
@lukeyeager lukeyeager force-pushed the lustre2-skip-brw-when-not-root branch from 73e6be0 to 4b5a0ad Compare March 26, 2024 17:22
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @lukeyeager!

@srebhan srebhan merged commit 3d45e32 into influxdata:master Mar 27, 2024
25 of 26 checks passed
@github-actions github-actions bot added this to the v1.31.0 milestone Mar 27, 2024
@lukeyeager lukeyeager deleted the lustre2-skip-brw-when-not-root branch March 27, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants