Currently, the partition form stores the size as a human readable value.
(e.g., 123456K, 1.1G, 1.876G, 100G). When we exit the size field (i.e., upon
losing focus), we convert the value to a number of bytes and then align
it up to the nearest MiB (or whatever the alignment requirement is).
Unfortunately, after computing the aligned value, we turn it back into a
human-readable string and store it as is. It is not okay because the
conversion does not ensure that the alignment requirement is still
honored.
For instance, if the user types in 1.1G, we do the following:
* convert it to a number of bytes -> 1181116006.4 (surprise, it is not
even an integer).
* round it up to the nearest MiB -> 1181745152 (this is the correct
value)
* transform it into a human readable string and store it as is -> 1.1G
- which actually corresponds to the original value.
This leads to an exception later when creating the partition:
File "subiquity/models/filesystem.py", line 1841, in add_partition
raise Exception(
Exception: ('size %s or offset %s not aligned to %s', 1181116006, 1048576, 1048576)
Fixed by storing the actual size as a number of bytes - alongside the
human readable size.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Move some of this logic to a common util, so it can be used in
unittests. The curtin version is close but expects to work on a storage
config, which is not a flat list.
LD_LIBRARY_PATH is set earlier than some of the other environment
variables like PATH or PYTHONPATH, so trying to save a clean version in
snapcraft is not viable. Remove it here.
In LP: 2009797, an exception of this form happens:
AttributeError: 'FilesystemController' object has no attribute '_start_task'
The installer client, u-d-i, is asking for storage information ASAP
after the socket starts listening, and in this case that happened before
all controllers were started. The sync primitive the probe is waiting
on wasn't created yet.
With one known exception, /meta/status, we really shouldn't be
responding to random API calls, and the startup sequence of the
controllers should be relatively quick (sub 1 second to be sure).
Just delay them, except for the special one.
SingleInstanceTask has distinct steps for creation of the object, and
starting the task. If a different coroutine is waiting on the
SingleInstanceTask, it isn't safe to directly call
SingleInstanceTask.wait() as the task may or may not have been created
yet.
Existing code usage of SingleInstanceTask is in 4 categories, with
reguards to SingleInstanceTask.wait():
1) using SingleInstanceTask without using SingleInstanceTask.wait().
This is unchanged.
2) using SingleInstanceTask.wait without a check on task is not None.
This may be safe now, but is fragile in the face of innocent-looking
refactors around the SingleInstanceTask.
3) using SingleInstanceTask.wait after confirming that the task is not
None. This is fine but a leaky abstraction.
4) directly waiting on the SingleInstanceTask.task. Another leaky
abstraction, but it's solving a cancellation problem. Leaving this
alone.
By enhancing SingleInstanceTask.wait(), cases 2 and 3 are improved. The
code not checking the task today is made safer, and the code checking
the task today can be simplified.
Network manager can create routes at metric aka priority above 20000.
These can stick around if they are not the best choice, or they may
disappear quickly.
Do not consider one of these routes as a valid default route for
has_network purposes.
The existing event based method of watching for has_network has a flaw.
The incoming route_change events from probert do not distinguish routes
on the same interface but a different metric, so if 2 routes on one
interface appear, we only get one event. Then if one of those routes is
removed, we will inappropriately remove this route from the
default_routes list.
Aside from the code watching the event stream, the set of default routes
is an elaborate boolean value.
Simplify the code by passing around a boolean, and when we get a
route_change event, use that to go looking again at the list of default
routes.
LP: #2004659
parameterized async tests run into cpython bug gh-101486
We use parameterized.expand, which works by creating new functions and
inserting those into the list of tests. It's a wonder this worked at
all before for the async tests.
Update the function generation to create a coroutine function, if
appropriate.
LP: #2007554
create_task has the following note:
Important: Save a reference to the result of this function, to avoid a
task disappearing mid-execution.
Convert existing usage of create_task to run_bg_task, if that
create_task is not actually storing the result.
We used to set LC_ALL=C unconditionally when executing a command. This
is a problem if we want the output of a specific command to be
translated (provided a locale definition and an actual translation exist
for the requested language).
This patch adds the clean_locale parameter, which can be used to specify
if we want the locale variable to be cleaned, to most of the helpers
that execute commands.
The value defaults to True so the default behavior is preserved.
If one wants a command to be translated, they have to explicitly pass
clean_locale=False.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
calling asyncio.create_task(...) without storing a reference to the
result can lead to the task being garbage collected before it actually
executed.
https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task
The documentation gives an example of a reliable way to run
fire-and-forget background tasks.
This patch adds an helper to do exactly that.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
The new ConfirmationOverlay object along with the
BaseView.ask_confirmation helper can be used to open a confirmation
dialog and get back the decision from the user.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Specifying the value of subprocess.PIPE as the stdin argument of
astart_command did not have any effect. This happened because the
parameter was not forwarded to the subprocess function. The parameter
was effectively unused.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Attempting to close an overlay that does not exist is pretty much always
a bug in the code. Making not_found_ok true by default will hide obvious
bugs from us ; which is not a good thing. Perhaps more importantly, we
might just remove the wrong overlay.
Instead, we should just pass not_found_ok=True as a workaround when we
know the code is buggy and don't have time to fix the bug cleanly.
This is what happens for SSH keys import. If the import fails, we remove
the loading animation. However for answers-based runs, we do not have a
loading animation so the code bails. Let's add not_found_ok=True in this
context and we can fix the code later.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
BaseView.remove_overlay() would crash with AttributeError if no overlay
was found. We now add a not_found_ok parameter (defaulting to True) that
makes the function silently return if the overlay could not be found.
Passing not_found_ok=False and catching OverlayNotFoundError can be
helpful in some scenarios to do something different if no overlay was
found.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
I implemented these a long time ago to help working on parts of the ui
by allowing a way to script moving past some screens. I haven't used
them in ages, it's pretty bad code and probably a fragmentary answers
file is a better solution to the same problem. What do you think?
When executing a command via arun_command with check=True, we forge
and then raise a CalledProcessError exception if the command exits
abnormally (i.e., exit code != 0).
When doing so, we only instantiate the exception with the exit code and
the command executed. This means that we lose access to any output
captured so far. This is usually fine for stdout but stderr oftentimes
contains invaluable information to understand what caused the command to
exit abnormally.
Back in Python 3.5, stdout and stderr were introduced as new attributes
for CalledProcessError.
We now also include stdout and stderr in the CalledProcessError
instances that we forge. This allows us to access stderr (if any) when
catching the exception with:
try:
...
except CalledProcessError as exc:
print(exc.stderr)
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
asyncio.create_task() calls asyncio.get_running_loop() under the hood so
there is no need to call get_running_loop() ourselves if the sole
purpose is to create a task.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
The behavior of asyncio.get_event_loop() will change in a future Python
version. It is deprecated starting Python 3.10.
The functions that we can use instead are:
* asyncio.new_event_loop() - which creates a new event loop
* asyncio.get_running_loop() - which returns the event loop only if it
is already running
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
We now have the view display the user-code fetched via u-a-c and
automatically validate the contract token when the contract selection
succeeds.
If the magic token expires (i.e., u-a-c times out), a new contract
selection is initiated.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Tests that use run_coro are at risk of being broken by the introduction
of another test using IsolatedAsyncioTestCase. Switch over to only use
IsolatedAsyncioTestCase.
Instead of using isinstance(self.widget, SubFormWidget) in
BoundFormField.is_in_error, we now implement a BoundSubFormField
class that inherits from BoundFormField. It is meant to be instantianted
for subforms fields only.
This class overrides is_in_error() to perform the additional checks in
the forms' sub-fields.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
When the validation of a field fails in a form, we disable the Done
button. With child forms, however, it did not work because they have
their own set of hidden buttons ; that are not the ones the users
interacts with.
This patch makes parent forms recurse on the child forms when checking
if any field is in error. Also, when a child form undertakes validation,
it now propagates to the parent, so that the done button can be
immediately enabled/disabled.
Having a validation error in a child form that is not currently enabled
should not prevent the user from moving forward, so we disable recursion
for child forms that are disabled.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
cancel_restart is a mode for SingleInstanceTask that changes the
behavior when starting the task - if the task is already running, do not
cancel it to start another.
We used to rely on the narrow non-breakable space to be displayed as a
star in basic mode. This is not great and could impact other screens.
We now make use of two check-marks (each replaced by a star in basic
mode) and mask one of them in rich mode using display attributes.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
The "**" signs replace the check-mark for a verified publisher when in
basic mode.
These "**" signs where still used green foreground color. This was the
only place where we tried to display colors in basic mode.
Keep the default colors instead. This should be more compatible.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
In addition to verified publishers being indicated by a check-mark, we
now have starred publishers indicated with a circled star.
If unicode support is not available, for instance with serial
connections, we use a different number of stars to represent:
* verified publishers: 2 stars
* starred publishers: 1 star
* others: no star
Because our mechanism to substitute unicode characters with ascii
equivalents expect a 1:1 mapping, we cannot simply replace the circled
start by two stars. To workaround the issue, we added a narrow
non-breakable space.
When support of unicode is available, this character shows up as a
normal space.
When support of unicode is not available, it gets replaced by a star.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
The following commit added an unconditional return statement in the try
block of _move_screen, effectively making the associated else block
unreachable.
a7bcc7fa add a way to wait for something with notification after 0.1s
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
The make_ui() function / coroutine returns a BaseView (i.e., a
screen to display to the user).
That being said, when the application calls, make_ui(), it does not come
with a guarantee that the view returned will be displayed to the user
immediately.
One of the reason is that there are multiple await statements before the
we call the ui.set_body function. Therefore, tasks running concurrently
cannot reliably expect that they execute after the display is refreshed.
Perhaps more importantly, when the make_ui() function takes more than .1
second to execute, we display a "Progress" screen that stays visible for
at least one second. This can effectively delay a lot the moment when
the view returned by make_ui() is shown to the user. A lot can happen in
the meantime.
As the result, the view returned by make_ui can be outdated by the time
we show it on the screen.
One way to work around this problem is to store in the controller a
reference to the view that it returns in make_ui(). This way, the
controller can modify the view and keep it up-to-date until it gets
shown to the user.
Unfortunately, some controllers (e.g., the storage controller) do not
modify / mutate the existing view object when a modification is needed ;
but instead instantiate a new view object.
This patch introduces a level of indirection that can be used by these
controllers. Instead of returning a view object from make_ui(), the
controllers are now allowed to return a callback ; which in turn will
return a view object.
https://bugs.launchpad.net/subiquity/+bug/1968161
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
We used to only accept lists of strings for commands. We now accept
sequences of strings instead ; which are lists of strings or tuple of
strings.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
After calling .communicate() on an asyncio.subprocess.Process object,
the attribute returncode gets set to a non-None value. Type checkers are
not able to figure this out.
Fixed by adding an assert to help type checkers out.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
If you run dryrun for console-conf, go to the network page, go to an
interface, then info, a crash of the form
TypeError: object str can't be used in 'await' expression
can be seen. The signature for the core version of get_info_for_netdev
is not async, but a non-async method returning str.
So mark the core version of the function async.
Co-authored-by: Michael Hudson-Doyle <michael.hudson@canonical.com>
Create open_perms context manager for custom or multiple writes.
Create generate_config for a small removal of redundancy.
0640 root:adm the resulting files.
Olivier Gayot