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

feat(cgroup): Add podman support #1455

Merged
merged 1 commit into from
May 24, 2024

Conversation

dave-tucker
Copy link
Collaborator

@dave-tucker dave-tucker commented May 20, 2024

This commit simplifies the containerID from Cgroup path parsing logic.
All container IDs from all supported runtimes (including podman) are
64 character alphanumeric strings.

As such we can simply strip the .scope suffix and take the last 64
characters.

Copy link

github-actions bot commented May 20, 2024

🤖 SeineSailor

Here's a concise summary of the pull request changes:

Summary: This pull request adds support for the "libpod" container runtime in the cgroup stats feature by modifying the container ID extraction logic in extractPodContainerIDfromPathWithCgroup and extractPodContainerIDfromPath functions. The changes include:

  • Extending runtime detection to include "libpod"
  • Simplifying container ID extraction by parsing the last 64 characters after stripping the .scope suffix from the Cgroup path
  • Replacing regex-based validation with a simpler alphanumeric pattern matcher
  • Adding new test functions and cases for Podman containers (rootless, rootful, and quadlets)

Impact: The changes ensure compatibility with Podman containers while maintaining existing functionality for other supported runtimes (crio, docker, and containerd). The external interface and behavior of the code remain unaffected.

Observations/Suggestions:

  • The changes are well-contained within the resolve_container.go and resolve_container_test.go files, with no impact on exported functions, global data structures, or variable signatures.
  • The addition of new test cases for Podman containers is a positive step towards ensuring compatibility and robustness.
  • It would be beneficial to consider adding documentation updates to reflect the added support for "libpod" container runtime.

This commit simplifies the containerID from Cgroup path parsing logic.
All container IDs from all supported runtimes (including podman) are
64 character alphanumeric strings.

As such we can simply strip the `.scope` suffix and take the last 64
characters.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker
Copy link
Collaborator Author

I've tested this on my local system and confirm that both rootful and rootless podman containers now show up when querying kepler metrics.

path = strings.TrimSuffix(path, "/container")
path = strings.TrimSuffix(path, ".scope")

// get the last 64 characters of the path
Copy link
Contributor

Choose a reason for hiding this comment

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

can you post the reference to this 64-char path length?

Copy link
Collaborator Author

@dave-tucker dave-tucker May 21, 2024

Choose a reason for hiding this comment

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

https://docs.docker.com/engine/reference/run/#container-identification

I've tried looking for a reference in the OCI spec but I couldn't find one, but from what I can see this appears to be the de-facto standard.

We could split the path string on all / and use the last path component, which wouldn't cause an issue in the case of something that's not exactly 64 chars long.

EDIT: 👆 won't work as in some of the test cases the container ID isn't preceded by a /

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, a uniform naming is surely better than all the if/else hacks.

For the transition consideration, would you please convert the existing code to a fallback path in case we do hit corner cases?

Copy link
Collaborator Author

@dave-tucker dave-tucker May 22, 2024

Choose a reason for hiding this comment

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

I could yes, but consider this - it's a 10x performance improvement over the legacy code + it doesn't allocate memory + all of our unit tests (including the newly added ones from podman) pass. Not only that, it's called only when the K8s watcher isn't enabled.

However, when the k8s watcher isn't enabled (i.e linux only, no k8s) this function is in the hot path - i.e it's called in createContainerStatsIfNotExist which is called multiple times every time we collect metrics. Adding a fallback path is expensive in terms of performance - I'd rather that users submit issues so we can expand our tests cases and have a performant and correct solution.

go test -benchmem -run=^$ -bench ^BenchmarkExtractPodContainerIDfromPathWithCgroup github.com/sustainable-computing-io/ke
pler/pkg/cgroup 
goos: linux
goarch: amd64
pkg: github.com/sustainable-computing-io/kepler/pkg/cgroup
cpu: AMD Ryzen 5 5600G with Radeon Graphics         
BenchmarkExtractPodContainerIDfromPathWithCgroup-12              1222920               930.0 ns/op             0 B/op          0 allocs/op
BenchmarkExtractPodContainerIDfromPathWithCgroupLegacy-12         123741              9918 ns/op             595 B/op          9 allocs/op
PASS
ok      github.com/sustainable-computing-io/kepler/pkg/cgroup   3.463s

@rootfs rootfs merged commit 3ac4f6b into sustainable-computing-io:main May 24, 2024
26 checks passed
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