Skip to content

refactor(discovery): use systemd D-Bus API#971

Merged
kakkoyun merged 5 commits intoparca-dev:mainfrom
maxbrunet:refactor/discovery/systemd-dbus
Nov 2, 2022
Merged

refactor(discovery): use systemd D-Bus API#971
kakkoyun merged 5 commits intoparca-dev:mainfrom
maxbrunet:refactor/discovery/systemd-dbus

Conversation

@maxbrunet
Copy link
Member

@maxbrunet maxbrunet commented Oct 31, 2022

This replaces the invocations of systemdctl CLI by the systemd D-Bus API.

This is still a polling approach, the module conveniently remembers the previous set of units and returns the changes only, allowing to trigger minimal discovery updates.

The D-Bus signals that would allow for this to be event-driven are unreliable, and although there is a workaround, I have observe many duplicated updates and easily fall into infinite loops of updates.

Using the D-Bus API permits to mount the D-Bus socket into the agent container to make it aware of the host systemd units. (note: I have also removed the /proc mount which is not needed, hostPID: true is enough)

Closes #760

@maxbrunet maxbrunet requested a review from a team as a code owner October 31, 2022 19:17
@javierhonduco
Copy link
Contributor

Nice work, will review it later today 🙇 .

(note: I have also removed the /proc mount which is not needed, hostPID: true is enough)

I think we still need it for the metadata providers, no?

Copy link
Contributor

@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.

It always awesome to have one less external dependency 👍

@maxbrunet
Copy link
Member Author

maxbrunet commented Nov 1, 2022

(note: I have also removed the /proc mount which is not needed, hostPID: true is enough)

I think we still need it for the metadata providers, no?

It was done to expose the kernel config, but after some testing, it has been proven unecessary: #892 (comment)

hostPID: true tells to run the pod in the host's PID namespace (same as --pid=host with docker run), so all information about the host's processes should be available to the agent.

From what I have documented here, everything gathered in /proc is under a PID, but we can check, which metadata provider are you worried?

@maxbrunet maxbrunet requested a review from kakkoyun November 1, 2022 22:09
@kakkoyun
Copy link
Contributor

kakkoyun commented Nov 2, 2022

Let's merge and test it further.

maxbrunet and others added 2 commits November 2, 2022 09:45
Co-authored-by: Kemal Akkoyun <kakkoyun@users.noreply.github.com>
@kakkoyun kakkoyun merged commit 63590da into parca-dev:main Nov 2, 2022
@javierhonduco
Copy link
Contributor

FYI, this feature uses ~15% of the CPU cycles as of 99d1c7927379775e328c9db0222dd6169db6a116 . Not sure how much we spent before spawning to systemctl but wanted to surface the current cost

image

https://pprof.me/8b20774

@kakkoyun
Copy link
Contributor

Shall we open an issue for this?

@javierhonduco
Copy link
Contributor

Might be worth it, as I am not sure of the previous performance implications so this might be a win, perf-wise, but I am worried about the extra GC pressure this is adding

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.

metadata: Use go-systemd instead of depending on systemctl

3 participants