Skip to content

Commit

Permalink
fix(libsinsp): address review comments
Browse files Browse the repository at this point in the history
Signed-off-by: Roberto Scolaro <[email protected]>
  • Loading branch information
therealbobo committed Jan 7, 2025
1 parent 01614a9 commit 513b0b5
Show file tree
Hide file tree
Showing 10 changed files with 17 additions and 24 deletions.
10 changes: 2 additions & 8 deletions userspace/libsinsp/container.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,17 +211,11 @@ class sinsp_container_manager : public libsinsp::container_engine::container_cac
*/
void set_lookup_status(const std::string& container_id,
sinsp_container_type ctype,
size_t engine_index,
sinsp_container_lookup::state state) override {
sinsp_container_lookup::state state,
size_t engine_index = 0) override {
m_lookups[container_id][ctype][engine_index] = state;
}

void set_lookup_status(const std::string& container_id,
sinsp_container_type ctype,
sinsp_container_lookup::state state) {
m_lookups[container_id][ctype][0] = state;
}

/**
* \brief do we want to start a new lookup for container metadata?
* @param container_id the container id we want to look up
Expand Down
2 changes: 1 addition & 1 deletion userspace/libsinsp/container_engine/bpm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ bool bpm::resolve(sinsp_threadinfo* tinfo, bool query_os_for_missing_info) {
}

tinfo->m_container_id = container_info.m_id;
if(container_cache().should_lookup(container_info.m_id, CT_BPM, 0)) {
if(container_cache().should_lookup(container_info.m_id, CT_BPM)) {
container_info.m_name = container_info.m_id;
container_info.set_lookup_status(sinsp_container_lookup::state::SUCCESSFUL);
container_cache().add_container(std::make_shared<sinsp_container_info>(container_info),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ class container_cache_interface {

virtual bool should_lookup(const std::string& container_id,
sinsp_container_type ctype,
size_t engine_index) = 0;
size_t engine_index = 0) = 0;

virtual void set_lookup_status(const std::string& container_id,
sinsp_container_type ctype,
size_t engine_index,
sinsp_container_lookup::state state) = 0;
sinsp_container_lookup::state state,
size_t engine_index = 0) = 0;

/**
* Get a container from the cache.
Expand Down
8 changes: 4 additions & 4 deletions userspace/libsinsp/container_engine/containerd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -339,16 +339,16 @@ bool libsinsp::container_engine::containerd::resolve(sinsp_threadinfo *tinfo,
return true;

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

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/container_engine/containerd.cpp#L339

Added line #L339 was not covered by tests
}

if(cache->should_lookup(request.container_id, request.container_type, 0)) {
if(cache->should_lookup(request.container_id, request.container_type)) {
libsinsp_logger()->format(sinsp_logger::SEV_DEBUG,
"containerd_async (%s): No existing container info",
request.container_id.c_str());

// give containerd a chance to return metadata for this container
cache->set_lookup_status(request.container_id,
request.container_type,
0,
sinsp_container_lookup::state::STARTED);
sinsp_container_lookup::state::STARTED,
0);
parse_containerd(request, cache);
}
return false;

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

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/container_engine/containerd.cpp#L354

Added line #L354 was not covered by tests
Expand All @@ -375,7 +375,7 @@ bool libsinsp::container_engine::containerd::resolve(sinsp_threadinfo *tinfo,
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, 0)) {
if(container_cache().should_lookup(container.m_id, CT_CONTAINERD)) {
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
4 changes: 2 additions & 2 deletions userspace/libsinsp/container_engine/cri.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ bool cri::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_info) {

cache->set_lookup_status(container_id,
get_cri_runtime_type(),
m_engine_index,
sinsp_container_lookup::state::STARTED);
sinsp_container_lookup::state::STARTED,
m_engine_index);

// sinsp_container_lookup is set-up to perform 5 retries at most, with
// an exponential backoff with 2000 ms of maximum wait time.
Expand Down
3 changes: 1 addition & 2 deletions userspace/libsinsp/container_engine/docker/base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,14 @@ bool docker_base::resolve_impl(sinsp_threadinfo *tinfo,
return true;
}

if(cache->should_lookup(request.container_id, request.container_type, 0)) {
if(cache->should_lookup(request.container_id, request.container_type)) {
libsinsp_logger()->format(sinsp_logger::SEV_DEBUG,
"docker_async (%s): No existing container info",
request.container_id.c_str());

// give docker a chance to return metadata for this container
cache->set_lookup_status(request.container_id,
request.container_type,
0,
sinsp_container_lookup::state::STARTED);
parse_docker(request, cache);
}
Expand Down
2 changes: 1 addition & 1 deletion userspace/libsinsp/container_engine/libvirt_lxc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ bool libvirt_lxc::resolve(sinsp_threadinfo* tinfo, bool query_os_for_missing_inf
}

tinfo->m_container_id = container.m_id;
if(container_cache().should_lookup(container.m_id, CT_LIBVIRT_LXC, 0)) {
if(container_cache().should_lookup(container.m_id, CT_LIBVIRT_LXC)) {
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
2 changes: 1 addition & 1 deletion userspace/libsinsp/container_engine/lxc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ bool lxc::resolve(sinsp_threadinfo *tinfo, bool query_os_for_missing_info) {
}

tinfo->m_container_id = container.m_id;
if(container_cache().should_lookup(container.m_id, CT_LXC, 0)) {
if(container_cache().should_lookup(container.m_id, CT_LXC)) {
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
2 changes: 1 addition & 1 deletion userspace/libsinsp/container_engine/mesos.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ bool libsinsp::container_engine::mesos::resolve(sinsp_threadinfo* tinfo,
return false;

tinfo->m_container_id = container.m_id;
if(container_cache().should_lookup(container.m_id, CT_MESOS, 0)) {
if(container_cache().should_lookup(container.m_id, CT_MESOS)) {
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
2 changes: 1 addition & 1 deletion userspace/libsinsp/container_engine/rkt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ bool rkt::rkt::resolve(sinsp_threadinfo* tinfo, bool query_os_for_missing_info)
}

tinfo->m_container_id = container.m_id;
if(!query_os_for_missing_info || !cache->should_lookup(container.m_id, CT_RKT, 0)) {
if(!query_os_for_missing_info || !cache->should_lookup(container.m_id, CT_RKT)) {
return true;
}

Expand Down

0 comments on commit 513b0b5

Please sign in to comment.