fix(libsinsp/runc): use old logic and fallback for containerd #2235
+41
−38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Roberto Scolaro <[email protected]>
poiana
added
kind/bug
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: therealbobo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
therealbobo
changed the title
Signed-off-by: Roberto Scolaro <[email protected]>
therealbobo
force-pushed
the
therealbobo-misc
branch
from
4ff571b to
810b2d6
Compare
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2235 +/- ##
==========================================
+ Coverage 75.09% 75.11% +0.02%
==========================================
Files 276 276
Lines 34391 34395 +4
Branches 5927 5923 -4
==========================================
+ Hits 25826 25836 +10
+ Misses 8565 8559 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
therealbobo
commented
Jan 15, 2025
| @@ -577,17 +578,18 @@ void sinsp_container_manager::create_engines() { | |||
| cri_settings.add_cri_unix_socket_path("/run/containerd/containerd.sock"); | |||
| cri_settings.add_cri_unix_socket_path("/run/crio/crio.sock"); | |||
| cri_settings.add_cri_unix_socket_path("/run/k3s/containerd/containerd.sock"); | |||
| cri_settings.add_cri_unix_socket_path("/run/host-containerd/containerd.sock"); | |||
There was a problem hiding this comment.
The bottlerocket can be used as a cri socket.
therealbobo
commented
Jan 15, 2025
| for(auto socket_path : cri_socket_paths) { | ||
| auto cri_engine = | ||
| std::make_shared<container_engine::cri>(*this, socket_path, engine_index); | ||
| m_container_engines.push_back(cri_engine); | ||
| m_container_engine_by_type[CT_CRI].push_back(cri_engine); | ||
| m_container_engine_by_type[CT_CRIO].push_back(cri_engine); | ||
| m_container_engine_by_type[CT_CONTAINERD].push_back(cri_engine); |
There was a problem hiding this comment.
There are various code path that assume that a cri runtime could have containerd as type.
therealbobo
commented
Jan 15, 2025
| @@ -617,7 +619,8 @@ void sinsp_container_manager::create_engines() { | |||
| m_container_engine_by_type[CT_BPM].push_back(bpm_engine); | |||
| } | |||
| if(m_container_engine_mask & (1 << CT_CONTAINERD)) { | |||
| auto containerd_engine = std::make_shared<container_engine::containerd>(*this); | |||
| auto containerd_engine = | |||
| std::make_shared<container_engine::containerd>(*this, engine_index); | |||
There was a problem hiding this comment.
The containerd engine should address also by engine_index given that also the cri engines can be of containerd type. This way we always access the right container cache.
therealbobo
commented
Jan 15, 2025
Comment on lines
+62
to
+67
| if(end_pos - start_pos == CONTAINER_ID_LENGTH && | ||
| cgroup.find_first_not_of(CONTAINER_ID_VALID_CHARACTERS, start_pos) >= CONTAINER_ID_LENGTH) { | ||
| container_id = cgroup.substr(start_pos, REPORTED_CONTAINER_ID_LENGTH); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Just going back to the old logic but, this time, we don't fail right away and we check if the cgroup name is compatible with a containerd one.
…een containerd sockets Signed-off-by: Roberto Scolaro <[email protected]>
therealbobo
force-pushed
the
therealbobo-misc
branch
from
a2deb8d to
46d0c28
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
The new logic is not always accurate. This readopts the old logic and adds a fallback to support containerd vanilla containers.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: