[logs] reduce journal size by default#103
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant SosCLI as Sos CLI
participant LogsPlugin as Logs Plugin
participant Journalctl as systemd/journalctl
SosCLI->>LogsPlugin: invoke with `logs.journal-since` option
LogsPlugin->>LogsPlugin: normalize value → finite cutoff or `all` (None)
alt cutoff is finite
LogsPlugin->>Journalctl: run journalctl with `--since=<cutoff>`
else cutoff is all
LogsPlugin->>Journalctl: run journalctl without `--since`
end
Journalctl-->>LogsPlugin: return journal output
LogsPlugin-->>SosCLI: bundle collected logs into report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sos/report/plugins/logs.py (1)
201-204: Prefer iterable unpacking forCosLogs.option_list.This keeps the composition a bit clearer and avoids the extra list concatenation noted by lint.
♻️ Proposed refactor
- option_list = LogsBase.option_list + [ - PluginOpt(name="log-days", default=3, - desc="the number of days logs to collect") - ] + option_list = [ + *LogsBase.option_list, + PluginOpt(name="log-days", default=3, + desc="the number of days logs to collect"), + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/logs.py` around lines 201 - 204, Replace the list concatenation used to build option_list with iterable unpacking: instead of option_list = LogsBase.option_list + [PluginOpt(...)] create a new list that unpacks the base options and appends the new PluginOpt, e.g. option_list = [*LogsBase.option_list, PluginOpt(name="log-days", default=3, desc="the number of days logs to collect")]; update the assignment where option_list is defined (referencing LogsBase.option_list and PluginOpt) so lint warnings about list concatenation are eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sos/report/plugins/logs.py`:
- Around line 17-35: The resolver currently advertised in
JOURNAL_SINCE_LONG_DESC is broader than what _resolve_journal_since_to_cutoff()
actually parses, causing unsupported inputs to silently fall back in
_collect_raw_journal(); update the code to either (A) extend
_resolve_journal_since_to_cutoff to accept systemd.time-like expressions
(support keywords like "now", "yesterday", "today", relative +/- prefixes, "X
unit" and "X unit ago" spans, space-separated spans, and common timezone-aware
timestamps—use a robust parser like python-dateutil or parsedatetime, normalize
to an mtime cutoff and return a timezone-aware datetime) and add clear logging
on parse failure, or (B) restrict JOURNAL_SINCE_LONG_DESC to exactly the
accepted patterns and add validation in _resolve_journal_since_to_cutoff that
logs/raises on unsupported inputs so _collect_raw_journal no longer silently
copies everything; refer to _resolve_journal_since_to_cutoff,
_collect_raw_journal, and JOURNAL_SINCE_LONG_DESC when applying the change.
---
Nitpick comments:
In `@sos/report/plugins/logs.py`:
- Around line 201-204: Replace the list concatenation used to build option_list
with iterable unpacking: instead of option_list = LogsBase.option_list +
[PluginOpt(...)] create a new list that unpacks the base options and appends the
new PluginOpt, e.g. option_list = [*LogsBase.option_list,
PluginOpt(name="log-days", default=3, desc="the number of days logs to
collect")]; update the assignment where option_list is defined (referencing
LogsBase.option_list and PluginOpt) so lint warnings about list concatenation
are eliminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: caff54c6-5963-4c16-9f7d-0e72ec8db254
📒 Files selected for processing (5)
sos-mlx-cloud-verification.confsos-nvdebug.confsos-nvidia.confsos/report/plugins/logs.pytests/report_tests/plugin_tests/logs.py
noamhofshi
force-pushed
the
nhofshi/add-journal-size-flag
branch
from
9e85eb8 to
3f7218a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sos/report/plugins/logs.py`:
- Around line 172-176: The glob returns sysroot-prefixed paths because pattern
is built with self.path_join(), but add_copy_spec() expects unprefixed paths and
will reapply sysroot; update the call in the loop so you strip the sysroot
before passing to add_copy_spec() by using self.strip_sysroot(path) instead of
path (locate the loop that uses self.path_join(..., "*.journal") and change the
argument to add_copy_spec accordingly), ensuring consistent behavior with other
usages (see add_copy_spec, path_join, strip_sysroot).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 030ef554-5d8c-4a2f-a9eb-079bd8efab43
📒 Files selected for processing (5)
sos-mlx-cloud-verification.confsos-nvdebug.confsos-nvidia.confsos/report/plugins/logs.pytests/report_tests/plugin_tests/logs.py
✅ Files skipped from review due to trivial changes (3)
- sos-nvdebug.conf
- sos-nvidia.conf
- sos-mlx-cloud-verification.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/report_tests/plugin_tests/logs.py
noamhofshi
force-pushed
the
nhofshi/add-journal-size-flag
branch
from
3f7218a to
cde71e1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sos/report/plugins/logs.py (2)
177-180: Add inline comment to theexceptclause to satisfy CodeQL.The rationale is already documented above the
tryblock (lines 177–178), but CodeQL flags the emptyexceptbecause the explanatory comment isn't syntactically inside the handler. Moving or duplicating the explanation satisfies the linter and keeps intent visible at the exception site.📝 Suggested fix
try: if os.path.getmtime(path) >= cutoff: self.add_copy_spec(self.strip_sysroot(path)) - # journald may rotate or vacuum a file between the glob - # and the stat, in which case we just skip it except OSError: - pass + # journald may rotate/vacuum between glob and stat; skip + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/logs.py` around lines 177 - 180, The empty except OSError handler should include an inline explanatory comment to satisfy CodeQL; inside the except OSError: block (the handler that currently only contains "pass") duplicate the rationale already above the try — e.g., "journald may rotate or vacuum a file between the glob and the stat, so skip this file" — so the exception handling intent is documented at the exception site (leave the pass as the behavior).
207-210: Consider iterable unpacking for modern Python style.The concatenation works correctly, but PEP 448 iterable unpacking is considered more idiomatic. This is a minor stylistic preference.
♻️ Optional refactor
- option_list = LogsBase.option_list + [ + option_list = [ + *LogsBase.option_list, PluginOpt(name="log-days", default=3, desc="the number of days logs to collect") ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/logs.py` around lines 207 - 210, The current code builds option_list by concatenating LogsBase.option_list with a list literal; change this to use PEP 448 iterable unpacking for a more idiomatic style by creating option_list with unpacked items from LogsBase.option_list followed by the new PluginOpt instance (referencing the option_list symbol and LogsBase.option_list and PluginOpt to locate the change). Ensure the resulting option_list value is equivalent to the original concatenation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sos/report/plugins/logs.py`:
- Around line 177-180: The empty except OSError handler should include an inline
explanatory comment to satisfy CodeQL; inside the except OSError: block (the
handler that currently only contains "pass") duplicate the rationale already
above the try — e.g., "journald may rotate or vacuum a file between the glob and
the stat, so skip this file" — so the exception handling intent is documented at
the exception site (leave the pass as the behavior).
- Around line 207-210: The current code builds option_list by concatenating
LogsBase.option_list with a list literal; change this to use PEP 448 iterable
unpacking for a more idiomatic style by creating option_list with unpacked items
from LogsBase.option_list followed by the new PluginOpt instance (referencing
the option_list symbol and LogsBase.option_list and PluginOpt to locate the
change). Ensure the resulting option_list value is equivalent to the original
concatenation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7bbd4caf-89ae-405d-9144-ffe048575eeb
📒 Files selected for processing (5)
sos-mlx-cloud-verification.confsos-nvdebug.confsos-nvidia.confsos/report/plugins/logs.pytests/report_tests/plugin_tests/logs.py
✅ Files skipped from review due to trivial changes (2)
- sos-nvdebug.conf
- sos-mlx-cloud-verification.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- sos-nvidia.conf
|
I can suggest a simplified version, support just duration of days and remove the |
| "/var/log/journal/*", | ||
| "/run/log/journal/*" | ||
| ]) | ||
| self._collect_raw_journal(since_arg) |
There was a problem hiding this comment.
suggesting to avoid this part and instead remove the default all_logs that we set in the past form the config files, then if use want to collect the binary he will be able to do so but it's not really needed because add_journal will collect the output into the sos_commands/logs/
There was a problem hiding this comment.
Done, removed it and reduced the custom filters I added
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/report_tests/plugin_tests/logs.py (1)
23-31:⚠️ Potential issue | 🔴 CriticalThe test
test_journal_runtime_collectedwill fail because raw journal files are not collected.The plugin uses
self.add_journal()to capture journal data via journalctl commands and output, but does not copy raw journal files from/var/log/journal/*. The--all-logsglobal flag affects output sizelimit only, not file copying. The test assertion at line 31 expects actual files matching/var/log/journal/*in the archive, which will not be present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/report_tests/plugin_tests/logs.py` around lines 23 - 31, The test fails because raw journal files under /var/log/journal/* are not being copied—only journalctl output is captured via self.add_journal(); update the plugin to also copy raw journal files by adding a call to self.add_copy('/var/log/journal') or self.add_copy('/var/log/journal/*') in the same place where add_journal is invoked so the archive will contain the expected files for test_journal_runtime_collected; reference the add_journal call and the test_journal_runtime_collected assertion to locate where to add the copy.
🧹 Nitpick comments (1)
sos/report/plugins/logs.py (1)
126-129: Consider iterable unpacking (optional style improvement).Ruff suggests using iterable unpacking for cleaner syntax. Both approaches are functionally equivalent.
♻️ Optional style improvement
- option_list = LogsBase.option_list + [ + option_list = [*LogsBase.option_list, PluginOpt(name="log-days", default=3, desc="the number of days logs to collect") ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/logs.py` around lines 126 - 129, The option_list construction in logs.py currently concatenates lists; simplify the style by using iterable unpacking: build option_list by using [*LogsBase.option_list, PluginOpt(...)] instead of LogsBase.option_list + [...]; update the symbol referenced (option_list, LogsBase.option_list, PluginOpt(name="log-days")) to use the unpacking form for clearer, more idiomatic code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/report_tests/plugin_tests/logs.py`:
- Around line 23-31: The test fails because raw journal files under
/var/log/journal/* are not being copied—only journalctl output is captured via
self.add_journal(); update the plugin to also copy raw journal files by adding a
call to self.add_copy('/var/log/journal') or self.add_copy('/var/log/journal/*')
in the same place where add_journal is invoked so the archive will contain the
expected files for test_journal_runtime_collected; reference the add_journal
call and the test_journal_runtime_collected assertion to locate where to add the
copy.
---
Nitpick comments:
In `@sos/report/plugins/logs.py`:
- Around line 126-129: The option_list construction in logs.py currently
concatenates lists; simplify the style by using iterable unpacking: build
option_list by using [*LogsBase.option_list, PluginOpt(...)] instead of
LogsBase.option_list + [...]; update the symbol referenced (option_list,
LogsBase.option_list, PluginOpt(name="log-days")) to use the unpacking form for
clearer, more idiomatic code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 568b429d-5948-4d81-b867-adaca5e43154
📒 Files selected for processing (3)
sos-nvidia.confsos/report/plugins/logs.pytests/report_tests/plugin_tests/logs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- sos-nvidia.conf
| self.add_journal(boot="this", tags='journal_since_boot') | ||
| self.add_journal(boot="last", tags='journal_last_boot') | ||
| if self.get_option("all_logs"): | ||
| self.add_copy_spec([ |
There was a problem hiding this comment.
no need to delete this option, if user will want he can keep the journals, just need to remove this from the config files
There was a problem hiding this comment.
Sure, kept it since I thought it makes sense for debug, removing
There was a problem hiding this comment.
it's a name of a tool not a debug configuration.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sos/report/plugins/logs.py (1)
78-93:⚠️ Potential issue | 🟠 MajorValidate
journal-sincebefore passing it toadd_journal().
add_journal()formatssinceinto thejournalctlcommand string, so forwarding raw config text here makes the new option shell-sensitive. Please escape or reject unsupported characters before calling the helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sos/report/plugins/logs.py` around lines 78 - 93, Validate and sanitize the user-provided since value (since_raw / since_arg from get_option("journal-since")) before passing it to add_journal: accept only a strict whitelist of safe formats (e.g., empty/"all", "boot"/"this"/"last" keywords, ISO-8601 timestamps, or simple relative durations like "1h"/"30m") via a regex/validator and reject or normalize anything containing shell-sensitive characters (e.g., ; & | $ ` > < \\) or not matching the allowed patterns; if validation fails, log a warning and set since_arg to None (or skip the add_journal calls), then call add_journal with the validated/normalized since_arg to avoid injecting unsanitized config into the journalctl command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sos/report/plugins/logs.py`:
- Around line 78-93: Validate and sanitize the user-provided since value
(since_raw / since_arg from get_option("journal-since")) before passing it to
add_journal: accept only a strict whitelist of safe formats (e.g., empty/"all",
"boot"/"this"/"last" keywords, ISO-8601 timestamps, or simple relative durations
like "1h"/"30m") via a regex/validator and reject or normalize anything
containing shell-sensitive characters (e.g., ; & | $ ` > < \\) or not matching
the allowed patterns; if validation fails, log a warning and set since_arg to
None (or skip the add_journal calls), then call add_journal with the
validated/normalized since_arg to avoid injecting unsanitized config into the
journalctl command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1655baec-5963-4916-83a5-5169706cfd73
📒 Files selected for processing (2)
sos-nvdebug.confsos/report/plugins/logs.py
| "expression (e.g. '-7days', '-24hours', '2 hours ago', " | ||
| "'yesterday', '2026-04-19', '2026-04-19 08:00:00'). Use " | ||
| "'all' (or leave empty) for no bound. Plugin default 'all', " | ||
| "overridden to '-7days' by sos-nvidia.conf, sos-nvdebug.conf, " |
There was a problem hiding this comment.
i would not mention all the configuration files, just say by default config
| option_list = [ | ||
| PluginOpt(name="journal-since", default="all", val_type=str, | ||
| desc=("journalctl --since value, or 'all' for no " | ||
| "bound (see systemd.time(7))"), |
There was a problem hiding this comment.
systemd.time(7) what does it mean?
There was a problem hiding this comment.
It's a linux util that converts a number into days :)
Changed the comment to be clearer
| @@ -11,7 +11,6 @@ journal_size = 10 | |||
There was a problem hiding this comment.
you should probably do the same as sos-nvidia.conf for this field
There was a problem hiding this comment.
Yes, since now I removed the collect all logs by default it makes sense
Reduce the journal size collection by default to 7 days instead of infinite. And remove the auto collect of all the archives by default Signed-off-by: Noam Hofshi <nhofshi@nvidia.com>
noamhofshi
force-pushed
the
nhofshi/add-journal-size-flag
branch
from
486f407 to
e76e446
Compare
3 participants
Reduce the journal size collection by default to 7 days instead of infinite.
Also added validation that we don't collect (by default) old "active" journal files from older OS installations.
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines
Summary by CodeRabbit