Skip to content

Conversation

@benkilimnik
Copy link
Member

@benkilimnik benkilimnik commented Aug 28, 2023

Summary: Adds new TraceProgram object to pxtrace which accepts a BPFtrace program and selectors to specify deployment restrictions as key value arguments. Also modifies the probe_fn argument to pxtrace.UpsertTracepoint to accept a single TraceProgram object or a list of them, extracting selectors and populating TracepointDeployment as required, so that the query broker can register tracepoints with agents that match those selectors.

Example usage

import pxtrace
import px

before_518_trace_program = pxtrace.TraceProgram(
  program="""$0""",
  max_kernel='5.18',
)
after_519_trace_program = pxtrace.TraceProgram(
  program="""$1""",
  min_kernel='5.19',
)

table_name = 'tcp_drop_table'
pxtrace.UpsertTracepoint('tcp_drop_tracer',
                          table_name,
                          [before_518_trace_program, after_519_trace_program],
                          pxtrace.kprobe(),
                          '10m')
)pxl";

Related issues: #1582. Closes #1659.

Type of change: /kind feature

Test Plan: Tested all existing targets, added compiler tests to src/carnot/planner/logical_planner_test.cc and src/carnot/planner/probes/probes_test.cc, and skaffolded changes to a cluster, verifying expected behavior and interoperability with selectors introduced in PR #1686.

Known issues

  • Two or more programs on one PEM: If selectors are specified such that two or more bpftrace programs are deployed to a PEM as part of a single TracepointDeployment, stirling throws an error stirling.cc:511 error::Internal("Only one Tracepoint is currently supported.");. This is an existing limitation, which is a little more exposed as a result of the selector feature introduced in this PR. To mitigate this, we could add documentation, improve the error message, and/or have just one of the bpftrace programs in the TracepointDeployment run.
  • Schemas differ: If the schemas of two bpftrace programs passed to one pxtrace.UpsertTracepoint call differ, only one of the schemas will be used to the create the output table (currently the first one that is processed). When the program whose schema differs tries to push a record to the table, we get the error source_connector.cc:64] Failed to push data. Message = Table_id ## doesn't exist. The user has to manually create another table/make another call to UpsertTracepoint if they want to deploy bpftrace programs with different schemas.

@benkilimnik benkilimnik changed the title [Kernel scoping 4/5] Add TraceProgram Object (#1659, #1582) [Kernel scoping 4/5] Add TraceProgram object Aug 28, 2023
@benkilimnik benkilimnik requested review from a team August 28, 2023 19:05
Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
@benkilimnik benkilimnik marked this pull request as ready for review September 25, 2023 17:41
Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
Comment on lines 709 to 712
literal_bpf_trace_min =
std::regex_replace(literal_bpf_trace_min, std::regex(R"(\\\n)"), R"(\\\\n)");
literal_bpf_trace_min = std::regex_replace(literal_bpf_trace_min, std::regex("\n"), "\\n");
literal_bpf_trace_min = std::regex_replace(literal_bpf_trace_min, std::regex("\""), "\\\"");
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to use regular expressions? It seems the other tests use absl::Substitute and imo it's much more readable.

Copy link
Member Author

@benkilimnik benkilimnik Sep 25, 2023

Choose a reason for hiding this comment

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

Thanks for pointing that out - I've removed a few calls to regex_replace which I believe are unneeded.

I think absl::Substitute is used for parameterized string substitution rather than pattern-based substitution which we're doing here. The existing test case for bpftrace strings on lines 635-649 also makes use of regex_replace, so I simply copied those over. They were added in this commit to escape newlines.

Another option would be to use absl::StrReplaceAll, though I ran into the error: no member named 'StrReplaceAll' in namespace 'absl'

Copy link
Member

@ddelnano ddelnano left a comment

Choose a reason for hiding this comment

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

Just one comment about the use of regular expressions, but otherwise lgtm!

Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
@JamesMBartlett JamesMBartlett merged commit 52e003d into pixie-io:main Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scope bpftrace/PxL scripts based on kernel version

3 participants