Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Build the collector binary directly in the docker image build #1827

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Build the collector binary directly in the docker image build #1827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a4c0800
Build the collector binary directly in the docker image build
Molter73 Sep 5, 2024
85ab2f0
Strip collector binary
Molter73 Sep 5, 2024
8d9b3a7
Debug ansible
Molter73 Sep 5, 2024
96817ea
Remove broken collector target
Molter73 Sep 5, 2024
0d76ded
Unify image and image-dev targets
Molter73 Sep 6, 2024
a828140
Copy the correct kernel-modules directory
Molter73 Sep 6, 2024
39a0b7c
Ignore as much stuff as possible from the repo
Molter73 Sep 6, 2024
c519303
Don't run the builder during image builds
Molter73 Sep 6, 2024
a5b9bf3
Minor cleanup
Molter73 Sep 6, 2024
04a8fbe
Only strip collector image on release builds
Molter73 Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .dockerignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
*
**
!collector/
!falcosecurity-libs/
!CMakeLists.txt
Comment on lines +1 to +5
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

27 changes: 16 additions & 11 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ NPROCS ?= $(shell nproc)
DEV_SSH_SERVER_KEY ?= $(CURDIR)/.collector_dev_ssh_host_ed25519_key
BUILD_BUILDER_IMAGE ?= false

DOCKERFILE = collector/container/Dockerfile
BUILD_TYPE = rhel

export COLLECTOR_VERSION := $(COLLECTOR_TAG)

.PHONY: tag
Expand All @@ -18,7 +21,7 @@ builder-tag:

.PHONY: container-dockerfile-dev
container-dockerfile-dev:
sed '1s/ubi-minimal/ubi/' $(CURDIR)/collector/container/Dockerfile > \
sed 's/ubi-minimal/ubi/' $(CURDIR)/collector/container/Dockerfile > \
$(CURDIR)/collector/container/Dockerfile.dev

.PHONY: builder
Expand All @@ -41,22 +44,24 @@ connscrape:
unittest:
make -C collector unittest

image: collector unittest
image:
make -C collector txt-files
docker buildx build --load --platform ${PLATFORM} \
--build-arg BUILD_TYPE="$(BUILD_TYPE)" \
--build-arg CMAKE_BUILD_TYPE="$(CMAKE_BUILD_TYPE)" \
--build-arg USE_VALGRIND="$(USE_VALGRIND)" \
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

--build-arg ADDRESS_SANITIZER="$(ADDRESS_SANITIZER)" \
--build-arg TRACE_SINSP_EVENTS="$(TRACE_SINSP_EVENTS)" \
--build-arg BPF_DEBUG_MODE="$(BPF_DEBUG_MODE)" \
--build-arg COLLECTOR_VERSION="$(COLLECTOR_TAG)" \
-f collector/container/Dockerfile \
--build-arg BUILDER_TAG="$(COLLECTOR_BUILDER_TAG)" \
-f "$(DOCKERFILE)" \
-t quay.io/stackrox-io/collector:$(COLLECTOR_TAG) \
$(COLLECTOR_BUILD_CONTEXT)

image-dev: collector unittest container-dockerfile-dev
make -C collector txt-files
docker buildx build --load --platform ${PLATFORM} \
--build-arg COLLECTOR_VERSION="$(COLLECTOR_TAG)" \
--build-arg BUILD_TYPE=devel \
-f collector/container/Dockerfile.dev \
-t quay.io/stackrox-io/collector:$(COLLECTOR_TAG) \
$(COLLECTOR_BUILD_CONTEXT)
image-dev: DOCKERFILE = collector/container/Dockerfile.dev
image-dev: BUILD_TYPE = devel
image-dev: container-dockerfile-dev image

.PHONY: integration-tests-report
integration-tests-report:
Expand Down
2 changes: 1 addition & 1 deletion Makefile-constants.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ TRACE_SINSP_EVENTS ?= false
DISABLE_PROFILING ?= false
BPF_DEBUG_MODE ?= false

COLLECTOR_BUILD_CONTEXT = collector/
COLLECTOR_BUILD_CONTEXT = $(CURDIR)
COLLECTOR_BUILDER_NAME ?= collector_builder_$(HOST_ARCH)
2 changes: 1 addition & 1 deletion ansible/ci-build-builder.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
- name: Build and push collector image
- name: Build and push collector-builder image
hosts: "{{ build_hosts | default('all') }}"

environment:
Expand Down
5 changes: 0 additions & 5 deletions ansible/ci-build-collector.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@
recursive: true
when: arch == "s390x"

- name: Run the builder image
community.general.make:
chdir: "{{ ansible_env.GITHUB_WORKSPACE | default(collector_root) }}"
target: start-builder

- name: Build the collector image
community.general.make:
chdir: "{{ ansible_env.GITHUB_WORKSPACE | default(collector_root) }}"
Expand Down
37 changes: 1 addition & 36 deletions collector/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,45 +5,10 @@ NPROCS ?= $(shell nproc)

CMAKE_BASE_DIR = cmake-build
CMAKE_DIR= $(BASE_PATH)/$(CMAKE_BASE_DIR)
COLLECTOR_BIN_DIR = $(CMAKE_DIR)/collector
LIBSINSP_BIN_DIR = $(CMAKE_DIR)/collector/EXCLUDE_FROM_DEFAULT_BUILD/libsinsp
SRC_MOUNT_DIR = /tmp/collector

HDRS := $(wildcard lib/*.h) $(shell find $(BASE_PATH)/falcosecurity-libs/userspace -name '*.h')

SRCS := $(wildcard lib/*.cpp) collector.cpp

COLLECTOR_BUILD_DEPS := $(HDRS) $(SRCS) $(shell find $(BASE_PATH)/falcosecurity-libs -name '*.h' -o -name '*.cpp' -o -name '*.c')

.SUFFIXES:

cmake-configure/collector:
docker exec $(COLLECTOR_BUILDER_NAME) \
cmake -S $(BASE_PATH) -B $(CMAKE_DIR) \
-DCMAKE_BUILD_TYPE=$(CMAKE_BUILD_TYPE) \
-DDISABLE_PROFILING=$(DISABLE_PROFILING) \
-DUSE_VALGRIND=$(USE_VALGRIND) \
-DADDRESS_SANITIZER=$(ADDRESS_SANITIZER) \
-DTRACE_SINSP_EVENTS=$(TRACE_SINSP_EVENTS) \
-DBPF_DEBUG_MODE=$(BPF_DEBUG_MODE) \
-DCOLLECTOR_VERSION=$(COLLECTOR_VERSION)

cmake-build/collector: cmake-configure/collector $(COLLECTOR_BUILD_DEPS)
docker exec $(COLLECTOR_BUILDER_NAME) \
cmake --build $(CMAKE_DIR) -- -j $(NPROCS)
docker exec $(COLLECTOR_BUILDER_NAME) \
bash -c "[ $(CMAKE_BUILD_TYPE) == Release ] && strip --strip-unneeded $(COLLECTOR_BIN_DIR)/collector || exit 0"

container/bin/collector: cmake-build/collector
mkdir -p container/bin
cp "$(COLLECTOR_BIN_DIR)/collector" container/bin/collector
cp "$(COLLECTOR_BIN_DIR)/self-checks" container/bin/self-checks

.PHONY: collector
collector: container/bin/collector txt-files
mkdir -p container/libs
docker cp $(COLLECTOR_BUILDER_NAME):/THIRD_PARTY_NOTICES/ container/

.PHONY: build-connscrape-test
build-connscrape-test:
docker build -f $(CURDIR)/connscrape-test/Dockerfile -t connscrape-test $(CURDIR)/connscrape-test
Expand All @@ -54,7 +19,7 @@ connscrape: build-connscrape-test
-v "$(BASE_PATH):$(SRC_MOUNT_DIR)" \
connscrape-test "$(SRC_MOUNT_DIR)/collector/connscrape-test/connscrape-test.sh"

unittest: collector
unittest:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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>

docker exec $(COLLECTOR_BUILDER_NAME) \
ctest -V --test-dir $(CMAKE_DIR)

Expand Down
48 changes: 42 additions & 6 deletions collector/container/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,39 @@
ARG BUILDER_TAG=master
FROM quay.io/stackrox-io/collector-builder:${BUILDER_TAG} AS builder

ARG CMAKE_BUILD_TYPE=Release
ARG USE_VALGRIND=false
ARG ADDRESS_SANITIZER=false
ARG TRACE_SINSP_EVENTS=false
ARG COLLECTOR_VERSION=0.0.0
ARG BPF_DEBUG_MODE=false

COPY . /collector-src
WORKDIR /collector-src

RUN --mount=type=cache,target=/collector-src/build \
if [[ "$(uname -m)" == "x86_64" ]]; \
then DISABLE_PROFILING="OFF"; \
else DISABLE_PROFILING="ON"; \
fi ; \
cmake -S . -B build/ \
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} \
-DDISABLE_PROFILING=${DISABLE_PROFILING} \
-DUSE_VALGRIND=${USE_VALGRIND} \
-DADDRESS_SANITIZER=${ADDRESS_SANITIZER} \
-DCOLLECTOR_VERSION=${COLLECTOR_VERSION} \
-DBPF_DEBUG_MODE=${BPF_DEBUG_MODE} \
-DTRACE_SINSP_EVENTS=${TRACE_SINSP_EVENTS} && \
cmake --build build/ -- -j$(nproc) && \
ctest -V --test-dir build && \
# podman does not bake cache mounts into the final image, so
# we need to move the required binaries out of it.
if [[ "${CMAKE_BUILD_TYPE}" == "Release" ]]; \
then strip -v --strip-unneeded -o /tmp/collector build/collector/collector; \
else cp build/collector/collector /tmp/; \
fi ; \
cp build/collector/self-checks /tmp/

FROM registry.access.redhat.com/ubi9/ubi-minimal:9.3

ARG BUILD_TYPE=rhel
Expand All @@ -16,17 +52,17 @@ LABEL name="collector" \

WORKDIR /

COPY container/${BUILD_TYPE}/install.sh /
COPY collector/container/${BUILD_TYPE}/install.sh /
RUN ./install.sh && rm -f install.sh

# Uncomment this line to enable generation of core for collector
# RUN echo '/core/core.%e.%p.%t' > /proc/sys/kernel/core_pattern

COPY container/THIRD_PARTY_NOTICES/ /THIRD_PARTY_NOTICES/
COPY kernel-modules /kernel-modules
COPY container/bin/collector /usr/local/bin/
COPY container/bin/self-checks /usr/local/bin/self-checks
COPY container/status-check.sh /usr/local/bin/status-check.sh
COPY --from=builder /THIRD_PARTY_NOTICES/ /THIRD_PARTY_NOTICES/
COPY collector/kernel-modules /kernel-modules
COPY --from=builder /tmp/collector /usr/local/bin/
COPY --from=builder /tmp/self-checks /usr/local/bin/self-checks
COPY collector/container/status-check.sh /usr/local/bin/status-check.sh

EXPOSE 8080 9090

Expand Down
Loading