kdump: T8868: Implement kernel crash dump support - #5325
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used🔍 Remote MCP vyos.devRelevant task context
🔀 Multi-repo context ansible/ansibleLinked repositories findingsansible/ansible
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds configurable kdump support with crashkernel validation and generation, kdump-tools packaging and service wiring, operational status and dump reporting, technical-support archival, image-installation integration, boot-argument utilities, and integration tests. ChangesKdump support
Merge Risk: ⚪ Minimal · up to The PR adds kdump configuration and diagnostics, and the supplied evidence identifies no concrete current-head defect; no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
660fdb2 to
3125f06
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
interface-definitions/system_option.xml.in (2)
433-446: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
dump-pathregex allows unsanitized paths.
/[^\s]+accepts anything starting with/, including..traversal segments (e.g./var/../etc). Sinceos.makedirs(dump_path, exist_ok=True)is later called on this value (persrc/conf_mode/system_option.py:531-538), a path with..could resolve outside the intended crash-dump tree. Low risk since it's admin-configured, but worth normalizing/validating.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interface-definitions/system_option.xml.in` around lines 433 - 446, The dump-path validation currently permits parent-directory traversal segments. Update the dump-path leafNode constraint and constraintErrorMessage to reject path components containing “..” (and other unsafe traversal forms) while still requiring an absolute path; ensure the validation matches the value consumed by system_option.py before os.makedirs is called.
415-431: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMemory regex is far looser than the actual
crashkernelparser.
auto|[0-9\-:,MG]+only checks the character set, not structure. Strings like,,--::MGor1M-2M(missing:size) will pass CLI validation but fail with aValueErrorinside_parse_crashkernel_rangesat commit time (persrc/conf_mode/system_option.py:202-240), producing a worse UX than catching the error at the CLI. ThevalueHelp/constraintErrorMessagealso only documentautoand a flat size (e.g.256M), omitting the tiered range syntax (<low>-<high>:<size>[,...]) that the smoketests and parser actually support (e.g.1M-256M:64M,256M-1G:128M,1G-64G:768M,64G-:1G).Consider tightening the regex to reflect the tiered-range grammar (or delegate to a dedicated validator script), and update the help/error text to mention the range syntax.
Example tightened regex
- <constraint> - <regex>auto|[0-9\-:,MG]+</regex> - </constraint> - <constraintErrorMessage>Memory must be 'auto' or a size like 256M or 1G</constraintErrorMessage> + <constraint> + <regex>auto|[0-9]+[MG]|([0-9]*[MG]?-[0-9]*[MG]?:[0-9]+[MG](,[0-9]*[MG]?-[0-9]*[MG]?:[0-9]+[MG])*)</regex> + </constraint> + <constraintErrorMessage>Memory must be 'auto', a size like 256M/1G, or a tiered range like 1G-8G:512M,8G-64G:768M,64G-:1G</constraintErrorMessage>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interface-definitions/system_option.xml.in` around lines 415 - 431, Replace the permissive memory constraint in the memory leafNode with validation matching _parse_crashkernel_ranges, accepting auto, a standalone size, and comma-separated <low>-<high>:<size> ranges while rejecting malformed values. Update valueHelp and constraintErrorMessage to document the tiered range syntax with an example such as 1M-256M:64M,256M-1G:128M.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoketest/scripts/cli/test_system_option.py`:
- Around line 181-194: Use a non-existent child path beneath self.dump_path.name
for dump_path in the test, rather than the already-created TemporaryDirectory
itself. Keep the cli_set/cli_commit flow and assert os.path.isdir(dump_path)
afterward so test_system_option verifies conf-mode directory creation during
commit.
In `@src/op_mode/generate_tech-support_archive.py`:
- Around line 118-134: Guard the best-effort crash-dump lookup around json.loads
in __generate_archived_files so malformed output from kdump.py cannot abort the
main archive process. Catch JSON parsing failures (and handle invalid dump data
safely), emit a warning, and continue with archive_dict unchanged so the
remaining archive entries are still generated.
---
Nitpick comments:
In `@interface-definitions/system_option.xml.in`:
- Around line 433-446: The dump-path validation currently permits
parent-directory traversal segments. Update the dump-path leafNode constraint
and constraintErrorMessage to reject path components containing “..” (and other
unsafe traversal forms) while still requiring an absolute path; ensure the
validation matches the value consumed by system_option.py before os.makedirs is
called.
- Around line 415-431: Replace the permissive memory constraint in the memory
leafNode with validation matching _parse_crashkernel_ranges, accepting auto, a
standalone size, and comma-separated <low>-<high>:<size> ranges while rejecting
malformed values. Update valueHelp and constraintErrorMessage to document the
tiered range syntax with an example such as 1M-256M:64M,256M-1G:128M.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dad06102-12a9-49db-8d70-88f5bd74287c
📒 Files selected for processing (13)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/utils/boot.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/op_mode/generate_tech-support_archive.pysrc/op_mode/kdump.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (4)
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / build_iso: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 586bfbf3661a8ba3c8a74ad50a1fff2d915f7e00
##[error]fatal: remote error: upload-pack: not our ref 586bfbf3661a8ba3c8a74ad50a1fff2d915f7e00
GitHub Actions: VyOS ISO Integration Test / set_config: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 586bfbf3661a8ba3c8a74ad50a1fff2d915f7e00
##[error]fatal: remote error: upload-pack: not our ref 586bfbf3661a8ba3c8a74ad50a1fff2d915f7e00
🧰 Additional context used
📓 Path-based instructions (7)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/defaults.pypython/vyos/utils/boot.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/defaults.pysmoketest/scripts/system/test_kernel_options.pysrc/op_mode/show_techsupport_report.pypython/vyos/utils/boot.pysrc/op_mode/kdump.pysmoketest/scripts/cli/test_system_option.pysrc/op_mode/generate_tech-support_archive.pysrc/conf_mode/system_option.py
data/templates/**/*.j2
📄 CodeRabbit inference engine (AGENTS.md)
Prefer storing Jinja2 templates as discrete files in
data/templates/rather than inline Python strings
Files:
data/templates/system/kdump_tools.conf.j2
**/*.j2
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates must pass linting validation
Files:
data/templates/system/kdump_tools.conf.j2
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.pysmoketest/scripts/cli/test_system_option.py
src/op_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Op-mode scripts must be located in
src/op_mode/
Files:
src/op_mode/show_techsupport_report.pysrc/op_mode/kdump.pysrc/op_mode/generate_tech-support_archive.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (3)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.pysmoketest/scripts/cli/test_system_option.py
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
src/op_mode/show_techsupport_report.pysrc/op_mode/kdump.pysmoketest/scripts/cli/test_system_option.pysrc/op_mode/generate_tech-support_archive.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_system_option.py
🪛 ast-grep (0.44.1)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
src/conf_mode/system_option.py
[error] 540-540: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 554-554: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', {'disabled': True})
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP
Additional review context
kdump-toolsuses/etc/default/kdump-toolsas its shell-sourced config file;USE_KDUMPcontrols whether the init script runs,KDUMP_COREDIRdefaults to/var/crash, andKDUMP_CMDLINE_APPENDdefaults toreset_devices systemd.unit=kdump-tools-dump.service nr_cpus=1 irqpoll usbcore.nousb. (manpages.debian.org)- Linux kernel docs support both
crashkernel=size[@offset]and RAM-dependentcrashkernel=range1:size1[,range2:size2,...][@offset], where the range syntax is based on system RAM. (kernel.org) - Debian package metadata shows
kdump-toolsalready ships/usr/lib/systemd/system/kdump-tools.service,/usr/lib/systemd/system/kdump-tools-dump.service,/etc/default/grub.d/kdump-tools.cfg, and an initramfs hook at/usr/share/initramfs-tools/hooks/kdump-tools. (packages.debian.org) - Debian changelog records architecture-specific
crashkerneldefaults inkdump-tools.cfg:128Mfor all butppc64el, and onppc64ela tiered range2G-4G:320M,4G-32G:512M,32G-64G:1024M,64G-128G:2048M,128G-:4096M@32M. (sources.debian.org)
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible contains generic kernel-command-line fact tests for
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py(lines 13–28). These validate parsing into thecrashkernelfact but do not reference VyOS’s newsystem kdumpconfiguration or APIs.[::ansible/ansible::] - No references were found to
kdump-tools,makedumpfile,kdump.py,get_kernel_boot_args, or the new VyOS service/report integration.[::ansible/ansible::]
🔇 Additional comments (18)
smoketest/scripts/cli/test_system_option.py (4)
19-38: LGTM!Also applies to: 140-140
232-263: LGTM!
265-278: LGTM!
280-317: LGTM!smoketest/scripts/system/test_kernel_options.py (1)
171-181: LGTM!Confirmed
CONFIG_KEXEC,CONFIG_CRASH_DUMP,CONFIG_DEBUG_INFO, andCONFIG_PROC_VMCOREare the standard documented kdump kernel prerequisites.debian/control (1)
113-115: LGTM!python/vyos/defaults.py (1)
52-52: LGTM!src/etc/systemd/system/kdump-tools.service.d/override.conf (2)
10-10: 🎯 Functional Correctness
EnvironmentFile=still depends on/etc/default/kdump-tools.src/conf_mode/system_option.pyrenders that file when conf-mode runs, but this tree does not show a packaged default undersrc/etc/default/. Confirm the unit cannot start before that render; otherwise prefixsrc/etc/systemd/system/kdump-tools.service.d/override.conf:10with-.
3-5: 🎯 Functional CorrectnessNo boot-ordering gap here
src/etc/systemd/system/kdump-tools.service.d/override.conf:3-5is still pulled in byvyos.target, andsrc/systemd/vyos.target:1-3is already ordered aftermulti-user.target, so removing the inheritedBefore=/After=entries does not leave kdump in an early-boot race.> Likely an incorrect or invalid review comment.src/conf_mode/system_option.py (3)
202-273: LGTM!
401-442: LGTM!
518-562: 🎯 Functional Correctness | ⚡ Quick win
dump_pathis passed to the template unguarded when unset.
kdump.get('dump_path')can beNone/falsy (line 536 only conditionally creates the directory), yet the fullkdumpdict — including a possibly-Nonedump_path— is passed torender()at line 541 regardless. See the companion comment ondata/templates/system/kdump_tools.conf.j2(Lines 13-14) for the resulting rendering defect.data/templates/system/kdump_tools.conf.j2 (2)
8-16: 🎯 Functional Correctness | ⚡ Quick winGuard
dump_paththe same waydisabledis guarded, to avoidKDUMP_COREDIR="None".Line 14 renders
dump_pathunconditionally. If it's unset (nullable insystem_option.py'sgenerate()— see theif dump_path:guard there, and insrc/op_mode/kdump.py's_get_raw_status), Jinja2 will render Python'sNoneas the literal string"None", producingKDUMP_COREDIR="None"in/etc/default/kdump-tools. That's an invalid path and silently breaks the "save dumps locally" behavior kdump-tools would otherwise apply if the variable were left unset entirely (kdump-tools defaults to/var/crashonly when the var is absent, not when it's a garbage string).Use the same
is vyos_definedguard already used fordisabled:🛡️ Proposed fix
# Local directory where crash dumps are stored -KDUMP_COREDIR="{{ dump_path }}" +{% if dump_path is vyos_defined %} +KDUMP_COREDIR="{{ dump_path }}" +{% endif %}This depends on whether
dump_pathtruly can be unset per the (not-included)interface-definitions/system_option.xml.in— if it has a mandatory default there, this is moot, but the multipleif dump_path:guards elsewhere in the codebase suggest it's optional.
14-14: 🔒 Security & Privacy | ⚡ Quick winUnescaped shell interpolation of
dump_path.
KDUMP_COREDIR="{{ dump_path }}"is written into a shell-sourced file (/etc/default/kdump-tools, sourced by root-run init scripts). If the interface-definition doesn't restrictdump_path's character set, a value containing"or backticks could inject shell commands executed as root when this file is sourced. Exploitability is limited to users who already have privileged config access, but worth confirming the schema constrains the path format.python/vyos/utils/boot.py (1)
47-83: LGTM!src/op_mode/kdump.py (1)
47-277: LGTM!op-mode-definitions/show-system.xml.in (1)
100-113: LGTM!src/op_mode/show_techsupport_report.py (1)
546-550: LGTM!
There was a problem hiding this comment.
Pull request overview
This PR adds kernel crash dump (kdump) support to VyOS by introducing new configuration CLI under system option kdump, new operational commands under show system kdump, and integrating kdump status/dumps into tech-support reporting and archives.
Changes:
- Add new op-mode command implementation (
kdump.py) and wire it into the op-mode XML tree. - Extend
system_optionto managecrashkernel=reservation, generate/etc/default/kdump-tools, and add an initramfs overlay workaround. - Enhance tech-support outputs (report + archive) to include kdump status and the most recent crash dump (with size cap), plus add smoketests and required Debian deps.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/op_mode/show_techsupport_report.py | Adds a new kernel-dump report section to include kdump status and dumps. |
| src/op_mode/kdump.py | Implements show system kdump / show system kdump dumps op-mode commands. |
| src/op_mode/generate_tech-support_archive.py | Archives the most recent crash dump directory (capped at 1 GiB) into tech-support archives. |
| src/etc/systemd/system/kdump-tools.service.d/override.conf | Adds a systemd override to integrate kdump-tools into vyos.target. |
| src/conf_mode/system_option.py | Adds kdump config handling: crashkernel=, dump dir creation, kdump-tools config rendering, initramfs hook. |
| smoketest/scripts/system/test_kernel_options.py | Adds kernel config smoketest coverage for kdump-related kernel options. |
| smoketest/scripts/cli/test_system_option.py | Adds CLI/op-mode integration tests for kdump config + op commands. |
| python/vyos/utils/boot.py | Adds helper to read/parse kernel cmdline args (get_kernel_boot_args). |
| python/vyos/defaults.py | Registers kdump-tools.service in systemd_services. |
| op-mode-definitions/show-system.xml.in | Adds show system kdump and show system kdump dumps op-mode nodes. |
| interface-definitions/system_option.xml.in | Adds system option kdump CLI subtree (memory + dump-path). |
| debian/control | Adds runtime dependencies for crash dump support (kdump-tools, makedumpfile). |
| data/templates/system/kdump_tools.conf.j2 | Adds template to generate /etc/default/kdump-tools based on VyOS config. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/conf_mode/system_option.py (1)
518-562: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
autoreservation is unvalidated and reserves nothing on sub-1G RAM.
verify()skips validation whenmemory == 'auto'(Line 410), but the expanded range here starts at1G-8G:512M. On a system with <1G RAM no tier matches, so the kernel reserves nothing and kdump silently never arms — with no config-time error. Consider adding a lowest tier or validatingautoagainst total RAM.The static-analysis HTML/XSS hints on Lines 540/554 are false positives —
render()targets a config file, not web output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf_mode/system_option.py` around lines 518 - 562, The auto crash-kernel reservation leaves systems with less than 1G RAM without any reservation. Update the auto-sizing logic in the kdump configuration block and its validation in verify() so every supported RAM size, including sub-1G systems, maps to a valid reservation; add a lowest tier or reject unsupported systems explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/conf_mode/system_option.py`:
- Around line 518-562: The auto crash-kernel reservation leaves systems with
less than 1G RAM without any reservation. Update the auto-sizing logic in the
kdump configuration block and its validation in verify() so every supported RAM
size, including sub-1G systems, maps to a valid reservation; add a lowest tier
or reject unsupported systems explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5e30ff5b-0e99-42d7-8891-594363dbe883
📒 Files selected for processing (13)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/utils/boot.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/op_mode/generate_tech-support_archive.pysrc/op_mode/kdump.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
✅ Files skipped from review due to trivial changes (1)
- data/templates/system/kdump_tools.conf.j2
🚧 Files skipped from review as they are similar to previous changes (10)
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- op-mode-definitions/show-system.xml.in
- python/vyos/defaults.py
- interface-definitions/system_option.xml.in
- src/op_mode/show_techsupport_report.py
- debian/control
- python/vyos/utils/boot.py
- src/op_mode/generate_tech-support_archive.py
- src/op_mode/kdump.py
- smoketest/scripts/cli/test_system_option.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (4)
GitHub Actions: VyOS ISO Integration Test / build_iso: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 27846041b8baf83626faf96fe0f5d6ef4fe626f9
##[error]fatal: remote error: upload-pack: not our ref 27846041b8baf83626faf96fe0f5d6ef4fe626f9
GitHub Actions: VyOS ISO Integration Test / set_config: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 27846041b8baf83626faf96fe0f5d6ef4fe626f9
##[error]fatal: remote error: upload-pack: not our ref 27846041b8baf83626faf96fe0f5d6ef4fe626f9
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
smoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (1)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
src/conf_mode/system_option.py
[error] 540-540: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 554-554: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', {'disabled': True})
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Additional review context
- Linux documents
crashkerneltiered syntax asrange1:size1[,range2:size2,...][@offset], with ranges formattedstart-end; unbounded upper ranges such as2G-are valid. This supports the PR’s tiered-memory parsing and generatedcrashkernelvalues. - Linux also supports scalar reservations in the form
crashkernel=size@offset. - systemd’s own unit examples use
Type=oneshot,RemainAfterExit=yes,ExecStop, and an[Install] WantedBy=...section for one-time initialization services, matching the general pattern of the added kdump override.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible contains generic kernel-command-line fact tests for
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py(lines 13–28). These validate parsing into thecrashkernelfact but do not reference VyOS’s newsystem kdumpconfiguration or APIs.[::ansible/ansible::] - No references were found to
kdump-tools,makedumpfile,kdump.py,get_kernel_boot_args, or the new VyOS service/report integration.[::ansible/ansible::]
🔇 Additional comments (8)
src/conf_mode/system_option.py (7)
28-34: LGTM!Also applies to: 70-78
174-178: LGTM!
202-251: LGTM!
254-272: LGTM!
275-292: LGTM!
428-442: LGTM!
414-427: 🩺 Stability & AvailabilityNo issue here
python/vyos/utils/convert.py::human_to_bytes()raisesValueErroron any non-scalar input, sosrc/conf_mode/system_option.py:414-427correctly falls through to_find_applicable_crashkernel_range()for tieredkdumpranges.> Likely an incorrect or invalid review comment.smoketest/scripts/system/test_kernel_options.py (1)
171-181: 📐 Maintainability & Code QualityNo issue here.
> Likely an incorrect or invalid review comment.
f948efe to
c45e981
Compare
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/system_option.py`:
- Around line 243-248: Validate the converted range in the entry-building logic
before appending it to entries: when both low_bytes and high_bytes are present,
reject the entry if low_bytes exceeds high_bytes, including the `64G-8G:1G`
case. Preserve valid bounded ranges and unbounded low/high forms, and ensure
invalid ranges cannot proceed to verification or output.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 47e6711f-6d2e-49ea-81fc-132964251f77
📒 Files selected for processing (13)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/utils/boot.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/op_mode/generate_tech-support_archive.pysrc/op_mode/kdump.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (9)
- debian/control
- data/templates/system/kdump_tools.conf.j2
- interface-definitions/system_option.xml.in
- op-mode-definitions/show-system.xml.in
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- python/vyos/defaults.py
- src/op_mode/generate_tech-support_archive.py
- src/op_mode/kdump.py
- smoketest/scripts/cli/test_system_option.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (4)
GitHub Actions: VyOS ISO Integration Test / set_config: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / build_iso: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 27846041b8baf83626faf96fe0f5d6ef4fe626f9
##[error]fatal: remote error: upload-pack: not our ref 27846041b8baf83626faf96fe0f5d6ef4fe626f9
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin 27846041b8baf83626faf96fe0f5d6ef4fe626f9
##[error]fatal: remote error: upload-pack: not our ref 27846041b8baf83626faf96fe0f5d6ef4fe626f9
🧰 Additional context used
📓 Path-based instructions (5)
src/op_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Op-mode scripts must be located in
src/op_mode/
Files:
src/op_mode/show_techsupport_report.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/op_mode/show_techsupport_report.pypython/vyos/utils/boot.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/utils/boot.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (2)
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
src/op_mode/show_techsupport_report.py
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 539-539: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 553-553: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', {'disabled': True})
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Relevant review context
- Linux supports both scalar
crashkernel=size[@offset]and tiered syntax such as512M-2G:64M,2G-:128M. The proposed configuration validation appears to allow scalar and tiered ranges but not@offset; confirm this is an intentional limitation. - The proposed
kdump-tools.serviceoverride follows a documented systemd pattern:Type=oneshot,RemainAfterExit=yes,ExecStop, and installation throughWantedBy=are used for one-time initialization services.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible tests generic parsing of
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS-specific kdump integration or changed API consumers were found. [::ansible/ansible::]
🔇 Additional comments (5)
python/vyos/utils/boot.py (1)
18-22: LGTM!Also applies to: 47-83
src/op_mode/show_techsupport_report.py (1)
546-550: LGTM!smoketest/scripts/system/test_kernel_options.py (1)
171-180: LGTM!src/conf_mode/system_option.py (2)
413-426: 🎯 Functional CorrectnessConfirm that rejecting
size@offsetis intentional.
src/conf_mode/system_option.pyLines 413-426 treat values such as256M@32Mas malformed tiered syntax, although Linux supports scalarcrashkernel=size@offset. Either support it or document the CLI limitation.Source: MCP tools
28-34: LGTM!Also applies to: 70-78, 174-178, 202-242, 249-291, 400-412, 427-441, 517-560
c45e981 to
2c372a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/system_option.py`:
- Around line 408-449: Update verify() so kdump memory set to 'auto' is expanded
and passed through the same _find_applicable_crashkernel_range validation as
manual tiered ranges, raising ConfigError when no range covers the system RAM;
preserve scalar validation and the existing total-memory check. Define or reuse
the KDUMP_AUTO_MEMORY constant, and update generate() to use it instead of the
inline tiered-range literal.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 21bc9394-be1b-447d-85a7-c8421cf45631
📒 Files selected for processing (13)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/utils/boot.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/op_mode/generate_tech-support_archive.pysrc/op_mode/kdump.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (10)
- op-mode-definitions/show-system.xml.in
- src/op_mode/show_techsupport_report.py
- debian/control
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- interface-definitions/system_option.xml.in
- src/op_mode/kdump.py
- data/templates/system/kdump_tools.conf.j2
- python/vyos/utils/boot.py
- smoketest/scripts/cli/test_system_option.py
- src/op_mode/generate_tech-support_archive.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (4)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/defaults.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/defaults.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (1)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 547-547: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 561-561: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', {'disabled': True})
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Relevant review context
- Linux supports scalar
crashkernel=size@offsetand ranged syntaxrange:size[,range:size...]@offset``; omitting the offset or using@0enables automatic placement. The PR’s validation appears to reject `@offset`, so confirm this limitation is intentional. - Kdump requires
CONFIG_KEXECorCONFIG_KEXEC_FILE,CONFIG_CRASH_DUMP,CONFIG_DEBUG_INFO, andCONFIG_PROC_VMCORE; the added kernel-config smoketest checks these symbols. - The proposed
Type=oneshot+RemainAfterExit=yes+ExecStopservice structure is an established systemd pattern for initialization services.WantedBy=installation is also consistent with systemd service enablement.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Generic command-line fact tests cover parsing
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS-specific kdump consumers or shared configuration contracts were found. [::ansible/ansible::]
🔇 Additional comments (8)
python/vyos/defaults.py (1)
52-52: LGTM!src/conf_mode/system_option.py (6)
546-556: Silently ignoring the static analysis "HTML string" hints.Lines 548 and 562 flagged by the linter as "html-string-from-parameters" (CWE-79) are false positives —
render()writes a Jinja2-rendered kdump-tools config file, not HTML output; there's no XSS surface here.
202-259: 🎯 Functional Correctness | ⚡ Quick winCrashkernel range parser rejects the standard
@offsetsuffix.Linux's
crashkernel=syntax allows an optional trailing@offseton both the scalar and ranged forms (e.g.crashkernel=512M-2G:64M,2G-:128M@16M)._parse_crashkernel_rangesrequires every comma-separated token to match<low>-<high>:<size>with no allowance for@offset; any offset suffix on the last token corruptssize_strand raisesValueError, and the same happens for a plaincrashkernel=<size>@<offset>scalar inverify()(Line 421-424) sincehuman_to_byteswill choke on the@. Confirm this is an intentional simplification (relying on kernel auto-placement) rather than an oversight, and if so consider documenting it in the docstring.
542-545: 🔒 Security & Privacy | ⚡ Quick win
dump_pathcreated with default permissions — crash dumps can leak sensitive kernel memory.
os.makedirs(dump_path, exist_ok=True)uses the default mode (0o777 & ~umask), so the directory (and by extension the vmcore files kdump writes into it) may end up world- or group-readable. Kernel crash dumps can contain secrets (keys, credentials, plaintext buffers) that were resident in memory at panic time, so the directory should be created with a restrictive mode.🛡️ Suggested fix
- dump_path = kdump.get('dump_path') - if dump_path: - os.makedirs(dump_path, exist_ok=True) + dump_path = kdump.get('dump_path') + if dump_path: + os.makedirs(dump_path, mode=0o700, exist_ok=True) + os.chmod(dump_path, 0o700)
174-178: LGTM!Managed-param entry for
crashkernelfollows the existingparse/clean/typepattern used by other string-typed cmdline params (e.g.default_hugepagesz,mitigations).
282-306: LGTM!
get_config()correctly derivesis_live_bootanddisabledfor thekdumpsubtree only when present, whichverify()/generate()then rely on directly.
28-34: 🎯 Functional Correctness
boot_configuration_completeis used later insrc/conf_mode/system_option.py(lines 677, 684); no W0611 issue here.> Likely an incorrect or invalid review comment.smoketest/scripts/system/test_kernel_options.py (1)
171-181: LGTM!
test_kdumpcorrectly checks the kernel config symbols required for kdump support, matching the documentedCONFIG_KEXEC/CONFIG_CRASH_DUMP/CONFIG_DEBUG_INFO/CONFIG_PROC_VMCORErequirement set.Note: the static analysis hints about XPath injection and ReDoS on Line 178-179 are false positives — this reads a local, non-attacker-controlled
/proc/config.gzfile, not user input.
2c372a7 to
ef4b3a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/system_option.py`:
- Around line 525-540: Update image_installer.py:get_cli_kernel_options() to
also read system.option.kdump and append the matching crashkernel parameter,
including disabled-state handling, explicit memory values, and the auto-sizing
mapping used by system_option.py. Preserve the existing kernel-option behavior
while ensuring new installs receive the kdump reservation.
- Around line 418-445: Update the kdump memory validation around
_find_applicable_crashkernel_range and the memory_bytes >= virtual_memory.total
check to compare against pre-reservation system RAM. Obtain or calculate the
total before the active crashkernel reservation, such as by adding back the
current crashkernel region, and use that value consistently for tier matching
and the final size validation.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 94fa2d52-8268-4228-8e59-f4d9990ede09
📒 Files selected for processing (13)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/utils/boot.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/op_mode/generate_tech-support_archive.pysrc/op_mode/kdump.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (10)
- debian/control
- interface-definitions/system_option.xml.in
- src/op_mode/kdump.py
- src/op_mode/show_techsupport_report.py
- data/templates/system/kdump_tools.conf.j2
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- op-mode-definitions/show-system.xml.in
- python/vyos/utils/boot.py
- src/op_mode/generate_tech-support_archive.py
- smoketest/scripts/cli/test_system_option.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (4)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/defaults.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/defaults.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (1)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 547-547: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 561-561: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', {'disabled': True})
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Additional review context
- The proposed
Type=oneshot+RemainAfterExit=yes+ExecStopstructure is a documented systemd pattern for one-time initialization with cleanup;WantedBy=is used for installation. This supports the service override design. - The kernel documentation query did not return relevant kdump or
crashkerneldocumentation, so it provides no additional validation of the PR’s crashkernel parsing or required kernel configuration checks.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Generic command-line fact tests cover parsing
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS-specific kdump consumers or shared configuration contracts were found. [::ansible/ansible::]
🔇 Additional comments (6)
python/vyos/defaults.py (1)
52-52: LGTM!smoketest/scripts/system/test_kernel_options.py (1)
171-180: LGTM!src/conf_mode/system_option.py (4)
28-34: LGTM!
70-78: LGTM!Also applies to: 174-178
202-299: LGTM!
542-569: LGTM!
|
I believe we also should update and the |
ff54d79 to
501a2f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/vyos/utils/kernel.py`:
- Around line 166-178: Update the console detection logic around
get_kernel_boot_arg and cmdline_console_re to inspect every console= argument in
the raw kernel command line rather than only the first value. Continue searching
until a matching serial console (ttyS or ttyAMA with its number and speed) is
found, while preserving the existing (None, None, None) result when no match
exists.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e99324bf-284e-4d45-9445-49472aa41bf5
📒 Files selected for processing (21)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/system/image.pypython/vyos/utils/boot.pypython/vyos/utils/kernel.pypython/vyos/utils/memory.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/helpers/run-config-activation.pysrc/helpers/run-config-migration.pysrc/helpers/vyos-boot-config-loader.pysrc/op_mode/generate_tech-support_archive.pysrc/op_mode/image_installer.pysrc/op_mode/kdump.pysrc/op_mode/memory.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- op-mode-definitions/show-system.xml.in
- debian/control
- interface-definitions/system_option.xml.in
- python/vyos/defaults.py
- src/op_mode/show_techsupport_report.py
- python/vyos/utils/boot.py
- src/op_mode/generate_tech-support_archive.py
- src/op_mode/kdump.py
- smoketest/scripts/cli/test_system_option.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run echo "### 🧪 Lint Results"
�[36;1mecho "### 🧪 Lint Results"�[0m
�[36;1mdarker_failed="1"�[0m
�[36;1mgraylint_failed=""�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Darker** check failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Darker** check passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "- ❌ **Graylint (ruff check)** failed"�[0m
�[36;1melse�[0m
�[36;1m echo "- ✅ **Graylint (ruff check)** passed"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif [[ "$darker_failed" == "1" || "$graylint_failed" == "1" ]]; then�[0m
�[36;1m echo "::error::One or more linters failed. See above for details."�[0m
🧰 Additional context used
📓 Path-based instructions (7)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
src/helpers/run-config-migration.pysrc/helpers/run-config-activation.pysrc/op_mode/memory.pysrc/helpers/vyos-boot-config-loader.pypython/vyos/utils/kernel.pysrc/op_mode/image_installer.pypython/vyos/system/image.pypython/vyos/utils/memory.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
data/templates/**/*.j2
📄 CodeRabbit inference engine (AGENTS.md)
Prefer storing Jinja2 templates as discrete files in
data/templates/rather than inline Python strings
Files:
data/templates/system/kdump_tools.conf.j2
**/*.j2
📄 CodeRabbit inference engine (AGENTS.md)
Jinja2 templates must pass linting validation
Files:
data/templates/system/kdump_tools.conf.j2
src/op_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Op-mode scripts must be located in
src/op_mode/
Files:
src/op_mode/memory.pysrc/op_mode/image_installer.py
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/utils/kernel.pypython/vyos/system/image.pypython/vyos/utils/memory.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (2)
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
src/op_mode/memory.pysrc/op_mode/image_installer.py
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
python/vyos/utils/memory.py
[warning] 24-24: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.finditer(regex, data, flags=re.MULTILINE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 574-574: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 588-588: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Relevant review context
- VyOS image updates via
add system imageprompt whether to use the current system configuration and SSH keys, so the installer path should be verified to carry the newsystem option kdumpconfiguration into the installed image. - VyOS documentation states that
savepersists configuration across reboot; kdump persistence should therefore be validated through the saved configuration used by image installation. - The repository documentation source was identified as the VyOS 1.x project, but the returned results did not expose concrete
image_installer.pyimplementation details or kdump-specific upgrade behavior.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible contains only generic command-line fact tests for
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS kdump consumers or shared configuration contracts were found. [::ansible/ansible::]
🔇 Additional comments (19)
src/op_mode/image_installer.py (2)
597-603: Skip crashkernel reservation when kdump is disabled.This still appends
crashkernel=wheneverkdump.memoryis present, even ifkdump.disabledis set.src/conf_mode/system_option.pyguards this path with the disabled state; image upgrades must use the same guard to avoid reserving capture-kernel memory for disabled kdump configurations.
50-50: LGTM!Also applies to: 529-533
data/templates/system/kdump_tools.conf.j2 (1)
1-7: LGTM!python/vyos/utils/kernel.py (2)
20-28: LGTM!
181-223: LGTM!python/vyos/system/image.py (1)
25-38: LGTM!Also applies to: 205-206, 267-268
src/helpers/run-config-activation.py (1)
33-33: LGTM!Also applies to: 58-58
src/helpers/run-config-migration.py (1)
22-22: LGTM!Also applies to: 83-83
src/helpers/vyos-boot-config-loader.py (1)
31-31: LGTM!Also applies to: 48-50
src/op_mode/memory.py (1)
21-30: LGTM!python/vyos/utils/memory.py (1)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating with
src/op_mode/memory.py.Both this module and
src/op_mode/memory.pyparse/proc/meminfo; a reviewer already flagged this duplication.src/conf_mode/system_option.py (7)
430-467: 🎯 Functional Correctness | 🏗️ Heavy liftStill using post-reservation
MemTotalfor the RAM-exceeds check.
get_memory_info()['MemTotal'](Line 434-436) is the same/proc/meminfovalue as the previously-flaggedpsutil.virtual_memory().total— it already excludes any currently activecrashkernelreservation. Increasingkdump memoryon a system that already has a smaller reservation loaded can be wrongly rejected here since the check compares against RAM with the old reservation still subtracted.
29-48: LGTM!Also applies to: 80-88, 184-188
212-289: LGTM!
292-317: LGTM!
556-575: 🎯 Functional CorrectnessConfirm the template doesn't need the expanded
memoryvalue.
kdump_memoryis expanded from'auto'toKDUMP_DEFAULT_MEMORY_AUTOonly in the local cmdline-building variable (Line 561-563); thekdumpdict passed intorender(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)at Line 575 still hasmemory == 'auto'unexpanded. Verifykdump_tools.conf.j2doesn't consumememory(or handles'auto'itself).
584-596: LGTM!
660-660: 🩺 Stability & AvailabilityNo issue:
get_kernel_boot_arg()accepts an omitted argument
python/vyos/utils/kernel.py:180definesget_kernel_boot_arg(argument=None), so the no-arg call insrc/conf_mode/system_option.py:660is valid.> Likely an incorrect or invalid review comment.smoketest/scripts/system/test_kernel_options.py (1)
171-181: 🎯 Functional CorrectnessCheck the CI kernel config for
CONFIG_DEBUG_INFOsmoketest/scripts/system/test_kernel_options.py:171-180reads/proc/config.gz, so this assertion depends on the booted kernel, not the repo. No repo-managed kernel fragment pinsCONFIG_DEBUG_INFO, so this can still vary across kernel builds.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smoketest/scripts/system/test_kernel_options.py`:
- Around line 178-180: Update the option-validation loop to wrap each iteration
in subTest with an option-specific message, while preserving the existing regex
check and assertion so failures identify the missing kernel option.
In `@src/conf_mode/system_option.py`:
- Around line 424-468: Update the kdump memory validation around get_memory_info
and memory_total so it compares the requested crashkernel size against
pre-reservation physical RAM, not MemTotal after an existing crashkernel
reservation. Reuse the appropriate unreserved-memory source or account for the
current reservation before applying the memory_bytes >= memory_total check,
while preserving the existing range parsing and ConfigError behavior.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 32734e28-15a3-4000-8034-9285dc372f28
📒 Files selected for processing (21)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/system/image.pypython/vyos/utils/boot.pypython/vyos/utils/kernel.pypython/vyos/utils/memory.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/helpers/run-config-activation.pysrc/helpers/run-config-migration.pysrc/helpers/vyos-boot-config-loader.pysrc/op_mode/generate_tech-support_archive.pysrc/op_mode/image_installer.pysrc/op_mode/kdump.pysrc/op_mode/memory.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (15)
- data/templates/system/kdump_tools.conf.j2
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- op-mode-definitions/show-system.xml.in
- src/op_mode/show_techsupport_report.py
- interface-definitions/system_option.xml.in
- debian/control
- src/op_mode/memory.py
- src/helpers/run-config-activation.py
- src/helpers/vyos-boot-config-loader.py
- src/op_mode/image_installer.py
- src/op_mode/kdump.py
- python/vyos/system/image.py
- python/vyos/defaults.py
- src/op_mode/generate_tech-support_archive.py
- smoketest/scripts/cli/test_system_option.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: build_iso
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Actions: Typos / typos: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-20T16%3A00%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-20T15%3A00%3A02Z&ske=2026-07-20T16%3A00%3A25Z&sks=b&skv=2018-11-09&sig=aINQf5GMFxlE6g09QAlnBOp%2FvoZdbLAlb6iolgFQHL4%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-20T16%3A00%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-20T15%3A00%3A02Z&ske=2026-07-20T16%3A00%3A25Z&sks=b&skv=2018-11-09&sig=aINQf5GMFxlE6g09QAlnBOp%2FvoZdbLAlb6iolgFQHL4%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.109.133, 185.199.110.133, 185.199.111.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.g...
GitHub Actions: Typos / 0_typos.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-20T16%3A00%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-20T15%3A00%3A02Z&ske=2026-07-20T16%3A00%3A25Z&sks=b&skv=2018-11-09&sig=aINQf5GMFxlE6g09QAlnBOp%2FvoZdbLAlb6iolgFQHL4%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-20T16%3A00%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-20T15%3A00%3A02Z&ske=2026-07-20T16%3A00%3A25Z&sks=b&skv=2018-11-09&sig=aINQf5GMFxlE6g09QAlnBOp%2FvoZdbLAlb6iolgFQHL4%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.109.133, 185.199.110.133, 185.199.111.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.g...
🧰 Additional context used
📓 Path-based instructions (4)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/utils/boot.pypython/vyos/utils/kernel.pypython/vyos/utils/memory.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/utils/boot.pysrc/helpers/run-config-migration.pysmoketest/scripts/system/test_kernel_options.pypython/vyos/utils/kernel.pypython/vyos/utils/memory.pysrc/conf_mode/system_option.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (1)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
smoketest/scripts/system/test_kernel_options.py
[warning] 178-178: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 178-178: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
python/vyos/utils/memory.py
[warning] 24-24: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.finditer(regex, data, flags=re.MULTILINE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 574-574: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 588-588: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Additional relevant context
- VyOS image upgrades use
add system image; the installer prompts whether to carry over the current system configuration and SSH keys. This directly supports reviewing whethersystem.option.kdumpis preserved byimage_installer.py. - VyOS configuration changes require
committo apply andsaveto persist them across reboot. Therefore, kdump image-update testing should explicitly verify behavior both with saved configuration and when declining configuration migration. - No concrete
image_installer.pyimplementation details or kdump-specific upgrade behavior were returned by the repository documentation query; this remains an implementation-level review item.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible has only generic cmdline fact tests covering
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS-specific kdump consumers or shared configuration contracts were identified. [::ansible/ansible::]
🔇 Additional comments (10)
python/vyos/utils/kernel.py (1)
214-220: Duplicate: inspect all repeated boot-argument values.The loop returns the first matching token, so
console=tty0 console=ttyS0,115200still yieldstty0and misses the serial console. Continue scanning allconsole=values before parsing, or preserve raw-command-line matching.python/vyos/utils/boot.py (1)
18-18: LGTM!src/helpers/run-config-migration.py (1)
22-22: LGTM!Also applies to: 83-83
python/vyos/utils/memory.py (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify total-system-memory reads instead of maintaining multiple implementations. The codebase now has at least three ways to obtain total system RAM: the new
/proc/meminforegex parser here,psutil.virtual_memory()used for the hugepages check, and (per a prior reviewer comment)src/op_mode/memory.py's own/proc/meminfohandling.
python/vyos/utils/memory.py#L19-28: keep this as the single canonical helper;src/op_mode/memory.py's existing meminfo parsing should be refactored to call it instead of duplicating the parsing logic (per prior reviewer note: "src/op_mode/memory.pyalso works with /proc/meminfo - combine it?").src/conf_mode/system_option.py#L390-436: switch the hugepages check (currentlypsutil.virtual_memory()) to useget_memory_info()as well, so total-RAM is sourced consistently within this file and across the codebase.
19-28: LGTM!src/conf_mode/system_option.py (5)
29-48: LGTM!
80-88: LGTM!Also applies to: 184-188
212-290: LGTM!
292-317: LGTM!
556-596: LGTM!Also applies to: 660-660
501a2f4 to
925b91f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@interface-definitions/system_option.xml.in`:
- Around line 432-435: Update the crash-kernel memory regex constraint to reject
zero-sized values, including standalone sizes and every size in tiered range
entries, while preserving valid nonzero formats and the existing auto option. If
zero is intended to disable crashkernel, represent that through an explicit
supported disable state rather than accepting 0M or 0G.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 82c3bf37-e309-46be-82d2-06dea3d91dbd
📒 Files selected for processing (21)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/system/image.pypython/vyos/utils/boot.pypython/vyos/utils/kernel.pypython/vyos/utils/memory.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/helpers/run-config-activation.pysrc/helpers/run-config-migration.pysrc/helpers/vyos-boot-config-loader.pysrc/op_mode/generate_tech-support_archive.pysrc/op_mode/image_installer.pysrc/op_mode/kdump.pysrc/op_mode/memory.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (14)
- data/templates/system/kdump_tools.conf.j2
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- debian/control
- src/op_mode/show_techsupport_report.py
- op-mode-definitions/show-system.xml.in
- src/helpers/run-config-migration.py
- python/vyos/defaults.py
- src/helpers/run-config-activation.py
- src/op_mode/memory.py
- src/helpers/vyos-boot-config-loader.py
- src/op_mode/generate_tech-support_archive.py
- src/op_mode/image_installer.py
- python/vyos/system/image.py
- python/vyos/utils/kernel.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (8)
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
fetch-depth: 0
fetch-tags: true
ref: T8868-rolling
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
##[endgroup]
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
fetch-depth: 0
fetch-tags: true
ref: T8868-rolling
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
##[endgroup]
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: Typos / typos: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T11%3A20%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T10%3A19%3A40Z&ske=2026-07-21T11%3A20%3A25Z&sks=b&skv=2018-11-09&sig=UtH6i16SryaO8nNq%2FWshZsxCSFeQNVupQ7Fl8qrWh5k%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T11%3A20%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T10%3A19%3A40Z&ske=2026-07-21T11%3A20%3A25Z&sks=b&skv=2018-11-09&sig=UtH6i16SryaO8nNq%2FWshZsxCSFeQNVupQ7Fl8qrWh5k%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.110.133, 185.199.111.133, 185.199.108.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.g...
GitHub Actions: Typos / 0_typos.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T11%3A20%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T10%3A19%3A40Z&ske=2026-07-21T11%3A20%3A25Z&sks=b&skv=2018-11-09&sig=UtH6i16SryaO8nNq%2FWshZsxCSFeQNVupQ7Fl8qrWh5k%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T11%3A20%3A25Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T10%3A19%3A40Z&ske=2026-07-21T11%3A20%3A25Z&sks=b&skv=2018-11-09&sig=UtH6i16SryaO8nNq%2FWshZsxCSFeQNVupQ7Fl8qrWh5k%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.110.133, 185.199.111.133, 185.199.108.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.g...
GitHub Actions: VyOS ISO Integration Test / set_config: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / build_iso: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
path: build/vyos-1x
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ref: rolling
persist-credentials: false
ssh-strict: true
ssh-user: git
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
GITHUB_***REDACTED***
BUILD_BY: autobuild@vyos.net
DEBIAN_MIRROR: http://deb.debian.org/debian/
DEBIAN_SECURITY_MIRROR: http://deb.debian.org/debian-security
##[endgroup]
##[command]/usr/bin/docker exec ***REDACTED*** sh -c "cat /etc/*release | grep ^ID"
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
path: build/vyos-1x
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ref: rolling
persist-credentials: false
ssh-strict: true
ssh-user: git
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
GITHUB_***REDACTED***
BUILD_BY: autobuild@vyos.net
DEBIAN_MIRROR: http://deb.debian.org/debian/
DEBIAN_SECURITY_MIRROR: http://deb.debian.org/debian-security
##[endgroup]
##[command]/usr/bin/docker exec ***REDACTED*** sh -c "cat /etc/*release | grep ^ID"
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
🧰 Additional context used
📓 Path-based instructions (5)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/utils/boot.pypython/vyos/utils/memory.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/utils/boot.pysmoketest/scripts/cli/test_system_option.pysrc/op_mode/kdump.pypython/vyos/utils/memory.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.py
src/op_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Op-mode scripts must be located in
src/op_mode/
Files:
src/op_mode/kdump.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (3)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.py
📚 Learning: 2026-05-26T06:04:29.163Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:118-120
Timestamp: 2026-05-26T06:04:29.163Z
Learning: In VyOS smoketest scripts under `smoketest/scripts/cli/`, it is intentional to call `self.cli_delete(['vrf'])` in both `setUpClass` and `tearDown` to wipe the entire VRF subtree and ensure a clean slate. During code review, do not recommend narrowing the delete to specific VRF identifiers or name subsets (e.g., `['vrf', 'name', 'mgmt']`)—the broad teardown behavior is the established project-wide pattern for these tests.
Applied to files:
smoketest/scripts/cli/test_system_option.py
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
smoketest/scripts/cli/test_system_option.pysrc/op_mode/kdump.py
🪛 ast-grep (0.44.1)
python/vyos/utils/memory.py
[warning] 24-24: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.finditer(regex, data, flags=re.MULTILINE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
smoketest/scripts/system/test_kernel_options.py
[warning] 179-179: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 179-179: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 578-578: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 592-592: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Relevant review context
- VyOS upgrades create a configuration copy per system image;
/configis copied to the new image, but other filesystem files are not automatically preserved. This makes explicit migration ofsystem option kdumpimportant duringadd system image. add system imageprompts whether to reuse the current system configuration and SSH keys.- VyOS distinguishes active, working, and saved configuration: changes require
committo activate andsaveto persist in/config/config.boot. Upgrade tests should therefore cover both saved kdump configuration and declining configuration migration. - The repository documentation query did not expose concrete
image_installer.pyimplementation details or kdump-specific upgrade behavior, so preservation ofsystem.option.kdumpremains an implementation-level review item.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible has only generic cmdline fact tests covering
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS-specific kdump consumers or shared configuration contracts were identified. [::ansible/ansible::]
🔇 Additional comments (8)
python/vyos/utils/boot.py (1)
18-18: 📐 Maintainability & Code QualityReuse the canonical kernel command-line constant.
python/vyos/utils/kernel.pyalready definesCMDLINE_PATHfor/proc/cmdline. Defining it again here duplicates the contract and allows the two values to diverge; import the existing constant or centralize command-line access invyos.utils.kernelinstead. This repeats the previously raised command-line parsing/centralization concern.smoketest/scripts/cli/test_system_option.py (2)
293-299: Use only unambiguously malformed negative cases.
1M-128M:64Mfollows the accepted range grammar from Line 263, and256G-512G:64Mcan be valid on a 256–512 GiB host. Either can makeassertRaisesfail.Proposed fix
- '1M-128M:64M', # Missing high bound, invalid - '256G-512G:64M', # Invalid high bound exceeds typical limits + '1M-128M64M', # Missing range/size separator + '256G512G:64M', # Missing range separator
20-288: LGTM!Also applies to: 306-345
src/op_mode/kdump.py (1)
1-255: LGTM!interface-definitions/system_option.xml.in (1)
416-421: 🎯 Functional Correctness
disabledis synthesized in conf-mode, so this schema change is not missing a commit-time contract.src/conf_mode/system_option.py:307-310setsoptions['kdump']['disabled'] = not conf.exists(base + ['kdump']), andinterface-definitions/system_option.xml.in:416-454only needsmemoryanddump-pathfor the enabled case.> Likely an incorrect or invalid review comment.python/vyos/utils/memory.py (1)
15-28: LGTM!smoketest/scripts/system/test_kernel_options.py (1)
171-181: LGTM!src/conf_mode/system_option.py (1)
29-49: LGTM!Also applies to: 81-89, 185-189, 213-311, 425-472, 560-600, 664-664
925b91f to
636291f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/conf_mode/system_option.py`:
- Around line 579-583: Update the dump directory creation in the kdump
configuration flow to enforce root-only permissions, such as mode 0700, when
creating dump_path. Ensure existing directories are also tightened to the same
restrictive mode rather than relying only on os.makedirs defaults, while
preserving the current conditional creation behavior.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6e744650-1101-4ed7-ba04-0bb6cbf4f096
📒 Files selected for processing (21)
data/templates/system/kdump_tools.conf.j2debian/controlinterface-definitions/system_option.xml.inop-mode-definitions/show-system.xml.inpython/vyos/defaults.pypython/vyos/system/image.pypython/vyos/utils/boot.pypython/vyos/utils/kernel.pypython/vyos/utils/memory.pysmoketest/scripts/cli/test_system_option.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.pysrc/etc/systemd/system/kdump-tools.service.d/override.confsrc/helpers/run-config-activation.pysrc/helpers/run-config-migration.pysrc/helpers/vyos-boot-config-loader.pysrc/op_mode/generate_tech-support_archive.pysrc/op_mode/image_installer.pysrc/op_mode/kdump.pysrc/op_mode/memory.pysrc/op_mode/show_techsupport_report.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
🚧 Files skipped from review as they are similar to previous changes (17)
- src/etc/systemd/system/kdump-tools.service.d/override.conf
- op-mode-definitions/show-system.xml.in
- python/vyos/utils/boot.py
- debian/control
- python/vyos/defaults.py
- data/templates/system/kdump_tools.conf.j2
- src/op_mode/show_techsupport_report.py
- src/helpers/vyos-boot-config-loader.py
- src/op_mode/memory.py
- interface-definitions/system_option.xml.in
- src/helpers/run-config-activation.py
- src/helpers/run-config-migration.py
- python/vyos/utils/kernel.py
- src/op_mode/image_installer.py
- smoketest/scripts/cli/test_system_option.py
- src/op_mode/generate_tech-support_archive.py
- src/op_mode/kdump.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (8)
GitHub Actions: Python Lint (Darker + Ruff) / 0_darker-ruff-lint _ darker-ruff-lint.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
fetch-depth: 0
fetch-tags: true
ref: T8868-rolling
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
##[endgroup]
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: Python Lint (Darker + Ruff) / darker-ruff-lint _ darker-ruff-lint: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
fetch-depth: 0
fetch-tags: true
ref: T8868-rolling
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
##[endgroup]
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: Typos / typos: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T13%3A10%3A22Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T12%3A10%3A22Z&ske=2026-07-21T13%3A10%3A22Z&sks=b&skv=2018-11-09&sig=56vzHMhZwniyxRvLvc47hQKwxn8P2yqYNRwb3Sgpl7k%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T13%3A10%3A22Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T12%3A10%3A22Z&ske=2026-07-21T13%3A10%3A22Z&sks=b&skv=2018-11-09&sig=56vzHMhZwniyxRvLvc47hQKwxn8P2yqYNRwb3Sgpl7k%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.111.133, 185.199.108.133, 185.199.109.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.githu...
GitHub Actions: Typos / 0_typos.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run $GITHUB_ACTION_PATH/action/entrypoint.sh
�[36;1m$GITHUB_ACTION_PATH/action/entrypoint.sh�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
INSTALL_DIR: /home/runner/work/_temp
INPUT_FILES:
INPUT_EXTEND_IDENTIFIERS:
INPUT_EXTEND_WORDS:
INPUT_ISOLATED: false
INPUT_WRITE_CHANGES: false
INPUT_CONFIG: .github-central/_typos.toml
##[endgroup]
Downloading 'typos' v1.47.2
---- https://github.com/crate-ci/typos/releases/download/v1.47.2/typos-v1.47.2-x86_64-unknown-linux-musl.tar.gz
Resolving github.com (github.com)... 140.82.114.4
Connecting to github.com (github.com)|140.82.114.4|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T13%3A10%3A22Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T12%3A10%3A22Z&ske=2026-07-21T13%3A10%3A22Z&sks=b&skv=2018-11-09&sig=56vzHMhZwniyxRvLvc47hQKwxn8P2yqYNRwb3Sgpl7k%3D&jwt=*** [following]
---- https://release-assets.githubusercontent.com/github-production-release-asset/181782286/5b1569da-eab0-4463-a428-f5f4422366a2?sp=r&sv=2018-11-09&sr=b&spr=https&se=2026-07-21T13%3A10%3A22Z&rscd=attachment%3B+filename%3Dtypos-v1.47.2-x86_64-unknown-linux-musl.tar.gz&rsct=application%2Foctet-stream&skoid=96c2d410-5711-43a1-aedd-ab1947aa7ab0&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skt=2026-07-21T12%3A10%3A22Z&ske=2026-07-21T13%3A10%3A22Z&sks=b&skv=2018-11-09&sig=56vzHMhZwniyxRvLvc47hQKwxn8P2yqYNRwb3Sgpl7k%3D&jwt=***
Resolving release-assets.githubusercontent.com (release-assets.githubusercontent.com)... 185.199.111.133, 185.199.108.133, 185.199.109.133, ...
Connecting to release-assets.githubusercontent.com (release-assets.githu...
GitHub Actions: VyOS ISO Integration Test / build_iso: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
path: build/vyos-1x
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ref: rolling
persist-credentials: false
ssh-strict: true
ssh-user: git
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
GITHUB_***REDACTED***
BUILD_BY: autobuild@vyos.net
DEBIAN_MIRROR: http://deb.debian.org/debian/
DEBIAN_SECURITY_MIRROR: http://deb.debian.org/debian-security
##[endgroup]
##[command]/usr/bin/docker exec ***REDACTED*** sh -c "cat /etc/*release | grep ^ID"
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: VyOS ISO Integration Test / set_config: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
GitHub Actions: VyOS ISO Integration Test / 8_build_iso.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run actions/checkout@v6
with:
path: build/vyos-1x
repository: alexandr-san4ez/vyos-1x
***REDACTED***
ref: rolling
persist-credentials: false
ssh-strict: true
ssh-user: git
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
allow-unsafe-pr-checkout: false
env:
GITHUB_***REDACTED***
BUILD_BY: autobuild@vyos.net
DEBIAN_MIRROR: http://deb.debian.org/debian/
DEBIAN_SECURITY_MIRROR: http://deb.debian.org/debian-security
##[endgroup]
##[command]/usr/bin/docker exec ***REDACTED*** sh -c "cat /etc/*release | grep ^ID"
##[error]Refusing to check out fork pull request code from a 'pull_request_target' workflow. This workflow runs with the base repository's GITHUB_TOKEN, secrets, default-branch cache scope, and runner access. Fetching and executing a fork's code in that trusted context commonly leads to "pwn request" vulnerabilities. To opt in, review the risks at https://gh.io/securely-using-pull_request_target and set 'allow-unsafe-pr-checkout: true' on the actions/checkout step.
GitHub Actions: VyOS ISO Integration Test / 9_set_config.txt: kdump: T8868: Implement kernel crash dump support
Conclusion: failure
##[group]Run if [[ "pull_request_target" == "pull_request_target" ]]; then
�[36;1mif [[ "pull_request_target" == "pull_request_target" ]]; then�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1melse�[0m
�[36;1m BRANCH="rolling"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mCONFIG=$(jq ".branches[\"${BRANCH}\"]" .github/config/smoketest-branches.json)�[0m
�[36;1m�[0m
�[36;1mif [ "$CONFIG" = "null" ] || [ -z "$CONFIG" ]; then�[0m
�[36;1m echo "::error::No smoketest configuration found for branch '${BRANCH}' in .github/config/smoketest-branches.json"�[0m
🧰 Additional context used
📓 Path-based instructions (4)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/system/image.pypython/vyos/utils/memory.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/system/image.pypython/vyos/utils/memory.pysmoketest/scripts/system/test_kernel_options.pysrc/conf_mode/system_option.py
smoketest/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Runtime smoketests must be located under
smoketest/and use nose2 framework
Files:
smoketest/scripts/system/test_kernel_options.py
src/conf_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Conf-mode entry-point scripts must be named after CLI components and located in
src/conf_mode/
Files:
src/conf_mode/system_option.py
🧠 Learnings (1)
📚 Learning: 2026-05-26T06:03:59.703Z
Learnt from: c-po
Repo: vyos/vyos-1x PR: 5109
File: smoketest/scripts/cli/test_service_https.py:206-207
Timestamp: 2026-05-26T06:03:59.703Z
Learning: In VyOS smoketests that verify processes running inside a VRF using iproute2, remember that `ip vrf pids <vrf>` outputs one entry per line as `<pid> <process_name>` (e.g., `300431 nginx`), not PIDs alone. Therefore, assertions should check for the presence of the expected process name in the command output (e.g., `assertIn(PROCESS_NAME, cmd(f'ip vrf pids {vrf}'))`) rather than trying to match PID-only output.
Applied to files:
smoketest/scripts/system/test_kernel_options.py
🪛 ast-grep (0.44.1)
python/vyos/utils/memory.py
[warning] 24-24: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.finditer(regex, data, flags=re.MULTILINE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
smoketest/scripts/system/test_kernel_options.py
[warning] 179-179: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 179-179: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.findall(f'{option}=y', self._config_data)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
src/conf_mode/system_option.py
[error] 584-584: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 598-598: Avoid HTML built in strings
Context: render(KDUMP_CONFIG_FILE, 'system/kdump_tools.conf.j2', kdump)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🔍 Remote MCP Context7
Additional review context
- VyOS configuration migration is tested through fixture files under
smoketests/configs/, with expected CLI output insmoketests/configs/assert/; listed assertions must match migrated output. This provides an appropriate place to add a regression test forsystem option kdumppreservation during image installation. - Repository documentation confirms migration scripts are located under
src/migration-scripts/, while image and operational-mode logic resides undersrc/op_mode/. - No concrete repository documentation was found describing existing
image_installer.pyhandling ofsystem.option.kdumpor kdump-specific upgrade behavior. The reviewer’s concern therefore remains unresolved and should be verified with an implementation-level test covering image updates/configuration migration.
🔀 Multi-repo context ansible/ansible
Linked repositories findings
ansible/ansible
- Ansible has only generic cmdline fact tests covering
crashkernel=autoandcrashkernel=128Mintest/units/module_utils/facts/system/test_cmdline.py:13-28; no VyOS-specific kdump consumers or shared configuration contracts were identified. [::ansible/ansible::]
🔇 Additional comments (10)
python/vyos/system/image.py (1)
25-38: LGTM!Also applies to: 205-206, 267-268
python/vyos/utils/memory.py (1)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate meminfo-parsing logic across two files.
This mirrors the existing concern already raised: c-po asked whether
src/op_mode/memory.py's own/proc/meminfohandling should be combined with this new helper. Consider havingsrc/op_mode/memory.pycallget_memory_info()instead of maintaining its own parsing logic.smoketest/scripts/system/test_kernel_options.py (1)
171-182: LGTM!src/conf_mode/system_option.py (7)
29-49: LGTM!Also applies to: 81-89, 185-189
213-291: LGTM!
293-318: LGTM!
425-478: LGTM! Pre-reservation RAM comparison now correctly accounts forget_crash_kernel_size(), and theautoexemption matches confirmed intended behavior.
566-578: LGTM!Also applies to: 584-606
616-669: LGTM!Also applies to: 671-708
670-670: 🎯 Functional CorrectnessNo issue:
get_kernel_boot_arg()returns the full kernel command line when called without an argument, socurrent_cmdlinestill contains the complete/proc/cmdlinecontents.> Likely an incorrect or invalid review comment.
636291f to
b7afb7e
Compare
c-po
left a comment
c-po
left a comment
There was a problem hiding this comment.
The addition/refactoring with vyos.utils.kernel.get_kernel_boot_arg() must be a dedicated comimt as this is a self-contained changed in this PR.
b7afb7e to
8e52239
Compare
|
@c-po I have updated the PR to take into account your objections. |
8e52239 to
affd304
Compare
|
Rebased to pull in CI changes necessary for integration tests. |
dmbaturin
left a comment
dmbaturin
left a comment
There was a problem hiding this comment.
It's a customer request and I believe it's useful, and I see no obvious implementation issues now.
Christian's change request is already implemented
Add new CLI subtree 'system option kdump' with memory reservation and local dump path configuration. Also add 'show system kdump' and 'show system kdump dumps' operation mode commands to get status of the configuration and list of dumps. Extend 'show tech-support report' and 'generate tech-support archive' with kdump service status, active config files and the recently kernel dump file.
- Replaced direct file reads from `/proc/cmdline` with `get_kernel_boot_arg()` for fetching kernel command-line arguments in multiple modules. - Improved memory information retrieval by utilizing `get_memory_info()`.
Co-authored-by: Daniil Baturin <daniil@baturin.org>
30e7d37 to
7b96000
Compare
|
CI integration ❌ failed! Details
|
vyos-bot
Bot
added
mirror-initiated
Change summary
Add new CLI subtree
system option kdumpwith memory reservation and local dump path configuration. Also addshow system kdumpoperation mode commands to get status of the configuration and list of dumps.Extend
show tech-support reportandgenerate tech-support archivewith kdump service status, active config files and the recently kernel dump file.Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Manual test
kdump-configtool:show tech-supporttool (only 'kernel-dump' section):generate tech-supportsaves the last kernel dump:Smoketest
Checklist: