fix: avoid misleading journalctl warning in logs on first boot - #190
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a check in src/main.rs to verify if a previous boot exists using journalctl --list-boots before attempting to query the journal for the previous boot (-b -1). This prevents errors on systems during their first boot. The review feedback suggests optimizing the check for the number of boots by using .nth(1).is_none() instead of counting all lines, which avoids unnecessary iteration over the entire command output.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
de51ed4 to
e0432e6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a check to verify if a previous boot exists using journalctl --list-boots before querying its journal, skipping the rollback check if no previous boot is found. It also improves a log message format. The feedback suggests inverting the success check for journalctl --list-boots to return early on failure, which reduces nesting and improves readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
LGTM, I like gemini comment that prevents the use of else . |
|
Can we do the implementation of checking if its a first time boot of the system only, systemd should provide such info, as i think system with non-persistent journald will skip the rollback check everytime without reporting error. |
knecasov
left a comment
There was a problem hiding this comment.
I added a few comments and gave a thumbs-up to the Gemini note.
e0432e6 to
3511b80
Compare
@say-paul Thanks for suggesting ! |
knecasov
left a comment
There was a problem hiding this comment.
LGTM, I would only recommend editing the PR description to reflect the final implementation.
3511b80 to
abc7f15
Compare
|
Thanks for review and approval @mcattamoredhat and @knecasov . Applied changes suggested by you @knecasov Thanks! |
knecasov
left a comment
There was a problem hiding this comment.
Thank you so much for fixes! The PR description looks good now - only the formatting seems to be broken.
Anyway, it is not blocking merging, so I am approving it!
abc7f15 to
fa0cafe
Compare
981bde7
e55bb40 to
fe72330
Compare
|
I have addressed @mmartinv's review comments, thanks for your feedback
Regarding tests:
|
RHEL 9.8 / CentOS 9 ostree test failure — analysis + fix pushedDug into the Root cause: I noticed 163b012 already added I also tried fixing it via the blueprint's What actually works: resolve the exact I also noticed a1f51ec already fixed the underlying journalctl message-matching bug for RHEL 9.8/systemd 252 ( Pushed 🤖 Assisted-by: OpenCode (Claude Sonnet 5) |
43564ed to
cbe3206
Compare
djach7
left a comment
There was a problem hiding this comment.
From what I can tell all feedback is addressed and I can't find any blockers. LGTM.
say-paul
left a comment
There was a problem hiding this comment.
The code changes looks good to me, and the RHEL-10.2 bootc test is passing.
I would create separate issue for tracking the CI failures: https://github.com/fedora-iot/greenboot-rs/actions/runs/31021970675/job/92360810239?pr=190
- Fedora bootc - its running the old binary
- Rhel 9.8-ostree - composer-cli conflict to install the main stream and the PR build of green boot.
1c3ccf8
cbe3206 to
1c3ccf8
Compare
Replace confusing warning with clear info message when no previous boot journal is available. This is expected behavior on first boot after installation or on systems with non-persistent journald. Support both systemd error message variants: - systemd 255+: 'No journal boot entry found for the specified boot' - systemd 252: 'No such boot ID in journal' For unexpected journalctl errors, warn and continue in degraded mode instead of failing - greenboot can still function without rollback detection. Signed-off-by: Sarita Mahajan <sarmahaj@redhat.com> Assisted-by: Claude (claude-sonnet-4.5)
Update test assertions to verify the new INFO message appears on first boot instead of checking for the absence of the old warning message. Changes: - Use 'journalctl -b 1' to get first boot logs (simpler than boot ID lookup) - Check for 'No previous boot journal available' message on first boot - Verify no false 'FALLBACK BOOT DETECTED!' on first boot - Split combined assertion into two distinct checks for clarity Signed-off-by: Sarita Mahajan <sarmahaj@redhat.com>
Fix CI test failures caused by package installation and build issues:
1. **Force Copr package installation**:
- CI tests were installing greenboot from official repos instead of
Copr because version numbers were identical
- Changed 'dnf install' to 'dnf reinstall || dnf install' to force
installation of PR build from Copr even when version matches
- Added 'dnf clean metadata' before installation
- This ensures tests run against the actual PR code changes
2. **Fix make-rpm job git ownership error**:
- Add git safe.directory configuration in Build RPMs step
- Fixes "fatal: detected dubious ownership in repository" error
that occurs when running in a container
Signed-off-by: Sarita Mahajan <sarmahaj@redhat.com>
Add greenboot and greenboot-default-health-checks to osbuild-composer blueprint to ensure they're pulled from Copr source instead of base image. Also add depsolve output verification. Signed-off-by: Sarita Mahajan <sarmahaj@redhat.com>
Listing greenboot in the blueprint with version = "*" isn't enough: dnf always installs the highest NEVRA across all enabled repos, and Copr snapshot builds conventionally use a Release starting at "0.<timestamp>..." -- the same convention official pre-GA/rebuilt packages use. Whenever BaseOS/AppStream ships a greenboot release that outranks the current Copr build, dnf silently installs the stock package and the test exercises unpatched code. I first tried fixing this via blueprint [[customizations.repositories]] priority + install_from, since that's the field osbuild-composer exposes for repo precedence. Verified against a real osbuild-composer (v176) on CentOS Stream 9, with a synthetic competing repo publishing greenboot at a deliberately higher release, that those fields have no effect on build-time depsolve -- composer-cli accepts and forwards them fine, but the resolver still just picks the highest NEVRA regardless. They only affect the .repo files written into the resulting image for its own future dnf use. What does work: resolve the exact version-release dnf sees in the Copr repo via 'dnf repoquery --latest-limit=1' and pin both greenboot packages to it directly. Verified this reliably wins even against the adversarial higher-release competing repo. Assisted-by: OpenCode (Claude Sonnet 5) rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
Add --tpm none to all virt-install commands to work around SELinux regression with swtpm on Fedora Rawhide hosts. TPM is not used in greenboot tests, so disabling it avoids the swtpm-related failures without affecting test coverage. Suggested-by: Klara Zatloukalova <kzatloka@redhat.com> Signed-off-by: Sarita Mahajan <sarmahaj@redhat.com>
1c3ccf8 to
565377c
Compare
knecasov
left a comment
There was a problem hiding this comment.
LGTM, thank you very much for all the fixes!
Add --tpm none to work around SELinux regression with swtpm on Fedora Rawhide hosts. TPM is not used in greenboot tests. From upstream PR fedora-iot#190. Signed-off-by: Sarita Mahajan <sarmahaj@redhat.com>
Use systemd-analyze condition ConditionFirstBoot=yes to detect first boot before querying the previous boot journal. This prevents a misleading warning on first boot when journalctl -b -1 fails because there is no prior boot. This also works correctly on systems with non-persistent journald storage.