[blivet][rhel7-branch] Tear down devices recursively when finding existing installations

[Vratislav Podzimek]

When trying to find existing installations we setup and mount devices (to check
if they have /etc/fstab). Setting a device up results in its parents being setup
too (recursively) so when tearing the device down we should also tear its
parents (recursively) down so that they are not left activated as that could
cause problems later (as for example in the bug where a thin pool was
activated/setup on setup of a thin LV and never deactivated/torn down).

Resolves: rhbz#1182229
Signed-off-by: Vratislav Podzimek 
---
 blivet/__init__.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index 47cb1dd..9c8bfac 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -3000,7 +3000,7 @@ def findExistingInstallations(devicetree):
             device.format.mount(options=options, mountpoint=getSysroot())
         except Exception: # pylint: disable=broad-except
             log_exception_info(log.warning, "mount of %s as %s failed", [device.name, device.format.type])
-            device.teardown()
+            device.teardown(recursive=True)
             continue

if not os.access(getSysroot() + "/etc/fstab", os.R_OK):
@@ -3021,7 +3021,7 @@ def findExistingInstallations(devicetree):
                         {"product": product, "version": version, "arch": architecture}

(mounts, swaps) = parseFSTab(devicetree, chroot=_sysroot)
-        device.teardown()
+        device.teardown(recursive=True)
         if not mounts and not swaps:
             # empty /etc/fstab. weird, but I've seen it happen.
             continue

[David Lehman]

On 06/29/2015 08:10 AM, Vratislav Podzimek wrote:
> When trying to find existing installations we setup and mount devices (to check
> if they have /etc/fstab). Setting a device up results in its parents being setup
> too (recursively) so when tearing the device down we should also tear its
> parents (recursively) down so that they are not left activated as that could
> cause problems later (as for example in the bug where a thin pool was
> activated/setup on setup of a thin LV and never deactivated/torn down).

It would be nicer to do one full teardown at the exit point of
findExistingInstallations instead of this, since this could cause a
significant amount of activity for some setups (esp. lvm on luks and/or
md).

David
> Resolves: rhbz#1182229
> Signed-off-by: Vratislav Podzimek
>
> blivet/__init__.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index 47cb1dd..9c8bfac 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -3000,7 +3000,7 @@ def findExistingInstallations(devicetree):
>               device.format.mount(options=options, mountpoint=getSysroot())
>           except Exception: # pylint: disable=broad-except
>               log_exception_info(log.warning, "mount of %s as %s failed", [device.name, device.format.type])
>        device.teardown()
>        device.teardown(recursive=True)
>         continue
>
>     if not os.access(getSysroot() + "/etc/fstab", os.R_OK):
>
> @@ -3021,7 +3021,7 @@ def findExistingInstallations(devicetree):
>                           {"product": product, "version": version, "arch": architecture}
>       (mounts, swaps) = parseFSTab(devicetree, chroot=_sysroot)
>    device.teardown()
>    device.teardown(recursive=True)
>     if not mounts and not swaps:
>         # empty /etc/fstab. weird, but I've seen it happen.
>         continue

[Vratislav Podzimek]

On Mon, 2015-06-29 at 08:40 -0500, David Lehman wrote:
> On 06/29/2015 08:10 AM, Vratislav Podzimek wrote:
> > When trying to find existing installations we setup and mount devices (to check
> > if they have /etc/fstab). Setting a device up results in its parents being setup
> > too (recursively) so when tearing the device down we should also tear its
> > parents (recursively) down so that they are not left activated as that could
> > cause problems later (as for example in the bug where a thin pool was
> > activated/setup on setup of a thin LV and never deactivated/torn down).
>
> It would be nicer to do one full teardown at the exit point of
> findExistingInstallations instead of this, since this could cause a
> significant amount of activity for some setups (esp. lvm on luks and/or
> md).

I didn't notice any while testing, but doing one full teardown is
definitely better. Sending v2 of this patch. Thanks!

[PATCHv2] Tear down all devices after finding existing installations

[Vratislav Podzimek]

When trying to find existing installations we setup and mount devices (to check
if they have /etc/fstab). Setting a device up results in its parents being setup
too (recursively) so when done we need to tear all/rest of the devices down so
that they are not left activated as that could cause problems later (as for
example in the bug where a thin pool was activated/setup on setup of a thin LV
and never deactivated/torn down).

Resolves: rhbz#1182229
Signed-off-by: Vratislav Podzimek 
---
 blivet/__init__.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index 47cb1dd..772ca16 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -490,6 +490,11 @@ class Blivet(object):
                 self.roots = findExistingInstallations(self.devicetree)
             except Exception: # pylint: disable=broad-except
                 log_exception_info(log.info, "failure detecting existing installations")
+            finally:
+                # make sure no devices are left activated after finding existing
+                # installations (we don't quite know which ones got activated in
+                # that process)
+                self.devicetree.teardownAll()

self.dumpState("initial")

[PATCHv2] Tear down all devices after finding existing installations

[David Lehman]

On 06/29/2015 10:17 AM, Vratislav Podzimek wrote:
> When trying to find existing installations we setup and mount devices (to check
> if they have /etc/fstab). Setting a device up results in its parents being setup
> too (recursively) so when done we need to tear all/rest of the devices down so
> that they are not left activated as that could cause problems later (as for
> example in the bug where a thin pool was activated/setup on setup of a thin LV
> and never deactivated/torn down).
>
> Resolves: rhbz#1182229
> Signed-off-by: Vratislav Podzimek
>
> blivet/__init__.py | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index 47cb1dd..772ca16 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -490,6 +490,11 @@ class Blivet(object):
>                   self.roots = findExistingInstallations(self.devicetree)
>               except Exception: # pylint: disable=broad-except
>                   log_exception_info(log.info, "failure detecting existing installations")
>        finally:
>            # make sure no devices are left activated after finding existing
>            # installations (we don't quite know which ones got activated in
>            # that process)
>            self.devicetree.teardownAll()
>
>         self.dumpState("initial")

ACK

[PATCHv2] Tear down all devices after finding existing installations

[Vratislav Podzimek]

On Mon, 2015-06-29 at 10:55 -0500, David Lehman wrote:
> On 06/29/2015 10:17 AM, Vratislav Podzimek wrote:
> > When trying to find existing installations we setup and mount devices (to check
> > if they have /etc/fstab). Setting a device up results in its parents being setup
> > too (recursively) so when done we need to tear all/rest of the devices down so
> > that they are not left activated as that could cause problems later (as for
> > example in the bug where a thin pool was activated/setup on setup of a thin LV
> > and never deactivated/torn down).
> >
> > Resolves: rhbz#1182229
> > Signed-off-by: Vratislav Podzimek
> >
> > blivet/__init__.py | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/blivet/__init__.py b/blivet/__init__.py
> > index 47cb1dd..772ca16 100644
> > --- a/blivet/__init__.py
> > +++ b/blivet/__init__.py
> > @@ -490,6 +490,11 @@ class Blivet(object):
> >                   self.roots = findExistingInstallations(self.devicetree)
> >               except Exception: # pylint: disable=broad-except
> >                   log_exception_info(log.info, "failure detecting existing installations")
> >        finally:
> >            # make sure no devices are left activated after finding existing
> >            # installations (we don't quite know which ones got activated in
> >            # that process)
> >            self.devicetree.teardownAll()
> >
> >         self.dumpState("initial")

The problem with this approach is that anaconda also calls
findExistingInstallations(). It seems to me a bit silly to add the
teardownAll() call to anaconda too so I'm proposing a bit different
approach of adding an extra argument to findExistingInstallations() and
tearing down all devices there. We also need one more patch that fixes
recursive teardown of inactive devices. Sending version 3.

[PATCHv3 1/2] Tear down all devices after finding existing installations

[Vratislav Podzimek]

When trying to find existing installations we setup and mount devices (to check
if they have /etc/fstab). Setting a device up results in its parents being setup
too (recursively) so when done we need to tear all/rest of the devices down so
that they are not left activated as that could cause problems later (as for
example in the bug where a thin pool was activated/setup on setup of a thin LV
and never deactivated/torn down).

Resolves: rhbz#1182229
Signed-off-by: Vratislav Podzimek 
---
 blivet/__init__.py | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index 47cb1dd..7cb1228 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -2979,7 +2979,14 @@ def getReleaseString():

return (relArch, relName, relVer)

-def findExistingInstallations(devicetree):
+def findExistingInstallations(devicetree, teardown_all=True):
+    """Find existing GNU/Linux installations on devices from the devicetree.
+    :param devicetree: devicetree to find existing installations in
+    :type devicetree: :class:`~.devicetree.DeviceTree`
+    :param bool teardown_all: whether to tear down all devices in the
+                              devicetree in the end
+
+    """
     if not os.path.exists(getTargetPhysicalRoot()):
         util.makedirs(getTargetPhysicalRoot())

@@ -3027,6 +3034,9 @@ def findExistingInstallations(devicetree):
             continue
         roots.append(Root(mounts=mounts, swaps=swaps, name=name))

+    if teardown_all:
+        devicetree.teardownAll()
+
     return roots

class Root(object):

[PATCH 2/2] Do not break the chain when an inactive device is torn down recursively

[Vratislav Podzimek]

If an inactive device is requested to be torn down recursively, we need to let
the chain of actions continue because there might be active parent devices that
need to be torn down.

Related: rhbz#1182229
Signed-off-by: Vratislav Podzimek 
---
 blivet/devices/storage.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py
index 73ba645..3a6f6cb 100644
--- a/blivet/devices/storage.py
+++ b/blivet/devices/storage.py
@@ -426,7 +426,12 @@ class StorageDevice(Device):
             raise errors.DeviceError("device has not been created", self.name)

if not self.status or not self.controllable:
-            return False
+            if not recursive:
+                return False
+            else:
+                # nothing special to do here, but we need to continue with the
+                # teardown (to tear down parents)
+                return True

if self.originalFormat.exists:
             self.originalFormat.teardown()

[PATCH 2/2] Do not break the chain when an inactive device is torn down recursively

[David Lehman]

On 07/01/2015 03:27 AM, Vratislav Podzimek wrote:
> If an inactive device is requested to be torn down recursively, we need to let
> the chain of actions continue because there might be active parent devices that
> need to be torn down.
>
> Related: rhbz#1182229
> Signed-off-by: Vratislav Podzimek
>
> blivet/devices/storage.py | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py
> index 73ba645..3a6f6cb 100644
> --- a/blivet/devices/storage.py
> +++ b/blivet/devices/storage.py
> @@ -426,7 +426,12 @@ class StorageDevice(Device):
>               raise errors.DeviceError("device has not been created", self.name)
>       if not self.status or not self.controllable:
>        return False
>        if not recursive:
>            return False
>        else:
>            # nothing special to do here, but we need to continue with the
>            # teardown (to tear down parents)
>            return True

I might prefer you do "return recursive" with an explanatory comment
here.
>       if self.originalFormat.exists:
>           self.originalFormat.teardown()

[PATCH 2/2] Do not break the chain when an inactive device is torn down recursively

[Vratislav Podzimek]

On Thu, 2015-07-02 at 11:16 -0500, David Lehman wrote:
> On 07/01/2015 03:27 AM, Vratislav Podzimek wrote:
> > If an inactive device is requested to be torn down recursively, we need to let
> > the chain of actions continue because there might be active parent devices that
> > need to be torn down.
> >
> > Related: rhbz#1182229
> > Signed-off-by: Vratislav Podzimek
> >
> > blivet/devices/storage.py | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py
> > index 73ba645..3a6f6cb 100644
> > --- a/blivet/devices/storage.py
> > +++ b/blivet/devices/storage.py
> > @@ -426,7 +426,12 @@ class StorageDevice(Device):
> >               raise errors.DeviceError("device has not been created", self.name)
> >       if not self.status or not self.controllable:
> >        return False
> >        if not recursive:
> >            return False
> >        else:
> >            # nothing special to do here, but we need to continue with the
> >            # teardown (to tear down parents)
> >            return True
>
> I might prefer you do "return recursive" with an explanatory comment
> here.

Sure thing, fixing locally.

[PATCHv3 1/2] Tear down all devices after finding existing installations

[David Lehman]

On 07/01/2015 03:27 AM, Vratislav Podzimek wrote:
> When trying to find existing installations we setup and mount devices (to check
> if they have /etc/fstab). Setting a device up results in its parents being setup
> too (recursively) so when done we need to tear all/rest of the devices down so
> that they are not left activated as that could cause problems later (as for
> example in the bug where a thin pool was activated/setup on setup of a thin LV
> and never deactivated/torn down).
>
> Resolves: rhbz#1182229
> Signed-off-by: Vratislav Podzimek
>
> blivet/__init__.py | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index 47cb1dd..7cb1228 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -2979,7 +2979,14 @@ def getReleaseString():
>   return (relArch, relName, relVer)
>
> -def findExistingInstallations(devicetree):
> +def findExistingInstallations(devicetree, teardown_all=True):
> """Find existing GNU/Linux installations on devices from the devicetree.
> :param devicetree: devicetree to find existing installations in
> :type devicetree: :class:`~.devicetree.DeviceTree`
> :param bool teardown_all: whether to tear down all devices in the
>                          devicetree in the end
> """
>  if not os.path.exists(getTargetPhysicalRoot()):
>      util.makedirs(getTargetPhysicalRoot())
>
> @@ -3027,6 +3034,9 @@ def findExistingInstallations(devicetree):
>               continue
>           roots.append(Root(mounts=mounts, swaps=swaps, name=name))
>
> if teardown_all:
>    devicetree.teardownAll()
> return roots
>
> class Root(object):

This function can raise exceptions pretty easily if people have screwy
stuff in their existing fstabs. We catch those and proceed in
Blivet.reset, but the devices won't be torn down with this patch.

As much as I hate it, maybe you should rename findExistingInstallations
_findExistingInstallations and add a findExistingInstallations that just
wraps a call to _findExistingInstallations like

try:
   _findExistingInstallations()
except Exception:
   log()
finally:
   teardownAll()

[PATCHv3 1/2] Tear down all devices after finding existing installations

[Vratislav Podzimek]

On Thu, 2015-07-02 at 11:19 -0500, David Lehman wrote:
> On 07/01/2015 03:27 AM, Vratislav Podzimek wrote:
> > When trying to find existing installations we setup and mount devices (to check
> > if they have /etc/fstab). Setting a device up results in its parents being setup
> > too (recursively) so when done we need to tear all/rest of the devices down so
> > that they are not left activated as that could cause problems later (as for
> > example in the bug where a thin pool was activated/setup on setup of a thin LV
> > and never deactivated/torn down).
> >
> > Resolves: rhbz#1182229
> > Signed-off-by: Vratislav Podzimek
> >
> > blivet/__init__.py | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/blivet/__init__.py b/blivet/__init__.py
> > index 47cb1dd..7cb1228 100644
> > --- a/blivet/__init__.py
> > +++ b/blivet/__init__.py
> > @@ -2979,7 +2979,14 @@ def getReleaseString():
> >   return (relArch, relName, relVer)
> >
> > -def findExistingInstallations(devicetree):
> > +def findExistingInstallations(devicetree, teardown_all=True):
> > """Find existing GNU/Linux installations on devices from the devicetree.
> > :param devicetree: devicetree to find existing installations in
> > :type devicetree: :class:`~.devicetree.DeviceTree`
> > :param bool teardown_all: whether to tear down all devices in the
> >                          devicetree in the end
> > """
> >  if not os.path.exists(getTargetPhysicalRoot()):
> >      util.makedirs(getTargetPhysicalRoot())
> >
> > @@ -3027,6 +3034,9 @@ def findExistingInstallations(devicetree):
> >               continue
> >           roots.append(Root(mounts=mounts, swaps=swaps, name=name))
> >
> > if teardown_all:
> >    devicetree.teardownAll()
> > return roots
> >
> > class Root(object):
>
> This function can raise exceptions pretty easily if people have screwy
> stuff in their existing fstabs. We catch those and proceed in
> Blivet.reset, but the devices won't be torn down with this patch.
>
> As much as I hate it, maybe you should rename findExistingInstallations
> _findExistingInstallations and add a findExistingInstallations that just
> wraps a call to _findExistingInstallations like
>
> try:
>    _findExistingInstallations()
> except Exception:
>    log()
> finally:
>    teardownAll()

Works for me. Fixing locally.