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

Migrate from cgroup profiling to system-wide profiling #627

Merged
merged 8 commits into from Jul 28, 2022

Conversation

javierhonduco
Copy link
Contributor

@javierhonduco javierhonduco commented Jul 28, 2022

This PR reworks how we attach the CPU profiler to create one global perf event, instead of creating one per cgroup. This has several advantages, including:

  • Enabling host-wide profiling: Allowing the profiling of the whole machine, without excluding a single process! As a side-effect, we will be able to profile processes that aren't containerised, too (*)
  • Reduced resource usage. Both in userland, NewCgroupProfilers instances linearly use memory and CPU, as well as kernel space (fewer data structures that the kernel needs to create, fewer events that have to be delivered, etc)
  • Making the architecture more flexible. This design will allow us to incorporate new profilers more easily

Design

The discovery mechanism (Kubernetes or Systemd) no longer drives the creation cgroup profilers, instead:

  1. A new CPU profiler is created when the agent starts, which is not attached to any cgroup;
  2. When the kubernetes discovery mechanism detects a new container, it adds some metadata, a mapping of PID to labels, that can be collected on-demand;
  3. Once a profiler 'round' finishes, we create the profiles. Instead of creating one per cgroup, we now generate one per PID. We check the mapping of PID to labels and add them to the profile

Changes vs the initial proposal

Cgroup filtering

One of the initial goals of this work was to keep compatibility with the current filtering that Parca Agent has. For example, adding a new flag to enable system-wide collection (--system-wide, for example) and otherwise we would collect profiles for the Kubernetes clusters or Cgroups that are discovered or specified via arguments.

However, this proved to be trickier than I anticipated, due to the fact that the bpf_get_current_cgroup_id() helper used to retrieve the Cgroup id in BPF does not work under cgroups v1, which is widely used in Kubernetes environments. While a migration to cgroups v2 is on the way, we definitely want to support both cgroup versions.

While this approach would have been the most efficient way to faithfully replicate the current behaviour, as we could do the filtering in BPF and avoid counting these stacks, as well as reading it in userspace, I tried fetching the Cgroup ID in userspace and do the filtering there. However, after trying out this idea, I was not able to read a cgroups v1 ID (one of the approaches).

In the end, I think that while the idea of trying to replicate the current behaviour (and in the future enable system-wide profile collection by default) has some validity, I am not sure if it's worth it. Not only this would be something temporary, but having to do the cgroup ID filtering in userspace will still generate more data and make us read the same data that we would have to do with system-wide enabled by default (minus the profile generation itself). Finding the correct paths for the cgroups can be difficult, too, which might bring bugs and will force us to do way more testing.

Profile generation

Initially, we proposed to generate a profile per cgroup to maintain the current behaviour and to allow filtering by cgroup (see point above).

In this PR we generate one profile per PID.

Other significant changes

  • The web UI has undergone a lot of changes, we only show the profilers now and downloading the pprof profilers has been removed. We can re-add this in the future if needed, but most of the time Parca Agent is tested with Parca with receives the profiles and will do symbolication, making the profiles easier.
  • Removal of the Systemd / Cgroup discovery mechanism, as it's no longer needed. We are losing the unit names, which we can re-add later on.

Future work

  • Remove unneeded code
  • Add back the systemd/cgroup metadata, if needed
  • Tackle the TODOs

Test plan

Bare metal

image

Minikube

image

Web UI
image

Fixes #260

@javierhonduco javierhonduco mentioned this pull request Jul 28, 2022
4 tasks
This also allows us to have multiple profilers in the future

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
Temporarily comments out UI code that should be dealt with later
@javierhonduco
Copy link
Contributor Author

Fix web tests

cmd/parca-agent/main.go Outdated Show resolved Hide resolved
Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

Woah. Amazing!! 💯

@kakkoyun
Copy link
Member

Other significant changes

  • The web UI has undergone a lot of changes, we only show the profilers now and downloading the pprof profilers has been removed. We can re-add this in the future if needed, but most of the time Parca Agent is tested with Parca with receives the profiles and will do symbolication, making the profiles easier.
  • Removal of the Systemd / Cgroup discovery mechanism, as it's no longer needed. We are losing the unit names, which we can re-add later on.

Future work

  • Remove unneeded code
  • Add back the systemd/cgroup metadata, if needed
  • Tackle the TODOs

Could you please open issues for all these?

Note to the releaser: These should be mentioned in the release notes.

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looking good. I did the first round, and I'll continue to review this later on.

Especially, I haven't checked pkg/profiler/cpu.go yet.

Awesome work so far 💯

cmd/parca-agent/main.go Outdated Show resolved Hide resolved
pkg/profiler/pool.go Outdated Show resolved Hide resolved
pkg/profiler/noop.go Show resolved Hide resolved
pkg/profiler/cpu.go Show resolved Hide resolved
This commit changes the array of groups to a single group, as this is
the interface that the kubernetes discovery mechanism uses, and removes
the systemd discoverer as it's no longer needed.

All cgroups and system units in the box should be profiled
automatically. If there's additional metadata, it will be added (see
commit above).

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco javierhonduco force-pushed the system-wide-v3 branch 2 times, most recently from 6de5176 to d298e12 Compare July 28, 2022 13:07
pkg/profiler/cpu.go Outdated Show resolved Hide resolved
cmd/parca-agent/main.go Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member

Just a couple of nits and then we can handle the rest in subsequent PRs

Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
@javierhonduco
Copy link
Contributor Author

Address feedback

@kakkoyun kakkoyun changed the title System wide v3 Migrate from cgroup profiling to system-wide profiling Jul 28, 2022
@kakkoyun kakkoyun merged commit 4772d95 into parca-dev:main Jul 28, 2022
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.

Allow profiling of arbitrary processes
3 participants