[tewaro]

Summary: Added Capturing of Cgroup ids to the ebpf probes in the socket tracer. This includes modifications to data structures for communicating that information to userspace and other data tables. It also includes some minimal testing for ensuring that cgroup ids are captured correctly. The reason for this change is given in the design document for the solution to this issue: in short it is the first step in using cgids rather than pids to associate a particular piece of communication data with the correct pod.

Test Plan: Added Cgroup id capture checks to existing socket_tracer tests.

Type of change: /kind Feature expansion

Signed-off-as: AdityaAtulTewari adityaatewari@gmail.com

[ddelnano]

@AdityaAtulTewari and I discussed this already but for those here, I'm planning to review this but it won't be until tomorrow or Monday.

[ddelnano]

@pixie-io/maintainers in the meantime would we be able to let the CI jobs run? @AdityaAtulTewari is interested in seeing if there are any failures to address.

[ddelnano]

@AdityaAtulTewari really appreciate the contribution and excited to address this short lived process issue! I have a few minor comments and some thoughts about the timeline for this fix's development.

We had previously discussed writing a trace bpf test that would exhibit this short lived process behavior. I think it's important to develop that before we make the change to store the cgroup ids (this PR). Essentially my thoughts are that we would take your containerized application from this branch, create a new trace bpf test with it and then follow it up with this change to verify it results in the records we expect.

I believe it's important to have confidence that a problematic application fails our existing pid based assertions but works with the cgroup id ones.

[ddelnano]

There are many shared fields amongst each data table definition and record builder. This seems like a good opportunity to provide a mechanism for adding new columns in the future. We could define a macro like so:

#define APPEND_SKT_TRACER_COLS(timestamp, upid, conn_tracker) \
  r.Append<r.ColIndex("time_")>(entry.resp.timestamp_ns); \
  r.Append<r.ColIndex("upid")>(upid.value()); \
  r.Append<r.ColIndex("remote_addr")>(conn_tracker.remote_endpoint().AddrStr()); \
  r.Append<r.ColIndex("remote_port")>(conn_tracker.remote_endpoint().port()); \
  r.Append<r.ColIndex("trace_role")>(conn_tracker.role());

Another approach would be to create a similar method on the RecordBuilder class. I think I prefer the macro one better because it's not a socket specific construct and these shared fields aren't universally applicable to all data tables.

I don't feel strongly about incorporating this into your change, however, it would be great to have a mechanism to ensure new tables include all the relevant "socket tracer" columns.

[tewaro]

We had previously discussed writing a trace bpf test that would exhibit this short lived process behavior. I think it's important to develop that before we make the change to store the cgroup ids (this PR). Essentially my thoughts are that we would take your containerized application from this branch, create a new trace bpf test with it and then follow it up with this change to verify it results in the records we expect.

This change does not fix the race condition, it just exposes the cgroup id so that a future pr can. The changes are broken up this way as it allows me to break apart the solution to this problem into easier to design pieces. I'm happy to add that test to the change in progress that fixes the issue. However I don't think it is appropriate here because it will still fail.

[ddelnano]

The test suite still needs to pass, but the changes look good to me pending that 👍

We will need to regenerate the data table docs with the following
tool: //src/carnot/docstring/tables:datatables. Happy to help with that after this is merged.

[ddelnano]

cc @etep since I believe you are interested in reviewing this as well.