Re: [anaconda][master/rhel7-branch] [PATCH 3/3] Add GUI and TUI logic to handle unformatted DASDs. (#1064423)

[Samantha N. Bueno]

On Tue, Feb 25, 2014 at 01:14:58PM +0100, Vratislav Podzimek wrote:
> On Tue, 2014-02-25 at 04:30 -0500, Samantha N. Bueno wrote:
> > In both graphical and text, if a user selects unformatted DASDs
> > from the local disk store, they are formatted when the user opts
> > to proceed (either by clicking 'Done' in the GUI or 'c' to
> > continue in the TUI).
> >
> > Following dasdfmt, storage initialization is run again in order
> > to properly add the new DASDs to the devicetree and seen by the
> > installer.
> > Resolves:rhbz#1064423
> >
> > pyanaconda/constants.py             |  1 +
> >  pyanaconda/ui/gui/spokes/storage.py | 73 ++++++++++++++++++++++++++++++++++++-
> >  pyanaconda/ui/tui/spokes/storage.py | 44 ++++++++++++++++++++--
> >  3 files changed, 113 insertions(+), 5 deletions(-)
> >
> > diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
> > index cf8a8ea..8de29a9 100644
> > --- a/pyanaconda/constants.py
> > +++ b/pyanaconda/constants.py
> > @@ -131,6 +131,7 @@ THREAD_ISCSI_LOGIN = "AnaIscsiLoginThread"
> >  THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread"
> >  THREAD_DATE_TIME = "AnaDateTimeThread"
> >  THREAD_TIME_INIT = "AnaTimeInitThread"
> > +THREAD_DASDFMT = "AnaDasdfmtThread"
> >
> > # Geolocation constants
> >
> > diff --git a/pyanaconda/ui/gui/spokes/storage.py b/pyanaconda/ui/gui/spokes/storage.py
> > index bdd48ef..a42c540 100644
> > --- a/pyanaconda/ui/gui/spokes/storage.py
> > +++ b/pyanaconda/ui/gui/spokes/storage.py
> > @@ -50,17 +50,20 @@ from pyanaconda.ui.gui.spokes.lib.cart import SelectedDisksDialog
> >  from pyanaconda.ui.gui.spokes.lib.passphrase import PassphraseDialog
> >  from pyanaconda.ui.gui.spokes.lib.detailederror import DetailedErrorDialog
> >  from pyanaconda.ui.gui.spokes.lib.resize import ResizeDialog
> > +from pyanaconda.ui.gui.spokes.lib.dasdfmt import DasdFormatDialog
> >  from pyanaconda.ui.gui.categories.system import SystemCategory
> > -from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait
> > +from pyanaconda.ui.gui.utils import enlightbox, gtk_call_once, gtk_action_wait, ignoreEscape
> >
> > from pyanaconda.kickstart import doKickstartStorage, getAvailableDiskSpace
> > +from blivet import storageInitialize, arch
> >  from blivet.size import Size
> >  from blivet.devices import MultipathDevice
> > -from blivet.errors import StorageError
> > +from blivet.errors import StorageError, DasdFormatError
> >  from blivet.errors import SanityError
> >  from blivet.errors import SanityWarning
> >  from blivet.platform import platform
> >  from blivet.devicelibs import swap as swap_lib
> > +from blivet.devicelibs.dasd import make_unformatted_dasd_list, format_dasd
> >  from pyanaconda.threads import threadMgr, AnacondaThread
> >  from pyanaconda.product import productName
> >  from pyanaconda.flags import flags
> > @@ -292,6 +295,11 @@ class StorageSpoke(NormalSpoke, StorageChecker):
> >          self.autoPartType = None
> >          self.clearPartType = CLEARPART_TYPE_NONE
> >    if self.data.zerombr.zerombr and arch.isS390():
> >        # run dasdfmt on any unformatted DASDs automatically
> >        threadMgr.add(AnacondaThread(name=constants.THREAD_DASDFMT,
> >                        target=self.run_dasdfmt))
> >    self._previous_autopart = False
> >
> >    self._last_clicked_overview = None
> >
> > @@ -357,6 +365,9 @@ class StorageSpoke(NormalSpoke, StorageChecker):
> >      def _doExecute(self):
> >          self._ready = False
> >          hubQ.send_not_ready(self.__class__.__name__)
> >    # on the off-chance dasdfmt is running, bail
> >    if threadMgr.get(constants.THREAD_DASDFMT):
> >        return
> >    hubQ.send_message(self.__class__.__name__, _("Saving storage configuration..."))
> >    try:
> >        doKickstartStorage(self.storage, self.data, self.instclass)
> >
> > @@ -409,6 +420,8 @@ class StorageSpoke(NormalSpoke, StorageChecker):
> >      if not self._confirmed:
> >          msg = _("Not configured")
> >    elif threadMgr.get(constants.THREAD_DASDFMT):
> >        msg = _("Formatting DASDs")
> >    elif flags.automatedInstall and not self.storage.rootDevice:
> >        return msg
> >    elif self.data.ignoredisk.onlyuse:
> >
> > @@ -681,6 +694,36 @@ class StorageSpoke(NormalSpoke, StorageChecker):
> >              if not selected and name in self.selected_disks:
> >                  self.selected_disks.remove(name)
> > def run_dasdfmt(self):
> >    """
> >    Though the same function exists in pyanaconda.ui.gui.spokes.lib.dasdfmt,
> >    this instance doesn't include any of the UI pieces and should only
> >    really be getting called on ks installations with "zerombr".
> >    """
> >    to_format = make_unformatted_dasd_list(self.selected_disks)
> >    if len(to_format) == 0:
> >        # nothing to do here; bail
> >        return
> >    for disk in to_format:
> >        try:
> >            format_dasd(disk)
> >        except DasdFormatError as err:
> >            # Log errors if formatting fails, but don't halt the installer
> >            log.error(str(err))
> >            continue
> >    # when finished formatting we need to reinitialize storage
> >    # however, check that we don't already have a storage thread running
> >    if threadMgr.get(constants.THREAD_STORAGE):
> >        threadMgr.wait(constants.THREAD_STORAGE)
>
> I believe you can call wait directly, it should be no-op if there is no
> such thread. But shouldn't this wait happen in the first place before
> formatting DASDs?

Ah yes, you are right, I will delete the get() since it's redundant.
> >    protectedNames = map(lambda d: d.name, self.storage.protectedDevices)
>
> You can use a list comprehension here:
>  [d.name for d in self.storage.protectedDevices]

I think you may have mentioned that last time and thought I'd changed
that; will do so now. Pylint was throwing a warning about that anyway.
:)
> >    threadMgr.add(AnacondaThread(name=constants.THREAD_STORAGE,
> >                    target=storageInitialize,
> >                    args=(self.storage, self.data, protectedNames)))
> >    threadMgr.wait(constants.THREAD_STORAGE)
> > # signal handlers
> >    def on_summary_clicked(self, button):
> >        # show the selected disks dialog
> >
> > @@ -769,6 +812,32 @@ class StorageSpoke(NormalSpoke, StorageChecker):
> >              NormalSpoke.on_back_clicked(self, button)
> >              return
> >    if arch.isS390():
> >        dasds = make_unformatted_dasd_list(self.selected_disks)
> >        if len(dasds) > 0:
> >            dialog = DasdFormatDialog(self.data, self.storage, dasds)
> >            ignoreEscape(dialog.window)
> >            rc = self.run_lightbox_dialog(dialog)
> >            if rc == 1:
> >                # User hit OK on the dialog, indicating they stayed on the
> >                # dialog until formatting completed and now needs to go back
> >                # to the main storage spoke.
> >                dialog.window.destroy()
> >                # make sure we stay on the storage spoke and don't return to
> >                # the summary hub
> >                self.skipTo = "StorageSpoke"
> >                self.refresh()
>
> I think refresh will get called when the spoke is entered again due to
> skipTo.

I thought it would as well and tried that initially, but it does not get
called. Thought about adding a comment; I'll add one now.
> >            elif rc == 2:
> >                # User clicked uri to return to hub.
> >                NormalSpoke.on_back_clicked(self, button)
> >                return
> >            elif rc != 2:
> >                # User either hit cancel on the dialog or closed it via escape, so
> >                # there was no formatting done.
> >                # NOTE: rc == 2 means the user clicked on the link that takes them
> >                # back to the hub.
> >                return
> >    # Figure out if the existing disk labels will work on this platform
> >    # you need to have at least one of the platform's labels in order for
> >    # any of the free space to be useful.
> >
> > diff --git a/pyanaconda/ui/tui/spokes/storage.py b/pyanaconda/ui/tui/spokes/storage.py
> > index b4d40f1..c20b649 100644
> > --- a/pyanaconda/ui/tui/spokes/storage.py
> > +++ b/pyanaconda/ui/tui/spokes/storage.py
> > @@ -27,15 +27,17 @@ from pyanaconda.ui.tui.spokes import NormalTUISpoke
> >  from pyanaconda.ui.tui.simpleline import TextWidget, CheckboxWidget
> >
> > from pykickstart.constants import AUTOPART_TYPE_LVM, AUTOPART_TYPE_BTRFS, AUTOPART_TYPE_PLAIN
> > +from blivet import storageInitialize, arch
> >  from blivet.size import Size
> > -from blivet.errors import StorageError
> > +from blivet.errors import StorageError, DasdFormatError
> >  from blivet.errors import SanityError
> >  from blivet.errors import SanityWarning
> >  from blivet.devices import DASDDevice, FcoeDiskDevice, iScsiDiskDevice, MultipathDevice, ZFCPDiskDevice
> > +from blivet.devicelibs.dasd import format_dasd, make_unformatted_dasd_list
> >  from pyanaconda.flags import flags
> >  from pyanaconda.kickstart import doKickstartStorage
> >  from pyanaconda.threads import threadMgr, AnacondaThread
> > -from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER
> > +from pyanaconda.constants import THREAD_STORAGE, THREAD_STORAGE_WATCHER, THREAD_DASDFMT
> >  from pyanaconda.i18n import _, P_, N_
> >  from pyanaconda.bootloader import BootLoaderError
> >
> > @@ -75,6 +77,9 @@ class StorageSpoke(NormalTUISpoke):
> >          self.errors = []
> >          self.warnings = []
> >    if self.data.zerombr.zerombr and arch.isS390():
> >        self.run_dasdfmt()
> >    if not flags.automatedInstall:
> >        # default to using autopart for interactive installs
> >        self.data.autopart.autopart = True
> >
> > @@ -89,7 +94,7 @@ class StorageSpoke(NormalTUISpoke):
> >      def ready(self):
> >          # By default, the storage spoke is not ready.  We have to wait until
> >          # storageInitialize is done.
> >    return self._ready and not threadMgr.get(THREAD_STORAGE_WATCHER)
> >    return self._ready and not (threadMgr.get(THREAD_STORAGE_WATCHER) or threadMgr.get(THREAD_DASDFMT))
> >
> > @property
> >    def mandatory(self):
> >
> > @@ -231,6 +236,13 @@ class StorageSpoke(NormalTUISpoke):
> >      if key == "c":
> >          if self.selected_disks:
> >            # check selected disks to see if we have any unformatted DASDs
> >            # if we're on s390x, since they need to be formatted before we
> >            # can use them.
> >            if arch.isS390():
> >                self.run_dasdfmt()
> >                return None
> >            newspoke = AutoPartSpoke(self.app, self.data, self.storage,
> >                                     self.payload, self.instclass)
> >            self.app.switch_screen_modal(newspoke)
> >
> > @@ -247,6 +259,32 @@ class StorageSpoke(NormalTUISpoke):
> >          except (ValueError, KeyError, IndexError):
> >              return key
> > def run_dasdfmt(self):
> >    """
> >    This generates the list of DASDs requiring dasdfmt and runs dasdfmt
> >    against them.
> >    """
> >    to_format = make_unformatted_dasd_list(self.selected_disks)
> >    if len(to_format) == 0:
>
> 'if not to_format' would do the same.

Yes, fixed locally and in the few other places I have that same line.
> >        # nothing to do here; bail
> >        return
> >    for disk in to_format:
> >        try:
> >            print(_("Formatting /dev/%s. This may take a moment." % disk))
> >            format_dasd(disk)
> >        except DasdFormatError as err:
> >            # Log errors if formatting fails, but don't halt the installer
> >            log.error(str(err))
> >            continue
> >    # when finished formatting we need to reinitialize storage
> >    protectedNames = map(lambda d: d.name, self.storage.protectedDevices)
>
> The same list comprehension as in the GUI could be used here.

Fixed locally.
> >    threadMgr.add(AnacondaThread(name=THREAD_STORAGE,
> >                    target=storageInitialize,
> >                    args=(self.storage, self.data, protectedNames)))
> >    threadMgr.wait(THREAD_STORAGE)
> > def apply(self):
> >        self.autopart = self.data.autopart.autopart
> >        self.data.ignoredisk.onlyuse = self.selected_disks[:]
>
> -- 
> Vratislav Podzimek
>
> Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches