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

Make boot not required 2 #5381

Merged
merged 2 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions pyanaconda/modules/common/structures/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -450,16 +450,16 @@ def get_root_device(self):
return self.mount_points.get("/")


class RequiredMountPointData(DBusData):
"""Constrains (filesystem and device types allowed) for mount points required
for the installed system
"""
class MountPointConstraintsData(DBusData):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the new name!

"""Constrains (filesystem and device types allowed) for mount points"""

def __init__(self):
self._mount_point = ""
self._required_filesystem_type = ""
self._encryption_allowed = False
self._logical_volume_allowed = False
self._required = False
self._recommended = False

@property
def mount_point(self) -> Str:
Expand Down Expand Up @@ -508,3 +508,27 @@ def logical_volume_allowed(self) -> Bool:
@logical_volume_allowed.setter
def logical_volume_allowed(self, logical_volume_allowed: Bool):
self._logical_volume_allowed = logical_volume_allowed

@property
def required(self) -> Bool:
"""Whether this mount point is required

:return: bool
"""
return self._required

@required.setter
def required(self, required: Bool):
self._required = required

@property
def recommended(self) -> Bool:
"""Whether this mount point is recommended

:return: bool
"""
return self._recommended

@recommended.setter
def recommended(self, recommended: Bool):
self._recommended = recommended
56 changes: 52 additions & 4 deletions pyanaconda/modules/storage/devicetree/viewer.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from pyanaconda.core.i18n import _
from pyanaconda.modules.common.errors.storage import UnknownDeviceError
from pyanaconda.modules.common.structures.storage import DeviceData, DeviceActionData, \
DeviceFormatData, OSData, RequiredMountPointData
DeviceFormatData, OSData, MountPointConstraintsData
from pyanaconda.modules.storage.devicetree.utils import get_required_device_size, \
get_supported_filesystems
from pyanaconda.modules.storage.platform import platform
Expand Down Expand Up @@ -462,13 +462,61 @@ def _get_os_data(self, root):
}
return data

def _get_mount_point_constraints_data(self, spec):
"""Get the mount point data.

:param spec: an instance of PartSpec
:return: an instance of MountPointConstraintsData
"""
data = MountPointConstraintsData()
data.mount_point = spec.mountpoint or ""
data.required_filesystem_type = spec.fstype or ""
data.encryption_allowed = spec.encrypted
data.logical_volume_allowed = spec.lv

return data

def get_mount_point_constraints(self):
"""Get list of constraints on mountpoints for the current platform

Also provides hints if the partition is required or recommended.

This includes mount points required to boot (e.g. /boot/efi, /boot)
and the / partition which is always considered to be required.

/boot is not required in general but can be required in some cases,
depending on the filesystem on the root partition (ie crypted root).

:return: a list of mount points with its constraints
"""

constraints = []

# Root partition is required
root_partition = PartSpec(mountpoint="/", lv=True, thin=True, encrypted=True)
root_constraint = self._get_mount_point_constraints_data(root_partition)
root_constraint.required = True
constraints.append(root_constraint)

# Platform partitions are required except for /boot partiotion which is recommended
for p in platform.partitions:
if p.mountpoint:
constraint = self._get_mount_point_constraints_data(p)
if p.mountpoint == "/boot":
constraint.recommended = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this shouldn't be actually defined in the Storage Constraints section of our configuration files. That section already contains other constraints like this one and it might be useful for Web UI and Cockpit Storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to me, but I hesitated to use configuration files perhaps because of the outlook of more dynamic API (like /boot being required depending on fs on /). Nevertheless the static information (/boot (a mount point) required or recommended in general) can still be in the configuration file.

Copy link
Contributor Author

@rvykydal rvykydal Dec 15, 2023

Choose a reason for hiding this comment

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

Something like

boot_partition_recommended = True

or even (not sure if it makes sense in this form) ?

recommended_mount_points = /boot
required_mount_points = /

Well, required_mount_points seems to be covered by existing must_not_be_on_root
Maybe we should wait for more use cases / requirements on required.
Actually, recommended_mount_points might become useful also for /home - seems like a good setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@poncovka would you be OK with handling it as a follow-up? We are dealing with test flakiness on webui part and it could be handy to have this merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want an issue about considering this followup? If this can be handled this year, I would not create one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it is definitely just a suggestion. I didn't want to block this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please create the issue.

else:
constraint.required = True
constraints.append(constraint)

return constraints

def _get_platform_mount_point_data(self, spec):
"""Get the mount point data.

:param spec: an instance of PartSpec
:return: an instance of RequiredMountPointData
:return: an instance of MountPointConstraintsData
"""
data = RequiredMountPointData()
data = MountPointConstraintsData()
data.mount_point = spec.mountpoint or ""
data.required_filesystem_type = spec.fstype or ""
data.encryption_allowed = spec.encrypted
Expand All @@ -482,7 +530,7 @@ def get_required_mount_points(self):
This includes mount points required to boot (e.g. /boot and /boot/efi)
and the / partition which is always considered to be required.

:return: a list of mount points
:return: a list of mount points with its constraints
"""
root_partition = PartSpec(mountpoint="/", lv=True, thin=True, encrypted=True)
ret = list(map(self._get_platform_mount_point_data,
Expand Down
20 changes: 17 additions & 3 deletions pyanaconda/modules/storage/devicetree/viewer_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from dasbus.typing import * # pylint: disable=wildcard-import
from pyanaconda.modules.common.constants.interfaces import DEVICE_TREE_VIEWER
from pyanaconda.modules.common.structures.storage import DeviceData, DeviceActionData, \
DeviceFormatData, OSData, RequiredMountPointData
DeviceFormatData, OSData, MountPointConstraintsData

__all__ = ["DeviceTreeViewerInterface"]

Expand Down Expand Up @@ -193,13 +193,27 @@ def GetExistingSystems(self) -> List[Structure]:
"""
return OSData.to_structure_list(self.implementation.get_existing_systems())

# FIXME: remove the replaced API
def GetMountPointConstraints(self) -> List[Structure]:
"""Get list of constraints on mountpoints for the current platform

Also provides hints if the partition is required or recommended.

This includes mount points required to boot (e.g. /boot/efi, /boot)
and the / partition which is always considered to be required.

:return: a list of mount points with its constraints
"""
return MountPointConstraintsData.to_structure_list(
self.implementation.get_mount_point_constraints())

def GetRequiredMountPoints(self) -> List[Structure]:
"""Get list of required mount points for the current platform

This includes mount points required to boot (e.g. /boot and /boot/efi)
and the / partition which is always considered to be required.

:return: a list of mount points
:return: a list of mount points with its constraints
"""
return RequiredMountPointData.to_structure_list(
return MountPointConstraintsData.to_structure_list(
self.implementation.get_required_mount_points())
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
from dasbus.typing import * # pylint: disable=wildcard-import
from pyanaconda.core.kernel import KernelArguments
from pyanaconda.modules.common.errors.storage import UnknownDeviceError, MountFilesystemError
from pyanaconda.modules.common.structures.storage import DeviceFormatData, RequiredMountPointData
from pyanaconda.modules.common.structures.storage import DeviceFormatData, \
MountPointConstraintsData
from pyanaconda.modules.storage.devicetree import DeviceTreeModule, create_storage, utils
from pyanaconda.modules.storage.devicetree.devicetree_interface import DeviceTreeInterface
from pyanaconda.modules.storage.devicetree.populate import FindDevicesTask
Expand Down Expand Up @@ -829,13 +830,38 @@ def test_set_device_mount_options(self):
self.interface.SetDeviceMountOptions("dev1", "")
assert dev1.format.options == "defaults"

def test_get_mount_point_constraints(self):
"""Test GetMountPointConstraints."""
result = self.interface.GetMountPointConstraints()
assert isinstance(result, list)
assert len(result) == 2

result = MountPointConstraintsData.from_structure_list(
self.interface.GetMountPointConstraints()
)
for mp in result:
assert mp.mount_point is not None
assert mp.required_filesystem_type is not None

# we are always adding / so it's a good candidate for testing
root = next(r for r in result if r.mount_point == "/")
assert root is not None
assert root.encryption_allowed is True
assert root.logical_volume_allowed is True
assert root.mount_point == "/"
assert root.required_filesystem_type == ""
assert root.required is True
assert root.recommended is False

def test_get_required_mount_points(self):
"""Test GetRequiredMountPoints."""
result = self.interface.GetRequiredMountPoints()
assert isinstance(result, list)
assert len(result) != 0

result = RequiredMountPointData.from_structure_list(self.interface.GetRequiredMountPoints())
result = MountPointConstraintsData.from_structure_list(
self.interface.GetRequiredMountPoints()
)
for mp in result:
assert mp.mount_point is not None
assert mp.required_filesystem_type is not None
Expand Down