snapm: add snapshot set mount management framework#444
snapm: add snapshot set mount management framework#444
Conversation
WalkthroughAdds systemd-tmpfiles support and wiring: new tmpfiles.d file, RPM spec tmpfiles handling, CI/container/VM install steps updated to install and apply the tmpfiles config, and module-level runtime path constants added to the manager module. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Installer as Installer (CI/VM/Manual)
participant FS as Filesystem
participant Tmpfiles as systemd-tmpfiles
participant Systemd as systemd
Installer->>FS: Copy `*.service`/`*.timer` -> /usr/lib/systemd/system
Installer->>FS: Copy `systemd/tmpfiles.d/snapm.conf` -> /usr/lib/tmpfiles.d/
Installer->>Tmpfiles: systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf
Tmpfiles->>FS: Create `/run/snapm` and `/run/snapm/mounts` (0700 root:root)
Note right of FS: Apply SELinux context to /run/snapm
Installer->>Systemd: systemctl daemon-reload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes
Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
from
d0eb6b6 to
b341ef5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (7)
container_tests/snapm.conf (1)
1-3: Avoid duplicate tmpfiles config; source the canonical one instead.This file duplicates systemd/tmpfiles.d/snapm.conf. Remove it to prevent drift; have Containerfile/Makefile copy from systemd/tmpfiles.d/.
-# snapm runtime directories -d /run/snapm 0700 root root - - -d /run/snapm/mounts 0700 root root - -systemd/tmpfiles.d/snapm.conf (1)
1-3: Permissions look good; optionally ensure SELinux contexts.If targeting SELinux systems, consider restoring default labels recursively.
# snapm runtime directories d /run/snapm 0700 root root - - d /run/snapm/mounts 0700 root root - - +Z /run/snapm - - - -snapm.spec (2)
101-103: Use %{_tmpfilesdir} macro instead of hard-coded path.Improves portability across Fedora/CS9 derivatives.
-mkdir -p ${RPM_BUILD_ROOT}/%{_prefix}/lib/tmpfiles.d -%{__install} -p -m 644 systemd/tmpfiles.d/snapm.conf ${RPM_BUILD_ROOT}/%{_prefix}/lib/tmpfiles.d +mkdir -p ${RPM_BUILD_ROOT}/%{_tmpfilesdir} +%{__install} -p -m 644 systemd/tmpfiles.d/snapm.conf ${RPM_BUILD_ROOT}/%{_tmpfilesdir}
116-121: Also switch %files entry to %{_tmpfilesdir}.Keep macros consistent between install and files lists.
%attr(644, -, -) %{_unitdir}/snapm-gc@.service %attr(644, -, -) %{_unitdir}/snapm-gc@.timer -%attr(644, -, -) %{_prefix}/lib/tmpfiles.d/snapm.conf +%attr(644, -, -) %{_tmpfilesdir}/snapm.conf.github/workflows/snapm.yml (1)
51-54: Harden CI step: ensure dirs exist and create runtime dirs in CI.GitHub runners don’t process tmpfiles at boot. Create dirs explicitly (or call systemd-tmpfiles) to avoid test flakiness.
- run: | - sudo cp systemd/*.service /usr/lib/systemd/system - sudo cp systemd/*.timer /usr/lib/systemd/system - sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d + run: | + set -euo pipefail + sudo install -d -m 0755 /usr/lib/systemd/system /usr/lib/tmpfiles.d + sudo cp systemd/*.service /usr/lib/systemd/system + sudo cp systemd/*.timer /usr/lib/systemd/system + sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d + # Ensure /run paths exist in CI (systemd-tmpfiles may not run automatically) + if command -v systemd-tmpfiles >/dev/null 2>&1; then + sudo systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf + else + sudo install -d -m 0700 /run/snapm /run/snapm/mounts + ficontainer_tests/Containerfile (1)
10-10: Nit: clarify destination is a directoryAdd a trailing slash to make the intent explicit. Functionally equivalent today, but clearer.
-COPY snapm.conf /usr/lib/tmpfiles.d +COPY snapm.conf /usr/lib/tmpfiles.d/container_tests/Makefile (1)
14-14: Add clean-up for the copied tmpfiles config to avoid stale build context
cleanremoves .service/.timer but leavessnapm.confbehind. Include it to keep the workspace tidy.clean: podman rm -f $(CONTAINER) 2>/dev/null || true rm -f $(CURDIR)/*.service rm -f $(CURDIR)/*.timer + rm -f $(CURDIR)/snapm.conf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.github/workflows/snapm.yml(1 hunks)CONTRIBUTING.md(1 hunks)container_tests/Containerfile(1 hunks)container_tests/Makefile(1 hunks)container_tests/snapm.conf(1 hunks)snapm.spec(2 hunks)snapm/manager/_manager.py(1 hunks)systemd/tmpfiles.d/snapm.conf(1 hunks)virt_tests/testvm.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
systemd/tmpfiles.d/snapm.conf.github/workflows/snapm.ymlvirt_tests/testvm.pycontainer_tests/Containerfilesnapm.specCONTRIBUTING.md
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: The upstream snapm.spec file in the snapm project only aims to work on centos-stream9 through Fedora rawhide. Downstreams maintain their own copies and adapt as needed.
Applied to files:
snapm.spec
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: python-basic-tests
🔇 Additional comments (2)
snapm/manager/_manager.py (1)
88-93: No hard-coded runtime paths left in Python codeThe search for literal “/run/snapm” references yielded only configuration and test files, not Python code:
- systemd/tmpfiles.d/snapm.conf (lines 2–3)
- container_tests/snapm.conf (lines 2–3)
The Python module
snapm/manager/_manager.pynow consistently uses the_SNAPM_RUNTIME_DIRand_SNAPM_MOUNTS_DIRconstants. Configuration files and container tests legitimately reference the paths directly and are out of scope for this patch.All set to approve!
snapm.spec (1)
95-100: Explicit mkdir + install for units: LGTM.Matches the preferred style from prior learnings.
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
from
b341ef5 to
9c932fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/snapm.yml (2)
51-54: Make the copy step robust (avoid glob failures; ensure dirs exist; set explicit modes).If no files match the globs, cp will fail and abort the job. Also, being explicit about directory creation and file modes improves reproducibility. Suggest switching to install with mkdir-style dir creation (aligns with the prior preference for explicit mkdir + install).
- - name: Copy systemd units to /usr/lib/systemd/system - run: | - sudo cp systemd/*.service /usr/lib/systemd/system - sudo cp systemd/*.timer /usr/lib/systemd/system - sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d + - name: Copy systemd units and tmpfiles config + run: | + set -euo pipefail + sudo install -d -m 0755 /usr/lib/systemd/system /usr/lib/tmpfiles.d + # Copy .service/.timer if present; no-op if none + for f in systemd/*.service systemd/*.timer; do + [ -e "$f" ] && sudo install -m 0644 "$f" /usr/lib/systemd/system + done + sudo install -m 0644 systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/snapm.conf
54-54: Apply tmpfiles rules after installing snapm.conf.To ensure /run/snapm and /run/snapm/mounts exist during tests, run systemd-tmpfiles --create for this config (daemon-reload doesn’t create runtime dirs).
Add a dedicated step after “Reload systemd”:
- name: Apply tmpfiles config run: sudo systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/snapm.yml(1 hunks)CONTRIBUTING.md(1 hunks)container_tests/Containerfile(1 hunks)container_tests/Makefile(1 hunks)container_tests/snapm.conf(1 hunks)snapm.spec(2 hunks)systemd/tmpfiles.d/snapm.conf(1 hunks)virt_tests/testvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- container_tests/snapm.conf
- container_tests/Makefile
- container_tests/Containerfile
- systemd/tmpfiles.d/snapm.conf
- virt_tests/testvm.py
- CONTRIBUTING.md
- snapm.spec
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
.github/workflows/snapm.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
.github/workflows/snapm.yml (1)
52-53: Confirm narrowing to only .service/.timer is intentional.If .path/.socket/.mount/.target units are introduced (mount framework suggests possible .mount units), they won’t be copied.
Do we expect any non-service/timer units now or imminently? If yes, extend the patterns or enumerate them explicitly.
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
from
9c932fe to
e277cd6
Compare
|
@coderabbitai re: "If no files match the globs, cp will fail and abort the job.". That's what we want. If those paths don't exist, something is irretrievably borked in the test run, and we should fail right away. |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru: Thank you for that clarification! You're absolutely right - failing fast when expected files don't exist is a sound design principle. If the systemd units or tmpfiles configuration are missing, it indicates a fundamental issue with the build/test setup that should be caught immediately rather than silently continuing with an incomplete installation. This fail-fast approach makes debugging much easier and prevents potentially confusing downstream failures that could occur from running with missing components. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
virt_tests/testvm.py (1)
533-534: Force directory semantics with trailing slashes on cp destinationsWithout a trailing “/”, copying snapm.conf to /usr/lib/tmpfiles.d could create a regular file if the directory were missing; with multiple sources the systemd units copy already requires a dir, but adding “/” makes intent explicit and fail-fast. This mirrors the project’s fail-fast philosophy.
- "cd /tmp/snapm && cp systemd/*.service systemd/*.timer /usr/lib/systemd/system", - "cd /tmp/snapm && cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d", + "cd /tmp/snapm && cp systemd/*.service systemd/*.timer /usr/lib/systemd/system/", + "cd /tmp/snapm && cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/",
🧹 Nitpick comments (2)
virt_tests/testvm.py (2)
532-532: Preserve attributes when installing /etc/snapmUse cp -a so mode/mtime are preserved (useful if configs ship non-default perms). Low-risk improvement.
- "cd /tmp/snapm && cp -r etc/snapm /etc", + "cd /tmp/snapm && cp -a etc/snapm /etc",
536-536: Optional: assert runtime dirs (and perms) right after tmpfilesQuick check helps catch tmpfiles syntax/SELinux issues early in CI.
"systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf", + "test -d /run/snapm/mounts || (echo '/run/snapm/mounts missing' >&2; exit 1)", + "stat -c '%n %a %U:%G' /run/snapm /run/snapm/mounts || true",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/snapm.yml(1 hunks)CONTRIBUTING.md(1 hunks)container_tests/Containerfile(1 hunks)container_tests/Makefile(2 hunks)snapm.spec(2 hunks)systemd/tmpfiles.d/snapm.conf(1 hunks)virt_tests/testvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- container_tests/Makefile
- .github/workflows/snapm.yml
- container_tests/Containerfile
- CONTRIBUTING.md
- snapm.spec
- systemd/tmpfiles.d/snapm.conf
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-27T21:01:32.273Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-26T19:07:42.290Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-25T19:20:21.720Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-25T19:20:21.720Z
Learning: In the snapm virt_tests workflow, optimizing Git checkouts by using virtiofs mapping is complex because it requires checkouts for both `snapm` and `boom` repositories. Git checkouts aren't the dominant time cost, so the current approach of cloning within VMs is more practical than adding workflow complexity and local user setup requirements.
Applied to files:
virt_tests/testvm.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
virt_tests/testvm.py (1)
535-536: Good: apply tmpfiles now so /run/snapm[/mounts] exists pre-reloadCalling systemd-tmpfiles immediately after installing the config aligns with the earlier guidance and avoids relying on reboot/service start to materialize runtime dirs.
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
4 times, most recently
from
0d32072 to
5e319a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snapm.spec (2)
101-103: Create the same directory you install into (use the macro consistently)Avoid assuming %{_tmpfilesdir} equals %{_prefix}/lib/tmpfiles.d on all targets; use the macro for mkdir too.
-mkdir -p ${RPM_BUILD_ROOT}/%{_prefix}/lib/tmpfiles.d +mkdir -p ${RPM_BUILD_ROOT}/%{_tmpfilesdir} %{__install} -p -m 644 systemd/tmpfiles.d/snapm.conf ${RPM_BUILD_ROOT}/%{_tmpfilesdir}/
120-120: Optionally apply tmpfiles config at install/remove timeFor immediate availability of /run/snapm without reboot or service start, add tmpfiles scriptlets. This follows Fedora/RHEL packaging practice and is safe for runtime dirs under /run.
Add outside this hunk:
BuildRequires: systemd-rpm-macros %post %tmpfiles_create %{_tmpfilesdir}/snapm.conf %postun %tmpfiles_delete %{_tmpfilesdir}/snapm.conf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.github/workflows/snapm.yml(1 hunks)CONTRIBUTING.md(1 hunks)container_tests/Containerfile(1 hunks)container_tests/Makefile(2 hunks)snapm.spec(2 hunks)snapm/command.py(1 hunks)snapm/manager/_manager.py(1 hunks)systemd/tmpfiles.d/snapm.conf(1 hunks)virt_tests/testvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- container_tests/Makefile
- snapm/manager/_manager.py
- CONTRIBUTING.md
- systemd/tmpfiles.d/snapm.conf
- container_tests/Containerfile
- .github/workflows/snapm.yml
- virt_tests/testvm.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
📚 Learning: 2025-08-25T00:18:37.114Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-25T00:18:37.114Z
Learning: In snapm/command.py, the use of print() vs _*_log() functions is intentional design. print() is used specifically for user output and structured reports (JSON, tabular) that are not suitable to pass through the log facility, while _*_log() functions are used for actual logging purposes.
Applied to files:
snapm/command.py
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
snapm.spec
📚 Learning: 2025-08-27T21:01:32.273Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
snapm.spec
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: The upstream snapm.spec file in the snapm project only aims to work on centos-stream9 through Fedora rawhide. Downstreams maintain their own copies and adapt as needed.
Applied to files:
snapm.spec
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (39)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: python-basic-tests
🔇 Additional comments (1)
snapm/command.py (1)
1221-1226: Fixed _log_info argument order — LGTMThe placeholders now match the provided values (sources, then snapset name).
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
from
5e319a0 to
3c4f532
Compare
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
CONTRIBUTING.md (1)
96-103: Nit: use trailing slashes in example commands for directory clarity.Good addition of the systemd-tmpfiles invocation; add trailing slashes to avoid ambiguity and match conventions used elsewhere.
- # cp -r etc/snapm /etc - # cp systemd/*.timer systemd/*.service /usr/lib/systemd/system - # cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d + # cp -r etc/snapm/ /etc/ + # cp systemd/*.timer systemd/*.service /usr/lib/systemd/system/ + # cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/ # systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.confvirt_tests/testvm.py (1)
532-536: Nit: add trailing slashes and assert dirs exist after creation (keeps fail-fast).
- Trailing slashes improve clarity.
- Add a quick check to fail immediately if tmpfiles rules didn’t apply.
- "cd /tmp/snapm && cp -r etc/snapm /etc", - "cd /tmp/snapm && cp systemd/*.service systemd/*.timer /usr/lib/systemd/system", - "cd /tmp/snapm && cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d", + "cd /tmp/snapm && cp -r etc/snapm/ /etc/", + "cd /tmp/snapm && cp systemd/*.service systemd/*.timer /usr/lib/systemd/system/", + "cd /tmp/snapm && cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/", # Apply tmpfiles change "systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf", + "test -d /run/snapm/mounts",
🧹 Nitpick comments (4)
snapm/manager/_manager.py (1)
88-93: Unused constants may trip flake8/pyflakes.If these are intentionally added for upcoming use, annotate to silence F841 until wired in.
Apply either of these diffs:
-#: Path to snapm runtime directory -_SNAPM_RUNTIME_DIR = "/run/snapm" +#: Path to snapm runtime directory +_SNAPM_RUNTIME_DIR = "/run/snapm" # noqa: F841 -#: Path to snapm snapshot set mount directory -_SNAPM_MOUNTS_DIR = join(_SNAPM_RUNTIME_DIR, "mounts") +#: Path to snapm snapshot set mount directory +_SNAPM_MOUNTS_DIR = join(_SNAPM_RUNTIME_DIR, "mounts") # noqa: F841or start using these in the mount management code in this PR to avoid the noqa.
snapm.spec (1)
135-140: Use of %tmpfiles_{create,delete} is correct; add build macro dep for robustness.On Fedora/CS9 the macros are available, but adding BuildRequires helps ensure spec parsing on all supported builders.
Apply:
BuildRequires: python3-wheel +BuildRequires: systemd-rpm-macros %if 0%{?fedora}Confirm your build roots already provide systemd-rpm-macros; if so, this is a no-op safety net.
.github/workflows/snapm.yml (2)
51-55: Harden the copy step: add set -euo pipefail and trailing slashes (clarity, safety).
- Keep fail-fast on missing globs; this preserves your intended behavior.
- Trailing slashes avoid accidental file-vs-dir ambiguity.
- - name: Copy systemd units to /usr/lib/systemd/system - run: | - sudo cp systemd/*.service /usr/lib/systemd/system - sudo cp systemd/*.timer /usr/lib/systemd/system - sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d + - name: Copy systemd units and tmpfiles.d config + run: | + set -euo pipefail + sudo cp systemd/*.service /usr/lib/systemd/system/ + sudo cp systemd/*.timer /usr/lib/systemd/system/ + sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/
56-57: Verify tmpfiles effects immediately (assert runtime dirs exist).Convert to multiline, keep fail-fast, and assert /run paths now exist.
- - name: Create tmpfiles.d paths - run: > - sudo systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf + - name: Create tmpfiles.d paths + run: | + set -euo pipefail + sudo systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf + # Sanity-check: fail fast if runtime dirs weren't created + sudo test -d /run/snapm/mounts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/snapm.yml(1 hunks)CONTRIBUTING.md(1 hunks)container_tests/Containerfile(1 hunks)container_tests/Makefile(2 hunks)snapm.spec(3 hunks)snapm/manager/_manager.py(1 hunks)systemd/tmpfiles.d/snapm.conf(1 hunks)virt_tests/testvm.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
CONTRIBUTING.mdcontainer_tests/Containerfilevirt_tests/testvm.pysnapm.specsystemd/tmpfiles.d/snapm.conf.github/workflows/snapm.yml
📚 Learning: 2025-08-27T21:01:32.273Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
CONTRIBUTING.mdcontainer_tests/Containerfilevirt_tests/testvm.pysnapm.specsystemd/tmpfiles.d/snapm.conf.github/workflows/snapm.yml
📚 Learning: 2025-08-26T19:07:42.290Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-25T19:20:21.720Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-25T19:20:21.720Z
Learning: In the snapm virt_tests workflow, optimizing Git checkouts by using virtiofs mapping is complex because it requires checkouts for both `snapm` and `boom` repositories. Git checkouts aren't the dominant time cost, so the current approach of cloning within VMs is more practical than adding workflow complexity and local user setup requirements.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: The upstream snapm.spec file in the snapm project only aims to work on centos-stream9 through Fedora rawhide. Downstreams maintain their own copies and adapt as needed.
Applied to files:
snapm.spec
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: python-basic-tests
🔇 Additional comments (7)
container_tests/Makefile (2)
14-14: Prep now includes tmpfiles.d config — LGTM.Copies snapm.conf into the build context; preserves fail-fast semantics if missing.
44-44: Clean target correctly removes tmpfiles.d artifact — LGTM.Keeps the context tidy between builds.
systemd/tmpfiles.d/snapm.conf (1)
1-4: Tmpfiles rules look correct and secure.
- Directories created with 0700 under /run.
- SELinux context restore with recursive 'Z' is appropriate.
container_tests/Containerfile (1)
10-10: Include snapm.conf in /usr/lib/tmpfiles.d — LGTM.Aligns with packaged config location; systemd inside the container will apply it at boot.
snapm.spec (2)
101-103: Install tmpfiles.d config with explicit mkdir + install — LGTM.Matches the project’s packaging preference.
120-120: %files lists tmpfiles.d config — LGTM.Ownership and mode are correct for packaged config.
virt_tests/testvm.py (1)
535-536: Good: apply tmpfiles so /run/snapm[/mounts] exists without reboot.
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
3 times, most recently
from
86a56bf to
3d88014
Compare
Add a tmpfiles.d drop-in for the snapm runtime directories. Resolves: #441
bmr-cymru
force-pushed
the
bmr-snapshot-set-mount-framework
branch
from
3d88014 to
006c16e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
container_tests/Containerfile (1)
10-10: Include tmpfiles config in image (good); consider pinning permsOptional: add --chmod to make intent explicit and avoid host-filemode surprises in build contexts.
Apply:
-COPY snapm.conf /usr/lib/tmpfiles.d/ +COPY --chmod=0644 snapm.conf /usr/lib/tmpfiles.d/snapm.spec (1)
19-19: Add scriptlet deps for tmpfiles to ensure systemd-tmpfiles availabilityYou’re invoking %tmpfiles_create/%tmpfiles_delete; add scriptlet Requires so installs don’t fail on minimal systems where systemd may be trimmed.
Name: snapm Version: 0.4.0 Release: 1%{?dist} Summary: %{summary} @@ BuildRequires: systemd-rpm-macros +Requires(post): systemd +Requires(postun): systemd @@ mkdir -p ${RPM_BUILD_ROOT}/%{_tmpfilesdir} %{__install} -p -m 644 systemd/tmpfiles.d/snapm.conf ${RPM_BUILD_ROOT}/%{_tmpfilesdir}/ @@ %attr(644, -, -) %{_tmpfilesdir}/snapm.conf @@ %post %tmpfiles_create %{_tmpfilesdir}/snapm.conf %postun %tmpfiles_delete %{_tmpfilesdir}/snapm.confAlso applies to: 102-104, 121-121, 136-141
virt_tests/testvm.py (1)
532-536: Minor cp robustness: end destination with “/”Trailing slashes make intent unambiguous and avoid edge-cases with cp when multiple sources are given.
- "cd /tmp/snapm && cp -r etc/snapm /etc", - "cd /tmp/snapm && cp systemd/*.service systemd/*.timer /usr/lib/systemd/system", - "cd /tmp/snapm && cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d", + "cd /tmp/snapm && cp -r etc/snapm /etc/", + "cd /tmp/snapm && cp systemd/*.service systemd/*.timer /usr/lib/systemd/system/", + "cd /tmp/snapm && cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/", # Apply tmpfiles change "systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf",.github/workflows/snapm.yml (3)
51-55: Prefer install over cp to enforce permissions explicitly.To make modes deterministic and align with the “mkdir -p + install” preference, switch to install and ensure dirs exist.
run: | - set -euo pipefail - sudo cp systemd/*.service /usr/lib/systemd/system/ - sudo cp systemd/*.timer /usr/lib/systemd/system/ - sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/ + set -euo pipefail + sudo install -d -m 0755 /usr/lib/systemd/system + sudo install -d -m 0755 /usr/lib/tmpfiles.d + sudo install -m 0644 systemd/*.service /usr/lib/systemd/system/ + sudo install -m 0644 systemd/*.timer /usr/lib/systemd/system/ + sudo install -m 0644 systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/
51-55: Make unmatched globs fail even earlier.Optional: enable failglob so the shell errors before invoking cp/install when patterns don’t match.
run: | set -euo pipefail + shopt -s failglob sudo cp systemd/*.service /usr/lib/systemd/system/ sudo cp systemd/*.timer /usr/lib/systemd/system/ sudo cp systemd/tmpfiles.d/snapm.conf /usr/lib/tmpfiles.d/
56-60: Verify tmpfiles results (permissions/ownership) for stronger guarantees.You already assert directory existence. Optionally assert mode/owner to catch subtle regressions in snapm.conf.
run: | set -euo pipefail sudo systemd-tmpfiles --create /usr/lib/tmpfiles.d/snapm.conf sudo test -d /run/snapm/mounts + # Optional: assert perms/ownership (adjust if snapm.conf differs) + sudo stat -c '%a %U:%G %n' /run/snapm /run/snapm/mounts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/snapm.yml(1 hunks)CONTRIBUTING.md(1 hunks)container_tests/Containerfile(1 hunks)container_tests/Makefile(2 hunks)snapm.spec(4 hunks)snapm/manager/_manager.py(1 hunks)systemd/tmpfiles.d/snapm.conf(1 hunks)virt_tests/testvm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- container_tests/Makefile
- CONTRIBUTING.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-27T21:01:32.273Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T21:01:32.273Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Applied to files:
container_tests/Containerfile.github/workflows/snapm.ymlvirt_tests/testvm.pysnapm.spec
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: For systemd unit installation in snapm.spec, prefer the explicit mkdir -p + install style over install -D for better clarity and to avoid obfuscating directory creation responsibility.
Applied to files:
container_tests/Containerfilesystemd/tmpfiles.d/snapm.conf.github/workflows/snapm.ymlvirt_tests/testvm.pysnapm.spec
📚 Learning: 2025-08-26T19:07:42.290Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-25T19:20:21.720Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-25T19:20:21.720Z
Learning: In the snapm virt_tests workflow, optimizing Git checkouts by using virtiofs mapping is complex because it requires checkouts for both `snapm` and `boom` repositories. Git checkouts aren't the dominant time cost, so the current approach of cloning within VMs is more practical than adding workflow complexity and local user setup requirements.
Applied to files:
virt_tests/testvm.py
📚 Learning: 2025-08-27T20:27:47.258Z
Learnt from: bmr-cymru
PR: snapshotmanager/snapm#0
File: :0-0
Timestamp: 2025-08-27T20:27:47.258Z
Learning: The upstream snapm.spec file in the snapm project only aims to work on centos-stream9 through Fedora rawhide. Downstreams maintain their own copies and adapt as needed.
Applied to files:
snapm.spec
🔇 Additional comments (3)
snapm/manager/_manager.py (1)
88-93: LGTM: runtime path constants align with tmpfiles configNamespacing and placement are consistent; no behavior change introduced.
systemd/tmpfiles.d/snapm.conf (1)
1-4: LGTM: creates /run/snapm and mounts dir with sane perms; SELinux relabel recursiveThe directives look correct and minimal.
.github/workflows/snapm.yml (1)
51-55: Good: explicit, fail-fast copies with strict shell flags.Using set -euo pipefail and separate cp commands matches the project’s fail-fast intent and makes failures obvious when expected files are missing.
|
Superseded by #541 |
1 participant
Resolves: #440
Resolves: #441
Resolves: #442
Summary by CodeRabbit
New Features
Documentation
Tests
Chores