composefs: Fix inverted rollback queued detection - #2406
Conversation
The systemd UKI arm (BLSConfigType::EFI) of the rollback detection in composefs_deployment_status_from() is missing the negation that the NonEFI and GRUB UKI arms have: it reports rollbackQueued=true exactly when the first (default) boot entry DOES reference the booted deployment. On an sd-boot UKI host this inverts everything derived from it: bootc status reports a queued rollback after every successful staged upgrade boot and reports clean when a rollback really is queued; bootc rollback prints "Reverting queued rollback state" while actually queueing one (and vice versa); and the guard refusing to delete the rollback deployment while it is queued to boot fires in the wrong cases. Extract the check into rollback_queued_from_first_entry() so the BLS arms share one negation, and add regression tests covering both entry types in both directions. Closes: bootc-dev#2405 Signed-off-by: Andrew Dunn <andrew@dunn.dev>
Johan-Liebert1
left a comment
Johan-Liebert1
left a comment
There was a problem hiding this comment.
This gets really annoying due to Grub's sorting being different than that of systemd-boot. GrubCC is its own separate case with it almost working like systemd-boot but sorting entries like Grub.
I believe the change is good, but could you please add this test to tmt/tests/booted/test-rollback.nu so we can have integration tests run for all bootloaders?
The GRUBClassic/Bls arm sorted entries descending (ascending=false), putting the non-default entry first, then applied the first-entry check to it — the same inversion the previous commit fixes for sd-boot UKI, entered from the other side. The reversed list also fed the rollback-candidate selection and soft-reboot capability code, both of which expect menu order (default first), as the BLSCompatible arm already provides. Sort ascending and route through rollback_queued_from_first_entry() so every Type1-reading arm shares one negation. Add a Grub-parameterized regression test pinning the premise the check stands on: under the descending filename-release comparator the primary entry is grub's default. Also correct the existing tests' fixture pairing to match production (sort-key "-0" rides filename release "1", per boot.rs) — the Systemd comparator ignored the filenames, the Grub comparator would not. Signed-off-by: Andrew Dunn <andrew@dunn.dev>
The rollback test rebooted through queued and consumed rollback states without ever checking what bootc status reported for them. Add an assertion helper and check rollbackQueued at each transition: a freshly booted deployment reports false, a queued rollback reports true, the double rollback queues then unqueues, and booting the intended deployment consumes the queue. On composefs sd-boot UKI hosts the pre-fix code inverted the first and last of these, so the test fails there without the previous commit and pins the regression across every bootloader configuration the plan runs on. Signed-off-by: Andrew Dunn <andrew@dunn.dev>
|
Added in the tmt commit: test-rollback.nu asserts rollbackQueued at each transition (false on a fresh boot, true once queued, the double rollback queues then unqueues, false again after booting the target), so every bootloader config the plans run exercises it. Writing it surfaced the same inversion in the GRUBClassic/Bls arm: ascending=false put the non-default entry first, and the new asserts would have tripped on the grub lanes. The second commit sorts ascending, routes that arm through the shared helper, adds a Grub-parameterized unit test pinning that the primary entry sorts first under the filename-release comparator, and corrects the earlier fixtures to production's sort-key/filename pairing. |
2 participants
The
BLSConfigType::EFIarm of the rollback detection incomposefs_deployment_status_fromis missing the negation theNonEFIand GRUB UKI arms have, so on a systemd-boot UKI hostbootc statusreportsrollbackQueued: trueafter every successful staged upgrade boot andfalsewhen a rollback really is queued.bootc rollbackmessaging/direction and the rollback-deployment delete guard invert with it. Details and on-host receipts in #2405.The check is extracted into
rollback_queued_from_first_entry()so the BLS arms share one negation, with regression tests for both entry types in both directions - fixture entries are parsed and sorted throughget_sorted_type1_boot_entries_helper, asserting the entry carryingprimary_sort_keyis the default and that the verdict flips with the booted digest.Testing: full
bootc-libsuite green; the new tests fail on the unfixed code (re-inverting either arm's negation is caught); on a physical Fedora 44 sealed UKI host whose sd-boot default entry is the booted deployment (confirmed withbootctl list), stock 1.16.9 reportsrollbackQueued: trueand the patched binary reportsfalseon the same on-disk state.Fixes #2405