Jv ebpf deprecation cleanup #1509

JoukoVirtanen · 2024-01-19T00:25:22Z

Description

Sets the default collection method to CORE_BPF for collector and the integration tests. Fixed unit tests that broke as a result of changing the default collection method to CORE_BPF. Also removed the hardfail config option for CORE_BPF. The code now behaves as though it is always true.

Checklist

Investigated and inspected CI test results
Ran integration tests locally
~~- [ ] Updated documentation accordingly~~

Automated testing
~~- [ ] Added unit tests~~
~~- [ ] Added integration tests~~
~~- [ ] Added regression tests~~

This changes the default collection method so testing it is not needed.

If any of these don't apply, please comment below.

Testing Performed

TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.

For more details, ref the Confluence page about this section.

collector/test/HostHeuristicsTest.cpp

JoukoVirtanen · 2024-01-25T03:23:51Z

collector/test/HostHeuristicsTest.cpp

@@ -52,33 +52,18 @@ class MockHostInfoHeuristics : public HostInfo {
  MOCK_METHOD0(GetDistro, std::string&());
 };

-// Note that in every test below, ProcessHostHeuristics will be called first
-// with creation of a config mock, which is somewhat annoying, but doesn't
-// cause any serious issues.


This did create a serious issue when the default collection method was changed to CORE_BPF. This is because ProcessHostHeuristics calls Process, which runs the following when the collection method is CORE_BPF

if (!host.HasBPFRingBufferSupport()) { CLOG(FATAL) << "Missing RingBuffer support, core_bpf is not available. " << "HINT: Change collection method to eBPF with collector.collectionMethod=EBPF."; }

Is it not possible to mock out Process as well? I'm not sure it's the best solution to go through config setup directly, because it makes unit tests less "unit", in the sense that they now less isolated and a bit dependent on the outside world (in the form of env vars).

I ended up solving this by moving most of the CollectorConfig constructor into an InitCollectorConfig function, including ProcessHostHeuristics, which called Process and caused the fatal error. I feel like the constructor for CollectorConfig was doing too much and splitting it into two is an improvement.

I considered alternatives such as moving ProcessHostHeuristics into either the HostHeuristics or CollectorConfig classes. Neither option seemed very good. I also considered creating a new class for ProcessHostHeuristics.

JoukoVirtanen · 2024-01-25T03:27:19Z

collector/test/HostHeuristicsTest.cpp

 TEST(HostHeuristicsTest, TestS390XRHEL84) {
  MockS390xHeuristics s390xHeuristics;
  MockHostInfoHeuristics host;
  KernelVersion version = KernelVersion("4.18.0-305.88.1.el8_4.s390x", "", "s390x");
  CollectorArgs* args = CollectorArgs::getInstance();
-  MockCollectorConfig config(args);
+  int argc = 3;
+  const char* argv[] = {"collector", "--collection-method", "ebpf"};


The collection method has to be set here, because CollectorConfig config(args) results in a call to ProcessHostHeuristics and in turn Process. If the collection method is set to the default CORE_BPF in Process a fatal error will be triggered.

Even if it's better than using a mock, it should be refactored to be more reusable.

I am now using the mock again.

collector/test/HostHeuristicsTest.cpp

JoukoVirtanen · 2024-01-25T03:30:56Z

collector/test/NetworkStatusNotifierTest.cpp

+  const char* argv[] = {"collector", "--collection-method", "ebpf"};
+  int exitCode = 0;
+  args->parse(argc, const_cast<char**>(argv), exitCode);
+  CollectorConfig config_(args);


Same as for the HostHeuristicsTests.

Using mocks again.

collector/test/NetworkStatusNotifierTest.cpp

erthalion

Thanks for the PR, few comments below. Also, I've noticed there are couple of
outdated mentions of ebpf in our docs, in "how to start" and "references"
sections -- could you address them as well?

erthalion · 2024-01-31T14:26:16Z

collector/test/HostHeuristicsTest.cpp

@@ -52,33 +52,18 @@ class MockHostInfoHeuristics : public HostInfo {
  MOCK_METHOD0(GetDistro, std::string&());
 };

-// Note that in every test below, ProcessHostHeuristics will be called first
-// with creation of a config mock, which is somewhat annoying, but doesn't
-// cause any serious issues.


Is it not possible to mock out Process as well? I'm not sure it's the best solution to go through config setup directly, because it makes unit tests less "unit", in the sense that they now less isolated and a bit dependent on the outside world (in the form of env vars).

erthalion · 2024-01-31T14:26:43Z

collector/test/HostHeuristicsTest.cpp

 TEST(HostHeuristicsTest, TestS390XRHEL84) {
  MockS390xHeuristics s390xHeuristics;
  MockHostInfoHeuristics host;
  KernelVersion version = KernelVersion("4.18.0-305.88.1.el8_4.s390x", "", "s390x");
  CollectorArgs* args = CollectorArgs::getInstance();
-  MockCollectorConfig config(args);
+  int argc = 3;
+  const char* argv[] = {"collector", "--collection-method", "ebpf"};


Even if it's better than using a mock, it should be refactored to be more reusable.

erthalion · 2024-01-31T14:27:07Z

collector/test/HostHeuristicsTest.cpp

@@ -92,10 +77,13 @@ TEST(HostHeuristicsTest, TestARM64Heuristic) {
  MockHostInfoHeuristics host;
  KernelVersion version = KernelVersion("6.5.5-200.fc38.aarch64", "", "aarch64");
  CollectorArgs* args = CollectorArgs::getInstance();
-  MockCollectorConfig config(args);
+  int argc = 3;
+  const char* argv[] = {"collector", "--collection-method", "ebpf"};


Why is it configured to use ebpf everywhere?

These tests originally specified ebpf as the collection method. I have not changed that. When we remove ebpf these tests will be changed or removed.

Even if it's better than using a mock, it should be refactored to be more reusable.

I am using mocks now, so this comment is outdated.

ovalenti

We probably want to default to core_bpf in the case where the collectionMethod provided by the user is not valid (or no more valid...).

https://github.com/stackrox/collector/pull/1509/files#diff-481ca574af93ac9fb58516ac8df6705b55eff51252c30680b711d4f453ab6f44L132-L135

…ng MockCollectorConfig again in tests

JoukoVirtanen · 2024-02-07T00:39:08Z

We probably want to default to core_bpf in the case where the collectionMethod provided by the user is not valid (or no more valid...).

https://github.com/stackrox/collector/pull/1509/files#diff-481ca574af93ac9fb58516ac8df6705b55eff51252c30680b711d4f453ab6f44L132-L135

Done

JoukoVirtanen added 2 commits January 18, 2024 16:12

X-Smart-Branch-Parent: master

bfb44dd

Changed default collection methods

4b7c4c0

JoukoVirtanen requested a review from a team as a code owner January 19, 2024 00:25

Fixed unit tests

e8716d6

JoukoVirtanen commented Jan 25, 2024

View reviewed changes

collector/test/HostHeuristicsTest.cpp Outdated Show resolved Hide resolved

JoukoVirtanen commented Jan 25, 2024

View reviewed changes

collector/test/HostHeuristicsTest.cpp Show resolved Hide resolved

JoukoVirtanen commented Jan 25, 2024

View reviewed changes

collector/test/HostHeuristicsTest.cpp Outdated Show resolved Hide resolved

JoukoVirtanen commented Jan 25, 2024

View reviewed changes

collector/test/NetworkStatusNotifierTest.cpp Outdated Show resolved Hide resolved

JoukoVirtanen commented Jan 25, 2024

View reviewed changes

collector/test/NetworkStatusNotifierTest.cpp Outdated Show resolved Hide resolved

Removed the hardfail config. It behaves as it is true

2614602

erthalion reviewed Jan 31, 2024

View reviewed changes

JoukoVirtanen added 2 commits February 2, 2024 16:34

Working on updating docs

6c10210

Made more updates to documentation

c244e8b

ovalenti reviewed Feb 5, 2024

View reviewed changes

JoukoVirtanen added 2 commits February 6, 2024 14:42

Moved most of CollectorConfig constructor to InitCollectorConfig. Usi…

ef66e06

…ng MockCollectorConfig again in tests

Making it so that invalid collection methods are switched to core bpf

0b7f5d5

JoukoVirtanen requested review from erthalion and ovalenti February 7, 2024 00:37

erthalion approved these changes Feb 7, 2024

View reviewed changes

JoukoVirtanen merged commit aea6d9d into master Feb 7, 2024
48 checks passed

JoukoVirtanen deleted the jv-ebpf-deprecation-cleanup branch February 7, 2024 16:35

erthalion mentioned this pull request Feb 12, 2024

Ebpf deprecation cleanup #1496

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jv ebpf deprecation cleanup #1509

Jv ebpf deprecation cleanup #1509

JoukoVirtanen commented Jan 19, 2024 •

edited

Loading

JoukoVirtanen Jan 25, 2024

erthalion Jan 31, 2024

JoukoVirtanen Feb 7, 2024 •

edited

Loading

JoukoVirtanen Jan 25, 2024

erthalion Jan 31, 2024

JoukoVirtanen Feb 7, 2024

JoukoVirtanen Jan 25, 2024

JoukoVirtanen Feb 7, 2024

erthalion left a comment

erthalion Jan 31, 2024

erthalion Jan 31, 2024

erthalion Jan 31, 2024

JoukoVirtanen Feb 7, 2024

JoukoVirtanen Feb 7, 2024

ovalenti left a comment

JoukoVirtanen commented Feb 7, 2024

Jv ebpf deprecation cleanup #1509

Jv ebpf deprecation cleanup #1509

Conversation

JoukoVirtanen commented Jan 19, 2024 • edited Loading

Description

Checklist

Testing Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoukoVirtanen Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erthalion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ovalenti left a comment

Choose a reason for hiding this comment

JoukoVirtanen commented Feb 7, 2024

JoukoVirtanen commented Jan 19, 2024 •

edited

Loading

JoukoVirtanen Feb 7, 2024 •

edited

Loading