-
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_analyzer): add host sysctl analyzer #1681
Conversation
@@ -38,7 +38,7 @@ func (a *AnalyzeHostTime) Analyze( | |||
getCollectedFileContents, | |||
collect.HostTimePath, | |||
collect.NodeInfoBaseDir, | |||
collect.HostMemoryFileName, | |||
collect.HostTimeFileName, |
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.
Noticed the time collector was using the memory collector file name, not really part of the bulk of this changes but thought it would be a useful fix.
pkg/collect/remote_collector.go
Outdated
case c.Collect.Sysctl != nil: | ||
hostCollect.HostSysctl = &troubleshootv1beta2.HostSysctl{ | ||
HostCollectorMeta: troubleshootv1beta2.HostCollectorMeta{ | ||
CollectorName: c.Collect.Sysctl.CollectorName, | ||
Exclude: c.Collect.Sysctl.Exclude, | ||
}, | ||
} |
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.
I'm unsure if it makes sense to have this collector support remote collection too given we also have - https://troubleshoot.sh/docs/collect/sysctl/#heading - but I decided to follow the pattern we're using for all of our host collectors.
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.
yeah, everything you see in the files named specifically around "remote_collector" is actually code we're planning to deprecate. So we don't need to add this here nor do we need to update the GVK for Remote Collectors
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.
+1
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 via - 258d883
type RemoteSysctl struct { | ||
RemoteCollectorMeta `json:",inline" yaml:",inline"` | ||
} | ||
|
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.
We can get rid of the changes in this file. This code path is going to be deprecated hopefully soon
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.
@diamonwiggins should I remove it right now before merge? Or do you mean that soon we'll be able to remove it?
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.
+1
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 via - 258d883
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
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.
Looks good. I left some comments
pkg/analyze/host_sysctl.go
Outdated
} | ||
|
||
param := matches[1] | ||
operator := matches[2] |
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.
You can use ParseComparisonOperator to parse the operator
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.
Ah thank you! This is pretty useful. I'll use that one too.
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 via - 258d883
pkg/collect/remote_collector.go
Outdated
case c.Collect.Sysctl != nil: | ||
hostCollect.HostSysctl = &troubleshootv1beta2.HostSysctl{ | ||
HostCollectorMeta: troubleshootv1beta2.HostCollectorMeta{ | ||
CollectorName: c.Collect.Sysctl.CollectorName, | ||
Exclude: c.Collect.Sysctl.Exclude, | ||
}, | ||
} |
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.
+1
type RemoteSysctl struct { | ||
RemoteCollectorMeta `json:",inline" yaml:",inline"` | ||
} | ||
|
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.
+1
Description, Motivation and Context
Fixes - #1675
Running it locally:
Checklist
Does this PR introduce a breaking change?