Release Upgrader

Merge ubuntu-release-upgrader:server-mode-check into ubuntu-release-upgrader:ubuntu/jammy

  1. Git
  2. lp:ubuntu-release-upgrader
  3. server-mode-check
  4. Merge into ubuntu/jammy
Proposed by Brian Murray
Status: Needs review
Proposed branch: ubuntu-release-upgrader:server-mode-check
Merge into: ubuntu-release-upgrader:ubuntu/jammy
Diff against target: 33 lines (+4/-4)
1 file modified
DistUpgrade/DistUpgradeController.py (+4/-4)
Reviewer Review Type Date Requested Status
Julian Andres Klode Approve
Review via email: mp+428312@code.staging.launchpad.net

This proposal supersedes a proposal from 2022-08-12.

Description of the change

Looking at the calls to self.cache.need_server_mode() in DistUpgradeController.py one of them can do more harm than good by switching from a "desktop" mode upgrade to a "server" mode upgrade. This changes that by only calling the function again if "server" mode is already set.

I tested an upgrade of an Ubuntu 20.04 server install with this change and it worked without issue. Additionally, I tested a desktop upgrade of Ubuntu 22.04 (only the calculation of the DistUpgrade) and it also worked without an issue.

It might also be necessary to reset `os.environ["RELEASE_UPGRADE_MODE"]`, however it looks like there is an existing bug with it not being set after changing mode.

To post a comment you must log in.
Revision history for this message
Julian Andres Klode (juliank) wrote : Posted in a previous version of this proposal

Please rebase and fix the conflicts, this is not reviewable as is.

review: Needs Fixing
Revision history for this message
Julian Andres Klode (juliank) wrote :

Looks ok, though I'd prefer to assign None to self.tasks and check for `is None`.

review: Approve
Reply
Revision history for this message
Nick Rosbrook (enr0n) wrote :

Is there a more general reason why it makes sense to not allow switching from desktop to server mode? Or is this just to avoid the AttributeError?

When I first looked at this issue, the question I had about the code was: why do we assign a `self.tasks` attribute at all, instead of accessing `self.cache.installedTasks` when needed? I.e.,

diff --git a/DistUpgrade/DistUpgradeController.py b/DistUpgrade/DistUpgradeController.py
index 29002e61..71a5e4f4 100644
--- a/DistUpgrade/DistUpgradeController.py
+++ b/DistUpgrade/DistUpgradeController.py
@@ -945,8 +945,6 @@ class DistUpgradeController(object):
             self.config.set("Options","foreignPkgs", "True")
         else:
             self.config.set("Options","foreignPkgs", "False")
- if self.serverMode:
- self.tasks = self.cache.installedTasks
         logging.debug("Foreign: %s" % " ".join(sorted(self.foreign_pkgs)))
         logging.debug("Obsolete: %s" % " ".join(sorted(self.obsolete_pkgs)))
         return True
@@ -1098,7 +1096,7 @@ class DistUpgradeController(object):
             return False

         if self.serverMode:
- if not self.cache.installTasks(self.tasks):
+ if not self.cache.installTasks(self.cache.installedTasks):
                 return False

         # show changes and confirm

Reply
Revision history for this message
Brian Murray (brian-murray) wrote :

> Is there a more general reason why it makes sense to not allow switching from
> desktop to server mode? Or is this just to avoid the AttributeError?

This was to avoid the AttributeError and trying to find a quick fix rather than sorting out why we check need_server_mode() so many times.

> When I first looked at this issue, the question I had about the code was: why
> do we assign a `self.tasks` attribute at all, instead of accessing
> `self.cache.installedTasks` when needed? I.e.,
>
> diff --git a/DistUpgrade/DistUpgradeController.py
> b/DistUpgrade/DistUpgradeController.py
> index 29002e61..71a5e4f4 100644
> --- a/DistUpgrade/DistUpgradeController.py
> +++ b/DistUpgrade/DistUpgradeController.py
> @@ -945,8 +945,6 @@ class DistUpgradeController(object):
> self.config.set("Options","foreignPkgs", "True")
> else:
> self.config.set("Options","foreignPkgs", "False")
> - if self.serverMode:
> - self.tasks = self.cache.installedTasks
> logging.debug("Foreign: %s" % " ".join(sorted(self.foreign_pkgs)))
> logging.debug("Obsolete: %s" % " ".join(sorted(self.obsolete_pkgs)))
> return True
> @@ -1098,7 +1096,7 @@ class DistUpgradeController(object):
> return False
>
> if self.serverMode:
> - if not self.cache.installTasks(self.tasks):
> + if not self.cache.installTasks(self.cache.installedTasks):
> return False
>
> # show changes and confirm

That makes more sense to me especially since this is the only place self.tasks is used at all.

Reply

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches