[policy] - Add rootless container support and log-line limiting - #4446
[policy] - Add rootless container support and log-line limiting#4446snagoor wants to merge 2 commits into
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
…container helpers Signed-off-by: Nagoor Shaik <nshaik@redhat.com> Assisted-by: Claude Code
629730f to
acbd9a7
Compare
fc7b883 to
913e643
Compare
913e643 to
860c97b
Compare
| elif _runtime is not None: | ||
| _cons = self.get_all_containers_by_regex(container, get_all) | ||
| for _con in _cons: | ||
| cmd = _runtime.get_logs_command(_con[1]) | ||
| self.add_cmd_output(cmd, **kwargs) | ||
| else: | ||
| self._log_debug(f"No container runtime available to collect " | ||
| f"logs for '{container}'") | ||
| continue | ||
| if _runtime is None: | ||
| self._log_debug(f"No container runtime available to collect " | ||
| f"logs for '{container}'") |
There was a problem hiding this comment.
We shouldn't be doing this check with every container in the list.
The runtime check should remain outside the loop, as it is currently.
There was a problem hiding this comment.
Thanks for the review @TurboTurtle , agreed. I've moved the runtime check and resolution back out of the loop with an early return if no runtime is available.
While restructuring, I also noticed get_containers_by_user() was being called once per container in the runas branch. Since the exact-match filtering doesn't depend on per-container runtime state, the user's rootless runtime is now queried just once before the loop and matched inside it.
| return vols | ||
|
|
||
| def get_logs_command(self, container): | ||
| def get_logs_command(self, container, log_lines=None): |
There was a problem hiding this comment.
If we're going to add this parameter, we should hook it up to the actual collection for the runtime. If the runtime doesn't support it, then we shouldn't make it available as a parameter.
There was a problem hiding this comment.
LXD collects logs via lxc info --show-log, which doesn't support limiting line output, so the parameter can't be meaningfully hooked up there.
Changes made:
-
Reverted
LxdContainerRuntime.get_logs_command()to its original signature withoutlog_lines. -
Added a
log_line_limitcapability attribute toContainerRuntime(defaultTrue,Falseon LXD).add_container_logs()now checks this capability and falls back to full logs with a debug message if line limits aren't supported. -
Applied the same logic to
runas:removed it from the CRI-O and LXDget_containers()overrides (since they are daemon-based and don't support rootless mode) and added arootlesscapability check. -
Overrode
CrioContainerRuntime.get_logs_command()so it usescrictl logs --tail Ncorrectly (crictluses -t for --tail, unlike Docker/Podman where-tmeanstimestamps).
There was a problem hiding this comment.
I don't see where we're using any of these new methods?
Also, I don't see why these would be methods at all. If they aren't class attrs being set like we do with PackageManager() classes, then they'd probably need to be properties.
There was a problem hiding this comment.
These four helpers form the runtime API surface is consumed by dependent PR #4450 (which refactors aap_containerized plugin to remove hardcoded su - <user> -c 'podman ...' strings). Because #4450 stacks on this PR, they appear unused in isolation.
On methods vs. properties: They are methods because they accept parameters (like container, cmd, get_all, or list_fmt), which properties can't do. This matches the established parameterized pattern on ContainerRuntime (e.g., get_logs_command() and get_copy_command()).
Fixed a docstring copy-paste error in info_command() and refactored exec_command() to reuse the self.run_cmd prefix as well.
860c97b to
a8c97c6
Compare
|
Summary of current changes:
|
b6239c4 to
a7dba9e
Compare
| :returns: Formatted runtime command to get logs from `container` | ||
| :rtype: ``str`` | ||
| """ | ||
| if log_lines is not None: |
There was a problem hiding this comment.
Same change applied to the CRI-O override, crictl logs --tail N is only emitted when log_line_limit is set.
| _log_lines = None | ||
| else: | ||
| _log_lines = log_lines | ||
| if _log_lines is not None: |
There was a problem hiding this comment.
This condition is redundant; if _log_lines is None, we can still call get_logs_command with log_lines=_log_lines due to the if log_lines is not None: test there.
There was a problem hiding this comment.
Agreed, the if _log_lines is not None: branch was redundant, since get_logs_command(..., log_lines=None) and get_logs_command(...) are equivalent (the parameter defaults to None).
I've dropped the intermediate _log_lines variable and pass log_lines straight through; the runtime itself now decides whether to append --tail (see the log_line_limit guard), and the plugin keeps only the debug message when the capability is off.
| return f"{self.binary} info --debug" | ||
| return f"{self.binary} info" | ||
|
|
||
| def list_command(self, get_all=False, list_fmt=None): |
There was a problem hiding this comment.
The list_fmt is not used even in the #4450, is it worth having it?
Further, calling this method makes sense "only" for non-root users (with runas set), right? Since runtime does automatically collect this info already (just for root). Since the #4450 uses this method for runas set, we cant reuse the runtime's collected info and the method is really necessary. (this took me a while to understand rationale for the method).
| system-level runtime. Container names are then | ||
| treated as *exact* matches, not regexes, against |
There was a problem hiding this comment.
Couldnt this inconsistency (regexp for root containers, exact match for rootless) be confusing for users using this method?
There was a problem hiding this comment.
Good catch, you're right that the inconsistency is confusing. A plugin author passing a regex like my-app.* would get regex semantics on the system-level runtime but silent no-matches on the rootless path.
Fixed: the rootless path now also uses re.match() when filtering the pre-fetched user container list, so both code paths behave identically.
981f09c to
a6618a3
Compare
…ontainer helpers Signed-off-by: Nagoor Shaik <nshaik@redhat.com> Assisted-by: Claude Code
a6618a3 to
1e148c5
Compare
This PR extends the container runtime helpers so sos plugins can collect data from rootless containers (containers started by a non-root user) and optionally limit the number of log lines collected per container.
Changes at a glance
sos/policies/runtimes/__init__.pyget_containers()gains arunasparameter. When set, thepscommand is run as that user viasos_get_command_output(runas=...), making the user's rootless containers visible.get_logs_command()gains alog_linesparameter. When set,--tail Nis appended to the logs command so output is capped.sos/policies/runtimes/podman.pyPodmanContainerRuntime:info_command(run_debug=False)— returnspodman info [--debug]list_command(get_all, list_fmt)— returns apodman pscommand with a format string (defaults to JSON)inspect_command(container)— returnspodman inspect <container>exec_command(container, cmd)— returnspodman exec <container> <cmd>sos/report/plugins/__init__.pyNew
get_containers_by_user(user, get_all=False)method onPlugin. Queries the loaded runtime asuserso plugins can discover rootless containers owned by a specific user.add_container_logs()gains two new parameters:runas— when set, containers are looked up in the named user's rootless runtime using an exact name match instead of a regex.log_lines— passed through toget_logs_command()to cap log output.Assisted-by:Claude Code