Add user-initiated rollback #381

detsch · 2024-12-19T16:13:34Z

No description provided.

detsch · 2024-12-19T16:20:03Z

Still missing handling of bootloader rollback protection. If enabled, we should not allow rollback after update is finalized.

src/liteclient.cc

mike-sul · 2024-12-20T10:41:41Z

LGTM, will test it a bit.

mike-sul · 2025-01-03T15:20:59Z

src/aklite_client_ext.cc

+      pending_target.IsUnknown() ? InstalledVersionUpdateMode::kBadTarget : InstalledVersionUpdateMode::kNone;
+
+  // Set current target as failing
+  storage->saveInstalledVersion("", Target::fromTufTarget(current), update_mode);


Is there guarantee that the current target is the same as the pending one if the pending is not null in all use-cases? For example, if ostree update is installed and the rollback command in invoked before device reboot?

I would print some logs saying that we are about to mark the given target as "failing/bad" target.

Is there guarantee that the current target is the same as the pending one if the pending is not null in all use-cases? For example, if ostree update is installed and the rollback command in invoked before device reboot?

The Set current target as failing comment there is wrong. There are 2 different things that we do there, depending if there is or not a pending target:

Setting the current target as bad (if the previous installation was finalized)

Creating a new log entry related to the re-installation of the current target

Both happen to be done by calling storage->saveInstalledVersion, so when simplifying the code I left only one call, and forgot do adjust the comment accordingly.

I've now adjusted the code to make this distinction more clear, and also added log messages. Next force push will include the changes,

On a related note, the whole e2e test sequence for rollback runs in 3 scenarios:

After a finalizing an installation;

After rebooting, but before finalizing; and

Before rebooting

This is done by using pytest parameters:

@pytest.mark.parametrize('do_reboot', [True, False]) @pytest.mark.parametrize('do_finalize', [True, False])

src/aklite_client_ext.cc

mike-sul · 2025-01-03T15:35:12Z

src/aklite_client_ext.cc

+  TufTarget rollback_target;
+  if (version == -1 && target_name.empty()) {
+    rollback_target = Target::toTufTarget(client_->getRollbackTarget(true));
+    if (rollback_target.IsUnknown() || rollback_target.Name() == current.Name()) {


I am not sure if this rollback_target.Name() == current.Name() works if a new ostree version is installed and a device is not installed yet.

src/aklite_client_ext.cc

mike-sul · 2025-01-03T15:39:59Z

src/aklite_client_ext.cc

+      return {InstallResult::Status::Failed};
+    }
+
+    if (rollback_target.Name() == current.Name()) {


I think the rollback to the current is possible (which is effectively "sync"), e.g. app only update (app update but not started and then the rollback is invoked), or ostree update - ostree installed and the rollback is invoked before reboot?

Perhaps we should allow calling aklite rollback <current_version> only when there is a pending installation?

iirc, there is a pending installation once ostree is installed, even before reboot.

Supposing we are running target 1, and there is a target 2 that has an ostree update.

Right after we call aklite update 2, before reboot, GetCurrent returns target 2. It would make no sense to call aklite rollback 2. Same after reboot and after finalizing.

Supposing we are running target 3, and there is a target 4 that has no ostree update (apps only).

The install step will already finalize the installation, so GetCurrent will return 4, and it makes no sense to try to rollback to 4.

Given that scenarios, it seems to me that the current if is correct.

The is_current field in installed_versions table only gets updated after finalization, but GetCurrent uses other data as well to decide which target is the current one.

This behavior (a target showing as current even before reboot) is specific to the test environment (config.booted != BootedType::kBooted). I'm taking a look to see if there is an easy fix. I may have to adjust the rollback-related code based on the correct behavior.

Yes, if there is an ostree update, then after installation (new ostree version is deployed) and before reboot, the "get current" returns the version a device is currently booted on provided that a device is booted on ostree.

Same after reboot and after finalizing.

That's inline with what I observe.

The install step will already finalize the installation, so GetCurrent will return 4, and it makes no sense to try to rollback to 4.

What if an install of an app-only update fails, then a device is at kind of "undefined" state, so a user needs to run either the rollback command or just install one of the previous version which is almost the same as the user-driven rollback.

The daemon client will auto-rollback to the previous version automatically in this case. However, the CLI client as well as the API should support it and allow rolling back to the current version in this case which is effectively the "sync" current operation.

I've done a quick adjustment in OstreeManager::getCurrent when config.booted != BootedType::kBooted, to make it consistent with a booted device.
I'm in the process of adjusting the rollback code and tests accordingly.

mike-sul · 2025-01-03T15:48:21Z

src/liteclient.cc


  Uptane::Target rollback_target{Uptane::Target::Unknown()};
  {
    std::vector<Uptane::Target> installed_versions;
    storage->loadPrimaryInstallationLog(&installed_versions,
-                                        true /* make sure that Target has been successfully installed */);
+                                        true /* make sure that Target has been successfully installed */, false);


The same question about considering the current target as the target to rollback to under certain circumstances.

A change here is probably needed, to allow current depending if the latest install has finished or not. I'll adjust it tomorrow.

mike-sul · 2025-01-03T15:53:42Z

src/aklite_client_ext.cc

+  OSTree::Sysroot::Ptr ostree_sysroot_;
+  auto ostree_sysroot = std::make_shared<OSTree::Sysroot>(client_->config.pacman);
+  auto bootloader_lite = bootloader::BootloaderLite(client_->config.bootloader, *client_->storage, ostree_sysroot);
+  return bootloader_lite.isRollbackProtectionEnabled();


I would add a method to LiteClient similar to LiteClient::isBootFwUpdateInProgress().

mike-sul · 2025-01-03T15:58:14Z

src/aklite_client_ext.cc

@@ -287,6 +294,11 @@ InstallResult AkliteClientExt::Rollback(const LocalUpdateSource* local_update_so
  auto update_mode =
      pending_target.IsUnknown() ? InstalledVersionUpdateMode::kBadTarget : InstalledVersionUpdateMode::kNone;

+  if (pending_target.IsUnknown() && isRollbackProtectionEnabled()) {


Is it suffice just to check whether bootloader rollback protection is enabled? What if the boot fw version of the rollback target is the same is as the current one?

I think it's problematic a bit to check it if the rollback target is not the previously installed target because we cannot fetch info about boot fw version of this target.

Maybe we don't need this check at all since the logic in the rootfs manager has the check if there is boot fw downgrade/rollback. So it will yield an error if it detects for the rollback target?

Currently approach is overly-protective, but having a more complete verification would add too much complexity. Assuming that a bootloader update is not very common, it makes sense to disable this check altogether.

I incline towards not doing this check at all at the beginning of the user-triggered process, anyway the installation will fail during this process if the bootloader rollback protection is turned ON because of the check in the rootfs manager.

mike-sul · 2025-01-03T16:01:01Z

LGTM, the direction is fine. I just have two concerns:

Not allowing the current target as "rollback" target under certain circumstances.
Doing the boot fw rollback protection check before invoking installation of the rollback target (we simply don't know boot fw version in advance before the version file is available of a file system).

This new operation marks the current target as a failing target, and proceeds to install the selected rollback target, which can be automatically selected (the most recent fully installed target), or explicitly defined by the caller of the function. A rollback can be initiated after an installation is completed, but also while finalization of system reboot is still pending. Signed-off-by: Andre Detsch <[email protected]>

Signed-off-by: Andre Detsch <[email protected]>

detsch requested a review from mike-sul December 19, 2024 16:13

detsch force-pushed the detsch-user-rollback branch from 3b57c51 to 2cc18d1 Compare December 19, 2024 16:16

mike-sul reviewed Dec 20, 2024

View reviewed changes

src/liteclient.cc Outdated Show resolved Hide resolved

detsch force-pushed the detsch-user-rollback branch from 2cc18d1 to a974c91 Compare January 3, 2025 00:04

mike-sul reviewed Jan 3, 2025

View reviewed changes

src/aklite_client_ext.cc Outdated Show resolved Hide resolved

mike-sul reviewed Jan 3, 2025

View reviewed changes

src/aklite_client_ext.cc Outdated Show resolved Hide resolved

mike-sul reviewed Jan 3, 2025

View reviewed changes

src/aklite_client_ext.cc Outdated Show resolved Hide resolved

mike-sul reviewed Jan 3, 2025

View reviewed changes

detsch force-pushed the detsch-user-rollback branch 2 times, most recently from f527c57 to 3fec0a3 Compare January 6, 2025 13:15

detsch added 3 commits January 9, 2025 17:39

cli: Add rollback operation to command line interface

324e139

Signed-off-by: Andre Detsch <[email protected]>

e2e-tests: Add tests for user initiated rollback

bba8f3f

Signed-off-by: Andre Detsch <[email protected]>

detsch force-pushed the detsch-user-rollback branch from 3fec0a3 to bba8f3f Compare January 9, 2025 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user-initiated rollback #381

Add user-initiated rollback #381

detsch commented Dec 19, 2024

detsch commented Dec 19, 2024

mike-sul commented Dec 20, 2024

mike-sul Jan 3, 2025

mike-sul Jan 3, 2025

detsch Jan 3, 2025

mike-sul Jan 3, 2025

mike-sul Jan 3, 2025

detsch Jan 5, 2025

mike-sul Jan 7, 2025

detsch Jan 7, 2025

detsch Jan 7, 2025

mike-sul Jan 8, 2025

mike-sul Jan 8, 2025

mike-sul Jan 8, 2025

detsch Jan 8, 2025

mike-sul Jan 3, 2025

detsch Jan 6, 2025

mike-sul Jan 3, 2025

mike-sul Jan 3, 2025

detsch Jan 6, 2025

mike-sul Jan 7, 2025

mike-sul commented Jan 3, 2025

Add user-initiated rollback #381

Are you sure you want to change the base?

Add user-initiated rollback #381

Conversation

detsch commented Dec 19, 2024

detsch commented Dec 19, 2024

mike-sul commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul commented Jan 3, 2025