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.

Sign up for GitHub

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

Jump to bottom

Add support for managing pool members #264

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Add support for managing pool members #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df05f06
Add support for managing pool members
vojtechtrefny Apr 21, 2022
63eb18c
Do not allow removing members from existing pools in safe mode
vojtechtrefny May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
122 changes: 93 additions & 29 deletions library/blivet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
from blivet3.callbacks import callbacks
from blivet3 import devicelibs
from blivet3 import devices
from blivet3.deviceaction import ActionConfigureFormat
from blivet3.deviceaction import ActionConfigureFormat, ActionAddMember, ActionRemoveMember
from blivet3.devicefactory import DEFAULT_THPOOL_RESERVE
from blivet3.flags import flags as blivet_flags
from blivet3.formats import get_format
Expand All @@ -135,7 +135,7 @@
from blivet.callbacks import callbacks
from blivet import devicelibs
from blivet import devices
from blivet.deviceaction import ActionConfigureFormat
from blivet.deviceaction import ActionConfigureFormat, ActionAddMember, ActionRemoveMember
from blivet.devicefactory import DEFAULT_THPOOL_RESERVE
from blivet.flags import flags as blivet_flags
from blivet.formats import get_format
Expand Down Expand Up @@ -1163,32 +1163,43 @@ def _apply_defaults(self):
if self._pool.get(name) is None:
self._pool[name] = default

def _create_one_member(self, disk):
if not disk.isleaf or disk.format.type is not None:
if safe_mode and disk.format.name != get_format(None).name:
raise BlivetAnsibleError(
"cannot remove existing formatting (%s) and/or devices on disk '%s' (pool '%s') in safe mode" %
(disk.format.type, disk.name, self._pool['name'])
)
else:
self._blivet.devicetree.recursive_remove(disk)

if use_partitions:
label = get_format("disklabel", device=disk.path)
self._blivet.format_device(disk, label)
member = self._blivet.new_partition(parents=[disk], size=Size("256MiB"), grow=True)
richm marked this conversation as resolved.
Show resolved Hide resolved
self._blivet.create_device(member)
else:
member = disk

if self._is_raid:
self._blivet.format_device(member, fmt=get_format("mdmember"))
else:
self._blivet.format_device(member, self._get_format())

if use_partitions:
try:
do_partitioning(self._blivet)
except Exception:
raise BlivetAnsibleError("failed to allocate partitions for pool '%s'" % self._pool['name'])

return member

def _create_members(self):
""" Schedule actions as needed to ensure pool member devices exist. """
members = list()

for disk in self._disks:
if not disk.isleaf or disk.format.type is not None:
if safe_mode and disk.format.name != get_format(None).name:
raise BlivetAnsibleError(
"cannot remove existing formatting and/or devices on disk '%s' (pool '%s') in safe mode" %
(disk.name, self._pool['name'])
)
else:
self._blivet.devicetree.recursive_remove(disk)

if use_partitions:
label = get_format("disklabel", device=disk.path)
self._blivet.format_device(disk, label)
member = self._blivet.new_partition(parents=[disk], size=Size("256MiB"), grow=True)
self._blivet.create_device(member)
else:
member = disk

if self._is_raid:
self._blivet.format_device(member, fmt=get_format("mdmember"))
else:
self._blivet.format_device(member, self._get_format())
member = self._create_one_member(disk)
members.append(member)

if self._is_raid:
Expand All @@ -1201,12 +1212,6 @@ def _create_members(self):
else:
result = members

if use_partitions:
try:
do_partitioning(self._blivet)
except Exception:
raise BlivetAnsibleError("failed to allocate partitions for pool '%s'" % self._pool['name'])

return result

def _get_volumes(self):
Expand All @@ -1221,6 +1226,9 @@ def _manage_volumes(self):
for bvolume in self._blivet_volumes:
bvolume.manage()

def _manage_members(self):
""" Schedule actions as needed to configure this pool's members. """

def manage(self):
""" Schedule actions to configure this pool according to the yaml input. """
global safe_mode
Expand All @@ -1242,6 +1250,10 @@ def manage(self):

# schedule create if appropriate
self._create()

if self._device:
self._manage_members()

self._manage_volumes()


Expand Down Expand Up @@ -1387,6 +1399,58 @@ def get_vol_size(volume):

return existing_thinlvs_obj + new_thinlvs_obj

def _manage_members(self):
""" Schedule actions as needed to configure this pool's members. """
if not self._device:
return

add_disks = [d for d in self._disks if d not in self._device.ancestors]
remove_disks = [pv for pv in self._device.pvs if not any(d in pv.ancestors for d in self._disks)]

if not (add_disks or remove_disks):
return

if remove_disks and safe_mode:
raise BlivetAnsibleError("cannot remove members '%s' from pool '%s' in safe mode" %
(", ".join(d.name for d in remove_disks),
self._device.name))

if self._is_raid:
raise BlivetAnsibleError("managing pool members is not supported with RAID")

for disk in add_disks:
member = self._create_one_member(disk)
member = self._manage_one_encryption(member)

try:
ac = ActionAddMember(self._device, member)
self._blivet.devicetree.actions.add(ac)
except Exception as e:
raise BlivetAnsibleError("failed to add disk '%s' to pool '%s': %s" % (disk.name,
self._pool['name'],
str(e)))

for disk in remove_disks:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Question: Should we consider removing the PV unsafe? Currently the code will remove the PV from the VG even with the safe mode on. This isn't a destructive operation, but I am also removing the PV from the disk after that and that might be potentially dangerous. And it also might not be user friendly to remove the PV just because user specified incomplete disks by accident.

Copy link

Choose a reason for hiding this comment

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

In my opinion, it makes sense to require storage_safe_mode = false to remove PV's from a VG. The storage_safe_mode description in the README states that "When true (the default), an error will occur instead of automatically removing existing devices and/or formatting.", and it seems that this operation would fall under "removing existing devices".

Does the role fail if the user tries to remove a PV from a VG and the PV has data on it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The storage_safe_mode description in the README states that "When true (the default), an error will occur instead of automatically removing existing devices and/or formatting.", and it seems that this operation would fall under "removing existing devices".

Thanks, I'll change the code to require safe mode to be turned off for this operation.

Does the role fail if the user tries to remove a PV from a VG and the PV has data on it?

If there is enough free space in the VG it will run pvmove to move the LV to an empty PV but it will return error if there isn't enough free space.

if self._device.free_space < disk.size:
raise BlivetAnsibleError("disk '%s' cannot be removed from pool '%s'" % (disk.name,
self._pool['name']))

try:
ac = ActionRemoveMember(self._device, disk)
# XXX: scheduling ActionRemoveMember is currently broken, we need to execute
# the action now manually, see https://bugzilla.redhat.com/show_bug.cgi?id=2076956
# self._blivet.devicetree.actions.add(ac)
ac.apply()
ac.execute()
except Exception as e:
raise BlivetAnsibleError("failed to remove disk '%s' from pool '%s': %s" % (disk.name,
self._pool['name'],
str(e)))
# XXX workaround for https://github.com/storaged-project/blivet/pull/1040
disk.format.vg_name = None

self._blivet.devicetree.recursive_remove(disk.raw_device)

def _create(self):
if not self._device:
members = self._manage_encryption(self._create_members())
Expand Down