feat(libsinsp/container_engine): proper containerd support #2195
Conversation
poiana
added
do-not-merge/work-in-progress
kind/feature
|
[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
force-pushed
the
broader-container-id-support
branch
from
893a76f to
7c3c360
Compare
userspace/libsinsp/container.cpp
Outdated
| if(m_container_engine_mask & (1 << CT_CONTAINERD)) { | ||
| auto containerd_engine = std::make_shared<container_engine::containerd>(*this); | ||
| m_container_engines.push_back(containerd_engine); | ||
| // m_container_engine_by_type[CT_CONTAINERD] = containerd_engine; |
There was a problem hiding this comment.
This depends on this #2141
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 #2195 +/- ##
==========================================
- Coverage 75.38% 75.05% -0.34%
==========================================
Files 264 266 +2
Lines 34014 34189 +175
Branches 5805 5914 +109
==========================================
+ Hits 25642 25661 +19
- Misses 8372 8528 +156
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
therealbobo
force-pushed
the
broader-container-id-support
branch
from
7c3c360 to
b0e0a90
Compare
therealbobo
force-pushed
the
broader-container-id-support
branch
from
b0e0a90 to
bc7d08d
Compare
therealbobo
force-pushed
the
broader-container-id-support
branch
5 times, most recently
from
82b4b68 to
3371202
Compare
therealbobo
force-pushed
the
broader-container-id-support
branch
2 times, most recently
from
2dc0158 to
3978470
Compare
therealbobo
force-pushed
the
broader-container-id-support
branch
from
3978470 to
3c6b970
Compare
therealbobo
changed the title
…one CRI path Signed-off-by: Leonardo Grasso <[email protected]>
…ltiple CRI engines simultaneously Signed-off-by: Leonardo Grasso <[email protected]>
…et path Signed-off-by: Leonardo Grasso <[email protected]>
Signed-off-by: Roberto Scolaro <[email protected]>
Signed-off-by: Roberto Scolaro <[email protected]>
Signed-off-by: Roberto Scolaro <[email protected]>
Signed-off-by: Roberto Scolaro <[email protected]>
Signed-off-by: Roberto Scolaro <[email protected]>
…ival Signed-off-by: Roberto Scolaro <[email protected]>
Signed-off-by: Roberto Scolaro <[email protected]>
therealbobo
force-pushed
the
broader-container-id-support
branch
from
cf13c54 to
13aead4
Compare
| @@ -393,7 +397,7 @@ TEST_F(sys_call_test, container_docker_bad_socket) { | |||
| ASSERT_NE(PPME_CONTAINER_JSON_2_E, param.m_evt->get_type()); | |||
|
|
|||
| sinsp_threadinfo* tinfo = param.m_evt->get_thread_info(false); | |||
| ASSERT_TRUE(tinfo->m_container_id.length() == 12); | |||
| ASSERT_TRUE(tinfo->m_container_id.length() <= 12); | |||
There was a problem hiding this comment.
Care to expand on this change?
There was a problem hiding this comment.
Until now the container.id field was just the truncated version of the container.full_id which was a 64 char string. This is not always the case (e.g. containerd permits to define an arbitrary container id string) and we should accept container id of arbitrary length. Given that, the container id can now be less than 12 chars.
| @@ -68,6 +70,7 @@ void event_capture::start(bool dump, libsinsp::events::set<ppm_sc_code>& sc_set) | |||
| } | |||
| } | |||
| } | |||
| m_before_open(m_inspector.get()); | |||
There was a problem hiding this comment.
Why did you need to introduce these new callback?
There was a problem hiding this comment.
Also, if it needs to stay, at the very least i'd rename before_close to after_capture too.
There was a problem hiding this comment.
The container engines are initialised before the start of the capture and so we need an hook to set the cri settings before the start of the capture and after the open.
Also, if it needs to stay, at the very least i'd rename before_close to after_capture too.
👍
|
|
||
| target_link_libraries(sinsp PUBLIC cri_v1alpha2 cri_v1) | ||
| target_link_libraries(sinsp PRIVATE containerd_interface) |
There was a problem hiding this comment.
Do you think that cri_v1alpha2 and cri_v1 can be moved to PRIVATE too?
There was a problem hiding this comment.
Shouldn't be a problem given that the libsinsp client shouldn't access the cri interfaces (at least not directly).
| @@ -250,6 +251,82 @@ function(prepare_cri_grpc api_version) | |||
| endif() | |||
| endfunction() | |||
|
|
|||
| function(prepare_containerd_grpc) | |||
There was a problem hiding this comment.
This function seems pretty similar to the prepare_cri_grpc above; do you think we can easily extract a single cmake helper?
I am not sure though, there are some subtle differences.
There was a problem hiding this comment.
This is not a great way of doing things but I'll clean up most of it with a later PR, once we upgrade protobuf we can use better helpers.
| m_container_engine_by_type[CT_CRI] = cri_engine; | ||
| m_container_engine_by_type[CT_CRIO] = cri_engine; | ||
| m_container_engine_by_type[CT_CONTAINERD] = cri_engine; | ||
| // Get CRI socket paths from settings |
There was a problem hiding this comment.
Is this PR still based on #2141?
These changes seems related to that PR. In this case, i'd move this PR as wip until the other gets merged and this one rebased.
| @@ -224,13 +232,20 @@ class sinsp_container_manager : public libsinsp::container_engine::container_cac | |||
| * This method effectively checks if m_lookups[container_id][ctype] | |||
| * exists, without creating unnecessary map entries along the way. | |||
| */ | |||
| bool should_lookup(const std::string& container_id, sinsp_container_type ctype) override { | |||
| bool should_lookup(const std::string& container_id, | |||
There was a problem hiding this comment.
Can you update the comment?
Also, can you expand on why were these changes needed?
There was a problem hiding this comment.
In the case of more than one cri runtime, we should have a way to address (in this case by index) all the runtimes caches. I'll add a comment.
| using namespace libsinsp::container_engine; | ||
| using namespace libsinsp::runc; | ||
|
|
||
| constexpr const cgroup_layout CONTAINERD_CGROUP_LAYOUT[] = {{"/default/", ""}, {nullptr, nullptr}}; |
There was a problem hiding this comment.
Do we have a full cgroup example to be added as a comment here?
| std::string container_id; | ||
| sinsp_container_type container_type; | ||
| unsigned long uid; | ||
| bool request_rw_size; |
There was a problem hiding this comment.
This is actually unused; do you plan to add support for update_with_size API to containerd or we can drop it?
| @@ -269,7 +245,7 @@ bool cri::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_info) { | |||
| } else { | |||
| cache->notify_new_container(container, tinfo); | |||
| } | |||
| return true; | |||
| return false; | |||
There was a problem hiding this comment.
Can you comment on this change?
| // In some container runtimes the container the container id is not | ||
| // necessarly CONTAINER_ID_LENGTH long and can be arbitrarly defined. | ||
| // To keep it simple we only discard the container id > of CONTAINER_ID_LENGTH. | ||
| if(end_pos - start_pos > CONTAINER_ID_LENGTH || end_pos - start_pos == 0) { |
There was a problem hiding this comment.
So, we only accept container_id whose length is <= CONTAINER_ID_LENGTH right?
There was a problem hiding this comment.
Below in this function we call
container_id = cgroup.substr(start_pos, REPORTED_CONTAINER_ID_LENGTH);
What if cgroup length is less than REPORTED_CONTAINER_ID_LENGTH? It would throw an out_of_range exception.
There was a problem hiding this comment.
ops! good point!
|
/hold |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Up until now the container information were populated only through the cri api. There are cases in which this is not sufficient (e.g.
bottlerocket): for example the containerd runtime keeps the containers creates through the cri api separated from the ones created with the containerd api. This PR aims to support also the containers created with the vanilla containerd api.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: