Skip to content

ROX-9409: sets DRIVER_NAME in collector CMakeLists#601

Merged
Stringy merged 21 commits into
masterfrom
giles/ROX-9409-setting-probe-name-and-probe-version
May 30, 2022
Merged

ROX-9409: sets DRIVER_NAME in collector CMakeLists#601
Stringy merged 21 commits into
masterfrom
giles/ROX-9409-setting-probe-name-and-probe-version

Conversation

@Stringy

@Stringy Stringy commented Feb 22, 2022

Copy link
Copy Markdown
Collaborator

Description

A detailed explanation of the changes in your PR.

Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

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.

@ghost

ghost commented Feb 22, 2022

Copy link
Copy Markdown
COLLECTOR_TAG=3.8.x-79-g71d9891f97
COLLECTOR_BUILDER_TAG=cache

Results for Performance Benchmarks on build #251959

Kernel Method Without Collector Time (secs) With Collector Time (secs) Baseline median (secs) Collector median (secs) PValue
cos.cos-85-lts ebpf 139.667 400.862 158.55 202.5 🔴
cos.cos-89-lts ebpf 144.515 434.698 182.27 233.56 🔴
cos.cos-beta ebpf 230.023 452.573 181.55 203.1 🔴
cos.cos-dev ebpf 162.929 336.802 187.73 203.53 🔴
cos.cos-stable ebpf 221.057 447.054 197.85 221.51 🔴
flatcar.flatcar-stable ebpf 201.675 436.472 208.76 214.79 🔴
flatcar.flatcar-stable module 204.849 224.534 204.44 206.5 🟢
garden-linux.garden-linux ebpf 244.398 468.9 230.59 251.63 🔴
garden-linux.garden-linux module 252.455 473.839 226.88 249.69 🔴
rhel.rhel-7 ebpf 98.632 296.855 173.52 212.23 🔴
rhel.rhel-7 module 246.71 279.454 183.38 208.09 🟢
rhel.rhel-8 ebpf 215.555 464.046 161.54 201.51 🔴
rhel.rhel-8 module 219.341 239.939 160.78 180.39 🟢
suse.sles-12 module 170.56 203.378 121.75 140.51 🔴
suse.sles-15 ebpf 97.531 318.267 122.24 162.71 🔴
suse.sles-15 module 173.93 195.676 120.48 138.34 🟢
ubuntu-os-pro.ubuntu-pro-1804-lts ebpf 188.005 457.849 165.7 206.6 🔴
ubuntu-os-pro.ubuntu-pro-1804-lts module 127.537 156.169 161.8 186.74 🟢
ubuntu-os.ubuntu-1804-lts ebpf 118.697 372.288 149.2 201.05 🔴
ubuntu-os.ubuntu-1804-lts module 197.111 227.75 162.92 186.49 🟢
ubuntu-os.ubuntu-2004-lts ebpf 209.898 447.169 150.75 175.6 🔴
ubuntu-os.ubuntu-2004-lts module 207.053 228.222 182.46 204.48 🟢
ubuntu-os.ubuntu-2204-lts ebpf 217.478 452.727 204.84 230.01 🔴
ubuntu-os.ubuntu-2204-lts module 217.261 232.969 201.46 211.76 🟢

@Stringy Stringy marked this pull request as ready for review February 22, 2022 14:53
@Stringy Stringy requested a review from Molter73 February 22, 2022 14:53
@Molter73

Molter73 commented Feb 25, 2022

Copy link
Copy Markdown
Collaborator

Can you change the commit in the falcosecurity-libs sub-module so it points to the head of the branch on stackrox/falcosecurity-libs#3? We'll roll it back before merging, but it would be nice to see how it interacts with CI.

We should also run our CI with no-cache and dockerized-no-cache to ensure compilation of drivers is not broken by this change.

@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from fb82e5f to 10606bc Compare March 14, 2022 10:40
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from 827f00a to c64bab0 Compare March 16, 2022 17:15
@Stringy Stringy requested a review from a team March 17, 2022 11:56

@Molter73 Molter73 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking good! Just have a couple of questions on some of the versioning stuff.

Comment thread collector/CMakeLists.txt Outdated
Comment thread .gitmodules Outdated
Comment thread kernel-modules/MODULE_VERSION
Comment thread collector/CMakeLists.txt Outdated
@Stringy Stringy requested a review from a team March 22, 2022 10:27

@Molter73 Molter73 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just one more comment but it shouldn't be a blocker. LGTM!

Comment thread collector/container/Dockerfile.ubi Outdated
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from 914dfe8 to 53c56dd Compare March 23, 2022 10:12
@Stringy Stringy requested a review from a team March 23, 2022 15:14
@Molter73

Copy link
Copy Markdown
Collaborator

I was just going through the CI run on a PR that builds onto these changes and noticed that the legacy drivers build procedure would be broken, since the prepare-src script now sets DRIVER_NAME but legacy drivers still expect the PROBE_NAME variable when running cmake. I think the easiest fix would be to simply set both variables in the cmake command inside prepare-src, the one that doesn't belong in the build will simply be ignored. Another possibility would be to use cmake -LA as detailed in this SO answer, grep'ing the output and adjust the arguments as needed, but I think this might be a little bit overkill.

We should also add the build-legacy-probes label to the PR and re-run CI to double check.

@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from 0c907d0 to ef3281b Compare March 24, 2022 14:33
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from ef3281b to 62b83d4 Compare April 29, 2022 14:13
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from 62b83d4 to fb11b76 Compare May 6, 2022 10:51
@Stringy Stringy removed the no-cache label May 10, 2022
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from 4f8f7a5 to be07b16 Compare May 12, 2022 15:28
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from be07b16 to e731220 Compare May 12, 2022 15:33
Stringy added 18 commits May 25, 2022 10:10
Instead of the entire kernel-modules directory.
@Stringy Stringy force-pushed the giles/ROX-9409-setting-probe-name-and-probe-version branch from e731220 to 1148f80 Compare May 25, 2022 09:12
@Stringy Stringy requested a review from Molter73 May 25, 2022 14:18

@Molter73 Molter73 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@Stringy Stringy merged commit 2bcb41a into master May 30, 2022
@Stringy Stringy deleted the giles/ROX-9409-setting-probe-name-and-probe-version branch May 30, 2022 07:20
@robbycochran

Copy link
Copy Markdown
Collaborator

@Stringy It looks like this change degraded the performance of the ebpf probe significantly based on the reported collector times and this regression remains on the custom probe for rhel7. I suspect that the culprit may be this commit which removed the usage of g_bpf_drop_syscalls.

@Molter73 Molter73 mentioned this pull request Jun 1, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants