[PATCH 5/5] Add error handling around significant ui-initiated storage operations.

David Lehman dlehman at redhat.com
Thu Sep 6 22:02:58 UTC 2012


If automatic partitioning fails, we reset the storage instance to its
prior state. The custom spoke operates on a copy of the storage
instance, so that means it gets reset to whatever the state was upon
entering the custom spoke. For the storage spoke we have little choice
but to actually call self.storage.reset() which actually re-scans the
devices.

For other operations within the custom spoke, the goal is to restore
things to the state prior to the failed action. If that fails, the
custom spoke will simply reset its storage instance as described above
for automatic partitioning failures.
---
 pyanaconda/storage/__init__.py      |  167 +++++++++++++++++++++++++++++++----
 pyanaconda/storage/devices.py       |   13 +++
 pyanaconda/storage/errors.py        |    3 +
 pyanaconda/storage/partitioning.py  |   23 +++---
 pyanaconda/ui/gui/spokes/custom.py  |  135 ++++++++++++++++++++++------
 pyanaconda/ui/gui/spokes/storage.py |   18 ++++-
 6 files changed, 299 insertions(+), 60 deletions(-)

diff --git a/pyanaconda/storage/__init__.py b/pyanaconda/storage/__init__.py
index 64ecffc..8ba1cac 100644
--- a/pyanaconda/storage/__init__.py
+++ b/pyanaconda/storage/__init__.py
@@ -1853,6 +1853,7 @@ class Storage(object):
             member.req_grow = True
 
         # set up new members as needed to accommodate the device
+        new_members = []
         for disk in add_disks:
             if factory.encrypted:
                 luks_format = factory.member_format
@@ -1860,8 +1861,13 @@ class Storage(object):
             else:
                 member_format = factory.member_format
 
-            member = self.newPartition(parents=[disk], grow=True,
-                                       fmt_type=member_format)
+            try:
+                member = self.newPartition(parents=[disk], grow=True,
+                                           fmt_type=member_format)
+            except StorageError as e:
+                log.error("failed to create new member partition: %s" % e)
+                continue
+
             self.createDevice(member)
             if factory.encrypted:
                 fmt = getFormat(luks_format)
@@ -1870,6 +1876,7 @@ class Storage(object):
                 self.createDevice(member)
 
             members.append(member)
+            new_members.append(member)
             if container:
                 container.addMember(member)
 
@@ -1880,7 +1887,14 @@ class Storage(object):
         for member in members:
             member.req_max_size = size_set.size
 
-        self.allocatePartitions()
+        try:
+            self.allocatePartitions()
+        except PartitioningError as e:
+            # try to clean up by destroying all newly added members before re-
+            # raising the exception
+            self.__cleanUpMemberDevices(new_members, container=container)
+            raise
+
         return members
 
     def allocatePartitions(self):
@@ -1888,8 +1902,7 @@ class Storage(object):
         try:
             doPartitioning(self)
         except StorageError as e:
-            log.error("UI: failed to allocate partitions: %s" % e)
-            # FIXME: revert container to its original state?
+            log.error("failed to allocate partitions: %s" % e)
             raise
 
     def getDeviceFactory(self, device_type, size, **kwargs):
@@ -1921,6 +1934,17 @@ class Storage(object):
 
         return container
 
+    def __cleanUpMemberDevices(self, members, container=None):
+        for member in members:
+            if container:
+                container.removeMember(member)
+
+            if isinstance(member, LUKSDevice):
+                self.destroyDevice(member)
+                member = member.slave
+
+            self.destroyDevice(member)
+
     def newDevice(self, device_type, size, **kwargs):
         """ Schedule creation of a device based on a top-down specification.
 
@@ -1942,6 +1966,22 @@ class Storage(object):
                 device              an already-defined but non-existent device
                                     to adjust instead of creating a new device
 
+
+            Error handling:
+
+                If device is None, meaning we're creating a device, the error
+                handling aims to remove all evidence of the attempt to create a
+                new device by removing unused container devices, reverting the
+                size of container devices that contain other devices, &c.
+
+                If the device is not None, meaning we're adjusting the size of
+                a defined device, the error handling aims to revert the device
+                and any container to it previous size.
+
+                In either case, we re-raise the exception so the caller knows
+                there was a failure. If we failed to clean up as described above
+                we raise ErrorRecoveryFailure to alert the caller that things
+                will likely be in an inconsistent state.
         """
         mountpoint = kwargs.get("mountpoint")
         fstype = kwargs.get("fstype")
@@ -1980,12 +2020,44 @@ class Storage(object):
         if device and device.type == "mdarray":
             members = device.parents
 
-        parents = self.setContainerMembers(container, factory, members=members)
+        try:
+            parents = self.setContainerMembers(container, factory,
+                                               members=members)
+        except PartitioningError as e:
+            # If this is a new device, just clean up and get out.
+            if device:
+                # If this is a defined device, try to clean up by reallocating
+                # members as before and then get out.
+                factory.disks = device.disks
+                factory.size = device.size  # this should work
+
+                if members:
+                    # If this is an md array we have to reset its member set
+                    # here.
+                    # If there is a container device, its member set was reset
+                    # in the exception handler in setContainerMembers.
+                    device.parents = members
+
+                try:
+                    self.setContainerMembers(container, factory,
+                                             members=members)
+                except StorageError as e:
+                    log.error("failed to revert device size: %s" % e)
+                    raise ErrorRecoveryFailure("failed to revert container")
+
+            raise
 
         # set up container
         if not container and factory.new_container_attr:
             log.debug("creating new container")
-            container = factory.new_container(parents=parents)
+            try:
+                container = factory.new_container(parents=parents)
+            except StorageError as e:
+                log.error("failed to create new device: %s" % e)
+                # Clean up by destroying the newly created member devices.
+                self.__cleanUpMemberDevices(parents)
+                raise
+
             self.createDevice(container)
 
         if container:
@@ -1997,22 +2069,79 @@ class Storage(object):
         if device:
             # We are adjusting the size of a device. The StorageDevice instance
             # exists, but the underlying device does not.
-            factory.post_create()
-            if not device.size:
-                raise StorageError("failed to adjust device")
+            old_size = device.size
+            e = None
+            try:
+                factory.post_create()
+            except StorageError as e:
+                log.error("device post-create method failed: %s" % e)
+            else:
+                if not device.size:
+                    e = StorageError("failed to adjust device")
+
+            if e:
+                # Clean up by reverting the device to its previous size.
+                factory.size = old_size
+                factory.set_device_size(container, device=device)
+                try:
+                    factory.post_create()
+                except StorageError as e:
+                    # yes, we're replacing e here.
+                    log.error("failed to revert device size: %s" % e)
+                    raise ErrorRecoveryFailure("failed to revert device size")
+
+                raise(e)
         elif factory.new_device_attr:
             log.debug("creating new device")
-            device = factory.new_device(parents=parents,
-                                        size=size,
-                                        fmt_type=fstype,
-                                        mountpoint=mountpoint,
-                                        fmt_args=fmt_args)
+            try:
+                device = factory.new_device(parents=parents,
+                                            size=size,
+                                            fmt_type=fstype,
+                                            mountpoint=mountpoint,
+                                            fmt_args=fmt_args)
+            except StorageError as e:
+                log.error("device instance creation failed: %s" % e)
+                # Clean up. If there is a container and it has other devices,
+                # try to revert it. If there is a container and it has no other
+                # devices, remove it. If there is not a container, remove all of
+                # the parents.
+                if container:
+                    if container.kids:
+                        factory.size = 0
+                        factory.disks = container.disks
+                        try:
+                            self.setContainerMembers(container, factory)
+                        except StorageError as e:
+                            log.error("failed to revert container: %s" % e)
+                            raise ErrorRecoveryFailure("failed to revert container")
+                    else:
+                        self.destroyDevice(container)
+                        self.__cleanUpMemberDevices(container.parents)
+                else:
+                    self.__cleanUpMemberDevices(parents)
+
+                raise
+
             self.createDevice(device)
-            factory.post_create()
-            if not device.size:
-                # FIXME: this is incomplete at best (container, &c)
+            e = None
+            try:
+                factory.post_create()
+            except StorageError as e:
+                log.error("device post-create method failed: %s" % e)
+            else:
+                if not device.size:
+                    e = StorageError("failed to create device")
+
+            if e:
+                # Clean up by destroying members, container, and device.
                 self.destroyDevice(device)
-                raise StorageError("failed to create device")
+                members = device.parents
+                if container:
+                    self.destroyDevice(container)
+                    members = container.parents
+
+                self.__cleanUpMemberDevices(members)
+                raise
 
     def copy(self):
         new = copy.deepcopy(self)
diff --git a/pyanaconda/storage/devices.py b/pyanaconda/storage/devices.py
index 0974455..d01693f 100644
--- a/pyanaconda/storage/devices.py
+++ b/pyanaconda/storage/devices.py
@@ -2213,6 +2213,9 @@ class LVMVolumeGroupDevice(DMDevice):
         # and update our pv count
         self.pvCount = len(self.parents)
 
+    def removeMember(self, member):
+        self._removePV(member)
+
     # We can't rely on lvm to tell us about our size, free space, &c
     # since we could have modifications queued, unless the VG and all of
     # its PVs already exist.
@@ -3987,6 +3990,16 @@ class BTRFSVolumeDevice(BTRFSDevice):
         self.parents.append(member)
         member.addChild()
 
+    def removeMember(self, member):
+        if member not in self.parents:
+            raise ValueError("member is not part of this volume")
+
+        if self.exists:
+            raise DeviceError("cannot remove member from an existing volume")
+
+        self.parents.remove(member)
+        member.removeChild()
+
     def _addSubVolume(self, vol):
         if vol.name in [v.name for v in self.subvolumes]:
             raise ValueError("subvolume %s already exists" % vol.name)
diff --git a/pyanaconda/storage/errors.py b/pyanaconda/storage/errors.py
index 90a9b13..bb936c6 100644
--- a/pyanaconda/storage/errors.py
+++ b/pyanaconda/storage/errors.py
@@ -28,6 +28,9 @@ class StorageError(Exception):
 class NoDisksError(StorageError):
     pass
 
+class ErrorRecoveryFailure(StorageError):
+    pass
+
 # Device
 class DeviceError(StorageError):
     pass
diff --git a/pyanaconda/storage/partitioning.py b/pyanaconda/storage/partitioning.py
index a61b1ce..0c1de17 100644
--- a/pyanaconda/storage/partitioning.py
+++ b/pyanaconda/storage/partitioning.py
@@ -266,23 +266,24 @@ def doAutoPartition(storage, data):
     disks = []
     devs = []
 
-    if storage.doAutoPart:
-        disks = _getCandidateDisks(storage)
-        devs = _scheduleImplicitPartitions(storage, disks)
-        log.debug("candidate disks: %s" % disks)
-        log.debug("devs: %s" % devs)
+    if not storage.doAutoPart:
+        return
+
+    disks = _getCandidateDisks(storage)
+    devs = _scheduleImplicitPartitions(storage, disks)
+    log.debug("candidate disks: %s" % disks)
+    log.debug("devs: %s" % devs)
 
-        if disks == []:
-            raise NoDisksError
+    if disks == []:
+        raise NoDisksError
 
-        _schedulePartitions(storage, disks)
+    _schedulePartitions(storage, disks)
 
     # run the autopart function to allocate and grow partitions
     try:
         doPartitioning(storage)
 
-        if storage.doAutoPart:
-            _scheduleVolumes(storage, devs)
+        _scheduleVolumes(storage, devs)
 
         # grow LVs
         growLVM(storage)
@@ -293,7 +294,6 @@ def doAutoPartition(storage, data):
     except PartitioningError as e:
         log.error(str(e))
         if errorHandler.cb(e) == ERROR_RAISE:
-            storage.reset()
             raise
 
     storage.setUpBootLoader()
@@ -307,7 +307,6 @@ def doAutoPartition(storage, data):
     if errors:
         exn = PartitioningError("\n".join(errors))
         if errorHandler.cb(exn) == ERROR_RAISE:
-            storage.reset()
             raise exn
 
 def partitionCompare(part1, part2):
diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py
index 2604b36..6b33b4f 100644
--- a/pyanaconda/ui/gui/spokes/custom.py
+++ b/pyanaconda/ui/gui/spokes/custom.py
@@ -49,6 +49,8 @@ from pyanaconda.storage import Root
 from pyanaconda.storage.partitioning import doPartitioning
 from pyanaconda.storage.partitioning import doAutoPartition
 from pyanaconda.storage.errors import StorageError
+from pyanaconda.storage.errors import NoDisksError
+from pyanaconda.storage.errors import ErrorRecoveryFailure
 from pyanaconda.storage.devicelibs import mdraid
 
 from pyanaconda.ui.gui import GUIObject
@@ -459,13 +461,15 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
         summaryLabel.set_use_markup(True)
         summaryLabel.set_markup("<span foreground='blue'><u>%s</u></span>" % summary)
 
-    def refresh(self):
-        NormalSpoke.refresh(self)
+    def _reset_storage(self):
         self.__storage = self.storage.copy()
         self.__storage.devicetree._actions = [] # keep things simple
         self._devices = self.__storage.devices
-        self._do_refresh()
 
+    def refresh(self):
+        NormalSpoke.refresh(self)
+        self._reset_storage()
+        self._do_refresh()
         # update our free space number based on Storage
         self._setCurrentFreeSpace()
 
@@ -667,9 +671,13 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
         current_device_type = device_types.get(device.type)
         if current_device_type != device_type:
             # remove the current device
+            self.window.clear_info()
             root = self._current_selector._root
             self._remove_from_root(root, device)
             self._destroy_device(device)
+            if device in self._devices:
+                # the removal failed. don't continue.
+                return
 
             with ui_storage_logger():
                 # XXX skipping encryption for now
@@ -681,10 +689,22 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
                 if container:
                     disks = list(set(disks).union(container.disks))
                 log.debug("disks: %s" % [d.name for d in disks])
-                self.__storage.newDevice(device_type, size, fstype=fs_type,
-                                         disks=disks,
-                                         mountpoint=mountpoint, label=label,
-                                         raid_level=raid_level)
+                try:
+                    self.__storage.newDevice(device_type, size, fstype=fs_type,
+                                             disks=disks,
+                                             mountpoint=mountpoint, label=label,
+                                             raid_level=raid_level)
+                except ErrorRecoveryFailure as e:
+                    self.window.set_info(Gtk.MessageType.ERROR,
+                                         _("Potentially fatal error."))
+                    self.window.show_all()
+                    self._reset_storage()
+                except StorageError as e:
+                    log.error("newDevice failed: %s" % e)
+                    self.window.set_info(Gtk.MessageType.ERROR,
+                                         _("Device configuration failed."))
+                    self.window.show_all()
+
                 self._devices = self.__storage.devices
 
             # newest device should be the one with the highest id
@@ -701,18 +721,36 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
 
         # new size means resize for existing devices and adjust for new ones
         if int(size) != int(device.size):
+            self.window.clear_info()
+            old_size = device.size
             if device.exists and device.resizable:
                 with ui_storage_logger():
-                    self.__storage.resizeDevice(device, size)
-                    log.debug("%r" % device)
-                    log.debug("new size: %s" % device.size)
-                    log.debug("target size: %s" % device.targetSize)
+                    try:
+                        self.__storage.resizeDevice(device, size)
+                    except StorageError as e:
+                        log.error("failed to schedule device resize: %s" % e)
+                        device.size = old_size
+                    else:
+                        log.debug("%r" % device)
+                        log.debug("new size: %s" % device.size)
+                        log.debug("target size: %s" % device.targetSize)
             else:
                 with ui_storage_logger():
-                    self.__storage.newDevice(device_type, size,
-                                             device=device,
-                                             disks=device.disks,
-                                             raid_level=raid_level)
+                    try:
+                        self.__storage.newDevice(device_type, size,
+                                                 device=device,
+                                                 disks=device.disks,
+                                                 raid_level=raid_level)
+                    except ErrorRecoveryFailure as e:
+                        self.window.set_info(Gtk.MessageType.ERROR,
+                                             _("Potentially fatal error."))
+                        self.window.show_all()
+                        self._reset_storage()
+                    except StorageError as e:
+                        log.error("newDevice failed: %s" % e)
+                        self.window.set_info(Gtk.MessageType.ERROR,
+                                             _("Device configuration failed."))
+                        self.window.show_all()
 
             log.debug("updating selector size to '%s'"
                        % str(Size(spec="%f MB" % device.size)).upper())
@@ -723,11 +761,17 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
         # for fstype we'll need to instantiate a new DeviceFormat and schedule
         # creation of it
         if fs_type != device.format.type:
+            self.window.clear_info()
             with ui_storage_logger():
+                old_format = device.format
                 new_format = getFormat(fs_type,
                                        mountpoint=mountpoint, label=label,
                                        device=device.path)
-                self.__storage.formatDevice(device, new_format)
+                try:
+                    self.__storage.formatDevice(device, new_format)
+                except StorageError as e:
+                    log.error("failed to register device format action: %s" % e)
+                    device.format = old_format
 
         # for encryption, I'm not sure if we'll want to modify the
         # container/device or just create a new one
@@ -1024,21 +1068,42 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
             device_type = AUTOPART_TYPE_PLAIN
 
         disks = self._clearpartDevices
+        self.window.clear_info()
 
         with ui_storage_logger():
-            self.__storage.newDevice(device_type,
-                                     size=float(size.convertTo(spec="mb")),
-                                     fstype=fstype,
-                                     mountpoint=mountpoint,
-                                     encrypted=encrypted,
-                                     disks=disks)
+            try:
+                self.__storage.newDevice(device_type,
+                                         size=float(size.convertTo(spec="mb")),
+                                         fstype=fstype,
+                                         mountpoint=mountpoint,
+                                         encrypted=encrypted,
+                                         disks=disks)
+            except ErrorRecoveryFailure as e:
+                log.error("error recovery failure")
+                self.window.set_info(Gtk.MessageType.ERROR,
+                                     _("Potentially fatal error."))
+                self.window.show_all()
+                self._reset_storage()
+            except StorageError as e:
+                log.error("newDevice failed: %s" % e)
+                self.window.set_info(Gtk.MessageType.ERROR,
+                                     _("Failed to add new device."))
+                self.window.show_all()
+
         self._devices = self.__storage.devices
         self._do_refresh()
         self._updateSpaceDisplay()
 
     def _destroy_device(self, device):
+        self.window.clear_info()
         with ui_storage_logger():
-            self.__storage.destroyDevice(device)
+            try:
+                self.__storage.destroyDevice(device)
+            except StorageError as e:
+                log.error("failed to schedule device removal: %s" % e)
+                self.window.set_info(Gtk.MessageType.ERROR,
+                                     _("Device removal failed."))
+                self.window.show_all()
 
         self._devices = self.__storage.devices
 
@@ -1212,11 +1277,27 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
         # There are never any non-existent devices around when this runs.
         log.debug("running automatic partitioning")
         self.__storage.doAutoPart = True
+        self.window.clear_info()
         with ui_storage_logger():
-            doAutoPartition(self.__storage, self.data)
-        self.__storage.doAutoPart = False
-        self._devices = self.__storage.devices
-        log.debug("finished automatic partitioning")
+            try:
+                doAutoPartition(self.__storage, self.data)
+            except NoDisksError as e:
+                # No handling should be required for this.
+                log.error("doAutoPartition failed: %s" % e)
+                self.window.set_info(Gtk.MessageType.ERROR,
+                                     _("No disks selected or not enough free "
+                                       "space on disks."))
+                self.window.show_all()
+            except StorageError as e:
+                log.error("doAutoPartition failed: %s" % e)
+                self.window.set_info(Gtk.MessageType.ERROR,
+                                     _("Automatic partitioning failed."))
+                self.window.show_all()
+            else:
+                self._devices = self.__storage.devices
+            finally:
+                self.__storage.doAutoPart = False
+                log.debug("finished automatic partitioning")
 
     def on_create_clicked(self, button):
         # Then do autopartitioning.  We do not do any clearpart first.  This is
diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py
index ddc701a..26cf76d 100644
--- a/pyanaconda/ui/gui/spokes/storage.py
+++ b/pyanaconda/ui/gui/spokes/storage.py
@@ -50,6 +50,7 @@ from pyanaconda.ui.gui.utils import enlightbox, gdk_threaded
 
 from pyanaconda.kickstart import doKickstartStorage
 from pyanaconda.storage.size import Size
+from pyanaconda.storage.errors import StorageError
 from pyanaconda.product import productName
 from pyanaconda.flags import flags
 
@@ -326,8 +327,21 @@ class StorageSpoke(NormalSpoke, StorageChecker):
         self.storage.config.clearNonExistent = self.data.autopart.autopart
 
     def execute(self):
-        doKickstartStorage(self.storage, self.data, self.instclass)
-        self.run()
+        try:
+            doKickstartStorage(self.storage, self.data, self.instclass)
+        except StorageError as e:
+            log.error("storage configuration failed: %s" % e)
+            communication.send_not_ready(self.__class__.__name__)
+            communication.send_message(self.__class__.__name__,
+                                   _("Failed to save storage configuration..."))
+            self.data.clearpart.type = CLEARPART_TYPE_NONE
+            self.data.clearpart.initAll = False
+            self.storage.config.update(self.data)
+            self.storage.autoPartType = self.data.clearpart.type
+            self.storage.reset()
+            communication.send_ready(self.__class__.__name__)
+        else:
+            self.run()
 
     @property
     def completed(self):
-- 
1.7.7.6



More information about the anaconda-patches mailing list