Build the collector binary directly in the docker image build

[Molter73]

Description

Historically, we've always built the collector binary inside a running container and then copied it from the host to the final image. This change makes it so the builder image is used as a first step to compile the collector binary, then the binary is copied over to the final image in a second stage.

Development workflow should not change too much, make image will still compile collector, run the unit tests and generate a final image. Multiple successive compilations are sped up by using a cache mount for the build directory.

Main benefit of the change is getting rid of the make targets that run cmake while exec'ed into the builder image, these don't really make much sense and are quite a bit more complex than they need to be IMO. Of note, execing into a builder image and running the cmake commands directly in there will still work, if that is the workflow of choice.

Second benefit is making image builds a bit closer to what konflux and downstream do, but not overly so, maybe one day we'll be able to have all image builds come from a single source of truth.

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

• Using a builder image that is not master works. (CI run)
• Multiarch builds?

[Molter73]

If we move the definition of the project to the collector/CMakeLists.txt file and move the falco directory into collector/ as well, we should be able to directly use that directory as the context for the image build. However, when I tried it, it give me a weird error locally and I don't want this PR to get out of hand, so we could consider doing that in a follow up.

[Molter73]

This target should still work, but you'll need to exec into the image and run cmake/make yourself, at that point you should be able to directly run ctest though.

[JoukoVirtanen]

That does not seem convenient. I should be able to run make unittest and have it work as normal.

[Molter73]

Honestly, I would rather just remove the target altogether, I just thought leaving it could be nice.

IMO, running the unit tests should be done by exec'ing into the container from a terminal anyways, cd cmake-build && ctest is really not that long to type and, once you are in there, it shortens to ctest or up arrow + <CR>

[ovalenti]

I think that we should keep the configuration/build steps inside Makefiles and then call make from within the Dockerfile. This would make it much easier to run this step manually from within the builder image.

edit: with this idea, env vars are going to be a problem...

[Molter73]

I think that we should keep the configuration/build steps inside Makefiles and then call make from within the Dockerfile. This would make it much easier to run this step manually from within the builder image.

edit: with this idea, env vars are going to be a problem...

I would agree to keeping some cmake commands in our makefiles if running them manually was a problem, but honestly, mkdir build && cd build && cmake .. && make works, the most complicated cmake command I've had to run is cmake -DADDRESS_SANITIZER=ON .., so even if it is not as convenient as make, it is still manageable I think.

An alternative we could consider is setting up a CMakePresets.json with different configurations so we could directly do things like cmake --preset=default or cmake --preset=asan. But again, we don't have that many options anyways.

[ovalenti]

so even if it is not as convenient as make, it is still manageable I think

I agree. I did not realize that COLLECTOR_VERSION could just be left unset.

I think that cmake presets would be overkill. Thanks for your answer :D

[JoukoVirtanen]

Does USE_VALGRIND even do anything now that the wrapper script has been removed? If it doesn't, it would be nice to make it work in another PR.

[Molter73]

It disables profiling here. Think that was due to tcmalloc not being compatible with valgrind because they both overwrite the malloc implementation.

[Molter73]

I don't know how I missed this when I wrote my answer, but USE_VALGRIND is also used for unit tests to be run under valgrind, so it still has a use.

[JoukoVirtanen]

I looked at the valgrind unit tests. Valgrind found many errors, but the test passed. They all seemed to have to do with RE2, so maybe it is not a huge concern. If valgrind finds errors, the test should fail. Or are we ignoring the RE2 errors?

[Molter73]

I remember trying to ignore them but can't remember why I gave up on it, there's a way to set up an ignore file for valgrind (https://valgrind.org/docs/manual/manual-core.html/manual-core.html#manual-core.suppress).

I also remember the errors where in a place that would be really hard to actually fix and didn't seem like too big a deal. IIRC, the error was on the initialization of the re2 regexes that Falco does statically, so if it was a serious issue we would see collector crashing at start up. I'm also pretty sure we don't use the regexes.