-
Notifications
You must be signed in to change notification settings - Fork 94
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(host_sysctl): add host sysctl collector #1676
Conversation
Co-authored-by: Evans Mungai <evans@replicated.com>
@banjoh I've re-implemented the collector using - https://github.com/lorenzosaino/go-sysctl - let me know what you think. I've regenerated the schemas and tested out the collector but for wtv reason it isn't picking up on it 🤔 :
I'm gonna follow up and try to get to the bottom of it after lunch but meanwhile if you have any leads let me know |
|
||
func (c *CollectHostSysctl) Collect(progressChan chan<- interface{}) (map[string][]byte, error) { | ||
client, err := sysctl.NewClient(sysctlVirtualFiles) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an error here
E1107 12:10:03.235230 56942 host_sysctl.go:39] Failed to initialize sysctl client: could not create client: directory /proc/sys/ does not exist
I think we should revert back to calling sysctl
instead of relying on a library, otherwise we need to handle those rare environments, e.g darwin that don't have /proc/sys
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah true, darwin won't have procfs
, however the props will be completely different too and even the output will differ between sysctl -a
in darwin vs linux. But agree it's probably the best approach out of both options. Gonna go back to that and have a common output from the collector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, let me know what you think @banjoh
Running this locally:
The file
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description, Motivation and Context
Fixes - #1674
Checklist
Does this PR introduce a breaking change?