Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: restricted probes must gather fs info #1636

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

dbungert
Copy link
Collaborator

@dbungert dbungert commented Apr 4, 2023

In ubuntu-desktop-installer/issues/1772, a user formatted an existing partition as ext4, which triggered the ESP to be "mounted". Except this mount step also formatted the ESP.

Previously, when we ran block probing, the restricted=False version failed, which caused it to run a restricted=True probe. The restricted=True probe looks at block devices, but does not gather filesystem information. The esp "mount" check is also willing to format the partition if it is unformatted. (We do not have a distinction between a partition that is unformatted and one for which we have not obtained filesystem information.)

This user had block probing restricted=False fail due to LP: #2012722, where Ventoy was in use and we handled the device mapper entry poorly. While recreating the failure case, simply retesting with a build with the fix for LP: #2012722 is enough to avoid this problem. This is because the restricted=False probe passes, which means filesystem info is gathered, which means the mount step doesn't attempt to format it.

There will inevitably be other block probing failure cases for restricted=False. restricted=True must not leave people so vulnerable to a partition reformat.

Gather filesystem information during a restricted=True probe.

In ubuntu-desktop-installer/issues/1772, a user formatted an existing
partition as ext4, which triggered the ESP to be "mounted".  Except this
mount step also formatted the ESP.

Previously, when we ran block probing, the restricted=False version
failed, which caused it to run a restricted=True probe.  The
restricted=True probe looks at block devices, but does not gather
filesystem information.  The esp "mount" check is also willing to format
the partition if it is unformatted.  (We do not have a distinction
between a partition that is unformatted and one for which we have not
obtained filesystem information.)

This user had block probing restricted=False fail due to LP: #2012722,
where Ventoy was in use and we handled the device mapper entry poorly.
While recreating the failure case, simply retesting with a build with
the fix for LP: #2012722 is enough to avoid this problem.  This is
because the restricted=False probe passes, which means filesystem info
is gathered, which means the mount step doesn't attempt to format it.

There will inevitably be other block probing failure cases for
restricted=False.  restricted=True must not leave people so vulnerable
to a partition reformat.

Gather filesystem information during a restricted=True probe.
Copy link
Member

@ogayot ogayot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job figuring out the exact sequence of events! Do you expect a big impact on the time spent probing when restricted=True? From the data I collected yesterday, it feels like os and filesystem_sizing probes are the ones that are the slowest (by far?); and they're both disabled when restricted=True. So I'm not too worried?

@dbungert
Copy link
Collaborator Author

dbungert commented Apr 5, 2023

Good job figuring out the exact sequence of events!

Thanks!

Do you expect a big impact on the time spent probing when restricted=True?

I don't. Have a look at probert/filesystem.py, the code path where we aren't gathering resizing data. I think it's low risk and expect that it's quick.

From the data I collected yesterday, it feels like os and filesystem_sizing probes are the ones that are the slowest (by far?);

I was also assuming those to be the slowest.

@dbungert dbungert merged commit 70a2a66 into canonical:main Apr 5, 2023
@dbungert dbungert deleted the udi-gh-1772 branch April 5, 2023 13:04
@dbungert dbungert mentioned this pull request Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants