falco

falco copied to clipboard

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

We recently increased the default threadtable size to accommodate modern infrastructures better as we have observed drops related to a full internal thread table. This PR makes this setting configurable by the end user.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(config): add state_engine.thread_table_size

/milestone 0.38.0

One thought after a quick look.

I would like to group some top-level config keys instead of continuously adding new ones.

I mean, the top-level config keys are growing exponentially, and this may not be ideal for users approaching the configuration.

What would it be? It seems that we don't have a new top level category for state_engine related configs that may grow over time?

maybe we could add a key called libsinsp or internals that in the future will collect all the libs configurations... for example, we already have the libs_logger config that we could move behind this new config (not now of course) WDYT?

@Andreagit97 that's a great idea actually, even if it is exposing some "internal knowledge" about who owns what. I'd rather call it just falco-libs:.

@Andreagit97 that's a great idea actually, even if it is exposing some "internal knowledge" about who owns what. I'd rather call it just falco-libs:.

falco_libs with underscore instead? Perhaps in the future the libs_logger can be moved under falco_libs as well. It should be less of a breaking change as likely few folks use that setting.

Additional question: Do we know what's wrong with the Windows build?

falco_libs with underscore instead?

Yep, we need to use the underscore

In general, I wouldn't say I like the idea of exposing internal names to the users (because internals can change, but the external function remains the same).

So perhaps something like libs_internals (to make it explicitly, these are internal settings and may potentially change very often)

However, falco_libs or just libs is still for me if the consensus goes toward this.

@geraldcombs using the latest libs commit and also rebased w/ falco master, still having the windows failure. Could you take another look? Thank you!

@geraldcombs using the latest libs commit and also rebased w/ falco master, still having the windows failure. Could you take another look? Thank you!

I'm able to replicate the issue here. If I make the following change here

--- a/.github/workflows/reusable_build_packages.yaml
+++ b/.github/workflows/reusable_build_packages.yaml
@@ -233,7 +233,7 @@ jobs:
         run: |
           mkdir build
           cd build
-          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DFALCO_VERSION=${{ inputs.version }} ..
+          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded -DFALCO_VERSION=${{ inputs.version }} ..

I can build and run falco_unit_tests.exe, although I'm not sure why that's necessary. If I build a standalone version of libs here without setting CMAKE_MSVC_RUNTIME_LIBRARY I get a static build.

Thanks @geraldcombs I'll then rebase, thanks!

@geraldcombs using the latest libs commit and also rebased w/ falco master, still having the windows failure. Could you take another look? Thank you!

I'm able to replicate the issue here. If I make the following change here

--- a/.github/workflows/reusable_build_packages.yaml
+++ b/.github/workflows/reusable_build_packages.yaml
@@ -233,7 +233,7 @@ jobs:
         run: |
           mkdir build
           cd build
-          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DFALCO_VERSION=${{ inputs.version }} ..
+          cmake -DCMAKE_BUILD_TYPE=Release -DMINIMAL_BUILD=On -DUSE_BUNDLED_DEPS=On -DBUILD_FALCO_UNIT_TESTS=On -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded -DFALCO_VERSION=${{ inputs.version }} ..

I can build and run falco_unit_tests.exe, although I'm not sure why that's necessary. If I build a standalone version of libs here without setting CMAKE_MSVC_RUNTIME_LIBRARY I get a static build.

@geraldcombs just double-checking if I interpret this correctly. You or someone else will open a PR and I shall rebase afterwards? Thanks in advance for clarifying!

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

@jasondellaluce already bumped libs before hence I could remove it here. However, the CI still seems to be in a not good state (unrelated to these changes).

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

@jasondellaluce Could you elaborate? I tried to pick a name that addresses all feedback. If this should not be it, what should the final name be? No strong opinion.

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

@jasondellaluce Could you elaborate? I tried to pick a name that addresses all feedback. If this should not be it, what should the final name be? No strong opinion.

I think I'd go with either falco_libs or libs_internals. But again, just my personal taste!

@incertum I have no major concerns here if not the naming decision, as pointed out above. I'm pretty much onboard with the suggestions of the other reviewers.

@jasondellaluce Could you elaborate? I tried to pick a name that addresses all feedback. If this should not be it, what should the final name be? No strong opinion.

I think I'd go with either falco_libs or libs_internals. But again, just my personal taste!

After thinking about this again, I believe falco_libs can be nice, especially if we start distributing Falco's libs at some point in the future.

SGTM, just changed it. Thanks.

LGTM label has been added.

Git tree hash: 2a6cb1f153d2edb03f88bba04b0f167decafb967

LGTM label has been added.

Git tree hash: 8be4088d216f0f73aa595b0db4a073ca47c744dd

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, incertum, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [Andreagit97,FedeDP,incertum,leogr]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

/unhold