mirror: if the mirror test fails, suggest an offline install #1968

ogayot · 2024-04-11T09:14:36Z

Client / TUI change

In the mirror screen, if the test fails and the user decides to ignore the failure, we used to continue the installation normally ; which in most scenarios resulted in an error at a later stage of the installation.

Instead, we now revert to an installation without network (i.e., only packages from the pool are considered for installation) if the user decides to ignore the failure.

Current design:

Proposed design:

Previously proposed design

Note that the behavior hasn't changed if the user decides to skip the test while it is running,

Backend change

The /mirror GET and POST endpoints now include a boolean field named use_during_installation.

if set to True, the mirror information will be used during installation to fetch packages online.
if set to False, we will only fetch packages from the pool.

In either case, the mirror information will still be used to build the etc/apt/ directory in the target system.

Currently, the way use_during_installation is implemented is coupled with the force_offline property of the network model. This means that Ubuntu Pro and other stuff will be disabled too if we're skipping the use of the archive.

NOTE: the default value for "use_during_installation" in the POST endpoint is null ; which means that we should not change the current value of force_offline.

LP:#2059898 and similar reports

The /mirror GET and POST endpoints now include a boolean field named "use_during_installation". * if set to True, the mirror information will be used during installation to fetch packages online. * if set to False, we will only fetch packages from the pool. In either case, the mirror information will still be used to build the etc/apt/ directory in the target system. Currently, the way use_during_installation is implemented is coupled with the force_offline property of the network model. This means that Ubuntu Pro and other stuff will be disabled too if we're skipping the use of the archive. NOTE: the default value for "use_during_installation" in the POST endpoint is `null` ; which means that we should not change the current value of force_offline. Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

ogayot · 2024-04-11T11:25:22Z

NOTE, when testing in a VM, I noticed another issue that is technically unrelated but prevents the user from going back to the mirror screen.

The order of the screens is "... -> Mirror -> Refresh -> Storage -> ...". After moving from Mirror to Storage, hitting Back moves us the Refresh page again ; which finds nothing to do and therefore calls .done(), moving forward to Storage again.

Will raise a launchpad bug for it. EDIT LP:#2060964

dbungert

I propose that we take the needed parts from this PR and add in the policy change from #1816 but do so across the board - OFFLINE_INSTALL as the fallback across the board. WDYT?

ogayot · 2024-04-12T07:11:14Z

I propose that we take the needed parts from this PR and add in the policy change from #1816 but do so across the board - OFFLINE_INSTALL as the fallback across the board. WDYT?

Honestly, I've never been a fan of fallback: offline-install by default ; at least without a clear indication that an offline installation is going on. Otherwise:

after installation, you might boot into an outdated (potentially vulnerable) system without realizing it
if you provide a wrong network configuration (e.g., missing proxy and stuff), I'd argue that you'd probably want the installation to stop and fix your config
it can't work properly (yet?) with other directives like packages: [...]. (unless everything is in the pool)

With that in mind, I'll still implement the change and we can discuss further.
#1971

dbungert · 2024-04-16T19:18:18Z

subiquity/ui/views/mirror.py


        log.debug("User input: {}".format(result.as_data()))
        if self.has_network and self.last_status in [
            MirrorCheckStatus.RUNNING,
            MirrorCheckStatus.FAILED,
            None,
        ]:
-            async_helpers.run_bg_task(confirm_continue_anyway())
+            status_is_failed = bool(self.last_status == MirrorCheckStatus.FAILED)


nitpick: bool() is redundant

Dropped the bool construct but kept the parenthesis for readability, thanks!

Well... black does not like the parentheses so I guess that's why I used bool() in the first place. Will do status_is_failed = self.last_status == MirrorCheckStatus.FAILED then

mwhudson

Just one (annoying) question but this looks good to me otherwise.

mwhudson · 2024-04-16T22:59:47Z

subiquity/ui/views/mirror.py

-            " likely that the installation will fail."
+            "The check of the mirror URL failed. If you decide to continue, only"
+            " packages present on the installation media will be considered for"
+            " installation."


It might be hard to do in a way that is short enough to be clear but should we say something about the consequences of this, i.e. missing security updates?

I was thinking of

- "The check of the mirror URL failed. You can continue, but it is very" - " likely that the installation will fail." + "The check of the mirror URL failed. If you decide to continue, only" + " packages present on the installation media will be considered for" + " installation. Remember to install security updates after booting" + " your newly installed system."

mwhudson · 2024-04-16T23:00:55Z

subiquity/ui/views/mirror.py

@@ -227,17 +228,19 @@ async def confirm_continue_anyway() -> None:
            )

            if confirmed:
-                self.controller.done(result.url.value)
+                skip_archive = True if continue_skip_archive else None


The True/False/None tristate is a bit confusing but well. One thing at a time.

Sorry about that 🙃

In the mirror screen, if the test fails and the user decides to ignore the failure, we used to continue the installation normally ; which in most scenarios resulted in an error at a later stage of the installation. Instead, we now revert to an installation without network (i.e., only packages from the pool are considered for installation) if the user decides to ignore the failure. Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

ogayot requested a review from dbungert April 11, 2024 09:59

dbungert reviewed Apr 12, 2024

View reviewed changes

ogayot mentioned this pull request Apr 12, 2024

mirror: switch default policy to OFFLINE_INSTALL by default #1971

Merged

ogayot requested review from dbungert and Chris-Peterson444 April 16, 2024 09:43

Chris-Peterson444 approved these changes Apr 16, 2024

View reviewed changes

dbungert approved these changes Apr 16, 2024

View reviewed changes

dbungert requested a review from mwhudson April 16, 2024 22:24

mwhudson approved these changes Apr 16, 2024

View reviewed changes

ogayot force-pushed the server-ignore-mirror-test-offline branch from ca71e23 to 0bb4915 Compare April 17, 2024 07:57

ogayot merged commit aaf0d56 into canonical:main Apr 17, 2024
10 checks passed

ogayot deleted the server-ignore-mirror-test-offline branch April 17, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mirror: if the mirror test fails, suggest an offline install #1968

mirror: if the mirror test fails, suggest an offline install #1968

ogayot commented Apr 11, 2024 •

edited

Loading

ogayot commented Apr 11, 2024 •

edited

Loading

dbungert left a comment

ogayot commented Apr 12, 2024 •

edited

Loading

dbungert Apr 16, 2024

ogayot Apr 17, 2024

ogayot Apr 17, 2024

mwhudson left a comment

mwhudson Apr 16, 2024

ogayot Apr 17, 2024

mwhudson Apr 16, 2024

ogayot Apr 17, 2024

mirror: if the mirror test fails, suggest an offline install #1968

mirror: if the mirror test fails, suggest an offline install #1968

Conversation

ogayot commented Apr 11, 2024 • edited Loading

Client / TUI change

Backend change

ogayot commented Apr 11, 2024 • edited Loading

dbungert left a comment

Choose a reason for hiding this comment

ogayot commented Apr 12, 2024 • edited Loading

dbungert Apr 16, 2024

Choose a reason for hiding this comment

ogayot Apr 17, 2024

Choose a reason for hiding this comment

ogayot Apr 17, 2024

Choose a reason for hiding this comment

mwhudson left a comment

Choose a reason for hiding this comment

mwhudson Apr 16, 2024

Choose a reason for hiding this comment

ogayot Apr 17, 2024

Choose a reason for hiding this comment

mwhudson Apr 16, 2024

Choose a reason for hiding this comment

ogayot Apr 17, 2024

Choose a reason for hiding this comment

ogayot commented Apr 11, 2024 •

edited

Loading

ogayot commented Apr 11, 2024 •

edited

Loading

ogayot commented Apr 12, 2024 •

edited

Loading