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

Simplify stats polling logic #7987

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Simplify stats polling logic #7987

wants to merge 2 commits into from

Conversation

bduffany
Copy link
Member

@bduffany bduffany commented Dec 2, 2024

This PR cleans up some stats logic in preparation for tracking firecracker stats using cgroups, instead of tracking stats using the guest vmexec server. The current logic is somewhat tangled up which would make the integration with firecracker a bit more messy than it needs to be.

  • Remove the logic to record prometheus metrics for running containers, which manually totals the results of c.Stats(). This logic is complex and its value is pretty questionable.
    • On Linux, a better way to do this would be to poll the stats from the action parent cgroup once executor.child_cgroups_enabled is rolled out.
    • On macOS we could probably just live with the node metrics instead of trying to separate the executor metrics and task metrics. The accuracy of the task metrics on macOS is especially questionable, since the poll rate is lower, and it requires walking the process tree, which can miss short-lived processes spawned during an execution.
  • Simplify the stats polling logic, which was tangled up and somewhat circular.
    • The previous logic would have each container call the TrackStats utility function, which would start a loop that calls back to the container's Stats() function, which was expected to call usageStats.Update() then return usageStats.TaskStats().
    • Now, TrackStats is a method of the UsageStats struct, and manages these careful state updates internally, without a circular call back to the container.Stats() function, and without expecting the caller to call usageStats.Reset() and usageStats.TaskStats()

@bduffany bduffany force-pushed the cgroup-poll-simplify branch 2 times, most recently from fd178bc to 667f2b8 Compare December 2, 2024 20:11
@bduffany bduffany changed the title Simplify cgroup stats logic Simplify stats logic Dec 2, 2024
@bduffany bduffany force-pushed the cgroup-poll-simplify branch 2 times, most recently from 7ea7fc2 to afeeaae Compare December 2, 2024 20:44
@bduffany bduffany changed the title Simplify stats logic Simplify stats polling logic Dec 2, 2024
@bduffany bduffany force-pushed the cgroup-poll-simplify branch from afeeaae to bb8f060 Compare December 2, 2024 20:56
@bduffany bduffany marked this pull request as ready for review December 2, 2024 21:18
Copy link
Contributor

Choose a reason for hiding this comment

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

is the TODO on line 126 completed now?

Copy link
Member Author

@bduffany bduffany Dec 3, 2024

Choose a reason for hiding this comment

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

Removed this TODO for now - if we wanted to remove this "baseline" tracking logic, but still be able to properly track usage for remote persistent workers, then we'd have to migrate all of the container processes to a new cgroup for each task executed, which seems a bit too complicated. We can maybe revisit later.

@@ -392,7 +298,7 @@ func TrackStats(ctx context.Context, c CommandContainer) (stop func(), res <-cha
// the container, which can take a few hundred ms or possibly longer
Copy link
Contributor

Choose a reason for hiding this comment

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

the mention of podman on the line above might be out of date.

Copy link
Member Author

@bduffany bduffany Dec 3, 2024

Choose a reason for hiding this comment

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

That part is still accurate, since podman doesn't (yet) have the logic where we create the cgroup then run the container in that cgroup that we created. Instead, podman manages the cgroup lifecycle itself.

I removed this note for now though, since it's not super important. The bigger TODO is to make sure that the container implementations don't delete the cgroup from under us (this is a problem with both ociruntime and podman). That way, we could remove this polling, and reliably read the cgroup stats once at the end of execution.

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.

3 participants