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

fix(libsinsp/runc): use old logic and fallback for containerd #2235

Merged
merged 3 commits into from
Jan 16, 2025
+41 −38
Merged

fix(libsinsp/runc): use old logic and fallback for containerd #2235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1edd005
feat(containers): add host-containerd socket
therealbobo Jan 15, 2025
810b2d6
fix(libsinsp/runc): use old logic and fallback for containerd
therealbobo Jan 15, 2025
46d0c28
fix(libsinsp/container_engine/containerd): avoid cache confusion betw…
therealbobo Jan 15, 2025
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
7 changes: 5 additions & 2 deletions userspace/libsinsp/container.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ void sinsp_container_manager::create_engines() {
m_container_engine_by_type[CT_DOCKER].push_back(docker_engine);
}

size_t engine_index = 0;
if(m_container_engine_mask & ((1 << CT_CRI) | (1 << CT_CRIO) | (1 << CT_CONTAINERD))) {
// Get CRI socket paths from settings
libsinsp::cri::cri_settings& cri_settings = libsinsp::cri::cri_settings::get();
Expand All @@ -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");
therealbobo marked this conversation as resolved.
Show resolved Hide resolved
}

const auto& cri_socket_paths = cri_settings.get_cri_unix_socket_paths();

size_t engine_index = 0;
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are various code path that assume that a cri runtime could have containerd as type.

engine_index++;
}
}
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right :/

m_container_engines.push_back(containerd_engine);
m_container_engine_by_type[CT_CONTAINERD].push_back(containerd_engine);
}
Expand Down
29 changes: 17 additions & 12 deletions userspace/libsinsp/container_engine/containerd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
};

bool containerd_async_source::is_ok() {
return m_container_stub != nullptr && m_image_stub != nullptr;
return m_container_stub && m_image_stub;
}

static inline void setup_grpc_client_context(grpc::ClientContext &context) {
Expand Down Expand Up @@ -144,8 +144,11 @@
return m_image_stub->Get(&context, req, &resp);
}

libsinsp::container_engine::containerd::containerd(container_cache_interface &cache):
libsinsp::container_engine::containerd::containerd(container_cache_interface &cache,
size_t engine_index):
container_engine_base(cache) {
m_engine_index = engine_index;

for(const auto &p : CONTAINERD_SOCKETS) {
if(p.empty()) {
continue;
Expand All @@ -158,11 +161,8 @@
}

container_cache_interface *cache_interface = &container_cache();
auto src = new containerd_async_source(socket_path,
containerd_async_source::NO_WAIT_LOOKUP,
10000,
cache_interface);
m_containerd_info_source.reset(src);
m_containerd_info_source =

Check warning on line 164 in userspace/libsinsp/container_engine/containerd.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/container_engine/containerd.cpp#L164 

Added line #L164 was not covered by tests
std::make_unique<containerd_async_source>(socket_path, 0, 10000, cache_interface);
if(!m_containerd_info_source->is_ok()) {
m_containerd_info_source.reset(nullptr);
continue;
Expand All @@ -172,6 +172,10 @@

bool containerd_async_source::parse(const containerd_lookup_request &request,
sinsp_container_info &container) {
if(!is_ok()) {
return false;
}

auto container_id = request.container_id;

libsinsp_logger()->format(sinsp_logger::SEV_DEBUG,
Expand Down Expand Up @@ -294,12 +298,13 @@
libsinsp_logger()->format(sinsp_logger::SEV_DEBUG,
"containerd_async (%s): Starting asynchronous lookup",
request.container_id.c_str());
done = m_containerd_info_source->lookup(request, result);
done = m_containerd_info_source && m_containerd_info_source->lookup(request, result);
} else {
libsinsp_logger()->format(sinsp_logger::SEV_DEBUG,
"containerd_async (%s): Starting synchronous lookup",
request.container_id.c_str());
done = m_containerd_info_source->lookup_sync(request, result);

done = m_containerd_info_source && m_containerd_info_source->lookup_sync(request, result);
}
if(done) {
// if a previous lookup call already found the metadata, process it now
Expand Down Expand Up @@ -339,7 +344,7 @@
return true;
}

if(cache->should_lookup(request.container_id, request.container_type)) {
if(cache->should_lookup(request.container_id, request.container_type, m_engine_index)) {
libsinsp_logger()->format(sinsp_logger::SEV_DEBUG,
"containerd_async (%s): No existing container info",
request.container_id.c_str());
Expand All @@ -348,7 +353,7 @@
cache->set_lookup_status(request.container_id,
request.container_type,
sinsp_container_lookup::state::STARTED,
0);
m_engine_index);
parse_containerd(request, cache);
}
return false;
Expand All @@ -375,7 +380,7 @@
container.m_cpu_period = limits.m_cpu_period;
container.m_cpuset_cpu_count = limits.m_cpuset_cpu_count;

if(container_cache().should_lookup(container.m_id, CT_CONTAINERD)) {
if(container_cache().should_lookup(container.m_id, CT_CONTAINERD, m_engine_index)) {
container.m_name = container.m_id;
container.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
container_cache().add_container(std::make_shared<sinsp_container_info>(container), tinfo);
Expand Down
3 changes: 2 additions & 1 deletion userspace/libsinsp/container_engine/containerd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,15 @@ class containerd_async_source : public container_async_source<containerd_lookup_

class containerd : public container_engine_base {
public:
containerd(container_cache_interface& cache);
containerd(container_cache_interface& cache, size_t engine_index);

void parse_containerd(const containerd_lookup_request& request,
container_cache_interface* cache);
bool resolve(sinsp_threadinfo* tinfo, bool query_os_for_missing_info) override;

private:
std::unique_ptr<containerd_async_source> m_containerd_info_source;
size_t m_engine_index;
};

} // namespace container_engine
Expand Down
37 changes: 15 additions & 22 deletions userspace/libsinsp/runc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

const size_t CONTAINER_ID_LENGTH = 64;
const size_t REPORTED_CONTAINER_ID_LENGTH = 12;
const char *CONTAINER_ID_VALID_CHARACTERS = "0123456789abcdefABCDEF";

static_assert(REPORTED_CONTAINER_ID_LENGTH <= CONTAINER_ID_LENGTH,
"Reported container ID length cannot be longer than actual length");
Expand All @@ -40,21 +41,6 @@
return s.rfind(suffix) == (s.size() - suffix.size());
}

inline static bool is_host(const std::string &cgroup) {
// A good approximation to minize false-positives is to exclude systemd suffixes.
if(endswith(cgroup, ".slice") || endswith(cgroup, ".service")) {
return true;
} else if(endswith(cgroup, ".scope")) {
if(cgroup.find("crio-") != std::string::npos ||
cgroup.find("docker-") != std::string::npos) {
return false;
}
return true;
}

return false;
}

// check if cgroup ends with <prefix><container_id><suffix>
// If true, set <container_id> to a truncated version of the id and return true.
// Otherwise return false and leave container_id unchanged
Expand All @@ -73,22 +59,29 @@
return false;
}

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;
}

Comment on lines +62 to +67
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// 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) {
return false;
}

if(is_host(cgroup)) {
return false;
if(cgroup.rfind("/default/") == 0 && !endswith(cgroup, ".service") &&
!endswith(cgroup, ".slice")) {
size_t reported_len = end_pos - start_pos >= REPORTED_CONTAINER_ID_LENGTH

Check warning on line 77 in userspace/libsinsp/runc.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/runc.cpp#L77 

Added line #L77 was not covered by tests
? REPORTED_CONTAINER_ID_LENGTH
: end_pos;
container_id = cgroup.substr(start_pos, reported_len);
return true;

Check warning on line 81 in userspace/libsinsp/runc.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/runc.cpp#L81 

Added line #L81 was not covered by tests
}

size_t reported_len = end_pos - start_pos >= REPORTED_CONTAINER_ID_LENGTH
? REPORTED_CONTAINER_ID_LENGTH
: end_pos;
container_id = cgroup.substr(start_pos, reported_len);
return true;
return false;
}

bool match_container_id(const std::string &cgroup,
Expand Down
3 changes: 2 additions & 1 deletion userspace/libsinsp/test/container_engine/cri_settings.ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ TEST_F(sinsp_with_test_input, default_cri_socket_paths) {

auto socket_paths = cri_settings.get_cri_unix_socket_paths();

ASSERT_EQ(socket_paths.size(), 3);
ASSERT_EQ(socket_paths.size(), 4);
ASSERT_TRUE("/run/containerd/containerd.sock" == socket_paths[0]);
ASSERT_TRUE("/run/crio/crio.sock" == socket_paths[1]);
ASSERT_TRUE("/run/k3s/containerd/containerd.sock" == socket_paths[2]);
ASSERT_TRUE("/run/host-containerd/containerd.sock" == socket_paths[3]);
}
#endif
Loading