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.

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

CRI-O container support #1310

Merged
merged 5 commits into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@gnosek
Copy link
Contributor

commented Feb 5, 2019

CRI-O uses CRI so most of the work is done already but it differs in some important ways from containerd and has to be supported explicitly.

@gnosek gnosek requested review from adalton and mattpag Feb 5, 2019

@gnosek gnosek force-pushed the cri-o branch from d2d8b7f to edca8e8 Feb 6, 2019

@speedyguy17
Copy link
Contributor

left a comment

looks mostly okay, some concerns about the string matching function though

namespace libsinsp {
namespace runc {

/// runc-based runtimes (Docker, containerd, CRI-O, probably others) use the same two cgroup layouts

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

i appreciate the comment, but still slightly confused. maybe just an exapmle for each of the runc runtimes we support?

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

Do you still find it confusing after looking at the instances of this struct (container_engine/cri.cpp, container_engine/docker_linux.cpp)?

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 13, 2019

Contributor

less so, but one don't want to have to dig into the implementation to understand the api.

namespace {

const size_t CONTAINER_ID_LENGTH = 64;
const size_t REPORTED_CONTAINER_ID_LENGTH = 12;

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

maybe a note what these values represent?

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

I'm not sure what to add that wouldn't duplicate the constant names or the static_assert below.

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 13, 2019

Contributor

what does "reported" mean here? is it what's reported to us? what we're reporting to someone else?


static_assert(REPORTED_CONTAINER_ID_LENGTH <= CONTAINER_ID_LENGTH, "Reported container ID length cannot be longer than actual length");

bool detect_one_container_id(const std::string& cgroup, const std::string& prefix, const std::string& suffix, std::string& container_id)

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

should these two functions be static?

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

also quick comment on which are inputs and ouputs of this function

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

They're already in a private namespace.

I'll add a comment.

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 13, 2019

Contributor

must have missed that they're still in the NS. that's fine then.

const char* suffix;
};

/// If any of the cgroups of the thread in `tinfo` matches the `layout`, set `container_id` to the found id

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

what if multiple cgroups match the layout?

I assume we're assuming that impossible, and if that's the case ,we should perhaps note the assumption

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

Then we take the first matching one (AFAICT, the iteration order matches contents of /proc//cgroups). If the cgroups are inconsistent (matching different containers), there's no single right answer we can choose. What do you propose?

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 13, 2019

Contributor

so long as we're okay with that, it's fine, but we should at least be explicit in that "if it matches multiple, we'll only return the first" or something


bool detect_container_id(const std::string& cgroup, const libsinsp::runc::cgroup_layout *layout, std::string &container_id)
{
for(size_t i = 0; layout[i].prefix && layout[i].suffix; ++i)

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

this way of doing bounds checking is somewhat risky, as it depends on someone putting null pointers at the end of arrays put in.

We should be able to statically determine the size of those arrays with sizeof(X)/sizeof(X[0]) and then that value can be passed in, or even better, we can wrap the array in a struct which stores the size of the array, calculated at compile time

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

sizeof won't work across function calls. Given that we have only two of these arrays, hardcoded in the source and not modifiable at runtime, I'm not sure it's a big deal (if you insist, I'd probably go for std::array).

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 13, 2019

Contributor

i don't think the number of instance is the issue as much as the fact that depending on the last entry of an array to be null in order to prevent a segfault is not really idiomatic, and there's no compelling reason to do it that way when safer options exist.

I didn't mean computing sizeof in the function itself, but along with the array
static const int foo = {1,5,7,2,7};
static const foo_length = sizeof(foo) / sizeof(foo[0]);

Then you use foo_length as your loop bounds, as it's clear and safe.

std::array is fine or even vector would work fine here as well

Show resolved Hide resolved userspace/libsinsp/runc.cpp Outdated

static_assert(std::string::npos == (size_t)(-1), "std::string::npos must be largest valid size_t");
size_t invalid_ch_pos = cgroup.find_first_not_of(CONTAINER_ID_VALID_CHARACTERS, pos);
if (invalid_ch_pos < CONTAINER_ID_LENGTH)

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

just match the ID against [0-9a-fA-F]{64} ?

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

This is literally what this code does. Just not using regexes.

#include "sinsp.h"
#include "sinsp_int.h"

namespace {
bool pod_uses_host_netns(const runtime::v1alpha2::PodSandboxStatusResponse& resp)

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 11, 2019

Contributor

is this common in the code base instead of static?

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 12, 2019

Author Contributor

I got tired of your (plural you) review comments asking me to switch from static to namespace{}. Make up your mind, will you? :P

This comment has been minimized.

Copy link
@speedyguy17

speedyguy17 Feb 13, 2019

Contributor

yeah i don't really care either way, i was just asking to ensure it's consistent

Show resolved Hide resolved userspace/libsinsp/cri.cpp
Show resolved Hide resolved userspace/libsinsp/container_info.h

@gnosek gnosek force-pushed the cri-o branch from edca8e8 to 2820a68 Feb 12, 2019


constexpr const cgroup_layout DOCKER_CGROUP_LAYOUT[] = {
{"/", ""}, // non-systemd docker
{"/docker-", ".scope"}, // systemd docker

This comment has been minimized.

Copy link
@krishnan-ramkumar

krishnan-ramkumar Feb 14, 2019

Contributor

How confident are we about the ".scope" suffix of systemd.
this page here:
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/resource_management_guide/sec-default_cgroup_hierarchies
seems to suggest we could have ".slice" too.

Will review the entire PR a little later. Wanted to capture this one thing first.

bool cri::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info)
{
sinsp_container_info container_info;

if(docker::detect_docker(tinfo, container_info.m_id))
if(matches_runc_cgroups(tinfo, CRI_CGROUP_LAYOUT, container_info.m_id))
{
container_info.m_type = s_cri_runtime_type;

This comment has been minimized.

Copy link
@krishnan-ramkumar

krishnan-ramkumar Feb 14, 2019

Contributor

Is the need here only to specify if it is a CRI-based run-time or not ? Do we not need to know if it is CT_CRIO or CT_CONTAINERD ?

This comment has been minimized.

Copy link
@anoop-sysd

anoop-sysd Feb 14, 2019

Contributor

yes, we must be as specific as possible. i think we should report a container type being "CRI" only as a last resort.

@@ -79,32 +109,27 @@ bool parse_cri(sinsp_container_manager *manager, sinsp_container_info *container
parse_cri_image(resp_container, container);
parse_cri_mounts(resp_container, container);

const auto &info_it = resp.info().find("info");
if(info_it == resp.info().end())
if(parse_containerd(resp, container, tinfo))

This comment has been minimized.

Copy link
@anoop-sysd

anoop-sysd Feb 14, 2019

Contributor

this call seems out of place. why call logic specific to containerd unconditionally when the container type is known at this point. if there is an expectation that the logic in parse_containerd() could apply to cri-o as well, why not rename the function to be clear?

This comment has been minimized.

Copy link
@anoop-sysd

anoop-sysd Feb 14, 2019

Contributor

btw, can't line 102 be removed since the same operation is done in the caller of this function?

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 25, 2019

Author Contributor

About the duplicate assignment, good catch, removed.

parse_containerd should be pretty cheap when the engine is not containerd compatible (no metadata in info) and if some other runtime decides that this is actually worth following, it saves us two extra api calls.

I'd leave this as is (currently it only applies to containerd and I don't have a better idea for the name).

bool cri::resolve(sinsp_container_manager* manager, sinsp_threadinfo* tinfo, bool query_os_for_missing_info)
{
sinsp_container_info container_info;

if(docker::detect_docker(tinfo, container_info.m_id))
if(matches_runc_cgroups(tinfo, CRI_CGROUP_LAYOUT, container_info.m_id))
{
container_info.m_type = s_cri_runtime_type;

This comment has been minimized.

Copy link
@anoop-sysd

anoop-sysd Feb 14, 2019

Contributor

yes, we must be as specific as possible. i think we should report a container type being "CRI" only as a last resort.

@@ -6151,6 +6151,9 @@ uint8_t* sinsp_filter_check_container::extract(sinsp_evt *evt, OUT uint32_t* len
case sinsp_container_type::CT_CONTAINERD:
m_tstr = "containerd";
break;
case sinsp_container_type::CT_CRIO:
m_tstr = "crio";

This comment has been minimized.

Copy link
@anoop-sysd

anoop-sysd Feb 14, 2019

Contributor

"cri-o"

This comment has been minimized.

Copy link
@gnosek

gnosek Feb 25, 2019

Author Contributor

Done (in both places). AFAIK this doesn't relate to the backend at all, just to container.type=cri-o style filters and Lua chisels

@@ -1193,6 +1193,10 @@ int lua_cbacks::get_container_table(lua_State *ls)
{
lua_pushstring(ls, "containerd");
}
else if(it->second.m_type == CT_CRIO)
{
lua_pushstring(ls, "crio");

This comment has been minimized.

Copy link
@anoop-sysd

anoop-sysd Feb 14, 2019

Contributor

"cri-o"

@anoop-sysd

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@gnosek , i just saw your comment on how the container type is stringified in the backend. so if "cri-o" is not possible, then ignore my comments about renaming "crio".

@gnosek gnosek force-pushed the cri-o branch 2 times, most recently from aa07d75 to e4e4ae4 Feb 15, 2019

@gnosek gnosek force-pushed the cri-o branch from e4e4ae4 to 033b0b8 Feb 25, 2019

gnosek added some commits Feb 4, 2019

Fetch missing CRIO metadata from additional API queries
While containerd returns everything in the `info` dictionary,
it's empty for CRIO, which means we need to get the IP address
and the image ID from extra API calls.

Unfortunately, it seems there's no way to get:
* environment variables
* privileged status
* resource limits
in any way from CRIO.
Remove redundant assignment
We set the container type in the calling method

@gnosek gnosek force-pushed the cri-o branch from 033b0b8 to ad5b647 Feb 28, 2019

@gnosek gnosek merged commit ecafb60 into dev Feb 28, 2019

2 of 3 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details
sign-off-checker The commit doesn't require sysdig sign-off CLA because it belongs to gnosek part of draios/sysdig collaborators
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.