Add support for conmon-rs log driver and heaptrack config #9402
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9402 +/- ##
==========================================
+ Coverage 67.22% 67.25% +0.02%
==========================================
Files 201 202 +1
Lines 27718 27781 +63
==========================================
+ Hits 18634 18684 +50
- Misses 7533 7542 +9
- Partials 1551 1555 +4 🚀 New features to boost your workflow:
|
|
/retest |
pkg/config/config.go
Outdated
| // MonitorHeaptrackOutputPath is the path to output heaptrack profiles | ||
| // generated by the container monitor. Works only for the "pod" | ||
| // runtime_type and creates profiles per pod. | ||
| MonitorHeaptrackOutputPath string `toml:"monitor_heaptrack_output_path,omitempty"` |
There was a problem hiding this comment.
hmm any ways we could make this more generic?
There was a problem hiding this comment.
MonitorExtraBinaries or something? and then the runtime interprets? could be key->value.. i dunno
There was a problem hiding this comment.
Some generic monitor_options? Then we could enable heaptrack via monitor_options=heaptrack_output=/path/to,heaptrack_binary=/foo/bar. We could also just use the existing monitor_env then, which is unused for conmon-rs right now.
Edit: Switched over to reuse monitor_env.
There was a problem hiding this comment.
IMHO we don't necessarily have the common options for conmon and conmon-rs.
conmon-rs is already quite different from conmon, and if we continue to add new options, I'm concerned more about the maintainability like parsing each option, similar options but different format between them etc.
I'd want separate configs for conmon and conmon-rs. It also helps users understand what feature is supported by the container monitors.
It's not blocking the change though.
d777086 to
a1c4d6a
Compare
monitor_heaptrack_output_path and monitor_log_drivera1c4d6a to
87dccbb
Compare
|
/test ci-e2e-conmonrs |
|
@cri-o/cri-o-maintainers PTAL |
|
/retest |
|
@bitoku @haircommander PTAL |
internal/oci/runtime_pod.go
Outdated
| case "HEAPTRACK_OUTPUT": | ||
| heaptrack.Enabled = true | ||
| heaptrack.OutputPath = filepath.Join(keyVal[1], "cri-o.conmon-rs."+c.ID()) | ||
|
|
||
| case "HEAPTRACK_BINARY_PATH": | ||
| heaptrack.Enabled = true | ||
| heaptrack.BinaryPath = keyVal[1] | ||
|
|
There was a problem hiding this comment.
what if one of them is specified? Do we need the validation or delegate it to conmonClient?
There was a problem hiding this comment.
I'd prefer to let conmon-rs client do that validation and just pass down the variables.
87dccbb to
9f1a1ee
Compare
Reuse `monitor_env` to pass down the options to the pod runtime (conmon-rs). Signed-off-by: Sascha Grunert <[email protected]>
9f1a1ee to
cc10ee3
Compare
|
/lgtm |
|
/test ci-cgroupv2-e2e-crun |
|
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Reuse
monitor_envto pass down the options to the pod runtime (conmon-rs).Which issue(s) this PR fixes:
None
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?