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

dbus: add GetUnitProcessesContext to list any unit's running processes #379

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

pphysch
Copy link

@pphysch pphysch commented Sep 20, 2021

This PR adds a binding for org.freedesktop.systemd1.Manager.GetUnitProcesses, taking a unit string and returning a list of all processes currently running under that unit (including child units).

This is convenient for getting a lot of information about a Slice's running processes, as it descends into child Slices/Scopes and includes those processes in the list. For example, if we target user-1000.slice we can learn about the different session-* Scopes under that user and all the processes therein.

My test (which may need adjustments to be more idiomatic/isolated) passes on systemd v239 (RHEL 8), but this Manager method doesn't exist on v219 (RHEL 7). I dug for a bit and can't find the exact version that implemented the GetProcesses API, but it seems to have been around 5 years ago.

This is my first PR here so please let me know how I can fix it! I used existing Manager method bindings as a template, so the code itself should be conformant even if the test needs work.

}

if !exists {
t.Errorf("PID %d ('/bin/sleep 400') not found in current Slice unit's process list", pid)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this test is failing on some CI runs. I suspect there is a race between spawning the child sleep and getting unit processes. It could be useful to retry the GetUnitProcessesContext() a few times until we get a non-empty set with /bin/sleep, or eventually timeout if we never get that.

dbus/methods.go Outdated
}

// GetUnitProcessesContext returns an array with all currently running processes in a unit *including* its child units.
func (c *Conn) GetUnitProcessesContext(ctx context.Context, unit string) ([]Process, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can directly call this GetUnitProcesses, plus avoid the separate *Internal helper.

@lucab
Copy link
Contributor

lucab commented Sep 21, 2021

Thanks for the PR! It looks like the new test is frequently failing in CI, I think because it is internally racing (see review).

@pphysch
Copy link
Author

pphysch commented Sep 21, 2021

Looks like the test is still failing even with delayed retries; I'll rewrite it so that it does more mocking and has less reliance on the system/container.

@pphysch pphysch requested a review from lucab September 21, 2021 17:53
@pphysch
Copy link
Author

pphysch commented Mar 8, 2023

Any chance this PR can get reviewed? Tests were passing as of the last commit.

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.

2 participants