a323fcaec0f5 — Alain Leufroy 3 years ago
interrup/preserve: refactorize and rename options
2 files changed, 115 insertions(+), 132 deletions(-)

M overlayctl
M test_overlayctl.py
M overlayctl +100 -122
@@ 54,6 54,10 @@ class OverlayctlError(SystemExit):
     """Raised on exception."""
 
 
+class ActiveBranchError(SystemExit):
+    """Raised when a layer branch has unexpected active descendants."""
+
+
 class NoResult(OverlayctlError):
     """Raised when no result where found."""
 

          
@@ 560,6 564,36 @@ def _get_linearized_lowers(layer, lowers
     return result[layer][1:]
 
 
+@contextmanager
+def _stop_branch(current, interrupt=False, preserve=False, restart=False, allbranch=False):
+    """Ensure layer and its descendant are stopped inside this context.
+    If current or any of its descendan is active, raise ActiveBranchError error.
+
+    :stop_others: automatically stop activated descendants. Then restart them at exit.
+
+    :kepp_others: make this a null context if True
+    """
+    if preserve:
+        yield
+    else:
+        branch = chain(_iter_descendants(current), [current])
+        if allbranch:
+            branch = chain(branch, (x for x in _iter_ascendants(current) if x.is_managed()))
+        activated = [x for x in branch if x.automountunit.is_active()]
+        if activated:
+            if interrupt:
+                for layer in activated:
+                    layer.stop()
+            else:
+                raise ActiveBranchError(
+                    'The following units should be stopped: %s'
+                    % ', '.join(x.name for x in activated))
+        yield
+        if interrupt and restart:
+            for layer in activated:
+                layer.start()
+
+
 def creater(name, lowers, start=None, permanent=None):
     """create an overlay at `name` that inherites from `lowerdir`.
 

          
@@ 605,89 639,54 @@ def _get_editor_on_file(filename):
 
 def editer(
         name, appended=None, prepended=None, removed=None,
-        stop_others=False, keep_others=False):
+        interrupt=False, preserve=False):
     """Ask user to edit lowers of the given overlay name, rewrite
     the metadata and unit files accordingly, finaly retart systemd units
     of the edited overlay.
     """
     layer = build_layer(name)
-    to_stop = []
-    if not keep_others:
-        to_stop[:] = [layer for layer in _iter_descendants(layer)
-                      if layer.automountunit.is_active()]
-        if to_stop:
-            if stop_others:
-                for descendant in to_stop:
-                    descendant.stop()
-            else:
-                logger.error(
-                    'You should prefere to stop the following overlays: \n\n%s\n',
-                    ', '.join(x.name for x in to_stop))
+    with _stop_branch(layer, interrupt=interrupt, preserve=preserve, restart=interrupt):
+        if not appended and not prepended and not removed:
+            with NamedTemporaryFile() as fobj:
+                fobj.write(b'# -*- encoding: utf-8 -*-\n')
+                fobj.write(b'# Note: Leave unchanged or fully empty to ignore changes\n')
+                fobj.write(b'\n'.join(lower.name.encode('utf-8') for lower in layer.lowers))
+                fobj.flush()
+                _get_editor_on_file(fobj.name)
+                fobj.seek(0)
+                lowers = [
+                    build_layer(line.strip())
+                    for line in fobj.read().decode('utf-8').splitlines()
+                    if not line.strip().startswith('#') and line.strip()
+                ]
+                fobj.seek(0)
+                if not fobj.read(1):
+                    return  # Ignore empty file for convenience
+        else:
+            prepended = map(build_layer, prepended or ())
+            appended = map(build_layer, appended or ())
+            removed = list(map(build_layer, removed or ()))
+            lowers = chain(
+                prepended,
+                (lower for lower in layer.lowers if lower not in removed),
+                appended)
+        if lowers == layer.lowers:
             return
-    if not appended and not prepended and not removed:
-        with NamedTemporaryFile() as fobj:
-            fobj.write(b'# -*- encoding: utf-8 -*-\n')
-            fobj.write(b'# Note: Leave unchanged or fully empty to ignore changes\n')
-            fobj.write(b'\n'.join(lower.name.encode('utf-8') for lower in layer.lowers))
-            fobj.flush()
-            _get_editor_on_file(fobj.name)
-            fobj.seek(0)
-            lowers = [
-                build_layer(line.strip())
-                for line in fobj.read().decode('utf-8').splitlines()
-                if not line.strip().startswith('#') and line.strip()
-            ]
-            fobj.seek(0)
-            if not fobj.read(1):
-                return  # Ignore empty file for convenience
-    else:
-        prepended = map(build_layer, prepended or ())
-        appended = map(build_layer, appended or ())
-        removed = list(map(build_layer, removed or ()))
-        lowers = chain(
-            prepended,
-            (lower for lower in layer.lowers if lower not in removed),
-            appended)
-    if lowers == layer.lowers:
-        return
-    with layer.temporary_stop():
         layer.lowers[:] = lowers
         layer.dump()
         systemctl.daemon_reload()
-    for descendant in to_stop:
-        descendant.start()
     # XXX propagate to descendants
     logger.debug("Systemd \"%s\" restarted", layer.unitname)
     logger.info("\"%s\" updated", layer.name)
 
 
-def starter(name, preserve_others=False, stop_others=False, permanent=False):
+def starter(name, interrupt=False, preserve=False, permanent=False):
     layer = build_layer(name)
     if not layer.is_managed():
         logger.info("\"%s\" not managed. Nothing to do.", layer.name)
         return
-    if not preserve_others:
-        to_stop = []
-        for _layer in chain(_iter_descendants(layer), _iter_ascendants(layer)):
-            if not _layer.is_managed():
-                continue
-            if _layer.automountunit.is_active():
-                to_stop.append(_layer)
-        if to_stop:
-            if not stop_others:
-                logger.error(
-                    'You should prefere to stop the following overlays: \n\n%s\n',
-                    ', '.join(x.name for x in to_stop))
-                logger.info(
-                    'You may want to use `--stop-others`. '
-                    'Or see --help for more options.')
-                return
-            for _layer in to_stop:
-                _layer.disable() if permanent else _layer.stop()
-            logger.info(
-                'The following overlays were stopped: \n\n'
-                + ', '.join(x.name for x in to_stop))
-    layer.start(permanent)
+    with _stop_branch(layer, interrupt, preserve, restart=False, allbranch=True):
+        layer.start(permanent)
 
 
 def stoper(name, permanent=False):

          
@@ 754,7 753,7 @@ def lister(
         raise NoResult("No overlay matches.")
 
 
-def deplacer(oldname, newname):
+def deplacer(oldname, newname, interrupt=False, preserve=False):
     # Checks
     _new = Layer(newname)
 

          
@@ 768,38 767,22 @@ def deplacer(oldname, newname):
         logger.info('Not a new unit name, nothing to do.')
         return
 
-    descendants = list(_iter_descendants(layer))
-    logger.debug(
-        '%s overlays affected: %s', len(descendants),
-        ', '.join(x.name for x in descendants)
-    )
-    activated = [x.name for x in descendants if x.automountunit.is_active()]
-    if activated:
-        # XXX add --stop
-        logger.error(
-            'The following units must be stopped: %s', ', '.join(activated))
+    with _stop_branch(layer, interrupt=interrupt, preserve=preserve, restart=interrupt):
 
-    is_automonted = layer.automountunit.is_active()
-    if is_automonted:
-        layer.stop()
-
-    # Move overlay dirs.
-    layer.upperdir.rename(_new.upperdir)
-    layer.workdir.rename(_new.workdir)
+        # Move overlay dirs.
+        layer.upperdir.rename(_new.upperdir)
+        layer.workdir.rename(_new.workdir)
 
-    layer.delete()
+        layer.delete()
 
-    layer.name = newname
-    layer.dump()
-
-    # Update existing layers that depends on the moved one.
-    for layer in descendants:
+        layer.name = newname
         layer.dump()
 
-    systemctl.daemon_reload()
+        # Update existing layers that depends on the moved one.
+        for layer in _iter_descendants(layer):
+            layer.dump()
 
-    if is_automonted:
-        layer.start()
+        systemctl.daemon_reload()
 
 
 def _setup_logger(level):

          
@@ 854,6 837,23 @@ def main():
         help="available commands', description='Valid commands to manipulate overlays.",
     )
 
+    def add_interrupt_preserve_arguments(parser):
+        parser.add_argument(
+            '--interrupt', '-i',
+            action='store_true',
+            default=False,
+            help=('automatically interrupt descendants to prevent broken mount point. '
+                  'Restart them afterward.')
+        )
+        parser.add_argument(
+            '--preserve', '-K',
+            action='store_true',
+            default=False,
+            help=('do not check if descendant overlays are started '
+                  '(may produce inconsistant behaviours)')
+        )
+
+
     # create
     create = subparser.add_parser(
         'create',

          
@@ 1024,20 1024,7 @@ def main():
     edit.add_argument(
         '-d', '--delete', action='append', help="append the following names",
     )
-    edit.add_argument(
-        '--stop-others', '-s',
-        action='store_true',
-        default=False,
-        help=('automatically stop descendants to prevent broken mount point. '
-              'Restart them afterward.')
-    )
-    edit.add_argument(
-        '--keep-others', '-K',
-        action='store_true',
-        default=False,
-        help=('do not check if descendant overlays are started '
-              '(may produce inconsistant behaviours)')
-    )
+    add_interrupt_preserve_arguments(edit)
     edit.add_argument(
         'mountdir',
         metavar='MOUNTDIR',

          
@@ 1048,7 1035,8 @@ def main():
     )
 
     def _editer(args):
-        editer(args.mountdir, args.append, args.prepend, args.delete, args.stop_others, args.keep_others)
+        editer(args.mountdir, args.append, args.prepend, args.delete,
+               interrupt=args.interrupt, preserve=args.preserve)
 
     edit.set_defaults(func=_editer)
 

          
@@ 1060,11 1048,12 @@ def main():
             "move an overlay created by this tool. "
             "Other overlays that depends on it are updated"),
     )
+    add_interrupt_preserve_arguments(move)
     move.add_argument('old', metavar='OLD', help="existing overlay name.")
     move.add_argument('new', metavar='NEW', help="New overlay name.")
 
     def _deplacer(args):
-        deplacer(args.old, args.new)
+        deplacer(args.old, args.new, interrupt=args.interrupt, preserve=args.preserve)
 
     move.set_defaults(func=_deplacer)
 

          
@@ 1086,19 1075,7 @@ def main():
             "it is assumed to be %s/{MOUNTDIR}" % (os.path.sep, MOUNTDIR)
         ),
     )
-    start.add_argument(
-        '--stop-others', '-s',
-        action='store_true',
-        default=False,
-        help='stop ascendants and descendants if needed'
-    )
-    start.add_argument(
-        '--keep-others', '-K',
-        action='store_true',
-        default=False,
-        help=('do not check if descendant or ascendants overlays are started '
-              '(may produce inconsistant behaviours)')
-    )
+    add_interrupt_preserve_arguments(start)
     start.add_argument(
         '--permanent', '-p',
         action='store_true', default=False,

          
@@ 1106,7 1083,8 @@ def main():
     )
 
     def _start(args):
-        starter(args.mountdir, args.keep_others, args.stop_others, args.permanent)
+        starter(args.mountdir, interrupt=args.interrupt,
+                preserve=args.preserve, permanent=args.permanent)
 
     start.set_defaults(func=_start)
 

          
M test_overlayctl.py +15 -10
@@ 298,6 298,7 @@ class TestStart(BaseTest):
         self.get_mount_dir('archbase').mkdir()
         overlayctl.creater('layer1', ['archbase'])
         self.systemctl.reset_mock()
+        self.systemctl.status.return_value = {'Active': 'inactive'}
         overlayctl.starter('layer1')
         self.assert_layer_started('layer1')
         self.systemctl.enable.assert_not_called()

          
@@ 309,6 310,7 @@ class TestStart(BaseTest):
         self.get_mount_dir('archbase').mkdir()
         overlayctl.creater('layer1', ['archbase'])
         self.systemctl.reset_mock()
+        self.systemctl.status.return_value = {'Active': 'inactive'}
         overlayctl.starter('layer1', permanent=True)
         self.assert_layer_started('layer1')
         self.assert_layer_enabled('layer1')

          
@@ 328,24 330,25 @@ class TestStart(BaseTest):
         overlayctl.creater('layer1', ['archbase'])
         overlayctl.creater('layer2', ['layer1'])
         self.systemctl.reset_mock()
-        overlayctl.starter('layer1')
+        with self.assertRaises(SystemExit):
+            overlayctl.starter('layer1')
         self.systemctl.start.assert_not_called()
 
     def test_user_can_force_to_start_a_lower_with_active_descendants(self):
         # A layer with active_descendants can be started with the
-        # --preserve_others option. This may be dangereous, but the
+        # --preserve option. This may be dangereous, but the
         # user is the master.
         self.systemctl.status.side_effect = [{'Active': 'inactive'}]
         self.get_mount_dir('archbase').mkdir()
         overlayctl.creater('layer1', ['archbase'])
         overlayctl.creater('layer2', ['layer1'])
         self.systemctl.reset_mock()
-        overlayctl.starter('layer1', preserve_others=True)
+        overlayctl.starter('layer1', preserve=True)
         self.assert_layer_started('layer1')
 
     def test_starting_stops_descendants_if_needed(self):
         # Active descendant layers can be stopped automatically before
-        # starting the layer with the --stop-others options.
+        # starting the layer with the --interrupt options.
         self.systemctl.status.side_effect = [
             {'Active': 'active'}, {'Active': 'active'},
             {'Active': 'inactive'}]

          
@@ 354,7 357,7 @@ class TestStart(BaseTest):
         overlayctl.creater('layer2', ['layer1'])
         overlayctl.creater('layer3', ['layer2'])
         self.systemctl.reset_mock()
-        overlayctl.starter('layer1', stop_others=True)
+        overlayctl.starter('layer1', interrupt=True)
         self.assert_layer_stopped('layer2')
         self.assert_layer_stopped('layer3')
         self.assert_layer_started('layer1')

          
@@ 479,7 482,8 @@ class TestEdit(BaseTest):
         overlayctl.creater('layer', ['lower'])
         self.systemctl.reset_mock()
         self.systemctl.status.return_value = {'Active': 'active'}
-        overlayctl.editer('lower', removed=['base'])
+        with self.assertRaises(SystemExit):
+            overlayctl.editer('lower', removed=['base'])
         self.check_mount_unit('lower', [self.get_mount_dir('base')])
 
     def test_editing_layer_with_active_descendant(self):

          
@@ 490,7 494,7 @@ class TestEdit(BaseTest):
         overlayctl.creater('layer', ['lower'])
         self.systemctl.reset_mock()
         self.systemctl.status.return_value = {'Active': 'active'}
-        overlayctl.editer('lower', removed=['base'], keep_others=True)
+        overlayctl.editer('lower', removed=['base'], preserve=True)
         self.check_mount_unit('lower', [])
 
 

          
@@ 536,13 540,13 @@ class TestMove(BaseTest):
         self.assert_daemon_reload_called()
 
     def test_moved_layer_is_restarted(self):
-        # Moving an activated terminal layer is ok, It's still
+        # Moving an activated terminal layer is ok with --interrupt, It's still
         # activated at the end.
         self.get_mount_dir('base').mkdir()
         overlayctl.creater('layer1', ['base'])
         self.systemctl.reset_mock()
         self.systemctl.status.return_value = {'Active': 'active'}
-        overlayctl.deplacer('layer1', 'renamed')
+        overlayctl.deplacer('layer1', 'renamed', interrupt=True)
         self.systemctl.stop.assert_any_call(self.get_unit_name('layer1') + '.mount')
         self.systemctl.stop.assert_any_call(self.get_unit_name('layer1') + '.automount')
         self.systemctl.start.assert_any_call(self.get_unit_name('renamed') + '.automount')

          
@@ 565,7 569,8 @@ class TestMove(BaseTest):
         overlayctl.creater('layer', ['lower'])
         self.systemctl.reset_mock()
         self.systemctl.status.return_value = {'Active': 'active'}
-        overlayctl.deplacer('lower', 'newlower')
+        with self.assertRaises(SystemExit):
+            overlayctl.deplacer('lower', 'newlower')
 
     def assert_layer_not_exist(self, name):
         self.assertFalse(self.get_unit_path(name, 'mount').exists())