[ssm-devel] [PATCH v2] ssmlib: missing pwquality should not crash the whole ssm

[Jan Tulak <jtulak@re...>]

The pwquality python binding is needed only for creating encrypted
volumes, and not every distribution has it easily available. Simplify
the life of the users by requiring it only for the creation of encrypted
devices, and even then allow the user to force the password if pwquality
is not available.

Signed-off-by: Jan Tulak <jtulak@...>

---
CHANGES:
v2
* instead of disabling all creations, ask the user if ssm should force
the password
---
 ssmlib/main.py    | 27 ++++++++++++++++++++-------
 ssmlib/problem.py | 14 ++++++++++++--
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/ssmlib/main.py b/ssmlib/main.py
index 9bbffd0..d5b1ce5 100644
--- a/ssmlib/main.py
+++ b/ssmlib/main.py
@@ -24,13 +24,18 @@ import stat
 import atexit
 import argparse
 import getpass
-import pwquality
 from ssmlib import misc
 from ssmlib import problem
 
 # Import backends
 from ssmlib.backends import lvm, crypt, btrfs, md, multipath
 
+# conditional import of pwquality
+try:
+    import pwquality
+except ImportError:
+    pwquality = None
+
 EXTN = ['ext2', 'ext3', 'ext4']
 SUPPORTED_FS = ['xfs', 'btrfs'] + EXTN
 SUPPORTED_BACKENDS = ['lvm', 'btrfs', 'crypt', 'multipath']
@@ -1786,12 +1791,20 @@ class StorageHandle(object):
             raise problem.GeneralError("The passwords entered do not match.")
 
         # check password strength before we do anything
-        try:
-            pwq = pwquality.PWQSettings()
-            pwq.check(password)
-        except pwquality.PWQError as ex:
-            if not PR.check(PR.WEAK_PASSWORD, ex[1]):
-                raise problem.WeakPassword(ex[1])
+        if pwquality:
+            # pwquality is installed
+            try:
+                pwq = pwquality.PWQSettings()
+                pwq.check(password)
+            except pwquality.PWQError as ex:
+                if not PR.check(PR.WEAK_PASSWORD, ex[1]):
+                    raise problem.WeakPassword(ex[1])
+                else:
+                    force_weak_password = True
+        else:
+            # pwquality is not installed
+            if not PR.check(PR.TOOL_MISSING_PROMPT, ["Python binding for pwquality", "SSM can't verify password strength."]):
+                raise problem.ToolMissingPrompt("Python binding for pwquality is not installed, SSM couldn't verify password strength.")
             else:
                 force_weak_password = True
 
diff --git a/ssmlib/problem.py b/ssmlib/problem.py
index 9d7805a..838977d 100644
--- a/ssmlib/problem.py
+++ b/ssmlib/problem.py
@@ -21,8 +21,9 @@ import sys
 __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
            "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
            "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
-           "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
-           "NotImplemented", "WeakPassword", "ExistingSignature", "DuplicateTarget"]
+           "ToolMissingPrompt", "CanNotRun", "CommandFailed", "UserInterrupted",
+           "NotSupported", "NotImplemented", "WeakPassword",
+           "ExistingSignature", "DuplicateTarget"]
 
 # Define prompt codes
 PROMPT_NONE =           0
@@ -123,6 +124,11 @@ class ToolMissing(SsmError):
         super(ToolMissing, self).__init__(msg, errcode)
 
 
+class ToolMissingPrompt(SsmError):
+    def __init__(self, msg, errcode=2010):
+        super(ToolMissingPrompt, self).__init__(msg, errcode)
+
+
 class CanNotRun(SsmError):
     def __init__(self, msg, errcode=2011):
         super(CanNotRun, self).__init__(msg, errcode)
@@ -265,6 +271,10 @@ class ProblemSet(object):
             ['The password is too weak: {0}.\n',
              PROMPT_CONTINUE, FL_FORCE_YES | FL_DEFAULT_NO, WeakPassword]
 
+        self.TOOL_MISSING_PROMPT = \
+            ['\'{0}\' is not installed on the system! {1}',
+             PROMPT_CONTINUE, FL_FORCE_YES | FL_DEFAULT_NO, ToolMissingPrompt]
+
     def _can_print_message(self, flags):
         if (flags & FL_DEBUG_ONLY):
             return self.options.debug
-- 
2.18.0

Re: [ssm-devel] [PATCH] ssmlib: missing pwquality should not crash the whole ssm

[Jan Tulak <jtulak@re...>]

On Tue, Sep 25, 2018 at 4:38 PM Lukas Czerner <lczerner@...> wrote:
>
> On Tue, Sep 25, 2018 at 04:05:29PM +0200, Jan Tulak wrote:
> > @@ -1803,6 +1808,9 @@ class StorageHandle(object):
> >          provided as arguments. If the device is not in the selected pool, then
> >          add() is called on the pool prior to create().
> >          """
> > +        if pwquality is None:
> > +            PR.error("Cannot create encrypted volume: Python binding for pwquality is not installed.")
> > +
>
> Hi Jan,
>
> If I am reading this correctly it will disable creating _everything_ in the
> case pwquality is not present. Have you tested it ?

I did, but to my jellified brain, it didn't seem weird. :-)

>
> When I think about it it's also rather drastic solution. How about
> either forcing the password in this case, or informing the user that
> we're not able to determine the quality of the password, if he still
> wants to continue regardless - force the password.

Good idea with the confirmation. I did not like the need to force
everything just to get over it, but asking, and then enforcing only
the password, sounds reasonable.

Cheers,
Jan

Re: [ssm-devel] [PATCH] ssmlib: missing pwquality should not crash the whole ssm

[Lukas Czerner <lczerner@re...>]

On Tue, Sep 25, 2018 at 04:05:29PM +0200, Jan Tulak wrote:
> The pwquality python binding is needed only for creating encrypted
> volumes, and not every distribution has it easily available. Simplify
> the life of the users by requiring it only for the creation, but not for
> other ssm commands.
> 
> Signed-off-by: Jan Tulak <jtulak@...>
> ---
>  ssmlib/main.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/ssmlib/main.py b/ssmlib/main.py
> index 9bbffd0..07b80e9 100644
> --- a/ssmlib/main.py
> +++ b/ssmlib/main.py
> @@ -24,13 +24,18 @@ import stat
>  import atexit
>  import argparse
>  import getpass
> -import pwquality
>  from ssmlib import misc
>  from ssmlib import problem
>  
>  # Import backends
>  from ssmlib.backends import lvm, crypt, btrfs, md, multipath
>  
> +# conditional import of pwquality
> +try:
> +    import pwquality
> +except ImportError:
> +    pwquality = None
> +
>  EXTN = ['ext2', 'ext3', 'ext4']
>  SUPPORTED_FS = ['xfs', 'btrfs'] + EXTN
>  SUPPORTED_BACKENDS = ['lvm', 'btrfs', 'crypt', 'multipath']
> @@ -1803,6 +1808,9 @@ class StorageHandle(object):
>          provided as arguments. If the device is not in the selected pool, then
>          add() is called on the pool prior to create().
>          """
> +        if pwquality is None:
> +            PR.error("Cannot create encrypted volume: Python binding for pwquality is not installed.")
> +

Hi Jan,

If I am reading this correctly it will disable creating _everything_ in the
case pwquality is not present. Have you tested it ?

When I think about it it's also rather drastic solution. How about
either forcing the password in this case, or informing the user that
we're not able to determine the quality of the password, if he still
wants to continue regardless - force the password.

What do you think ?

-Lukas

>          if args.mnt_options and not self._mpoint:
>              PR.warn("Mount options are set, but no mount point was " +
>                      "provided. Device will not be mounted")
> -- 
> 2.18.0
> 
> 
> 
> _______________________________________________
> storagemanager-devel mailing list
> storagemanager-devel@...
> https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)

[ssm-devel] [PATCH] ssmlib: missing pwquality should not crash the whole ssm

[Jan Tulak <jtulak@re...>]

The pwquality python binding is needed only for creating encrypted
volumes, and not every distribution has it easily available. Simplify
the life of the users by requiring it only for the creation, but not for
other ssm commands.

Signed-off-by: Jan Tulak <jtulak@...>
---
 ssmlib/main.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ssmlib/main.py b/ssmlib/main.py
index 9bbffd0..07b80e9 100644
--- a/ssmlib/main.py
+++ b/ssmlib/main.py
@@ -24,13 +24,18 @@ import stat
 import atexit
 import argparse
 import getpass
-import pwquality
 from ssmlib import misc
 from ssmlib import problem
 
 # Import backends
 from ssmlib.backends import lvm, crypt, btrfs, md, multipath
 
+# conditional import of pwquality
+try:
+    import pwquality
+except ImportError:
+    pwquality = None
+
 EXTN = ['ext2', 'ext3', 'ext4']
 SUPPORTED_FS = ['xfs', 'btrfs'] + EXTN
 SUPPORTED_BACKENDS = ['lvm', 'btrfs', 'crypt', 'multipath']
@@ -1803,6 +1808,9 @@ class StorageHandle(object):
         provided as arguments. If the device is not in the selected pool, then
         add() is called on the pool prior to create().
         """
+        if pwquality is None:
+            PR.error("Cannot create encrypted volume: Python binding for pwquality is not installed.")
+
         if args.mnt_options and not self._mpoint:
             PR.warn("Mount options are set, but no mount point was " +
                     "provided. Device will not be mounted")
-- 
2.18.0

[ssm-devel] [PATCH] lvm: index thinpools as vg-lv pair instead of just lv name

[Jan Tulak <jtulak@re...>]

This fixes an issue where ssm incorrectly displayed only one of
the same-name thinpools in different VGs.

Eg. in vg1/thinpool and vg2/thinpool, ssm saw only vg1/thinpool.

Signed-off-by: Jan Tulak <jtulak@...>
---
 ssmlib/backends/lvm.py          |  2 +-
 tests/bashtests/011-lvm-list.sh | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
index ce97672..925ec6a 100644
--- a/ssmlib/backends/lvm.py
+++ b/ssmlib/backends/lvm.py
@@ -612,7 +612,7 @@ class ThinPool(LvmInfo, template.BackendPool):
 
 
     def _data_index(self, row):
-        return row['pool_name']
+        return "{}/{}".format(row['parent_pool'], row['pool_name'])
 
     def _skip_data(self, row):
         if row['attr'][0] not in ['t', 'T']:
diff --git a/tests/bashtests/011-lvm-list.sh b/tests/bashtests/011-lvm-list.sh
index 80d3ea0..027c5fc 100755
--- a/tests/bashtests/011-lvm-list.sh
+++ b/tests/bashtests/011-lvm-list.sh
@@ -43,6 +43,8 @@ pool1=$vg2
 pool2=$vg3
 pool3=$vg4
 
+tpool=${SSM_PREFIX_FILTER}_thinpool
+
 TEST_FS=
 which mkfs.ext2 && TEST_FS+="ext2 "
 which mkfs.ext3 && TEST_FS+="ext3 "
@@ -338,3 +340,21 @@ ssm -f remove $SSM_LVM_DEFAULT_POOL
 
 # Some situation should fail
 not ssm list wrong_type
+
+# test for same thinpool name in different VGs
+ssm create -v 1G -n $lvol1 -p $vg1 $dev1
+ssm create -v 1G -n $lvol1 -p $vg2 $dev2
+
+# ssm creates thinpool name automatically as ${vg}_${lv}
+# so we haveto rename it here to the same name in both VGs
+lvrename $vg1 ${vg1}_thin001 $tpool
+lvrename $vg2 ${vg2}_thin001 $tpool
+# check command has issues with multiple occurencies of the same name
+out=$(ssm list pools)
+echo "$out" | grep -c "^$tpool.*$vg1$"
+echo "$out" | grep -c "^$tpool.*$vg2$"
+
+# and remove it
+ssm -f remove $vg1/$tpool
+ssm -f remove $vg2/$tpool
+ssm -f remove --all
-- 
2.18.0

[ssm-devel] Release 1.1

[Jan Tulak <jtulak@re...>]

A new SSM v1.1 was released.

Changes:
- unittests: expand migrate testing (Jan Tulak)
- migrate: check for source and target being the same device (Jan Tulak)
- ssm: Rework migrate command (Lukas Czerner)
- crypt: crypt backend need to have a pool in database (Lukas Czerner)
- lvm: Remove pool_name key if pv not in pool (Lukas Czerner)
- lvm: fix backtrace when vg/lv is inactive or broken (Lukas Czerner)
- tests: better name matching for test.py (Jan Tulak)
- resize: fix extN crash when no size was provided (Jan Tulak)
- tests: replace exclamation mark with not (Jan Tulak)
- btrfs: send udev change event after some more btrfs operations (Lukas Czerner)
- btrfs: Send udev change event for every device after fs creation
(Lukas Czerner)
- mount: Check the volume argument in parser (Lukas Czerner)
- docs: add a manpage to the root (Jan Tulak)
- ssmlib: use pwquality to test password strength for cryptsetup (Jan Tulak)
- multipath: make all mp objects to be devices (Jan Tulak)
- ssmlib: info with non-existent target exits with err code (Jan Tulak)
- docs: add missing commands into the docs (Jan Tulak)
- Revert "tests: add a test for exported lvm volume" (Lukas Czerner)
- tests: Test ssm list with exported volume groups (Lukas Czerner)
- lvm: Move lvm specific error handling to lvm backend (Lukas Czerner)
- misc: Return stderr output in run() as well (Lukas Czerner)
- btrfs: Avoid unnecessary loops when initializing data (Lukas Czerner)

-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH] migrate: check for source and target being the same device

[Jan Tulak <jtulak@re...>]

On Fri, Aug 10, 2018 at 11:37 AM Lukas Czerner <lczerner@...> wrote:
>
> On Thu, Aug 09, 2018 at 02:59:42PM +0200, Jan Tulak wrote:
> > Abort when we find out that source and target are the same device.
>
> Good catch. Thanks!
>
> Reviewed-by: Lukas Czerner <lczerner@...>
>
> Please add it to bash test as well.
>

ok, thanks

> -Lukas
> >
> > Signed-off-by: Jan Tulak <jtulak@...>
> > ---
> > Applies after [PATCH 4/4] ssm: Rework migrate command
> > ---
> >
> >  ssmlib/main.py    |  4 ++++
> >  ssmlib/problem.py | 10 +++++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/ssmlib/main.py b/ssmlib/main.py
> > index 43fa586..b006b90 100644
> > --- a/ssmlib/main.py
> > +++ b/ssmlib/main.py
> > @@ -2130,6 +2130,10 @@ class StorageHandle(object):
> >          if target and 'pool_name' in target:
> >              target_pool = self.pool[target['pool_name']]
> >
> > +        if source.name == target.name:
> > +            raise problem.DuplicateTarget(
> > +                "The source and target can't be the same device! ({})".format(target.name))
> > +
> >          if target_pool:
> >              if not source_pool or (target_pool.name != source_pool.name):
> >                  if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> > diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> > index b76d738..0a5cfc8 100644
> > --- a/ssmlib/problem.py
> > +++ b/ssmlib/problem.py
> > @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
> >             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
> >             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
> >             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> > -           "NotImplemented", "ExistingSignature"]
> > +           "NotImplemented", "ExistingSignature", "DuplicateTarget"]
> >
> >  # Define prompt codes
> >  PROMPT_NONE =           0
> > @@ -158,6 +158,10 @@ class ExistingSignature(SsmError):
> >      def __init__(self, msg, errcode=2017):
> >          super(ExistingSignature, self).__init__(msg, errcode)
> >
> > +class DuplicateTarget(SsmError):
> > +    def __init__(self, msg, errcode=2018):
> > +        super(DuplicateTarget, self).__init__(msg, errcode)
> > +
> >
> >  class ProblemSet(object):
> >
> > @@ -242,6 +246,10 @@ class ProblemSet(object):
> >              ['{0} is not supported!',
> >               PROMPT_NONE, FL_FATAL, NotSupported]
> >
> > +        self.DUPLICATE_TARGET = \
> > +            ['Source and target can\'t be the same device! ({0})',
> > +             PROMPT_NONE, FL_FATAL, DuplicateTarget]
> > +
> >          self.NOT_IMPLEMENTED = \
> >              ['\'{0}\' functionality is not implemented by {1}!',
> >              PROMPT_NONE, FL_FATAL, NotImplemented]
> > --
> > 2.16.2
> >



-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH] migrate: check for source and target being the same device

[Lukas Czerner <lczerner@re...>]

On Thu, Aug 09, 2018 at 02:59:42PM +0200, Jan Tulak wrote:
> Abort when we find out that source and target are the same device.

Good catch. Thanks!

Reviewed-by: Lukas Czerner <lczerner@...>

Please add it to bash test as well.

-Lukas
> 
> Signed-off-by: Jan Tulak <jtulak@...>
> ---
> Applies after [PATCH 4/4] ssm: Rework migrate command
> ---
> 
>  ssmlib/main.py    |  4 ++++
>  ssmlib/problem.py | 10 +++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/ssmlib/main.py b/ssmlib/main.py
> index 43fa586..b006b90 100644
> --- a/ssmlib/main.py
> +++ b/ssmlib/main.py
> @@ -2130,6 +2130,10 @@ class StorageHandle(object):
>          if target and 'pool_name' in target:
>              target_pool = self.pool[target['pool_name']]
>  
> +        if source.name == target.name:
> +            raise problem.DuplicateTarget(
> +                "The source and target can't be the same device! ({})".format(target.name))
> +
>          if target_pool:
>              if not source_pool or (target_pool.name != source_pool.name):
>                  if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> index b76d738..0a5cfc8 100644
> --- a/ssmlib/problem.py
> +++ b/ssmlib/problem.py
> @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
>             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
>             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
>             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> -           "NotImplemented", "ExistingSignature"]
> +           "NotImplemented", "ExistingSignature", "DuplicateTarget"]
>  
>  # Define prompt codes
>  PROMPT_NONE =           0
> @@ -158,6 +158,10 @@ class ExistingSignature(SsmError):
>      def __init__(self, msg, errcode=2017):
>          super(ExistingSignature, self).__init__(msg, errcode)
>  
> +class DuplicateTarget(SsmError):
> +    def __init__(self, msg, errcode=2018):
> +        super(DuplicateTarget, self).__init__(msg, errcode)
> +
>  
>  class ProblemSet(object):
>  
> @@ -242,6 +246,10 @@ class ProblemSet(object):
>              ['{0} is not supported!',
>               PROMPT_NONE, FL_FATAL, NotSupported]
>  
> +        self.DUPLICATE_TARGET = \
> +            ['Source and target can\'t be the same device! ({0})',
> +             PROMPT_NONE, FL_FATAL, DuplicateTarget]
> +
>          self.NOT_IMPLEMENTED = \
>              ['\'{0}\' functionality is not implemented by {1}!',
>              PROMPT_NONE, FL_FATAL, NotImplemented]
> -- 
> 2.16.2
> 

Re: [ssm-devel] [PATCH] migrate: check for source and target being the same device

[Jan Tulak <jtulak@re...>]

On Thu, Aug 9, 2018 at 2:59 PM Jan Tulak <jtulak@...> wrote:
>
> Abort when we find out that source and target are the same device.
>

This bug has a regression test in the unittests patch I just submitted.
(unittests: expand migrate testing)


> Signed-off-by: Jan Tulak <jtulak@...>
> ---
> Applies after [PATCH 4/4] ssm: Rework migrate command
> ---
>
>  ssmlib/main.py    |  4 ++++
>  ssmlib/problem.py | 10 +++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/ssmlib/main.py b/ssmlib/main.py
> index 43fa586..b006b90 100644
> --- a/ssmlib/main.py
> +++ b/ssmlib/main.py
> @@ -2130,6 +2130,10 @@ class StorageHandle(object):
>          if target and 'pool_name' in target:
>              target_pool = self.pool[target['pool_name']]
>
> +        if source.name == target.name:
> +            raise problem.DuplicateTarget(
> +                "The source and target can't be the same device! ({})".format(target.name))
> +
>          if target_pool:
>              if not source_pool or (target_pool.name != source_pool.name):
>                  if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> index b76d738..0a5cfc8 100644
> --- a/ssmlib/problem.py
> +++ b/ssmlib/problem.py
> @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
>             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
>             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
>             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> -           "NotImplemented", "ExistingSignature"]
> +           "NotImplemented", "ExistingSignature", "DuplicateTarget"]
>
>  # Define prompt codes
>  PROMPT_NONE =           0
> @@ -158,6 +158,10 @@ class ExistingSignature(SsmError):
>      def __init__(self, msg, errcode=2017):
>          super(ExistingSignature, self).__init__(msg, errcode)
>
> +class DuplicateTarget(SsmError):
> +    def __init__(self, msg, errcode=2018):
> +        super(DuplicateTarget, self).__init__(msg, errcode)
> +
>
>  class ProblemSet(object):
>
> @@ -242,6 +246,10 @@ class ProblemSet(object):
>              ['{0} is not supported!',
>               PROMPT_NONE, FL_FATAL, NotSupported]
>
> +        self.DUPLICATE_TARGET = \
> +            ['Source and target can\'t be the same device! ({0})',
> +             PROMPT_NONE, FL_FATAL, DuplicateTarget]
> +
>          self.NOT_IMPLEMENTED = \
>              ['\'{0}\' functionality is not implemented by {1}!',
>              PROMPT_NONE, FL_FATAL, NotImplemented]
> --
> 2.16.2
>


--
Jan Tulak
jtulak@... / jan@...

[ssm-devel] [PATCH] unittests: expand migrate testing

[Jan Tulak <jtulak@re...>]

Add unittests for migrate testing.

Signed-off-by: Jan Tulak <jtulak@...>
---
 tests/unittests/test_btrfs.py | 21 ++++++++++++++-----
 tests/unittests/test_lvm.py   | 20 ++++++++++++++++--
 tests/unittests/test_ssm.py   | 49 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/tests/unittests/test_btrfs.py b/tests/unittests/test_btrfs.py
index 936931d..36db292 100644
--- a/tests/unittests/test_btrfs.py
+++ b/tests/unittests/test_btrfs.py
@@ -452,14 +452,25 @@ class BtrfsFunctionCheck(MockSystemDataSource):
 
     def test_btrfs_migrate(self):
         # Generate some storage data
-        self._addPool('my_pool', ['/dev/sdc1', '/dev/sdc2', '/dev/sdc3'])
-        self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
+        self._addDevice('/dev/sdd1', 11489037516)
+        self._addDevice('/dev/sdd2', 11489037516)
         self._addDevice('/dev/sde', 11489037516)
+        self._addPool('my_pool', ['/dev/sdc1', '/dev/sdc2'])
+        self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
+        self._addPool('my_pool2', ['/dev/sdd1', '/dev/sdd2'])
+        self._addVol('vol002', 237284225, 1, 'my_pool2', ['/dev/sdd1'])
 
-        try:
+        with self.assertRaises(problem.DeviceUsed):
             main.main("ssm migrate /dev/sdc1 /dev/sdc2")
-        except problem.DeviceUsed:
-            pass
+        with self.assertRaises(problem.UserInterrupted):
+            main.main("ssm migrate /dev/sde /dev/sdc2")
 
         self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
         "btrfs replace start -B /dev/sdc2 /dev/sde /tmp/mount")
+
+        # test moving a dev from one fs to another
+        with self.assertRaises(problem.UserInterrupted):
+            main.main("ssm migrate /dev/sdc1 /dev/sdd2")
+        self._checkCmd("ssm -f migrate /dev/sdc1 /dev/sdd2", [],
+            "btrfs replace start -f -B /dev/sdc1 /dev/sdd2 /tmp/mount")
+        self.assertTrue("btrfs device delete /dev/sdd2 /tmp/mount" in self.run_data)
diff --git a/tests/unittests/test_lvm.py b/tests/unittests/test_lvm.py
index c3b1142..6489ba8 100644
--- a/tests/unittests/test_lvm.py
+++ b/tests/unittests/test_lvm.py
@@ -252,12 +252,29 @@ class LvmFunctionCheck(MockSystemDataSource):
 
     def test_lvm_migrate(self):
         # Generate some storage data
+        self._addDevice('/dev/sdd1', 11489037516)
+        self._addDevice('/dev/sdd2', 11489037516)
+        self._addDevice('/dev/sde', 11489037516)
         self._addPool('my_pool', ['/dev/sdc1', '/dev/sdc2'])
         self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
-        self._addDevice('/dev/sde', 11489037516)
+        self._addPool('my_pool2', ['/dev/sdd1', '/dev/sdd2'])
+        self._addVol('vol002', 237284225, 1, 'my_pool2', ['/dev/sdd1'])
 
         self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
             "lvm pvmove --atomic /dev/sdc1 /dev/sdc2")
+        self._checkCmd("ssm migrate /dev/sdc1 /dev/sde", [],
+            "lvm pvmove --atomic /dev/sdc1 /dev/sde")
+        with self.assertRaises(problem.UserInterrupted):
+            main.main("ssm migrate /dev/sde /dev/sdc2")
+
+        # test moving a pv from one pool to another
+        with self.assertRaises(problem.UserInterrupted):
+            main.main("ssm migrate /dev/sdc1 /dev/sdd2")
+        self._checkCmd("ssm -f migrate /dev/sdc1 /dev/sdd2", [],
+            "lvm pvmove --atomic /dev/sdc1 /dev/sdd2")
+        self.assertTrue("lvm vgreduce -f my_pool2 /dev/sdd2" in self.run_data)
+        self.assertTrue("lvm vgextend -f my_pool /dev/sdd2" in self.run_data)
+
 
     def test_lvm_resize(self):
         # Generate some storage data
@@ -446,4 +463,3 @@ class LvmFunctionCheck(MockSystemDataSource):
         self._cmdEq("mount -o discard /dev/default_pool/vol001 /mnt/test")
         main.main("ssm mount --options rw,discard,neco=44 /dev/my_pool/vol002 /mnt/test1")
         self._cmdEq("mount -o rw,discard,neco=44 /dev/my_pool/vol002 /mnt/test1")
-
diff --git a/tests/unittests/test_ssm.py b/tests/unittests/test_ssm.py
index 0e4920e..ed4bf9c 100644
--- a/tests/unittests/test_ssm.py
+++ b/tests/unittests/test_ssm.py
@@ -315,6 +315,9 @@ class SsmFunctionCheck(MockSystemDataSource):
         self._addDevice('/dev/sdc2', 29826161, 2)
         self._addDevice('/dev/sdc3', 1042177280, 3)
         self._addDevice('/dev/sdd', 11673)
+        self._addDevice('/dev/sde', 11673)
+        self._addDevice('/dev/sdf', 11673)
+        self._addDevice('/dev/sdg', 11673)
 
     def tearDown(self):
         super(SsmFunctionCheck, self).tearDown()
@@ -777,6 +780,43 @@ class SsmFunctionCheck(MockSystemDataSource):
         main.main("ssm mount --options rw,discard,neco=44 /dev/my_pool/vol002 /mnt/test1")
         self._cmdEq("mount -o rw,discard,neco=44 /dev/my_pool/vol002 /mnt/test1")
 
+    def test_migrate(self):
+        # Generate some storage data
+        self._addPool('default_pool', ['/dev/sda', '/dev/sdb'])
+        self._addPool('my_pool', ['/dev/sdc2', '/dev/sdc3', '/dev/sdc1'])
+        self._addVol('vol001', 117283225, 1, 'default_pool', ['/dev/sda'])
+        self._addVol('vol002', 237284225, 1, 'default_pool', ['/dev/sda'])
+        self._addVol('vol003', 1024, 1, 'default_pool', ['/dev/sdb'],)
+        self._addVol('vol004', 209715200, 2, 'default_pool', ['/dev/sda',
+                     '/dev/sdb'])
+
+        # migrate a pv to an unused device
+        main.main("ssm migrate /dev/sda /dev/sdd")
+        self._cmdEq("migrate default_pool /dev/sda /dev/sdd")
+        # migrate data between pvs within a vg
+        main.main("ssm migrate /dev/sda /dev/sdb")
+        self._cmdEq("migrate default_pool /dev/sda /dev/sdb")
+
+        # try a used device without -f
+        with self.assertRaises(problem.UserInterrupted):
+            main.main("ssm migrate /dev/sdc2 /dev/sda")
+
+        # again, this time with -f
+        main.main("ssm -f migrate /dev/sdc2 /dev/sda")
+        self._cmdEq("force migrate my_pool /dev/sdc2 /dev/sda")
+
+        # migrate raw devices
+        main.main("ssm migrate /dev/sdf /dev/sdg")
+        self._cmdEq("dd if=/dev/sdf of=/dev/sdg conv=fsync")
+
+        with self.assertRaises(SystemExit):
+            main.main("ssm migrate /dev/sdc2")
+        with self.assertRaises(SystemExit):
+            main.main("ssm migrate /dev/foo /dev/bar")
+        with self.assertRaises(SystemExit):
+            main.main("ssm migrate /dev/sda /dev/sdb /dev/sde")
+        with self.assertRaises(problem.DuplicateTarget):
+            main.main("ssm migrate /dev/sda /dev/sda")
 
 class MyInfo(object):
     def __init__(self, options, data=None):
@@ -835,6 +875,10 @@ class PoolInfo(MyInfo):
         cmd.extend(devices)
         misc.run(cmd)
 
+    def migrate(self, pool, source, target):
+        cmd = [self.f, self.v, self.y, 'migrate', pool, source.name, target]
+        misc.run(cmd)
+
     def create(self, pool, size='', name='', devs='',
                options=None):
         options = options or {}
@@ -887,6 +931,11 @@ class DevInfo(MyInfo):
         cmd.extend(devices)
         misc.run(cmd)
 
+    def migrate(self, source, target):
+        cmd = [self.f, self.v, self.y, 'migrate', source, target]
+        misc.run(cmd)
+
+
 
 class Pool(main.Storage):
     def __init__(self, *args, **kwargs):
-- 
2.16.2

[ssm-devel] [PATCH] migrate: check for source and target being the same device

[Jan Tulak <jtulak@re...>]

Abort when we find out that source and target are the same device.

Signed-off-by: Jan Tulak <jtulak@...>
---
Applies after [PATCH 4/4] ssm: Rework migrate command
---

 ssmlib/main.py    |  4 ++++
 ssmlib/problem.py | 10 +++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/ssmlib/main.py b/ssmlib/main.py
index 43fa586..b006b90 100644
--- a/ssmlib/main.py
+++ b/ssmlib/main.py
@@ -2130,6 +2130,10 @@ class StorageHandle(object):
         if target and 'pool_name' in target:
             target_pool = self.pool[target['pool_name']]
 
+        if source.name == target.name:
+            raise problem.DuplicateTarget(
+                "The source and target can't be the same device! ({})".format(target.name))
+
         if target_pool:
             if not source_pool or (target_pool.name != source_pool.name):
                 if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
diff --git a/ssmlib/problem.py b/ssmlib/problem.py
index b76d738..0a5cfc8 100644
--- a/ssmlib/problem.py
+++ b/ssmlib/problem.py
@@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
            "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
            "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
            "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
-           "NotImplemented", "ExistingSignature"]
+           "NotImplemented", "ExistingSignature", "DuplicateTarget"]
 
 # Define prompt codes
 PROMPT_NONE =           0
@@ -158,6 +158,10 @@ class ExistingSignature(SsmError):
     def __init__(self, msg, errcode=2017):
         super(ExistingSignature, self).__init__(msg, errcode)
 
+class DuplicateTarget(SsmError):
+    def __init__(self, msg, errcode=2018):
+        super(DuplicateTarget, self).__init__(msg, errcode)
+
 
 class ProblemSet(object):
 
@@ -242,6 +246,10 @@ class ProblemSet(object):
             ['{0} is not supported!',
              PROMPT_NONE, FL_FATAL, NotSupported]
 
+        self.DUPLICATE_TARGET = \
+            ['Source and target can\'t be the same device! ({0})',
+             PROMPT_NONE, FL_FATAL, DuplicateTarget]
+
         self.NOT_IMPLEMENTED = \
             ['\'{0}\' functionality is not implemented by {1}!',
             PROMPT_NONE, FL_FATAL, NotImplemented]
-- 
2.16.2

Re: [ssm-devel] [PATCH] tests: better name matching for test.py

[Lukas Czerner <lczerner@re...>]

On Thu, Aug 09, 2018 at 12:00:36PM +0200, Jan Tulak wrote:
> Change the name matching to be more useful - for bash tests, it now
> takes also the number (no need to type a whole name). For unittests, it
> is no longer necessary to spell out the whole name too. Instead of
> 'tests.unittests.test_btrfs.BtrfsFunctionCheck.test_btrfs_resize'
> it is enough to just type 'BtrfsFunctionCheck.test_btrfs_resize'.
> 
> The --help description was adequately expanded.
> 
> Signed-off-by: Jan Tulak <jtulak@...>
> ---
>  test.py | 49 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/test.py b/test.py
> index 0183477..240feea 100755
> --- a/test.py
> +++ b/test.py
> @@ -32,6 +32,7 @@ from ssmlib import main
>  from ssmlib import misc
>  from ssmlib.backends import lvm, crypt, btrfs, multipath
>  
> +import tests.unittests as tests_module
>  from tests.unittests import *
>  
>  def run_bash_tests(names):
> @@ -51,7 +52,7 @@ def run_bash_tests(names):
>          script = script.strip()
>          if not re.match("^\d\d\d-.*\.sh$", script):
>              continue
> -        if names and script not in names:
> +        if names and script not in names and script[:3] not in names:
>              continue
>          count += 1
>          sys.stdout.write("{0:<29}".format(script) + " ")
> @@ -119,13 +120,43 @@ def unit_tests(names):
>  
>      if names:
>          for name in names:
> -            if name[-3:] == ".sh":
> +            if name[-3:] == ".sh" or name.isdigit():
> +                # bash test, skip here
>                  continue
> +
> +            # first try a full name
> +            try:
> +                tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(
> +                    name)])
> +                continue
> +            except (ImportError, AttributeError):
> +                pass
> +
> +            # then a name with the prefix omitted
>              try:
> -                tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(name)])
> -            except:
> -                # maybe the name is in bash tests, just skip...
> +                tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(
> +                    "tests.unittests." + name)])
> +                continue
> +            except (ImportError, AttributeError):
>                  pass
> +
> +            # ok, maybe even the file name was omitted, so we are down to class.method name
> +            tests_classes = [cname for cname in dir(tests_module) if cname[:5] == 'test_']
> +            found = False
> +            for c in tests_classes:
> +                try:
> +                    tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(
> +                        "tests.unittests.{}.{}".format(c,name))])
> +                    found = True
> +                    break
> +                except (ImportError, AttributeError):
> +                    pass
> +            if found:
> +                continue
> +
> +            # still nothing found... it might be a method only, but TODO that
> +            print("Warning: Test {} was not found.".format(name))
> +
>          if tests.countTestCases() == 0:
>              print("[+] No unittest matches the name(s)")
>              return
> @@ -143,14 +174,16 @@ def unit_tests(names):
>  
>  if __name__ == '__main__':
>      result = 0
> -    parser = argparse.ArgumentParser(description="Run the test suite for SSM."
> -            "If both --bash and --unit arguments are ommited, run both groups.")
> +    parser = argparse.ArgumentParser(description="Run the test suite for SSM. "
> +            "If both --bash and --unit arguments are ommited, run both groups. "
> +            "If a test name is specified, only matching tests are run.")
>      parser.add_argument('-b', '--bash', dest='bash', action='store_true',
>                      help='run only bash tests')
>      parser.add_argument('-u', '--unit', dest='unit', action='store_true',
>                      help='run only unit tests')
>      parser.add_argument('tests', metavar='TEST', type=str, nargs='*',
> -                    help='specific tests to be run')
> +                    help='Specific tests to be run. For bash tests, that means either a full name (001-foo.sh), or just the number. '
> +                         'For unit tests, it means something like BtrfsFunctionCheck.test_btrfs_resize for a specific test, BtrfsFunctionCheck for specific test suite and test_btrfs for a whole file of tests.')

Lines need to be wrapped here. Other than that it looks good.

Reviewed-by: Lukas Czerner <lczerner@...>


Thanks!
-Lukas

>  
>      args = parser.parse_args()
>      run_all = not args.unit and not args.bash
> -- 
> 2.16.2
> 

[ssm-devel] [PATCH] tests: better name matching for test.py

[Jan Tulak <jtulak@re...>]

Change the name matching to be more useful - for bash tests, it now
takes also the number (no need to type a whole name). For unittests, it
is no longer necessary to spell out the whole name too. Instead of
'tests.unittests.test_btrfs.BtrfsFunctionCheck.test_btrfs_resize'
it is enough to just type 'BtrfsFunctionCheck.test_btrfs_resize'.

The --help description was adequately expanded.

Signed-off-by: Jan Tulak <jtulak@...>
---
 test.py | 49 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/test.py b/test.py
index 0183477..240feea 100755
--- a/test.py
+++ b/test.py
@@ -32,6 +32,7 @@ from ssmlib import main
 from ssmlib import misc
 from ssmlib.backends import lvm, crypt, btrfs, multipath
 
+import tests.unittests as tests_module
 from tests.unittests import *
 
 def run_bash_tests(names):
@@ -51,7 +52,7 @@ def run_bash_tests(names):
         script = script.strip()
         if not re.match("^\d\d\d-.*\.sh$", script):
             continue
-        if names and script not in names:
+        if names and script not in names and script[:3] not in names:
             continue
         count += 1
         sys.stdout.write("{0:<29}".format(script) + " ")
@@ -119,13 +120,43 @@ def unit_tests(names):
 
     if names:
         for name in names:
-            if name[-3:] == ".sh":
+            if name[-3:] == ".sh" or name.isdigit():
+                # bash test, skip here
                 continue
+
+            # first try a full name
+            try:
+                tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(
+                    name)])
+                continue
+            except (ImportError, AttributeError):
+                pass
+
+            # then a name with the prefix omitted
             try:
-                tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(name)])
-            except:
-                # maybe the name is in bash tests, just skip...
+                tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(
+                    "tests.unittests." + name)])
+                continue
+            except (ImportError, AttributeError):
                 pass
+
+            # ok, maybe even the file name was omitted, so we are down to class.method name
+            tests_classes = [cname for cname in dir(tests_module) if cname[:5] == 'test_']
+            found = False
+            for c in tests_classes:
+                try:
+                    tests = unittest.TestSuite([tests, test_loader.loadTestsFromName(
+                        "tests.unittests.{}.{}".format(c,name))])
+                    found = True
+                    break
+                except (ImportError, AttributeError):
+                    pass
+            if found:
+                continue
+
+            # still nothing found... it might be a method only, but TODO that
+            print("Warning: Test {} was not found.".format(name))
+
         if tests.countTestCases() == 0:
             print("[+] No unittest matches the name(s)")
             return
@@ -143,14 +174,16 @@ def unit_tests(names):
 
 if __name__ == '__main__':
     result = 0
-    parser = argparse.ArgumentParser(description="Run the test suite for SSM."
-            "If both --bash and --unit arguments are ommited, run both groups.")
+    parser = argparse.ArgumentParser(description="Run the test suite for SSM. "
+            "If both --bash and --unit arguments are ommited, run both groups. "
+            "If a test name is specified, only matching tests are run.")
     parser.add_argument('-b', '--bash', dest='bash', action='store_true',
                     help='run only bash tests')
     parser.add_argument('-u', '--unit', dest='unit', action='store_true',
                     help='run only unit tests')
     parser.add_argument('tests', metavar='TEST', type=str, nargs='*',
-                    help='specific tests to be run')
+                    help='Specific tests to be run. For bash tests, that means either a full name (001-foo.sh), or just the number. '
+                         'For unit tests, it means something like BtrfsFunctionCheck.test_btrfs_resize for a specific test, BtrfsFunctionCheck for specific test suite and test_btrfs for a whole file of tests.')
 
     args = parser.parse_args()
     run_all = not args.unit and not args.bash
-- 
2.16.2

Re: [ssm-devel] [PATCH 4/4] ssm: Rework migrate command

[Jan Tulak <jtulak@re...>]

On Wed, Aug 8, 2018 at 4:05 PM Lukas Czerner <lczerner@...> wrote:

snip

> > > -        command = ['pvmove', '--atomic', source.name, target ]
> > > -        self.run_lvm(command)
> > > -        self.reduce(vg, source.name)
> > > -        command = ['pvremove', source.name]
> > > -        self.run_lvm(command)
> > > +        # If the source is not used we do not have to do anything
> > > +        if float(source['dev_used']) > 0.0:
> > > +            command = ['pvmove', '--atomic', source.name, target ]
> > > +            self.run_lvm(command)
> > >
> > >          misc.send_udev_event(source.name, "change")
> > >          misc.send_udev_event(target, "change")
> >
> > I would include the udev changes inside of the condition - the devices
> > are not changed outside of it.
>
> While it's true for the source device, it's not necessarily true
> for the target device.
>
> I do not see any harm in leaving it here, but I can move the source
> inside the condition. Also we're not very consistent with it throughout
> the ssm backends so we should probably do something about it.
>
>

OK, in that case, it doesn't matter.

Thanks,
Jan

Re: [ssm-devel] [PATCH 4/4] ssm: Rework migrate command

[Lukas Czerner <lczerner@re...>]

On Tue, Aug 07, 2018 at 05:15:19PM +0200, Jan Tulak wrote:
> On Thu, Aug 2, 2018 at 5:00 PM Lukas Czerner <lczerner@...> wrote:
> >
> > Migrate command was not working as expected in many ways so here is a
> > rework of it. The 018 bashtest is also reworked and significantly
> > extended to cover more possible scenarios and test the actual results of
> > the operation.
> >
> > Lvm migrate now does not remove the pv from the volume group as I feel
> > there are perfectly valid scenarios where user would like to migrate
> > used pv to a different device and repurpose the source device.
> >
> > unittests for lvm and btrfs are only changed so that they not fail,
> > however the unittests needs to be extended for btrfs and lvm as well.
> > Generic migrate unittests (test_ssm.py) are missing entirely.
> >
> > Also some more fine grained testing on plain devices is completely
> > missing at the moment because in this way the testing filter actually
> > interfere. Those tests are commented out in the 018.So there is still
> > some work to be done.
> >
> > Signed-off-by: Lukas Czerner <lczerner@...>
> 
> Thank you for this work. It looks good, there are just three small
> things I did notice, see the code.
> 
> > ---
> >  ssmlib/backends/btrfs.py       |  10 +-
> >  ssmlib/backends/lvm.py         |  18 +-
> >  ssmlib/backends/template.py    |   2 +-
> >  ssmlib/main.py                 | 199 ++++++++++------------
> >  ssmlib/problem.py              |  14 +-
> >  tests/bashtests/018-migrate.sh | 298 +++++++++++++++++++++++++--------
> >  tests/bashtests/lib/check.sh   |  46 +++++
> >  tests/unittests/test_btrfs.py  |   7 +-
> >  tests/unittests/test_lvm.py    |  13 --
> >  9 files changed, 397 insertions(+), 210 deletions(-)
> >
> > diff --git a/ssmlib/backends/btrfs.py b/ssmlib/backends/btrfs.py
> > index bd7db3f..39ebbce 100644
> > --- a/ssmlib/backends/btrfs.py
> > +++ b/ssmlib/backends/btrfs.py
> > @@ -600,12 +600,10 @@ class BtrfsPool(Btrfs, template.BackendPool):
> >              pool['mount'] = tmp
> >
> >          dev = BtrfsDev(options=self.options).data
> > -        if dev.get(target) and dev.get(source.name):
> > -            # both source and target are already members of the pool
> > -            # so we are going to only remove source
> > -            command = ['device', 'delete', source.name, pool['mount']]
> > -        else:
> > -            command = ['replace', 'start', '-B', source.name, target, pool['mount']]
> > +        command = ['replace', 'start']
> > +        if self.options.force:
> > +            command.extend(['-f'])
> > +        command.extend(['-B', source.name, target, pool['mount']])
> >
> >          self.run_btrfs(command)
> >          misc.send_udev_event(target, "change")
> > diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
> > index e726d9c..f1373bd 100644
> > --- a/ssmlib/backends/lvm.py
> > +++ b/ssmlib/backends/lvm.py
> > @@ -348,23 +348,17 @@ class VgsInfo(LvmInfo, template.BackendPool):
> >          target : [str]
> >              Path to the target device.
> >          """
> > -        pvsinfo = PvsInfo(options=self.options).data
> > -        if target in pvsinfo:
> > -            tgt = pvsinfo[target]
> > -            if tgt['pool_name'] != vg:
> > -                self.extend(vg, [target])
> > -        else:
> > -            self.extend(vg, [target])
> > +        # We're ready to do pvmove source and target must be already in the
> > +        # pool
> >
> >          # pvmove does not accept -f, temporarily unset it
> >          force = self.options.force
> >          self.options.force = False
> >
> > -        command = ['pvmove', '--atomic', source.name, target ]
> > -        self.run_lvm(command)
> > -        self.reduce(vg, source.name)
> > -        command = ['pvremove', source.name]
> > -        self.run_lvm(command)
> > +        # If the source is not used we do not have to do anything
> > +        if float(source['dev_used']) > 0.0:
> > +            command = ['pvmove', '--atomic', source.name, target ]
> > +            self.run_lvm(command)
> >
> >          misc.send_udev_event(source.name, "change")
> >          misc.send_udev_event(target, "change")
> 
> I would include the udev changes inside of the condition - the devices
> are not changed outside of it.

While it's true for the source device, it's not necessarily true
for the target device.

I do not see any harm in leaving it here, but I can move the source
inside the condition. Also we're not very consistent with it throughout
the ssm backends so we should probably do something about it.


> 
> > diff --git a/ssmlib/backends/template.py b/ssmlib/backends/template.py
> > index 8fbb8d2..a17cbb7 100644
> > --- a/ssmlib/backends/template.py
> > +++ b/ssmlib/backends/template.py
> > @@ -76,7 +76,7 @@ class BackendPool(Backend):
> >
> >      def migrate(self, pool, sources, targets=None):
> >          self.problem.check(self.problem.NOT_IMPLEMENTED,
> > -                        ["Migrating data in pools", "{} backend".format(self.type)])
> > +                        ["Migrating devices", "{} backend".format(self.type)])
> >
> >
> >  class BackendVolume(Backend):
> > diff --git a/ssmlib/main.py b/ssmlib/main.py
> > index 9a0a2f5..1841edd 100644
> > --- a/ssmlib/main.py
> > +++ b/ssmlib/main.py
> > @@ -443,6 +443,41 @@ class DeviceInfo(object):
> >                   "to achieve by removing " +
> >                   "{0}".format(device))
> >
> > +    def migrate(self, vg, source_dev, target_dev):
> > +        """ Migrate (in this case clone) the device onto another one.
> > +            Pure dd is used.
> > +        """
> > +        # TODO some possibilities:
> > +        # - error handling: conv=noerror,sync (continue on error and pad
> > +        #   with zeros), or better to fail on error?
> > +        source = self.data[source_dev]
> > +        target = self.data[target_dev]
> > +
> > +        if misc.get_signature(source_dev) == 'btrfs':
> > +            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> > +
> > +        if source['dev_size'] > target['dev_size']:
> > +            raise problem.GeneralError("Target device is smaller than source device")
> > +
> > +        if 'mount' in target:
> > +            if PR.check(PR.FS_MOUNTED, [target_dev, target['mount']]):
> > +                misc.do_umount(target_dev)
> > +
> > +        if 'mount' in source:
> > +            if PR.check(PR.FS_MOUNTED, [source_dev, source['mount']]):
> > +                misc.do_umount(source_dev)
> > +
> > +        print("Migrating {s} ({size}) to {t} using dd . This may take a long time...".format(
> > +            s=source_dev,
> > +            t=target_dev,
> > +            size=misc.humanize_size(source['dev_size'])))
> > +
> > +        command = ['dd', 'if={}'.format(source_dev), 'of={}'. \
> > +                   format(target_dev), 'conv=fsync']
> > +        misc.run(command)
> > +        misc.send_udev_event(source_dev, "change")
> > +        misc.send_udev_event(target_dev, "change")
> > +
> >      def set_globals(self, options):
> >          self.options = options
> >
> > @@ -962,36 +997,6 @@ class DeviceItem(Item):
> >                      return True
> >          return False
> >
> > -    @classmethod
> > -    def migrate(cls, source, target):
> > -        """ Migrate (in this case clone) the device onto another one.
> > -            Pure dd is used.
> > -        """
> > -        # TODO some possibilities:
> > -        # - error handling: conv=noerror,sync (continue on error and pad
> > -        #   with zeros), or better to fail on error?
> > -
> > -        if misc.get_signature(source) == 'btrfs':
> > -            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> > -
> > -        if misc.get_device_size(source) > misc.get_device_size(target):
> > -            raise problem.GeneralError("Target device is smaller than source device")
> > -
> > -        if misc.get_mountinfo(source):
> > -            raise problem.GeneralError("You have to umount the source device first")
> > -        if misc.get_mountinfo(target):
> > -            raise problem.GeneralError("You have to umount the target device first")
> > -
> > -        print("Migrating {s} ({size}) to {t}. This may take a long time...".format(
> > -            s=source,
> > -            t=target,
> > -            size=misc.humanize_size(misc.get_device_size(source))))
> > -
> > -        command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> > -        misc.run(command)
> > -        misc.send_udev_event(source, "change")
> > -        misc.send_udev_event(target, "change")
> > -
> >
> >  class VolumeItem(Item):
> >      def __init__(self, *args, **kwargs):
> > @@ -2111,88 +2116,70 @@ class StorageHandle(object):
> >                                                         args.options))
> >
> >      def migrate(self, args):
> > -        """ Migrate data from a device to a device"""
> > -        source = self._find_device_record(args.source)
> > -        target = self._find_device_record(args.target)
> > -
> > -        # decide if we have PVs or non-PV block devices
> > -        found_pv = False
> > -        pool = None
> > -        check_fs = True
> > -        signature = misc.get_signature(target)
> > -
> > -        # get source dev and try to detect if it is a member of a pool
> > -        source_dev = self.dev[source]
> > -        if source_dev and 'pool_name' in source_dev:
> > -            found_pv = True
> > -            pool = self.pool[source_dev['pool_name']]
> > +        """
> > +        Migrate data from a device to a device
> > +        """
> > +
> > +        source_pool = None
> > +        target_pool = None
> > +        changed = False
> > +        source = self.dev[args.source]
> > +        if source and 'pool_name' in source:
> > +            source_pool = self.pool[source['pool_name']]
> > +        target = self.dev[args.target]
> > +        if target and 'pool_name' in target:
> > +            target_pool = self.pool[target['pool_name']]
> > +
> > +        if target_pool:
> > +            if not source_pool or (target_pool.name != source_pool.name):
> > +                if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> > +                    raise problem.UserInterrupted("Terminated by user!")
> > +                target_pool.reduce(target.name)
> > +                target_pool = None
> > +                changed = True
> > +            elif target_pool.type == "btrfs":
> > +                raise problem.DeviceUsed(
> > +                    "Target and source devices are in the same btrfs pool!")
> >          else:
> > -            pass
> > -            # This seems to be only a case for some tests where the /tmp/<TEMP>/dev/...
> > -            # devices gets incorrectly identified (sometimes they do, sometimes they don't).
> > -            # Keep it commented out for now, because it seems to be rather an issue with
> > -            # the tests
> > -            # source_signature = misc.get_signature(source)
> > -            # if source_signature == 'btrfs':
> > -            #    print("found btrfs")
> > -            #    found_pv = True
> > -            #    import pdb; pdb.set_trace()
> > -
> > -        target_dev = self.dev[target]
> > -        if target_dev:
> > -            if 'pool_name' in target_dev and\
> > -                (signature != 'btrfs' or (pool and int(pool['dev_count']) > 1)):
> > -                check_fs = False
> > -
> > -                if target_dev['pool_name'] != '':
> > -                    if pool and target_dev['pool_name'] != pool.name:
> > -                        if PR.check(PR.DEVICE_USED,
> > -                            [target_dev.name, target_dev['pool_name']]):
> > -                            remove_args = Struct()
> > -                            remove_args.all = False
> > -                            remove_args.items = [target_dev]
> > -                            if not self.remove(remove_args):
> > -                                raise problem.CommandFailed("The device {} could not be removed from the pool {}".format(
> > -                                    target,
> > -                                    target_dev['pool_name']
> > -                                ))
> > -                            misc.wipefs(target_dev.name, signature)
> > -                        else:
> > -                            raise problem.UserInterrupted("Aborted")
> > +            # Check signature of existing file system on the device
> > +            # and ask user whether to use it or not.
> > +            if target and 'mount' in target:
> > +                if PR.check(PR.FS_MOUNTED, [args.target, target['mount']]):
> > +                    misc.do_umount(target.name)
> >              else:
> > -                check_fs = True
> > +                signature = misc.get_signature(args.target)
> > +                if signature and \
> > +                   PR.check(PR.EXISTING_SIGNATURE,
> > +                            [signature, args.target]):
> > +                    misc.wipefs(args.target, signature)
> > +                    changed = True
> > +                elif signature:
> > +                    raise problem.UserInterrupted("Terminated by user!")
> >
> > +        # TODO: If the source is in lvm pool and the target is not in any pool
> > +        # just add it to the source pool. This should not be here because
> > +        # we try to be as generic as possible. However untill we have all the
> > +        # data accessible in the backend we need do it here to avoid
> > +        # re-initializing information unnecessarily
> > +        if source_pool and source_pool.type == "lvm" and not target_pool:
> > +            source_pool.extend(args.target)
> >
> >
> > -        if check_fs:
> > -            mounts = misc.get_mountinfo(target)
> > -            if mounts:
> > -                if PR.check(PR.FS_MOUNTED, [target, mounts[target]['mp']]):
> > -                    misc.do_umount(target)
> > -                else:
> > -                    raise problem.CommandFailed("Umount of {} was not possible".format(target))
> > -
> > -            if signature and not self.options.force and\
> > -                PR.check(PR.EXISTING_FILESYSTEM,
> > -                        [signature, target]):
> > -                misc.wipefs(target, signature)
> > -            elif signature and not self.options.force:
> > -                # user wants to skip
> > -                raise problem.UserInterrupted("Aborted")
> > -            elif signature and self.options.force:
> > -                misc.wipefs(target, signature)
> > -
> > -        PR.info("Migrating from device {} to {}. This may take a long time...".format(
> > -            source,
> > -            target
> > -        ))
> > -        if not found_pv:
> > -            DeviceItem.migrate(source, target)
> > -
> > -        elif found_pv:
> > -            pool.migrate(source_dev, target)
> > +        if changed:
> > +            self.reinit_dev()
> > +            source = self.dev[args.source]
> > +            target = self.dev[args.target]
> > +
> > +        if source_pool:
> > +            PR.info("Migrating from device {} to {}. This may take a while...".format(
> > +                args.source,
> > +                args.target
> > +            ))
> > +            source_pool.migrate(source, args.target)
> >          else:
> > -            raise problem.ProgrammingError("Neither PV or non-PV was found. This is a bug")
> > +            target = self.dev[args.target]
> > +            source = self.dev[args.source]
> > +            source.migrate(args.source, args.target)
> 
> The target and source assignments are not used.

Right, I can drop it.

> 
> >
> >      def can_check(self, device):
> >          fs = self.is_fs(device)
> > diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> > index 1148ca5..b76d738 100644
> > --- a/ssmlib/problem.py
> > +++ b/ssmlib/problem.py
> > @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
> >             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
> >             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
> >             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> > -           "NotImplemented"]
> > +           "NotImplemented", "ExistingSignature"]
> >
> >  # Define prompt codes
> >  PROMPT_NONE =           0
> > @@ -148,11 +148,17 @@ class ExistingFilesystem(SsmError):
> >      def __init__(self, msg, errcode=2015):
> >          super(ExistingFilesystem, self).__init__(msg, errcode)
> >
> > +
> >  class NotImplemented(SsmError):
> >      def __init__(self, msg, errcode=2016):
> >          super(NotImplemented, self).__init__(msg, errcode)
> >
> >
> > +class ExistingSignature(SsmError):
> > +    def __init__(self, msg, errcode=2017):
> > +        super(ExistingSignature, self).__init__(msg, errcode)
> > +
> > +
> >  class ProblemSet(object):
> >
> >      def __init__(self, options):
> > @@ -212,6 +218,10 @@ class ProblemSet(object):
> >              ['Filesystem \'{0}\' detected on the device \'{1}\'!',
> >               PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingFilesystem]
> >
> > +        self.EXISTING_SIGNATURE = \
> > +            ['Signature \'{0}\' detected on the device \'{1}\'!',
> > +             PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingSignature]
> > +
> >          self.NO_DEVICES = \
> >              ['No devices available to use for the \'{0}\' pool!',
> >               PROMPT_NONE, FL_FATAL, NoDevices]
> > @@ -233,7 +243,7 @@ class ProblemSet(object):
> >               PROMPT_NONE, FL_FATAL, NotSupported]
> >
> >          self.NOT_IMPLEMENTED = \
> > -            ['\'{0}\' function is not implemented by {1}!',
> > +            ['\'{0}\' functionality is not implemented by {1}!',
> >              PROMPT_NONE, FL_FATAL, NotImplemented]
> >
> >          self.CREATE_DIRECTORY = \
> > diff --git a/tests/bashtests/018-migrate.sh b/tests/bashtests/018-migrate.sh
> > index 98b57f0..44e8c2d 100755
> > --- a/tests/bashtests/018-migrate.sh
> > +++ b/tests/bashtests/018-migrate.sh
> > @@ -21,7 +21,7 @@ export test_description='Test the migrate command.'
> >  . lib/test
> >
> >  export COLUMNS=1024
> > -DEV_COUNT=6
> > +DEV_COUNT=8
> >  DEV_SIZE=200
> >  aux prepare_devs $DEV_COUNT $DEV_SIZE
> >  aux prepare_mnts 2
> > @@ -31,6 +31,8 @@ export SSM_DEFAULT_BACKEND='lvm'
> >  export SSM_LVM_DEFAULT_POOL=$vg1
> >  export SSM_BTRFS_DEFAULT_POOL=$vg1
> >  export SSM_NONINTERACTIVE='1'
> > +lvol1=${LVOL_PREFIX}001
> > +lvol2=${LVOL_PREFIX}002
> >
> >  cleanup() {
> >         aux teardown
> > @@ -38,102 +40,262 @@ cleanup() {
> >
> >  trap cleanup EXIT
> >
> > -
> >  function compare_hash()
> >  {
> > -       a=$(sha1sum $1 | cut -f1 -d' ')
> > -       b=$(sha1sum $2 | cut -f1 -d' ')
> > +       a=$(cksum $1 | awk '{print $1 $2}')
> > +       b=$(cksum $2 | awk '{print $1 $2}')
> >         test "$a" = "$b"
> >  }
> >
> > -function reset_devs()
> > -{
> > -       # try to clean anything that could linger on any test dev
> > -       yes 'c' | ssm -f remove $SSM_LVM_DEFAULT_POOL || true
> > -       for dev in $TEST_DEVS; do
> > -               pvremove $dev || umount $dev || true
> > -               pvcreate -f $dev
> > -       done
> > -
> > -}
> > -
> > -function test_mount()
> > +function test_volume()
> >  {
> > -       mount $1 $mnt1
> > +       ssm check $1
> > +       ssm mount $1 $mnt1
> >         umount $mnt1
> >  }
> >
> > -mkfs.ext4 $dev1
> > -mkfs.ext4 $dev2
> > -ssm migrate $dev1 $dev3
> > -! ssm migrate $dev2 $dev3
> > -ssm -f migrate $dev2 $dev3
> > +# Test some nonsense
> > +not ssm migrate foobar1 foobar2
> > +not ssm migrate foobar1
> > +not ssm migrate $dev1 $mnt1
> > +not ssm migrate $mnt1 $dev1
> >
> > -reset_devs
> > +# Migrate plain device
> 
> I would add a TODO to here with the information about why it is commented out.

Yeah, makes sense.

Thanks!
-Lukas

> 
> Thanks,
> Jan
> 
> > +#dd if=/dev/urandom of=$dev1 bs=1k count=1
> > +#dd if=/dev/urandom of=$dev2 bs=1k count=1
> > +#sync
> > +#ssm -f migrate $dev1 $dev2
> > +#compare_hash $dev1 $dev2
> > +#
> > +# Migrate filesystem on plain device
> > +#mkfs.ext4 $dev1
> > +#mkfs.ext4 $dev2
> > +#ssm migrate $dev1 $dev3
> > +#not ssm migrate $dev2 $dev3
> > +#ssm -f migrate $dev2 $dev3
> > +#wipefs -a $dev1
> > +#wipefs -a $dev2
> >
> >  # BTRFS
> >  export SSM_DEFAULT_BACKEND='btrfs'
> >
> > -ssm create $dev1
> > -ssm -f migrate $dev1 $dev2
> > -test_mount $dev2
> > -ssm -f migrate $dev2 $dev3
> > -test_mount $dev3
> > -reset_devs
> > +# Simple btrfs migrate to plain device
> > +#ssm create $dev1
> > +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> > +#ssm migrate $dev1 $dev2
> > +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> > +#test_volume $SSM_BTRFS_DEFAULT_POOL
> > +# Btrfs migrate on device with signature (ext4)
> > +#not ssm migrate $dev2 $dev3
> > +#ssm -f migrate $dev2 $dev3
> > +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev3
> > +#test_volume $SSM_BTRFS_DEFAULT_POOL
> > +#ssm -f remove --all
> >
> > +# Simple btrfs migrate of mounted fs
> > +ssm create $dev1 $mnt1
> > +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> > +not ssm migrate $mnt1 $dev2
> > +ssm migrate $dev1 $dev2
> > +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> > +umount $mnt1
> > +ssm check $SSM_BTRFS_DEFAULT_POOL
> > +ssm -f remove --all
> > +
> > +# Migrate device from multi-device btrfs
> >  ssm create -p $vg1 $dev1 $dev2
> >  ssm create -p $vg2 $dev3 $dev4 $dev5
> > -ssm list
> >  ssm -f migrate $dev2 $dev6
> > -test_mount $dev1
> > -test_mount $dev6
> > +check btrfs_devices $vg1 $dev1 $dev6
> > +check btrfs_devices $vg2 $dev3 $dev4 $dev5
> > +test_volume $vg1
> > +test_volume $vg2
> >
> > -! ssm migrate $dev1 $dev3
> > +# Migrate to a device already used in btrfs pool
> > +not ssm migrate $dev1 $dev3
> >  ssm -f migrate $dev1 $dev3
> > -test_mount $dev3
> > -test_mount $dev4
> > -reset_devs
> > +check btrfs_devices $vg1 $dev3 $dev6
> > +check btrfs_devices $vg2 $dev4 $dev5
> > +test_volume $vg1
> > +test_volume $vg2
> > +ssm -f remove --all
> >
> > -ssm create $dev1 $dev2
> > -ssm -f migrate $dev1 $dev3
> > -test_mount $dev2
> > -test_mount $dev3
> > -reset_devs
> > +# Do not allow migrate between devices within a single pool in btrfs
> > +ssm create $dev1 $dev2 $dev3 $dev4
> > +not ssm migrate $dev1 $dev2
> > +not ssm -f migrate $dev1 $dev2
> > +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> > +ssm -f remove --all
> >
> > -reset_devs
> > -ssm -b btrfs create $dev1 $dev2 $dev3
> > -ssm list
> > -ssm migrate $dev1 $dev2
> > +# migrate plain device to a device in btrfs pool - not used
> > +#ssm create -p $vg1 $dev1 $dev2 $dev3
> > +#mkfs.ext4 -F $dev4
> > +#not ssm migrate $dev4 $dev3
> > +#ssm -f migrate $dev4 $dev3
> > +#check btrfs_devices $vg1 $dev1 $dev2
> > +#test_volume $vg1
> > +#fsck.ext4 -fn $dev3
> > +
> > +# migrate plain device to a device in btrfs pool - used completely
> > +#ssm -f add -p $vg1 $dev3
> > +#ssm mount $vg1 $mnt1
> > +# Fill it up as much as we can, we do not care if dd fails
> > +#! dd if=/dev/zero of=$mnt1/file bs=1M
> > +#sync
> > +#umount $mnt1
> > +#not ssm migrate $dev4 $dev1
> > +#not ssm -f migrate $dev4 $dev1
> > +#check btrfs_devices $vg1 $dev1 $dev2 $dev3
> > +#test_volume $vg1
> > +#ssm -f remove --all
> >
> >  export SSM_DEFAULT_BACKEND='lvm'
> >
> >  # LVM
> > -! ssm migrate foobar1 foobar2
> > -ssm -f migrate $dev1 $dev2
> > -reset_devs
> >
> > -# should not pass because there is already a signature and -f is not used
> > -! ssm migrate $dev3 $dev4
> > -! ssm migrate $dev5 foobar2
> > -! ssm migrate foobar2 $dev5
> > -reset_devs
> > +# Simple lvm migrate to plain device
> > +#ssm create --fs ext4 $dev1
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1
> > +#ssm migrate $dev1 $dev2
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> > +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> > +#mkfs.ext4 $dev3
> > +# lvm migrate to a device with signature (ext4)
> > +#not ssm migrate $dev2 $dev3
> > +#ssm -f migrate $dev2 $dev3
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> > +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> > +#ssm -f remove --all
> >
> > -ssm create --size $DEV_SIZE $dev1 $dev2
> > -ssm migrate $dev1 $dev2
> > -ssm -f migrate $dev2 $dev3
> > -reset_devs
> > +# Simple lvm migrate without pv being used
> > +ssm add $dev1 $dev2 $dev3
> > +ssm migrate $dev1 $dev5
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> > +# Migrate to a device in the same pool
> > +ssm migrate $dev2 $dev3
> > +ssm -f remove --all
> >
> > -ssm create --size $DEV_SIZE $dev1 $dev2 $dev3
> > -! ssm -n migrate $dev3 $dev3
> > -reset_devs
> > +# Migrate used device to free that's already in the pool
> > +ssm create $dev1 $dev2
> > +ssm -f add $dev3 $dev4
> > +ssm migrate $dev1 $dev3
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> > +ssm -f remove --all
> >
> > -# should fail - the target is already mounted or used in another pool
> > -mkfs.ext4 $dev1
> > -mount $dev1 $mnt1
> > +# All devices are used
> > +ssm create $dev1
> >  ssm create $dev2
> > -! ssm migrate $dev2 $dev1
> > -! ssm migrate $dev1 $dev2
> > -ssm -f migrate $dev2 $dev1
> > -reset_devs
> > +not ssm migrate $dev1 $dev2
> > +# Only specified devices are used
> > +ssm add $dev3
> > +not ssm migrate $dev1 $dev2
> > +ssm migrate $dev2 $dev3
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> > +ssm -f remove --all
> > +
> > +# Simple lvm migrate of mounted fs
> > +ssm create --fs ext4 $dev1 $mnt1
> > +wipefs -a $dev2
> > +not ssm migrate $mnt1 $dev2
> > +ssm migrate $dev1 $dev2
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> > +umount $mnt1
> > +ssm check $SSM_LVM_DEFAULT_POOL/$lvol1
> > +ssm -f remove --all
> > +
> > +# Migrate to a device from different pool
> > +ssm create -p $vg1 $dev1 $dev2 $dev3
> > +ssm add -p $vg1 $dev7
> > +ssm create -p $vg2 $dev4 $dev5 $dev6
> > +ssm add -p $vg2 $dev8
> > +not ssm migrate $dev1 $dev8
> > +ssm -f migrate $dev1 $dev8
> > +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3 $dev7
> > +check vg_devices $vg2 $dev4 $dev5 $dev6
> > +# Migrate device that is not used
> > +not ssm migrate $dev4 $dev7
> > +ssm -f migrate $dev4 $dev7
> > +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3
> > +check vg_devices $vg2 $dev4 $dev5 $dev6 $dev7
> > +# Migrate device that is used
> > +not ssm add -p $vg1 $dev1 # We're not removing the pv after migrate
> > +not ssm migrate $dev5 $dev1
> > +ssm -f migrate $dev5 $dev1
> > +ssm -f remove --all
> > +
> > +# Migrate plain device to a device in lvm
> > +ssm create $dev1 $dev2 $dev3
> > +ssm add -p $SSM_LVM_DEFAULT_POOL $dev4 $dev5
> > +# target device used
> > +not ssm migrate $dev6 $dev3
> > +not ssm -f migrate $dev6 $dev3
> > +# target device not used
> > +not ssm migrate $dev6 $dev4
> > +ssm -f migrate $dev6 $dev4
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> > +# Migrate plain device with a file system
> > +#mkfs.ext4 -F $dev4
> > +# target device used
> > +#not ssm migrate $dev4 $dev1
> > +#not ssm -f migrate $dev4 $dev1
> > +# target not used
> > +#not ssm migrate $dev4 $dev5
> > +#ssm -f migrate $dev4 $dev5
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> > +#fsck.ext4 -fn $dev5
> > +#test_volume $dev5
> > +ssm -f remove --all
> > +
> > +# migrate from btrfs pool to lvm
> > +wipefs -a $dev4 $dev5
> > +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> > +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> > +not ssm migrate $dev5 $dev3
> > +ssm -f migrate $dev5 $dev3
> > +check btrfs_devices $vg1 $dev1 $dev2
> > +check vg_devices $vg2 $dev4 $dev5 $dev3
> > +test_volume $vg1
> > +test_volume $vg2/$lvol1
> > +ssm -f remove --all
> > +
> > +# migrate from lvm to btrfs pool
> > +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> > +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> > +ssm add -p $vg2 $dev6
> > +not ssm migrate $dev3 $dev6
> > +ssm -f migrate $dev3 $dev6
> > +check btrfs_devices $vg1 $dev1 $dev2 $dev6
> > +check vg_devices $vg2 $dev4 $dev5
> > +test_volume $vg1
> > +test_volume $vg2/$lvol1
> > +ssm -f remove --all
> > +
> > +# CRYPT
> > +
> > +# cryptsetup will ask for password. If luks extension is used
> > +# it will ask when creating header and then when opening the device
> > +pass() {
> > +       echo -e "${passwd}\n${passwd}"
> > +}
> > +
> > +export SSM_DEFAULT_BACKEND='crypt'
> > +export SSM_CRYPT_DEFAULT_POOL=$vg3
> > +export CRYPT_VOL_PREFIX="${SSM_PREFIX_FILTER}enc"
> > +export SSM_CRYPT_DEFAULT_VOL_PREFIX=$CRYPT_VOL_PREFIX
> > +crypt_vol1=${CRYPT_VOL_PREFIX}001
> > +
> > +passwd="cai0ohMo8M"
> > +pass | ssm create $dev1
> > +check crypt_vol_field $crypt_vol1 type LUKS1
> > +check crypt_vol_field $crypt_vol1 device $dev1
> > +check list_table "$(ssm list vol)" $crypt_vol1 $SSM_CRYPT_DEFAULT_POOL none crypt
> > +
> > +# Migrate in crypt is not currently supported so this whould both fail
> > +not ssm migrate $dev1 $dev2
> > +not ssm -f migrate $dev1 $dev2
> > +
> > +# Device can't be simply removed from the crypt backend so this should fail
> > +not ssm migrate $dev2 $dev1
> > +not ssm -f migrate $dev2 $dev1
> >
> > -exit 0
> > \ No newline at end of file
> > +exit 0
> > diff --git a/tests/bashtests/lib/check.sh b/tests/bashtests/lib/check.sh
> > index 4dad7c4..2bdc790 100644
> > --- a/tests/bashtests/lib/check.sh
> > +++ b/tests/bashtests/lib/check.sh
> > @@ -257,6 +257,29 @@ lv_field()
> >         fi
> >  }
> >
> > +vg_devices()
> > +{
> > +       # Check the expected list of devices in the fs with the reality
> > +       # $1 is the vg we're inspecting
> > +       # the rest is list of devices
> > +       tmp=`mktemp`
> > +
> > +       devices=`vgs --no-headings -o pv_name $1 | awk {'print $1'}`
> > +       shift
> > +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> > +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> > +
> > +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> > +       if [ $? -eq 0 ]; then
> > +               echo "ACTUAL"
> > +               cat ${tmp}.actual
> > +               echo "EXPECTED"
> > +               cat ${tmp}.expected
> > +               exit 1
> > +       fi
> > +       exit 0
> > +}
> > +
> >  compare_fields()
> >  {
> >         local cmd1=$1;
> > @@ -334,6 +357,29 @@ crypt_vol_field()
> >         fi
> >  }
> >
> > +btrfs_devices()
> > +{
> > +       # Check the expected list of devices in the fs with the reality
> > +       # $1 is label for the file system we want to test
> > +       # the rest is list of devices
> > +       tmp=`mktemp`
> > +
> > +       devices=`btrfs filesystem show $1| grep devid | awk {'print $NF'}`
> > +       shift
> > +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> > +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> > +
> > +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> > +       if [ $? -eq 0 ]; then
> > +               echo "ACTUAL"
> > +               cat ${tmp}.actual
> > +               echo "EXPECTED"
> > +               cat ${tmp}.expected
> > +               exit 1
> > +       fi
> > +       exit 0
> > +}
> > +
> >  btrfs_fs_field()
> >  {
> >         lines=$(btrfs filesystem show 2> /dev/null | grep -A 1 "^Label:.*$1" || true)
> > diff --git a/tests/unittests/test_btrfs.py b/tests/unittests/test_btrfs.py
> > index a73420f..1e8d7e1 100644
> > --- a/tests/unittests/test_btrfs.py
> > +++ b/tests/unittests/test_btrfs.py
> > @@ -20,6 +20,7 @@
> >
> >  import unittest
> >  from ssmlib import main
> > +from ssmlib import problem
> >  from ssmlib.backends import btrfs
> >  from tests.unittests.common import *
> >
> > @@ -461,8 +462,10 @@ class BtrfsFunctionCheck(MockSystemDataSource):
> >          self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
> >          self._addDevice('/dev/sde', 11489037516)
> >
> > -        self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> > -        "btrfs device delete /dev/sdc1 /tmp/mount")
> > +        try:
> > +            main.main("ssm migrate /dev/sdc1 /dev/sdc2")
> > +        except problem.DeviceUsed:
> > +            pass
> >
> >          self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
> >          "btrfs replace start -B /dev/sdc2 /dev/sde /tmp/mount")
> > diff --git a/tests/unittests/test_lvm.py b/tests/unittests/test_lvm.py
> > index fe87c4c..ec565e0 100644
> > --- a/tests/unittests/test_lvm.py
> > +++ b/tests/unittests/test_lvm.py
> > @@ -257,21 +257,8 @@ class LvmFunctionCheck(MockSystemDataSource):
> >          self._addDevice('/dev/sde', 11489037516)
> >
> >          self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> > -        "lvm pvremove /dev/sdc1")
> > -        self.assertEqual(self.run_data[-2],
> > -            "lvm vgreduce my_pool /dev/sdc1")
> > -        self.assertEqual(self.run_data[-3],
> >              "lvm pvmove --atomic /dev/sdc1 /dev/sdc2")
> >
> > -        self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
> > -        "lvm pvremove /dev/sdc2")
> > -        self.assertEqual(self.run_data[-2],
> > -            "lvm vgreduce my_pool /dev/sdc2")
> > -        self.assertEqual(self.run_data[-3],
> > -            "lvm pvmove --atomic /dev/sdc2 /dev/sde")
> > -        self.assertEqual(self.run_data[-4],
> > -            "lvm vgextend my_pool /dev/sde")
> > -
> >      def test_lvm_resize(self):
> >          # Generate some storage data
> >          self._addPool('default_pool', ['/dev/sda', '/dev/sdb'])
> > --
> > 2.17.1
> >
> >
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > storagemanager-devel mailing list
> > storagemanager-devel@...
> > https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> > System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)
> 
> 
> 
> -- 
> Jan Tulak
> jtulak@... / jan@...

Re: [ssm-devel] [PATCH] migrate: remove fs signatures when using dd

[Lukas Czerner <lczerner@re...>]

On Tue, Aug 07, 2018 at 02:56:23PM +0200, Jan Tulak wrote:
> On Tue, Aug 7, 2018 at 11:32 AM Jan Tulak <jtulak@...> wrote:
> >
> > On Fri, Aug 3, 2018 at 8:33 AM Lukas Czerner <lczerner@...> wrote:
> > >
> > > On Tue, Jul 31, 2018 at 01:50:33PM +0200, Jan Tulak wrote:
> > > > On Tue, Jul 31, 2018 at 11:13 AM Lukas Czerner <lczerner@...> wrote:
> > > > >
> > > > > On Mon, Jul 30, 2018 at 04:58:19PM +0200, Jan Tulak wrote:
> > > > > > Make the migrate command behave the same way in all cases. For btrfs and
> > > > > > lvm the signatures are removed by the respective utilities, so do the
> > > > > > same for dd-copied devices as well.
> > > > > >
> > > > > > No need of the manual is necessary, it already states that accessing the
> > > > > > data on the source device can be difficult afterwards.
> > > > >
> > > > > I gave it some more though lately and I am starting to think that while
> > > > > consistency is useful we also need to consider the expectations and
> > > > > convenience. I am not really sure what's the right call with the plain
> > > > > file system case, but maybe wiping the signature makes the most sense.
> > > > >
> > > > > However as for lvm, I am strongly considering changing it so that the pv
> > > > > is _not_ removed from the vg after the migrate. For example it would be
> > > > > a perfectly valid use case to replace a ssd pv to make use of it in a
> > > > > different lv where we want it to be faster, or we want to use it for
> > > > > caching. And removing it just complicates it for the user. Unless you
> > > > > have any strong arguments against it I am going to change it.
> > > >
> > > > Yes, I see the case for it. However, in that case, we need to be more
> > > > explicit about the result, printing out what happened, and maybe what
> > > > to do next or how to revert it, because while such a change is useful,
> > > > I don't think that it is an obvious and expected result.
> > >
> > > I'd actually argue that this is obvious and expected behavior. In lvm
> > > the device does not have to be removed from the pool to be unused. It's
> > > perfectly ok just sitting in the volume group not being used by
> > > anything. And 'migrate' does not really imply 'remove the volume from
> > > the pool'.
> > >
> > > As for btrfs, it needs to remove it from the filesystem since it does
> > > not really have any notion of "pool" it's ssm construct. It can't be in
> > > the file system and still be (and remain) unused.
> >
> > OK, I have no issue with it. But anyway, is there anything wrong with
> > this patch were are discussing under (e.g. adding an explicit message
> > in v2 of this patch?) Or can you add reviewed-by?
> >
> 
> Or better to say, it has to be rebased against your changes of
> migrate, so I will send a v2 anyway. Other than that, is there any
> issue?

Hi,

it does seem kind of fragile especially when we're not even sure that
the new copy works, so I am not completely sure about this. But I'll leave
this one up to you.

-Lukas

> 
> Jan
> 
> > Thanks,
> > Jan
> >
> > >
> > > -Lukas
> > >
> > > >
> > > > Jan
> > > >
> > > > >
> > > > > Thanks!
> > > > > -Lukas
> > > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Jan Tulak <jtulak@...>
> > > > > > ---
> > > > > >  ssmlib/main.py | 6 ++++++
> > > > > >  1 file changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/ssmlib/main.py b/ssmlib/main.py
> > > > > > index 51e5033..4011d21 100644
> > > > > > --- a/ssmlib/main.py
> > > > > > +++ b/ssmlib/main.py
> > > > > > @@ -990,8 +990,14 @@ class DeviceItem(Item):
> > > > > >
> > > > > >          command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> > > > > >          misc.run(command)
> > > > > > +
> > > > > > +        fs = misc.get_fs_type(source)
> > > > > > +        if fs:
> > > > > > +            misc.wipefs(source, fs)
> > > > > > +
> > > > > >          misc.send_udev_event(source, "change")
> > > > > >          misc.send_udev_event(target, "change")
> > > > > > +        misc.udev_settle()
> > > > > >
> > > > > >
> > > > > >  class VolumeItem(Item):
> > > > > > --
> > > > > > 2.16.2
> > > > > >
> > > > > >
> > > > > > ------------------------------------------------------------------------------
> > > > > > Check out the vibrant tech community on one of the world's most
> > > > > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > > > > > _______________________________________________
> > > > > > storagemanager-devel mailing list
> > > > > > storagemanager-devel@...
> > > > > > https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> > > > > > System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)
> > > >
> > > >
> > > >
> > > > --
> > > > Jan Tulak
> > > > jtulak@... / jan@...
> >
> >
> >
> > --
> > Jan Tulak
> > jtulak@... / jan@...
> 
> 
> 
> -- 
> Jan Tulak
> jtulak@... / jan@...

Re: [ssm-devel] [PATCH 4/4] ssm: Rework migrate command

[Jan Tulak <jtulak@re...>]

On Thu, Aug 2, 2018 at 5:00 PM Lukas Czerner <lczerner@...> wrote:
>
> Migrate command was not working as expected in many ways so here is a
> rework of it. The 018 bashtest is also reworked and significantly
> extended to cover more possible scenarios and test the actual results of
> the operation.
>
> Lvm migrate now does not remove the pv from the volume group as I feel
> there are perfectly valid scenarios where user would like to migrate
> used pv to a different device and repurpose the source device.
>
> unittests for lvm and btrfs are only changed so that they not fail,
> however the unittests needs to be extended for btrfs and lvm as well.
> Generic migrate unittests (test_ssm.py) are missing entirely.
>
> Also some more fine grained testing on plain devices is completely
> missing at the moment because in this way the testing filter actually
> interfere. Those tests are commented out in the 018.So there is still
> some work to be done.
>
> Signed-off-by: Lukas Czerner <lczerner@...>

Thank you for this work. It looks good, there are just three small
things I did notice, see the code.

> ---
>  ssmlib/backends/btrfs.py       |  10 +-
>  ssmlib/backends/lvm.py         |  18 +-
>  ssmlib/backends/template.py    |   2 +-
>  ssmlib/main.py                 | 199 ++++++++++------------
>  ssmlib/problem.py              |  14 +-
>  tests/bashtests/018-migrate.sh | 298 +++++++++++++++++++++++++--------
>  tests/bashtests/lib/check.sh   |  46 +++++
>  tests/unittests/test_btrfs.py  |   7 +-
>  tests/unittests/test_lvm.py    |  13 --
>  9 files changed, 397 insertions(+), 210 deletions(-)
>
> diff --git a/ssmlib/backends/btrfs.py b/ssmlib/backends/btrfs.py
> index bd7db3f..39ebbce 100644
> --- a/ssmlib/backends/btrfs.py
> +++ b/ssmlib/backends/btrfs.py
> @@ -600,12 +600,10 @@ class BtrfsPool(Btrfs, template.BackendPool):
>              pool['mount'] = tmp
>
>          dev = BtrfsDev(options=self.options).data
> -        if dev.get(target) and dev.get(source.name):
> -            # both source and target are already members of the pool
> -            # so we are going to only remove source
> -            command = ['device', 'delete', source.name, pool['mount']]
> -        else:
> -            command = ['replace', 'start', '-B', source.name, target, pool['mount']]
> +        command = ['replace', 'start']
> +        if self.options.force:
> +            command.extend(['-f'])
> +        command.extend(['-B', source.name, target, pool['mount']])
>
>          self.run_btrfs(command)
>          misc.send_udev_event(target, "change")
> diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
> index e726d9c..f1373bd 100644
> --- a/ssmlib/backends/lvm.py
> +++ b/ssmlib/backends/lvm.py
> @@ -348,23 +348,17 @@ class VgsInfo(LvmInfo, template.BackendPool):
>          target : [str]
>              Path to the target device.
>          """
> -        pvsinfo = PvsInfo(options=self.options).data
> -        if target in pvsinfo:
> -            tgt = pvsinfo[target]
> -            if tgt['pool_name'] != vg:
> -                self.extend(vg, [target])
> -        else:
> -            self.extend(vg, [target])
> +        # We're ready to do pvmove source and target must be already in the
> +        # pool
>
>          # pvmove does not accept -f, temporarily unset it
>          force = self.options.force
>          self.options.force = False
>
> -        command = ['pvmove', '--atomic', source.name, target ]
> -        self.run_lvm(command)
> -        self.reduce(vg, source.name)
> -        command = ['pvremove', source.name]
> -        self.run_lvm(command)
> +        # If the source is not used we do not have to do anything
> +        if float(source['dev_used']) > 0.0:
> +            command = ['pvmove', '--atomic', source.name, target ]
> +            self.run_lvm(command)
>
>          misc.send_udev_event(source.name, "change")
>          misc.send_udev_event(target, "change")

I would include the udev changes inside of the condition - the devices
are not changed outside of it.

> diff --git a/ssmlib/backends/template.py b/ssmlib/backends/template.py
> index 8fbb8d2..a17cbb7 100644
> --- a/ssmlib/backends/template.py
> +++ b/ssmlib/backends/template.py
> @@ -76,7 +76,7 @@ class BackendPool(Backend):
>
>      def migrate(self, pool, sources, targets=None):
>          self.problem.check(self.problem.NOT_IMPLEMENTED,
> -                        ["Migrating data in pools", "{} backend".format(self.type)])
> +                        ["Migrating devices", "{} backend".format(self.type)])
>
>
>  class BackendVolume(Backend):
> diff --git a/ssmlib/main.py b/ssmlib/main.py
> index 9a0a2f5..1841edd 100644
> --- a/ssmlib/main.py
> +++ b/ssmlib/main.py
> @@ -443,6 +443,41 @@ class DeviceInfo(object):
>                   "to achieve by removing " +
>                   "{0}".format(device))
>
> +    def migrate(self, vg, source_dev, target_dev):
> +        """ Migrate (in this case clone) the device onto another one.
> +            Pure dd is used.
> +        """
> +        # TODO some possibilities:
> +        # - error handling: conv=noerror,sync (continue on error and pad
> +        #   with zeros), or better to fail on error?
> +        source = self.data[source_dev]
> +        target = self.data[target_dev]
> +
> +        if misc.get_signature(source_dev) == 'btrfs':
> +            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> +
> +        if source['dev_size'] > target['dev_size']:
> +            raise problem.GeneralError("Target device is smaller than source device")
> +
> +        if 'mount' in target:
> +            if PR.check(PR.FS_MOUNTED, [target_dev, target['mount']]):
> +                misc.do_umount(target_dev)
> +
> +        if 'mount' in source:
> +            if PR.check(PR.FS_MOUNTED, [source_dev, source['mount']]):
> +                misc.do_umount(source_dev)
> +
> +        print("Migrating {s} ({size}) to {t} using dd . This may take a long time...".format(
> +            s=source_dev,
> +            t=target_dev,
> +            size=misc.humanize_size(source['dev_size'])))
> +
> +        command = ['dd', 'if={}'.format(source_dev), 'of={}'. \
> +                   format(target_dev), 'conv=fsync']
> +        misc.run(command)
> +        misc.send_udev_event(source_dev, "change")
> +        misc.send_udev_event(target_dev, "change")
> +
>      def set_globals(self, options):
>          self.options = options
>
> @@ -962,36 +997,6 @@ class DeviceItem(Item):
>                      return True
>          return False
>
> -    @classmethod
> -    def migrate(cls, source, target):
> -        """ Migrate (in this case clone) the device onto another one.
> -            Pure dd is used.
> -        """
> -        # TODO some possibilities:
> -        # - error handling: conv=noerror,sync (continue on error and pad
> -        #   with zeros), or better to fail on error?
> -
> -        if misc.get_signature(source) == 'btrfs':
> -            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> -
> -        if misc.get_device_size(source) > misc.get_device_size(target):
> -            raise problem.GeneralError("Target device is smaller than source device")
> -
> -        if misc.get_mountinfo(source):
> -            raise problem.GeneralError("You have to umount the source device first")
> -        if misc.get_mountinfo(target):
> -            raise problem.GeneralError("You have to umount the target device first")
> -
> -        print("Migrating {s} ({size}) to {t}. This may take a long time...".format(
> -            s=source,
> -            t=target,
> -            size=misc.humanize_size(misc.get_device_size(source))))
> -
> -        command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> -        misc.run(command)
> -        misc.send_udev_event(source, "change")
> -        misc.send_udev_event(target, "change")
> -
>
>  class VolumeItem(Item):
>      def __init__(self, *args, **kwargs):
> @@ -2111,88 +2116,70 @@ class StorageHandle(object):
>                                                         args.options))
>
>      def migrate(self, args):
> -        """ Migrate data from a device to a device"""
> -        source = self._find_device_record(args.source)
> -        target = self._find_device_record(args.target)
> -
> -        # decide if we have PVs or non-PV block devices
> -        found_pv = False
> -        pool = None
> -        check_fs = True
> -        signature = misc.get_signature(target)
> -
> -        # get source dev and try to detect if it is a member of a pool
> -        source_dev = self.dev[source]
> -        if source_dev and 'pool_name' in source_dev:
> -            found_pv = True
> -            pool = self.pool[source_dev['pool_name']]
> +        """
> +        Migrate data from a device to a device
> +        """
> +
> +        source_pool = None
> +        target_pool = None
> +        changed = False
> +        source = self.dev[args.source]
> +        if source and 'pool_name' in source:
> +            source_pool = self.pool[source['pool_name']]
> +        target = self.dev[args.target]
> +        if target and 'pool_name' in target:
> +            target_pool = self.pool[target['pool_name']]
> +
> +        if target_pool:
> +            if not source_pool or (target_pool.name != source_pool.name):
> +                if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> +                    raise problem.UserInterrupted("Terminated by user!")
> +                target_pool.reduce(target.name)
> +                target_pool = None
> +                changed = True
> +            elif target_pool.type == "btrfs":
> +                raise problem.DeviceUsed(
> +                    "Target and source devices are in the same btrfs pool!")
>          else:
> -            pass
> -            # This seems to be only a case for some tests where the /tmp/<TEMP>/dev/...
> -            # devices gets incorrectly identified (sometimes they do, sometimes they don't).
> -            # Keep it commented out for now, because it seems to be rather an issue with
> -            # the tests
> -            # source_signature = misc.get_signature(source)
> -            # if source_signature == 'btrfs':
> -            #    print("found btrfs")
> -            #    found_pv = True
> -            #    import pdb; pdb.set_trace()
> -
> -        target_dev = self.dev[target]
> -        if target_dev:
> -            if 'pool_name' in target_dev and\
> -                (signature != 'btrfs' or (pool and int(pool['dev_count']) > 1)):
> -                check_fs = False
> -
> -                if target_dev['pool_name'] != '':
> -                    if pool and target_dev['pool_name'] != pool.name:
> -                        if PR.check(PR.DEVICE_USED,
> -                            [target_dev.name, target_dev['pool_name']]):
> -                            remove_args = Struct()
> -                            remove_args.all = False
> -                            remove_args.items = [target_dev]
> -                            if not self.remove(remove_args):
> -                                raise problem.CommandFailed("The device {} could not be removed from the pool {}".format(
> -                                    target,
> -                                    target_dev['pool_name']
> -                                ))
> -                            misc.wipefs(target_dev.name, signature)
> -                        else:
> -                            raise problem.UserInterrupted("Aborted")
> +            # Check signature of existing file system on the device
> +            # and ask user whether to use it or not.
> +            if target and 'mount' in target:
> +                if PR.check(PR.FS_MOUNTED, [args.target, target['mount']]):
> +                    misc.do_umount(target.name)
>              else:
> -                check_fs = True
> +                signature = misc.get_signature(args.target)
> +                if signature and \
> +                   PR.check(PR.EXISTING_SIGNATURE,
> +                            [signature, args.target]):
> +                    misc.wipefs(args.target, signature)
> +                    changed = True
> +                elif signature:
> +                    raise problem.UserInterrupted("Terminated by user!")
>
> +        # TODO: If the source is in lvm pool and the target is not in any pool
> +        # just add it to the source pool. This should not be here because
> +        # we try to be as generic as possible. However untill we have all the
> +        # data accessible in the backend we need do it here to avoid
> +        # re-initializing information unnecessarily
> +        if source_pool and source_pool.type == "lvm" and not target_pool:
> +            source_pool.extend(args.target)
>
>
> -        if check_fs:
> -            mounts = misc.get_mountinfo(target)
> -            if mounts:
> -                if PR.check(PR.FS_MOUNTED, [target, mounts[target]['mp']]):
> -                    misc.do_umount(target)
> -                else:
> -                    raise problem.CommandFailed("Umount of {} was not possible".format(target))
> -
> -            if signature and not self.options.force and\
> -                PR.check(PR.EXISTING_FILESYSTEM,
> -                        [signature, target]):
> -                misc.wipefs(target, signature)
> -            elif signature and not self.options.force:
> -                # user wants to skip
> -                raise problem.UserInterrupted("Aborted")
> -            elif signature and self.options.force:
> -                misc.wipefs(target, signature)
> -
> -        PR.info("Migrating from device {} to {}. This may take a long time...".format(
> -            source,
> -            target
> -        ))
> -        if not found_pv:
> -            DeviceItem.migrate(source, target)
> -
> -        elif found_pv:
> -            pool.migrate(source_dev, target)
> +        if changed:
> +            self.reinit_dev()
> +            source = self.dev[args.source]
> +            target = self.dev[args.target]
> +
> +        if source_pool:
> +            PR.info("Migrating from device {} to {}. This may take a while...".format(
> +                args.source,
> +                args.target
> +            ))
> +            source_pool.migrate(source, args.target)
>          else:
> -            raise problem.ProgrammingError("Neither PV or non-PV was found. This is a bug")
> +            target = self.dev[args.target]
> +            source = self.dev[args.source]
> +            source.migrate(args.source, args.target)

The target and source assignments are not used.

>
>      def can_check(self, device):
>          fs = self.is_fs(device)
> diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> index 1148ca5..b76d738 100644
> --- a/ssmlib/problem.py
> +++ b/ssmlib/problem.py
> @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
>             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
>             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
>             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> -           "NotImplemented"]
> +           "NotImplemented", "ExistingSignature"]
>
>  # Define prompt codes
>  PROMPT_NONE =           0
> @@ -148,11 +148,17 @@ class ExistingFilesystem(SsmError):
>      def __init__(self, msg, errcode=2015):
>          super(ExistingFilesystem, self).__init__(msg, errcode)
>
> +
>  class NotImplemented(SsmError):
>      def __init__(self, msg, errcode=2016):
>          super(NotImplemented, self).__init__(msg, errcode)
>
>
> +class ExistingSignature(SsmError):
> +    def __init__(self, msg, errcode=2017):
> +        super(ExistingSignature, self).__init__(msg, errcode)
> +
> +
>  class ProblemSet(object):
>
>      def __init__(self, options):
> @@ -212,6 +218,10 @@ class ProblemSet(object):
>              ['Filesystem \'{0}\' detected on the device \'{1}\'!',
>               PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingFilesystem]
>
> +        self.EXISTING_SIGNATURE = \
> +            ['Signature \'{0}\' detected on the device \'{1}\'!',
> +             PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingSignature]
> +
>          self.NO_DEVICES = \
>              ['No devices available to use for the \'{0}\' pool!',
>               PROMPT_NONE, FL_FATAL, NoDevices]
> @@ -233,7 +243,7 @@ class ProblemSet(object):
>               PROMPT_NONE, FL_FATAL, NotSupported]
>
>          self.NOT_IMPLEMENTED = \
> -            ['\'{0}\' function is not implemented by {1}!',
> +            ['\'{0}\' functionality is not implemented by {1}!',
>              PROMPT_NONE, FL_FATAL, NotImplemented]
>
>          self.CREATE_DIRECTORY = \
> diff --git a/tests/bashtests/018-migrate.sh b/tests/bashtests/018-migrate.sh
> index 98b57f0..44e8c2d 100755
> --- a/tests/bashtests/018-migrate.sh
> +++ b/tests/bashtests/018-migrate.sh
> @@ -21,7 +21,7 @@ export test_description='Test the migrate command.'
>  . lib/test
>
>  export COLUMNS=1024
> -DEV_COUNT=6
> +DEV_COUNT=8
>  DEV_SIZE=200
>  aux prepare_devs $DEV_COUNT $DEV_SIZE
>  aux prepare_mnts 2
> @@ -31,6 +31,8 @@ export SSM_DEFAULT_BACKEND='lvm'
>  export SSM_LVM_DEFAULT_POOL=$vg1
>  export SSM_BTRFS_DEFAULT_POOL=$vg1
>  export SSM_NONINTERACTIVE='1'
> +lvol1=${LVOL_PREFIX}001
> +lvol2=${LVOL_PREFIX}002
>
>  cleanup() {
>         aux teardown
> @@ -38,102 +40,262 @@ cleanup() {
>
>  trap cleanup EXIT
>
> -
>  function compare_hash()
>  {
> -       a=$(sha1sum $1 | cut -f1 -d' ')
> -       b=$(sha1sum $2 | cut -f1 -d' ')
> +       a=$(cksum $1 | awk '{print $1 $2}')
> +       b=$(cksum $2 | awk '{print $1 $2}')
>         test "$a" = "$b"
>  }
>
> -function reset_devs()
> -{
> -       # try to clean anything that could linger on any test dev
> -       yes 'c' | ssm -f remove $SSM_LVM_DEFAULT_POOL || true
> -       for dev in $TEST_DEVS; do
> -               pvremove $dev || umount $dev || true
> -               pvcreate -f $dev
> -       done
> -
> -}
> -
> -function test_mount()
> +function test_volume()
>  {
> -       mount $1 $mnt1
> +       ssm check $1
> +       ssm mount $1 $mnt1
>         umount $mnt1
>  }
>
> -mkfs.ext4 $dev1
> -mkfs.ext4 $dev2
> -ssm migrate $dev1 $dev3
> -! ssm migrate $dev2 $dev3
> -ssm -f migrate $dev2 $dev3
> +# Test some nonsense
> +not ssm migrate foobar1 foobar2
> +not ssm migrate foobar1
> +not ssm migrate $dev1 $mnt1
> +not ssm migrate $mnt1 $dev1
>
> -reset_devs
> +# Migrate plain device

I would add a TODO to here with the information about why it is commented out.

Thanks,
Jan

> +#dd if=/dev/urandom of=$dev1 bs=1k count=1
> +#dd if=/dev/urandom of=$dev2 bs=1k count=1
> +#sync
> +#ssm -f migrate $dev1 $dev2
> +#compare_hash $dev1 $dev2
> +#
> +# Migrate filesystem on plain device
> +#mkfs.ext4 $dev1
> +#mkfs.ext4 $dev2
> +#ssm migrate $dev1 $dev3
> +#not ssm migrate $dev2 $dev3
> +#ssm -f migrate $dev2 $dev3
> +#wipefs -a $dev1
> +#wipefs -a $dev2
>
>  # BTRFS
>  export SSM_DEFAULT_BACKEND='btrfs'
>
> -ssm create $dev1
> -ssm -f migrate $dev1 $dev2
> -test_mount $dev2
> -ssm -f migrate $dev2 $dev3
> -test_mount $dev3
> -reset_devs
> +# Simple btrfs migrate to plain device
> +#ssm create $dev1
> +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> +#ssm migrate $dev1 $dev2
> +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> +#test_volume $SSM_BTRFS_DEFAULT_POOL
> +# Btrfs migrate on device with signature (ext4)
> +#not ssm migrate $dev2 $dev3
> +#ssm -f migrate $dev2 $dev3
> +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev3
> +#test_volume $SSM_BTRFS_DEFAULT_POOL
> +#ssm -f remove --all
>
> +# Simple btrfs migrate of mounted fs
> +ssm create $dev1 $mnt1
> +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> +not ssm migrate $mnt1 $dev2
> +ssm migrate $dev1 $dev2
> +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> +umount $mnt1
> +ssm check $SSM_BTRFS_DEFAULT_POOL
> +ssm -f remove --all
> +
> +# Migrate device from multi-device btrfs
>  ssm create -p $vg1 $dev1 $dev2
>  ssm create -p $vg2 $dev3 $dev4 $dev5
> -ssm list
>  ssm -f migrate $dev2 $dev6
> -test_mount $dev1
> -test_mount $dev6
> +check btrfs_devices $vg1 $dev1 $dev6
> +check btrfs_devices $vg2 $dev3 $dev4 $dev5
> +test_volume $vg1
> +test_volume $vg2
>
> -! ssm migrate $dev1 $dev3
> +# Migrate to a device already used in btrfs pool
> +not ssm migrate $dev1 $dev3
>  ssm -f migrate $dev1 $dev3
> -test_mount $dev3
> -test_mount $dev4
> -reset_devs
> +check btrfs_devices $vg1 $dev3 $dev6
> +check btrfs_devices $vg2 $dev4 $dev5
> +test_volume $vg1
> +test_volume $vg2
> +ssm -f remove --all
>
> -ssm create $dev1 $dev2
> -ssm -f migrate $dev1 $dev3
> -test_mount $dev2
> -test_mount $dev3
> -reset_devs
> +# Do not allow migrate between devices within a single pool in btrfs
> +ssm create $dev1 $dev2 $dev3 $dev4
> +not ssm migrate $dev1 $dev2
> +not ssm -f migrate $dev1 $dev2
> +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> +ssm -f remove --all
>
> -reset_devs
> -ssm -b btrfs create $dev1 $dev2 $dev3
> -ssm list
> -ssm migrate $dev1 $dev2
> +# migrate plain device to a device in btrfs pool - not used
> +#ssm create -p $vg1 $dev1 $dev2 $dev3
> +#mkfs.ext4 -F $dev4
> +#not ssm migrate $dev4 $dev3
> +#ssm -f migrate $dev4 $dev3
> +#check btrfs_devices $vg1 $dev1 $dev2
> +#test_volume $vg1
> +#fsck.ext4 -fn $dev3
> +
> +# migrate plain device to a device in btrfs pool - used completely
> +#ssm -f add -p $vg1 $dev3
> +#ssm mount $vg1 $mnt1
> +# Fill it up as much as we can, we do not care if dd fails
> +#! dd if=/dev/zero of=$mnt1/file bs=1M
> +#sync
> +#umount $mnt1
> +#not ssm migrate $dev4 $dev1
> +#not ssm -f migrate $dev4 $dev1
> +#check btrfs_devices $vg1 $dev1 $dev2 $dev3
> +#test_volume $vg1
> +#ssm -f remove --all
>
>  export SSM_DEFAULT_BACKEND='lvm'
>
>  # LVM
> -! ssm migrate foobar1 foobar2
> -ssm -f migrate $dev1 $dev2
> -reset_devs
>
> -# should not pass because there is already a signature and -f is not used
> -! ssm migrate $dev3 $dev4
> -! ssm migrate $dev5 foobar2
> -! ssm migrate foobar2 $dev5
> -reset_devs
> +# Simple lvm migrate to plain device
> +#ssm create --fs ext4 $dev1
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1
> +#ssm migrate $dev1 $dev2
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> +#mkfs.ext4 $dev3
> +# lvm migrate to a device with signature (ext4)
> +#not ssm migrate $dev2 $dev3
> +#ssm -f migrate $dev2 $dev3
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> +#ssm -f remove --all
>
> -ssm create --size $DEV_SIZE $dev1 $dev2
> -ssm migrate $dev1 $dev2
> -ssm -f migrate $dev2 $dev3
> -reset_devs
> +# Simple lvm migrate without pv being used
> +ssm add $dev1 $dev2 $dev3
> +ssm migrate $dev1 $dev5
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> +# Migrate to a device in the same pool
> +ssm migrate $dev2 $dev3
> +ssm -f remove --all
>
> -ssm create --size $DEV_SIZE $dev1 $dev2 $dev3
> -! ssm -n migrate $dev3 $dev3
> -reset_devs
> +# Migrate used device to free that's already in the pool
> +ssm create $dev1 $dev2
> +ssm -f add $dev3 $dev4
> +ssm migrate $dev1 $dev3
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> +ssm -f remove --all
>
> -# should fail - the target is already mounted or used in another pool
> -mkfs.ext4 $dev1
> -mount $dev1 $mnt1
> +# All devices are used
> +ssm create $dev1
>  ssm create $dev2
> -! ssm migrate $dev2 $dev1
> -! ssm migrate $dev1 $dev2
> -ssm -f migrate $dev2 $dev1
> -reset_devs
> +not ssm migrate $dev1 $dev2
> +# Only specified devices are used
> +ssm add $dev3
> +not ssm migrate $dev1 $dev2
> +ssm migrate $dev2 $dev3
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> +ssm -f remove --all
> +
> +# Simple lvm migrate of mounted fs
> +ssm create --fs ext4 $dev1 $mnt1
> +wipefs -a $dev2
> +not ssm migrate $mnt1 $dev2
> +ssm migrate $dev1 $dev2
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> +umount $mnt1
> +ssm check $SSM_LVM_DEFAULT_POOL/$lvol1
> +ssm -f remove --all
> +
> +# Migrate to a device from different pool
> +ssm create -p $vg1 $dev1 $dev2 $dev3
> +ssm add -p $vg1 $dev7
> +ssm create -p $vg2 $dev4 $dev5 $dev6
> +ssm add -p $vg2 $dev8
> +not ssm migrate $dev1 $dev8
> +ssm -f migrate $dev1 $dev8
> +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3 $dev7
> +check vg_devices $vg2 $dev4 $dev5 $dev6
> +# Migrate device that is not used
> +not ssm migrate $dev4 $dev7
> +ssm -f migrate $dev4 $dev7
> +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3
> +check vg_devices $vg2 $dev4 $dev5 $dev6 $dev7
> +# Migrate device that is used
> +not ssm add -p $vg1 $dev1 # We're not removing the pv after migrate
> +not ssm migrate $dev5 $dev1
> +ssm -f migrate $dev5 $dev1
> +ssm -f remove --all
> +
> +# Migrate plain device to a device in lvm
> +ssm create $dev1 $dev2 $dev3
> +ssm add -p $SSM_LVM_DEFAULT_POOL $dev4 $dev5
> +# target device used
> +not ssm migrate $dev6 $dev3
> +not ssm -f migrate $dev6 $dev3
> +# target device not used
> +not ssm migrate $dev6 $dev4
> +ssm -f migrate $dev6 $dev4
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> +# Migrate plain device with a file system
> +#mkfs.ext4 -F $dev4
> +# target device used
> +#not ssm migrate $dev4 $dev1
> +#not ssm -f migrate $dev4 $dev1
> +# target not used
> +#not ssm migrate $dev4 $dev5
> +#ssm -f migrate $dev4 $dev5
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> +#fsck.ext4 -fn $dev5
> +#test_volume $dev5
> +ssm -f remove --all
> +
> +# migrate from btrfs pool to lvm
> +wipefs -a $dev4 $dev5
> +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> +not ssm migrate $dev5 $dev3
> +ssm -f migrate $dev5 $dev3
> +check btrfs_devices $vg1 $dev1 $dev2
> +check vg_devices $vg2 $dev4 $dev5 $dev3
> +test_volume $vg1
> +test_volume $vg2/$lvol1
> +ssm -f remove --all
> +
> +# migrate from lvm to btrfs pool
> +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> +ssm add -p $vg2 $dev6
> +not ssm migrate $dev3 $dev6
> +ssm -f migrate $dev3 $dev6
> +check btrfs_devices $vg1 $dev1 $dev2 $dev6
> +check vg_devices $vg2 $dev4 $dev5
> +test_volume $vg1
> +test_volume $vg2/$lvol1
> +ssm -f remove --all
> +
> +# CRYPT
> +
> +# cryptsetup will ask for password. If luks extension is used
> +# it will ask when creating header and then when opening the device
> +pass() {
> +       echo -e "${passwd}\n${passwd}"
> +}
> +
> +export SSM_DEFAULT_BACKEND='crypt'
> +export SSM_CRYPT_DEFAULT_POOL=$vg3
> +export CRYPT_VOL_PREFIX="${SSM_PREFIX_FILTER}enc"
> +export SSM_CRYPT_DEFAULT_VOL_PREFIX=$CRYPT_VOL_PREFIX
> +crypt_vol1=${CRYPT_VOL_PREFIX}001
> +
> +passwd="cai0ohMo8M"
> +pass | ssm create $dev1
> +check crypt_vol_field $crypt_vol1 type LUKS1
> +check crypt_vol_field $crypt_vol1 device $dev1
> +check list_table "$(ssm list vol)" $crypt_vol1 $SSM_CRYPT_DEFAULT_POOL none crypt
> +
> +# Migrate in crypt is not currently supported so this whould both fail
> +not ssm migrate $dev1 $dev2
> +not ssm -f migrate $dev1 $dev2
> +
> +# Device can't be simply removed from the crypt backend so this should fail
> +not ssm migrate $dev2 $dev1
> +not ssm -f migrate $dev2 $dev1
>
> -exit 0
> \ No newline at end of file
> +exit 0
> diff --git a/tests/bashtests/lib/check.sh b/tests/bashtests/lib/check.sh
> index 4dad7c4..2bdc790 100644
> --- a/tests/bashtests/lib/check.sh
> +++ b/tests/bashtests/lib/check.sh
> @@ -257,6 +257,29 @@ lv_field()
>         fi
>  }
>
> +vg_devices()
> +{
> +       # Check the expected list of devices in the fs with the reality
> +       # $1 is the vg we're inspecting
> +       # the rest is list of devices
> +       tmp=`mktemp`
> +
> +       devices=`vgs --no-headings -o pv_name $1 | awk {'print $1'}`
> +       shift
> +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> +
> +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> +       if [ $? -eq 0 ]; then
> +               echo "ACTUAL"
> +               cat ${tmp}.actual
> +               echo "EXPECTED"
> +               cat ${tmp}.expected
> +               exit 1
> +       fi
> +       exit 0
> +}
> +
>  compare_fields()
>  {
>         local cmd1=$1;
> @@ -334,6 +357,29 @@ crypt_vol_field()
>         fi
>  }
>
> +btrfs_devices()
> +{
> +       # Check the expected list of devices in the fs with the reality
> +       # $1 is label for the file system we want to test
> +       # the rest is list of devices
> +       tmp=`mktemp`
> +
> +       devices=`btrfs filesystem show $1| grep devid | awk {'print $NF'}`
> +       shift
> +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> +
> +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> +       if [ $? -eq 0 ]; then
> +               echo "ACTUAL"
> +               cat ${tmp}.actual
> +               echo "EXPECTED"
> +               cat ${tmp}.expected
> +               exit 1
> +       fi
> +       exit 0
> +}
> +
>  btrfs_fs_field()
>  {
>         lines=$(btrfs filesystem show 2> /dev/null | grep -A 1 "^Label:.*$1" || true)
> diff --git a/tests/unittests/test_btrfs.py b/tests/unittests/test_btrfs.py
> index a73420f..1e8d7e1 100644
> --- a/tests/unittests/test_btrfs.py
> +++ b/tests/unittests/test_btrfs.py
> @@ -20,6 +20,7 @@
>
>  import unittest
>  from ssmlib import main
> +from ssmlib import problem
>  from ssmlib.backends import btrfs
>  from tests.unittests.common import *
>
> @@ -461,8 +462,10 @@ class BtrfsFunctionCheck(MockSystemDataSource):
>          self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
>          self._addDevice('/dev/sde', 11489037516)
>
> -        self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> -        "btrfs device delete /dev/sdc1 /tmp/mount")
> +        try:
> +            main.main("ssm migrate /dev/sdc1 /dev/sdc2")
> +        except problem.DeviceUsed:
> +            pass
>
>          self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
>          "btrfs replace start -B /dev/sdc2 /dev/sde /tmp/mount")
> diff --git a/tests/unittests/test_lvm.py b/tests/unittests/test_lvm.py
> index fe87c4c..ec565e0 100644
> --- a/tests/unittests/test_lvm.py
> +++ b/tests/unittests/test_lvm.py
> @@ -257,21 +257,8 @@ class LvmFunctionCheck(MockSystemDataSource):
>          self._addDevice('/dev/sde', 11489037516)
>
>          self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> -        "lvm pvremove /dev/sdc1")
> -        self.assertEqual(self.run_data[-2],
> -            "lvm vgreduce my_pool /dev/sdc1")
> -        self.assertEqual(self.run_data[-3],
>              "lvm pvmove --atomic /dev/sdc1 /dev/sdc2")
>
> -        self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
> -        "lvm pvremove /dev/sdc2")
> -        self.assertEqual(self.run_data[-2],
> -            "lvm vgreduce my_pool /dev/sdc2")
> -        self.assertEqual(self.run_data[-3],
> -            "lvm pvmove --atomic /dev/sdc2 /dev/sde")
> -        self.assertEqual(self.run_data[-4],
> -            "lvm vgextend my_pool /dev/sde")
> -
>      def test_lvm_resize(self):
>          # Generate some storage data
>          self._addPool('default_pool', ['/dev/sda', '/dev/sdb'])
> --
> 2.17.1
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> storagemanager-devel mailing list
> storagemanager-devel@...
> https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)



-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH] migrate: remove fs signatures when using dd

[Jan Tulak <jtulak@re...>]

On Tue, Aug 7, 2018 at 11:32 AM Jan Tulak <jtulak@...> wrote:
>
> On Fri, Aug 3, 2018 at 8:33 AM Lukas Czerner <lczerner@...> wrote:
> >
> > On Tue, Jul 31, 2018 at 01:50:33PM +0200, Jan Tulak wrote:
> > > On Tue, Jul 31, 2018 at 11:13 AM Lukas Czerner <lczerner@...> wrote:
> > > >
> > > > On Mon, Jul 30, 2018 at 04:58:19PM +0200, Jan Tulak wrote:
> > > > > Make the migrate command behave the same way in all cases. For btrfs and
> > > > > lvm the signatures are removed by the respective utilities, so do the
> > > > > same for dd-copied devices as well.
> > > > >
> > > > > No need of the manual is necessary, it already states that accessing the
> > > > > data on the source device can be difficult afterwards.
> > > >
> > > > I gave it some more though lately and I am starting to think that while
> > > > consistency is useful we also need to consider the expectations and
> > > > convenience. I am not really sure what's the right call with the plain
> > > > file system case, but maybe wiping the signature makes the most sense.
> > > >
> > > > However as for lvm, I am strongly considering changing it so that the pv
> > > > is _not_ removed from the vg after the migrate. For example it would be
> > > > a perfectly valid use case to replace a ssd pv to make use of it in a
> > > > different lv where we want it to be faster, or we want to use it for
> > > > caching. And removing it just complicates it for the user. Unless you
> > > > have any strong arguments against it I am going to change it.
> > >
> > > Yes, I see the case for it. However, in that case, we need to be more
> > > explicit about the result, printing out what happened, and maybe what
> > > to do next or how to revert it, because while such a change is useful,
> > > I don't think that it is an obvious and expected result.
> >
> > I'd actually argue that this is obvious and expected behavior. In lvm
> > the device does not have to be removed from the pool to be unused. It's
> > perfectly ok just sitting in the volume group not being used by
> > anything. And 'migrate' does not really imply 'remove the volume from
> > the pool'.
> >
> > As for btrfs, it needs to remove it from the filesystem since it does
> > not really have any notion of "pool" it's ssm construct. It can't be in
> > the file system and still be (and remain) unused.
>
> OK, I have no issue with it. But anyway, is there anything wrong with
> this patch were are discussing under (e.g. adding an explicit message
> in v2 of this patch?) Or can you add reviewed-by?
>

Or better to say, it has to be rebased against your changes of
migrate, so I will send a v2 anyway. Other than that, is there any
issue?

Jan

> Thanks,
> Jan
>
> >
> > -Lukas
> >
> > >
> > > Jan
> > >
> > > >
> > > > Thanks!
> > > > -Lukas
> > > >
> > > >
> > > > >
> > > > > Signed-off-by: Jan Tulak <jtulak@...>
> > > > > ---
> > > > >  ssmlib/main.py | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/ssmlib/main.py b/ssmlib/main.py
> > > > > index 51e5033..4011d21 100644
> > > > > --- a/ssmlib/main.py
> > > > > +++ b/ssmlib/main.py
> > > > > @@ -990,8 +990,14 @@ class DeviceItem(Item):
> > > > >
> > > > >          command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> > > > >          misc.run(command)
> > > > > +
> > > > > +        fs = misc.get_fs_type(source)
> > > > > +        if fs:
> > > > > +            misc.wipefs(source, fs)
> > > > > +
> > > > >          misc.send_udev_event(source, "change")
> > > > >          misc.send_udev_event(target, "change")
> > > > > +        misc.udev_settle()
> > > > >
> > > > >
> > > > >  class VolumeItem(Item):
> > > > > --
> > > > > 2.16.2
> > > > >
> > > > >
> > > > > ------------------------------------------------------------------------------
> > > > > Check out the vibrant tech community on one of the world's most
> > > > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > > > > _______________________________________________
> > > > > storagemanager-devel mailing list
> > > > > storagemanager-devel@...
> > > > > https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> > > > > System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)
> > >
> > >
> > >
> > > --
> > > Jan Tulak
> > > jtulak@... / jan@...
>
>
>
> --
> Jan Tulak
> jtulak@... / jan@...



-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH 1/4] lvm: fix backtrace when vg/lv is inactive or broken

[Jan Tulak <jtulak@re...>]

On Tue, Aug 7, 2018 at 11:52 AM Lukas Czerner <lczerner@...> wrote:
>
snip

> > > --- a/ssmlib/backends/lvm.py
> > > +++ b/ssmlib/backends/lvm.py
> > > @@ -552,8 +552,13 @@ class SnapInfo(LvmInfo):
> > >          snap['snap_path'] = snap['dev_name']
> > >
> > >          if snap['type'] != "thin":
> > > -            size = float(snap['vol_size']) * float(snap['snap_size'])
> > > -            snap['snap_size'] = str(size / 100.00)
> > > +            # It's possible that snap_percent is not defined. For example
> >
> > And here s/snap_percent/snap_size/
>
> Actually I ment snap_percent. It's the lvm field ssm takes snap_size
> from, so both are fine.

Ah, ok. In that case I would rewrite it to "It's possible that in lvs
output the field snap_percent is not defined." Or something like that.

Jan

-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH 4/4] ssm: Rework migrate command

[Lukas Czerner <lczerner@re...>]

On Tue, Aug 07, 2018 at 11:30:09AM +0200, Jan Tulak wrote:
> On Thu, Aug 2, 2018 at 5:00 PM Lukas Czerner <lczerner@...> wrote:
> >
> > Migrate command was not working as expected in many ways so here is a
> > rework of it. The 018 bashtest is also reworked and significantly
> > extended to cover more possible scenarios and test the actual results of
> > the operation.
> >
> > Lvm migrate now does not remove the pv from the volume group as I feel
> > there are perfectly valid scenarios where user would like to migrate
> > used pv to a different device and repurpose the source device.
> >
> > unittests for lvm and btrfs are only changed so that they not fail,
> > however the unittests needs to be extended for btrfs and lvm as well.
> > Generic migrate unittests (test_ssm.py) are missing entirely.
> >
> > Also some more fine grained testing on plain devices is completely
> > missing at the moment because in this way the testing filter actually
> > interfere. Those tests are commented out in the 018.So there is still
> > some work to be done.
> >
> > Signed-off-by: Lukas Czerner <lczerner@...>
> 
> What revision is this based against? I couldn't get it apply all the
> way to v1.0 tag. I tried to apply it on top of the previous three
> patches in this set.
> Thanks,

Hi this is obviously based on those three. Additionally it also requires
two patches I sent previously:

btrfs: send udev change event after some more btrfs operations
btrfs: Send udev change event for every device after fs creation

and that's based on the devel branch.

-Lukas

> 
> Jan
> 
> 
> > ---
> >  ssmlib/backends/btrfs.py       |  10 +-
> >  ssmlib/backends/lvm.py         |  18 +-
> >  ssmlib/backends/template.py    |   2 +-
> >  ssmlib/main.py                 | 199 ++++++++++------------
> >  ssmlib/problem.py              |  14 +-
> >  tests/bashtests/018-migrate.sh | 298 +++++++++++++++++++++++++--------
> >  tests/bashtests/lib/check.sh   |  46 +++++
> >  tests/unittests/test_btrfs.py  |   7 +-
> >  tests/unittests/test_lvm.py    |  13 --
> >  9 files changed, 397 insertions(+), 210 deletions(-)
> >
> > diff --git a/ssmlib/backends/btrfs.py b/ssmlib/backends/btrfs.py
> > index bd7db3f..39ebbce 100644
> > --- a/ssmlib/backends/btrfs.py
> > +++ b/ssmlib/backends/btrfs.py
> > @@ -600,12 +600,10 @@ class BtrfsPool(Btrfs, template.BackendPool):
> >              pool['mount'] = tmp
> >
> >          dev = BtrfsDev(options=self.options).data
> > -        if dev.get(target) and dev.get(source.name):
> > -            # both source and target are already members of the pool
> > -            # so we are going to only remove source
> > -            command = ['device', 'delete', source.name, pool['mount']]
> > -        else:
> > -            command = ['replace', 'start', '-B', source.name, target, pool['mount']]
> > +        command = ['replace', 'start']
> > +        if self.options.force:
> > +            command.extend(['-f'])
> > +        command.extend(['-B', source.name, target, pool['mount']])
> >
> >          self.run_btrfs(command)
> >          misc.send_udev_event(target, "change")
> > diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
> > index e726d9c..f1373bd 100644
> > --- a/ssmlib/backends/lvm.py
> > +++ b/ssmlib/backends/lvm.py
> > @@ -348,23 +348,17 @@ class VgsInfo(LvmInfo, template.BackendPool):
> >          target : [str]
> >              Path to the target device.
> >          """
> > -        pvsinfo = PvsInfo(options=self.options).data
> > -        if target in pvsinfo:
> > -            tgt = pvsinfo[target]
> > -            if tgt['pool_name'] != vg:
> > -                self.extend(vg, [target])
> > -        else:
> > -            self.extend(vg, [target])
> > +        # We're ready to do pvmove source and target must be already in the
> > +        # pool
> >
> >          # pvmove does not accept -f, temporarily unset it
> >          force = self.options.force
> >          self.options.force = False
> >
> > -        command = ['pvmove', '--atomic', source.name, target ]
> > -        self.run_lvm(command)
> > -        self.reduce(vg, source.name)
> > -        command = ['pvremove', source.name]
> > -        self.run_lvm(command)
> > +        # If the source is not used we do not have to do anything
> > +        if float(source['dev_used']) > 0.0:
> > +            command = ['pvmove', '--atomic', source.name, target ]
> > +            self.run_lvm(command)
> >
> >          misc.send_udev_event(source.name, "change")
> >          misc.send_udev_event(target, "change")
> > diff --git a/ssmlib/backends/template.py b/ssmlib/backends/template.py
> > index 8fbb8d2..a17cbb7 100644
> > --- a/ssmlib/backends/template.py
> > +++ b/ssmlib/backends/template.py
> > @@ -76,7 +76,7 @@ class BackendPool(Backend):
> >
> >      def migrate(self, pool, sources, targets=None):
> >          self.problem.check(self.problem.NOT_IMPLEMENTED,
> > -                        ["Migrating data in pools", "{} backend".format(self.type)])
> > +                        ["Migrating devices", "{} backend".format(self.type)])
> >
> >
> >  class BackendVolume(Backend):
> > diff --git a/ssmlib/main.py b/ssmlib/main.py
> > index 9a0a2f5..1841edd 100644
> > --- a/ssmlib/main.py
> > +++ b/ssmlib/main.py
> > @@ -443,6 +443,41 @@ class DeviceInfo(object):
> >                   "to achieve by removing " +
> >                   "{0}".format(device))
> >
> > +    def migrate(self, vg, source_dev, target_dev):
> > +        """ Migrate (in this case clone) the device onto another one.
> > +            Pure dd is used.
> > +        """
> > +        # TODO some possibilities:
> > +        # - error handling: conv=noerror,sync (continue on error and pad
> > +        #   with zeros), or better to fail on error?
> > +        source = self.data[source_dev]
> > +        target = self.data[target_dev]
> > +
> > +        if misc.get_signature(source_dev) == 'btrfs':
> > +            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> > +
> > +        if source['dev_size'] > target['dev_size']:
> > +            raise problem.GeneralError("Target device is smaller than source device")
> > +
> > +        if 'mount' in target:
> > +            if PR.check(PR.FS_MOUNTED, [target_dev, target['mount']]):
> > +                misc.do_umount(target_dev)
> > +
> > +        if 'mount' in source:
> > +            if PR.check(PR.FS_MOUNTED, [source_dev, source['mount']]):
> > +                misc.do_umount(source_dev)
> > +
> > +        print("Migrating {s} ({size}) to {t} using dd . This may take a long time...".format(
> > +            s=source_dev,
> > +            t=target_dev,
> > +            size=misc.humanize_size(source['dev_size'])))
> > +
> > +        command = ['dd', 'if={}'.format(source_dev), 'of={}'. \
> > +                   format(target_dev), 'conv=fsync']
> > +        misc.run(command)
> > +        misc.send_udev_event(source_dev, "change")
> > +        misc.send_udev_event(target_dev, "change")
> > +
> >      def set_globals(self, options):
> >          self.options = options
> >
> > @@ -962,36 +997,6 @@ class DeviceItem(Item):
> >                      return True
> >          return False
> >
> > -    @classmethod
> > -    def migrate(cls, source, target):
> > -        """ Migrate (in this case clone) the device onto another one.
> > -            Pure dd is used.
> > -        """
> > -        # TODO some possibilities:
> > -        # - error handling: conv=noerror,sync (continue on error and pad
> > -        #   with zeros), or better to fail on error?
> > -
> > -        if misc.get_signature(source) == 'btrfs':
> > -            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> > -
> > -        if misc.get_device_size(source) > misc.get_device_size(target):
> > -            raise problem.GeneralError("Target device is smaller than source device")
> > -
> > -        if misc.get_mountinfo(source):
> > -            raise problem.GeneralError("You have to umount the source device first")
> > -        if misc.get_mountinfo(target):
> > -            raise problem.GeneralError("You have to umount the target device first")
> > -
> > -        print("Migrating {s} ({size}) to {t}. This may take a long time...".format(
> > -            s=source,
> > -            t=target,
> > -            size=misc.humanize_size(misc.get_device_size(source))))
> > -
> > -        command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> > -        misc.run(command)
> > -        misc.send_udev_event(source, "change")
> > -        misc.send_udev_event(target, "change")
> > -
> >
> >  class VolumeItem(Item):
> >      def __init__(self, *args, **kwargs):
> > @@ -2111,88 +2116,70 @@ class StorageHandle(object):
> >                                                         args.options))
> >
> >      def migrate(self, args):
> > -        """ Migrate data from a device to a device"""
> > -        source = self._find_device_record(args.source)
> > -        target = self._find_device_record(args.target)
> > -
> > -        # decide if we have PVs or non-PV block devices
> > -        found_pv = False
> > -        pool = None
> > -        check_fs = True
> > -        signature = misc.get_signature(target)
> > -
> > -        # get source dev and try to detect if it is a member of a pool
> > -        source_dev = self.dev[source]
> > -        if source_dev and 'pool_name' in source_dev:
> > -            found_pv = True
> > -            pool = self.pool[source_dev['pool_name']]
> > +        """
> > +        Migrate data from a device to a device
> > +        """
> > +
> > +        source_pool = None
> > +        target_pool = None
> > +        changed = False
> > +        source = self.dev[args.source]
> > +        if source and 'pool_name' in source:
> > +            source_pool = self.pool[source['pool_name']]
> > +        target = self.dev[args.target]
> > +        if target and 'pool_name' in target:
> > +            target_pool = self.pool[target['pool_name']]
> > +
> > +        if target_pool:
> > +            if not source_pool or (target_pool.name != source_pool.name):
> > +                if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> > +                    raise problem.UserInterrupted("Terminated by user!")
> > +                target_pool.reduce(target.name)
> > +                target_pool = None
> > +                changed = True
> > +            elif target_pool.type == "btrfs":
> > +                raise problem.DeviceUsed(
> > +                    "Target and source devices are in the same btrfs pool!")
> >          else:
> > -            pass
> > -            # This seems to be only a case for some tests where the /tmp/<TEMP>/dev/...
> > -            # devices gets incorrectly identified (sometimes they do, sometimes they don't).
> > -            # Keep it commented out for now, because it seems to be rather an issue with
> > -            # the tests
> > -            # source_signature = misc.get_signature(source)
> > -            # if source_signature == 'btrfs':
> > -            #    print("found btrfs")
> > -            #    found_pv = True
> > -            #    import pdb; pdb.set_trace()
> > -
> > -        target_dev = self.dev[target]
> > -        if target_dev:
> > -            if 'pool_name' in target_dev and\
> > -                (signature != 'btrfs' or (pool and int(pool['dev_count']) > 1)):
> > -                check_fs = False
> > -
> > -                if target_dev['pool_name'] != '':
> > -                    if pool and target_dev['pool_name'] != pool.name:
> > -                        if PR.check(PR.DEVICE_USED,
> > -                            [target_dev.name, target_dev['pool_name']]):
> > -                            remove_args = Struct()
> > -                            remove_args.all = False
> > -                            remove_args.items = [target_dev]
> > -                            if not self.remove(remove_args):
> > -                                raise problem.CommandFailed("The device {} could not be removed from the pool {}".format(
> > -                                    target,
> > -                                    target_dev['pool_name']
> > -                                ))
> > -                            misc.wipefs(target_dev.name, signature)
> > -                        else:
> > -                            raise problem.UserInterrupted("Aborted")
> > +            # Check signature of existing file system on the device
> > +            # and ask user whether to use it or not.
> > +            if target and 'mount' in target:
> > +                if PR.check(PR.FS_MOUNTED, [args.target, target['mount']]):
> > +                    misc.do_umount(target.name)
> >              else:
> > -                check_fs = True
> > +                signature = misc.get_signature(args.target)
> > +                if signature and \
> > +                   PR.check(PR.EXISTING_SIGNATURE,
> > +                            [signature, args.target]):
> > +                    misc.wipefs(args.target, signature)
> > +                    changed = True
> > +                elif signature:
> > +                    raise problem.UserInterrupted("Terminated by user!")
> >
> > +        # TODO: If the source is in lvm pool and the target is not in any pool
> > +        # just add it to the source pool. This should not be here because
> > +        # we try to be as generic as possible. However untill we have all the
> > +        # data accessible in the backend we need do it here to avoid
> > +        # re-initializing information unnecessarily
> > +        if source_pool and source_pool.type == "lvm" and not target_pool:
> > +            source_pool.extend(args.target)
> >
> >
> > -        if check_fs:
> > -            mounts = misc.get_mountinfo(target)
> > -            if mounts:
> > -                if PR.check(PR.FS_MOUNTED, [target, mounts[target]['mp']]):
> > -                    misc.do_umount(target)
> > -                else:
> > -                    raise problem.CommandFailed("Umount of {} was not possible".format(target))
> > -
> > -            if signature and not self.options.force and\
> > -                PR.check(PR.EXISTING_FILESYSTEM,
> > -                        [signature, target]):
> > -                misc.wipefs(target, signature)
> > -            elif signature and not self.options.force:
> > -                # user wants to skip
> > -                raise problem.UserInterrupted("Aborted")
> > -            elif signature and self.options.force:
> > -                misc.wipefs(target, signature)
> > -
> > -        PR.info("Migrating from device {} to {}. This may take a long time...".format(
> > -            source,
> > -            target
> > -        ))
> > -        if not found_pv:
> > -            DeviceItem.migrate(source, target)
> > -
> > -        elif found_pv:
> > -            pool.migrate(source_dev, target)
> > +        if changed:
> > +            self.reinit_dev()
> > +            source = self.dev[args.source]
> > +            target = self.dev[args.target]
> > +
> > +        if source_pool:
> > +            PR.info("Migrating from device {} to {}. This may take a while...".format(
> > +                args.source,
> > +                args.target
> > +            ))
> > +            source_pool.migrate(source, args.target)
> >          else:
> > -            raise problem.ProgrammingError("Neither PV or non-PV was found. This is a bug")
> > +            target = self.dev[args.target]
> > +            source = self.dev[args.source]
> > +            source.migrate(args.source, args.target)
> >
> >      def can_check(self, device):
> >          fs = self.is_fs(device)
> > diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> > index 1148ca5..b76d738 100644
> > --- a/ssmlib/problem.py
> > +++ b/ssmlib/problem.py
> > @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
> >             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
> >             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
> >             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> > -           "NotImplemented"]
> > +           "NotImplemented", "ExistingSignature"]
> >
> >  # Define prompt codes
> >  PROMPT_NONE =           0
> > @@ -148,11 +148,17 @@ class ExistingFilesystem(SsmError):
> >      def __init__(self, msg, errcode=2015):
> >          super(ExistingFilesystem, self).__init__(msg, errcode)
> >
> > +
> >  class NotImplemented(SsmError):
> >      def __init__(self, msg, errcode=2016):
> >          super(NotImplemented, self).__init__(msg, errcode)
> >
> >
> > +class ExistingSignature(SsmError):
> > +    def __init__(self, msg, errcode=2017):
> > +        super(ExistingSignature, self).__init__(msg, errcode)
> > +
> > +
> >  class ProblemSet(object):
> >
> >      def __init__(self, options):
> > @@ -212,6 +218,10 @@ class ProblemSet(object):
> >              ['Filesystem \'{0}\' detected on the device \'{1}\'!',
> >               PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingFilesystem]
> >
> > +        self.EXISTING_SIGNATURE = \
> > +            ['Signature \'{0}\' detected on the device \'{1}\'!',
> > +             PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingSignature]
> > +
> >          self.NO_DEVICES = \
> >              ['No devices available to use for the \'{0}\' pool!',
> >               PROMPT_NONE, FL_FATAL, NoDevices]
> > @@ -233,7 +243,7 @@ class ProblemSet(object):
> >               PROMPT_NONE, FL_FATAL, NotSupported]
> >
> >          self.NOT_IMPLEMENTED = \
> > -            ['\'{0}\' function is not implemented by {1}!',
> > +            ['\'{0}\' functionality is not implemented by {1}!',
> >              PROMPT_NONE, FL_FATAL, NotImplemented]
> >
> >          self.CREATE_DIRECTORY = \
> > diff --git a/tests/bashtests/018-migrate.sh b/tests/bashtests/018-migrate.sh
> > index 98b57f0..44e8c2d 100755
> > --- a/tests/bashtests/018-migrate.sh
> > +++ b/tests/bashtests/018-migrate.sh
> > @@ -21,7 +21,7 @@ export test_description='Test the migrate command.'
> >  . lib/test
> >
> >  export COLUMNS=1024
> > -DEV_COUNT=6
> > +DEV_COUNT=8
> >  DEV_SIZE=200
> >  aux prepare_devs $DEV_COUNT $DEV_SIZE
> >  aux prepare_mnts 2
> > @@ -31,6 +31,8 @@ export SSM_DEFAULT_BACKEND='lvm'
> >  export SSM_LVM_DEFAULT_POOL=$vg1
> >  export SSM_BTRFS_DEFAULT_POOL=$vg1
> >  export SSM_NONINTERACTIVE='1'
> > +lvol1=${LVOL_PREFIX}001
> > +lvol2=${LVOL_PREFIX}002
> >
> >  cleanup() {
> >         aux teardown
> > @@ -38,102 +40,262 @@ cleanup() {
> >
> >  trap cleanup EXIT
> >
> > -
> >  function compare_hash()
> >  {
> > -       a=$(sha1sum $1 | cut -f1 -d' ')
> > -       b=$(sha1sum $2 | cut -f1 -d' ')
> > +       a=$(cksum $1 | awk '{print $1 $2}')
> > +       b=$(cksum $2 | awk '{print $1 $2}')
> >         test "$a" = "$b"
> >  }
> >
> > -function reset_devs()
> > -{
> > -       # try to clean anything that could linger on any test dev
> > -       yes 'c' | ssm -f remove $SSM_LVM_DEFAULT_POOL || true
> > -       for dev in $TEST_DEVS; do
> > -               pvremove $dev || umount $dev || true
> > -               pvcreate -f $dev
> > -       done
> > -
> > -}
> > -
> > -function test_mount()
> > +function test_volume()
> >  {
> > -       mount $1 $mnt1
> > +       ssm check $1
> > +       ssm mount $1 $mnt1
> >         umount $mnt1
> >  }
> >
> > -mkfs.ext4 $dev1
> > -mkfs.ext4 $dev2
> > -ssm migrate $dev1 $dev3
> > -! ssm migrate $dev2 $dev3
> > -ssm -f migrate $dev2 $dev3
> > +# Test some nonsense
> > +not ssm migrate foobar1 foobar2
> > +not ssm migrate foobar1
> > +not ssm migrate $dev1 $mnt1
> > +not ssm migrate $mnt1 $dev1
> >
> > -reset_devs
> > +# Migrate plain device
> > +#dd if=/dev/urandom of=$dev1 bs=1k count=1
> > +#dd if=/dev/urandom of=$dev2 bs=1k count=1
> > +#sync
> > +#ssm -f migrate $dev1 $dev2
> > +#compare_hash $dev1 $dev2
> > +#
> > +# Migrate filesystem on plain device
> > +#mkfs.ext4 $dev1
> > +#mkfs.ext4 $dev2
> > +#ssm migrate $dev1 $dev3
> > +#not ssm migrate $dev2 $dev3
> > +#ssm -f migrate $dev2 $dev3
> > +#wipefs -a $dev1
> > +#wipefs -a $dev2
> >
> >  # BTRFS
> >  export SSM_DEFAULT_BACKEND='btrfs'
> >
> > -ssm create $dev1
> > -ssm -f migrate $dev1 $dev2
> > -test_mount $dev2
> > -ssm -f migrate $dev2 $dev3
> > -test_mount $dev3
> > -reset_devs
> > +# Simple btrfs migrate to plain device
> > +#ssm create $dev1
> > +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> > +#ssm migrate $dev1 $dev2
> > +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> > +#test_volume $SSM_BTRFS_DEFAULT_POOL
> > +# Btrfs migrate on device with signature (ext4)
> > +#not ssm migrate $dev2 $dev3
> > +#ssm -f migrate $dev2 $dev3
> > +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev3
> > +#test_volume $SSM_BTRFS_DEFAULT_POOL
> > +#ssm -f remove --all
> >
> > +# Simple btrfs migrate of mounted fs
> > +ssm create $dev1 $mnt1
> > +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> > +not ssm migrate $mnt1 $dev2
> > +ssm migrate $dev1 $dev2
> > +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> > +umount $mnt1
> > +ssm check $SSM_BTRFS_DEFAULT_POOL
> > +ssm -f remove --all
> > +
> > +# Migrate device from multi-device btrfs
> >  ssm create -p $vg1 $dev1 $dev2
> >  ssm create -p $vg2 $dev3 $dev4 $dev5
> > -ssm list
> >  ssm -f migrate $dev2 $dev6
> > -test_mount $dev1
> > -test_mount $dev6
> > +check btrfs_devices $vg1 $dev1 $dev6
> > +check btrfs_devices $vg2 $dev3 $dev4 $dev5
> > +test_volume $vg1
> > +test_volume $vg2
> >
> > -! ssm migrate $dev1 $dev3
> > +# Migrate to a device already used in btrfs pool
> > +not ssm migrate $dev1 $dev3
> >  ssm -f migrate $dev1 $dev3
> > -test_mount $dev3
> > -test_mount $dev4
> > -reset_devs
> > +check btrfs_devices $vg1 $dev3 $dev6
> > +check btrfs_devices $vg2 $dev4 $dev5
> > +test_volume $vg1
> > +test_volume $vg2
> > +ssm -f remove --all
> >
> > -ssm create $dev1 $dev2
> > -ssm -f migrate $dev1 $dev3
> > -test_mount $dev2
> > -test_mount $dev3
> > -reset_devs
> > +# Do not allow migrate between devices within a single pool in btrfs
> > +ssm create $dev1 $dev2 $dev3 $dev4
> > +not ssm migrate $dev1 $dev2
> > +not ssm -f migrate $dev1 $dev2
> > +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> > +ssm -f remove --all
> >
> > -reset_devs
> > -ssm -b btrfs create $dev1 $dev2 $dev3
> > -ssm list
> > -ssm migrate $dev1 $dev2
> > +# migrate plain device to a device in btrfs pool - not used
> > +#ssm create -p $vg1 $dev1 $dev2 $dev3
> > +#mkfs.ext4 -F $dev4
> > +#not ssm migrate $dev4 $dev3
> > +#ssm -f migrate $dev4 $dev3
> > +#check btrfs_devices $vg1 $dev1 $dev2
> > +#test_volume $vg1
> > +#fsck.ext4 -fn $dev3
> > +
> > +# migrate plain device to a device in btrfs pool - used completely
> > +#ssm -f add -p $vg1 $dev3
> > +#ssm mount $vg1 $mnt1
> > +# Fill it up as much as we can, we do not care if dd fails
> > +#! dd if=/dev/zero of=$mnt1/file bs=1M
> > +#sync
> > +#umount $mnt1
> > +#not ssm migrate $dev4 $dev1
> > +#not ssm -f migrate $dev4 $dev1
> > +#check btrfs_devices $vg1 $dev1 $dev2 $dev3
> > +#test_volume $vg1
> > +#ssm -f remove --all
> >
> >  export SSM_DEFAULT_BACKEND='lvm'
> >
> >  # LVM
> > -! ssm migrate foobar1 foobar2
> > -ssm -f migrate $dev1 $dev2
> > -reset_devs
> >
> > -# should not pass because there is already a signature and -f is not used
> > -! ssm migrate $dev3 $dev4
> > -! ssm migrate $dev5 foobar2
> > -! ssm migrate foobar2 $dev5
> > -reset_devs
> > +# Simple lvm migrate to plain device
> > +#ssm create --fs ext4 $dev1
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1
> > +#ssm migrate $dev1 $dev2
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> > +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> > +#mkfs.ext4 $dev3
> > +# lvm migrate to a device with signature (ext4)
> > +#not ssm migrate $dev2 $dev3
> > +#ssm -f migrate $dev2 $dev3
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> > +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> > +#ssm -f remove --all
> >
> > -ssm create --size $DEV_SIZE $dev1 $dev2
> > -ssm migrate $dev1 $dev2
> > -ssm -f migrate $dev2 $dev3
> > -reset_devs
> > +# Simple lvm migrate without pv being used
> > +ssm add $dev1 $dev2 $dev3
> > +ssm migrate $dev1 $dev5
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> > +# Migrate to a device in the same pool
> > +ssm migrate $dev2 $dev3
> > +ssm -f remove --all
> >
> > -ssm create --size $DEV_SIZE $dev1 $dev2 $dev3
> > -! ssm -n migrate $dev3 $dev3
> > -reset_devs
> > +# Migrate used device to free that's already in the pool
> > +ssm create $dev1 $dev2
> > +ssm -f add $dev3 $dev4
> > +ssm migrate $dev1 $dev3
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> > +ssm -f remove --all
> >
> > -# should fail - the target is already mounted or used in another pool
> > -mkfs.ext4 $dev1
> > -mount $dev1 $mnt1
> > +# All devices are used
> > +ssm create $dev1
> >  ssm create $dev2
> > -! ssm migrate $dev2 $dev1
> > -! ssm migrate $dev1 $dev2
> > -ssm -f migrate $dev2 $dev1
> > -reset_devs
> > +not ssm migrate $dev1 $dev2
> > +# Only specified devices are used
> > +ssm add $dev3
> > +not ssm migrate $dev1 $dev2
> > +ssm migrate $dev2 $dev3
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> > +ssm -f remove --all
> > +
> > +# Simple lvm migrate of mounted fs
> > +ssm create --fs ext4 $dev1 $mnt1
> > +wipefs -a $dev2
> > +not ssm migrate $mnt1 $dev2
> > +ssm migrate $dev1 $dev2
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> > +umount $mnt1
> > +ssm check $SSM_LVM_DEFAULT_POOL/$lvol1
> > +ssm -f remove --all
> > +
> > +# Migrate to a device from different pool
> > +ssm create -p $vg1 $dev1 $dev2 $dev3
> > +ssm add -p $vg1 $dev7
> > +ssm create -p $vg2 $dev4 $dev5 $dev6
> > +ssm add -p $vg2 $dev8
> > +not ssm migrate $dev1 $dev8
> > +ssm -f migrate $dev1 $dev8
> > +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3 $dev7
> > +check vg_devices $vg2 $dev4 $dev5 $dev6
> > +# Migrate device that is not used
> > +not ssm migrate $dev4 $dev7
> > +ssm -f migrate $dev4 $dev7
> > +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3
> > +check vg_devices $vg2 $dev4 $dev5 $dev6 $dev7
> > +# Migrate device that is used
> > +not ssm add -p $vg1 $dev1 # We're not removing the pv after migrate
> > +not ssm migrate $dev5 $dev1
> > +ssm -f migrate $dev5 $dev1
> > +ssm -f remove --all
> > +
> > +# Migrate plain device to a device in lvm
> > +ssm create $dev1 $dev2 $dev3
> > +ssm add -p $SSM_LVM_DEFAULT_POOL $dev4 $dev5
> > +# target device used
> > +not ssm migrate $dev6 $dev3
> > +not ssm -f migrate $dev6 $dev3
> > +# target device not used
> > +not ssm migrate $dev6 $dev4
> > +ssm -f migrate $dev6 $dev4
> > +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> > +# Migrate plain device with a file system
> > +#mkfs.ext4 -F $dev4
> > +# target device used
> > +#not ssm migrate $dev4 $dev1
> > +#not ssm -f migrate $dev4 $dev1
> > +# target not used
> > +#not ssm migrate $dev4 $dev5
> > +#ssm -f migrate $dev4 $dev5
> > +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> > +#fsck.ext4 -fn $dev5
> > +#test_volume $dev5
> > +ssm -f remove --all
> > +
> > +# migrate from btrfs pool to lvm
> > +wipefs -a $dev4 $dev5
> > +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> > +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> > +not ssm migrate $dev5 $dev3
> > +ssm -f migrate $dev5 $dev3
> > +check btrfs_devices $vg1 $dev1 $dev2
> > +check vg_devices $vg2 $dev4 $dev5 $dev3
> > +test_volume $vg1
> > +test_volume $vg2/$lvol1
> > +ssm -f remove --all
> > +
> > +# migrate from lvm to btrfs pool
> > +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> > +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> > +ssm add -p $vg2 $dev6
> > +not ssm migrate $dev3 $dev6
> > +ssm -f migrate $dev3 $dev6
> > +check btrfs_devices $vg1 $dev1 $dev2 $dev6
> > +check vg_devices $vg2 $dev4 $dev5
> > +test_volume $vg1
> > +test_volume $vg2/$lvol1
> > +ssm -f remove --all
> > +
> > +# CRYPT
> > +
> > +# cryptsetup will ask for password. If luks extension is used
> > +# it will ask when creating header and then when opening the device
> > +pass() {
> > +       echo -e "${passwd}\n${passwd}"
> > +}
> > +
> > +export SSM_DEFAULT_BACKEND='crypt'
> > +export SSM_CRYPT_DEFAULT_POOL=$vg3
> > +export CRYPT_VOL_PREFIX="${SSM_PREFIX_FILTER}enc"
> > +export SSM_CRYPT_DEFAULT_VOL_PREFIX=$CRYPT_VOL_PREFIX
> > +crypt_vol1=${CRYPT_VOL_PREFIX}001
> > +
> > +passwd="cai0ohMo8M"
> > +pass | ssm create $dev1
> > +check crypt_vol_field $crypt_vol1 type LUKS1
> > +check crypt_vol_field $crypt_vol1 device $dev1
> > +check list_table "$(ssm list vol)" $crypt_vol1 $SSM_CRYPT_DEFAULT_POOL none crypt
> > +
> > +# Migrate in crypt is not currently supported so this whould both fail
> > +not ssm migrate $dev1 $dev2
> > +not ssm -f migrate $dev1 $dev2
> > +
> > +# Device can't be simply removed from the crypt backend so this should fail
> > +not ssm migrate $dev2 $dev1
> > +not ssm -f migrate $dev2 $dev1
> >
> > -exit 0
> > \ No newline at end of file
> > +exit 0
> > diff --git a/tests/bashtests/lib/check.sh b/tests/bashtests/lib/check.sh
> > index 4dad7c4..2bdc790 100644
> > --- a/tests/bashtests/lib/check.sh
> > +++ b/tests/bashtests/lib/check.sh
> > @@ -257,6 +257,29 @@ lv_field()
> >         fi
> >  }
> >
> > +vg_devices()
> > +{
> > +       # Check the expected list of devices in the fs with the reality
> > +       # $1 is the vg we're inspecting
> > +       # the rest is list of devices
> > +       tmp=`mktemp`
> > +
> > +       devices=`vgs --no-headings -o pv_name $1 | awk {'print $1'}`
> > +       shift
> > +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> > +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> > +
> > +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> > +       if [ $? -eq 0 ]; then
> > +               echo "ACTUAL"
> > +               cat ${tmp}.actual
> > +               echo "EXPECTED"
> > +               cat ${tmp}.expected
> > +               exit 1
> > +       fi
> > +       exit 0
> > +}
> > +
> >  compare_fields()
> >  {
> >         local cmd1=$1;
> > @@ -334,6 +357,29 @@ crypt_vol_field()
> >         fi
> >  }
> >
> > +btrfs_devices()
> > +{
> > +       # Check the expected list of devices in the fs with the reality
> > +       # $1 is label for the file system we want to test
> > +       # the rest is list of devices
> > +       tmp=`mktemp`
> > +
> > +       devices=`btrfs filesystem show $1| grep devid | awk {'print $NF'}`
> > +       shift
> > +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> > +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> > +
> > +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> > +       if [ $? -eq 0 ]; then
> > +               echo "ACTUAL"
> > +               cat ${tmp}.actual
> > +               echo "EXPECTED"
> > +               cat ${tmp}.expected
> > +               exit 1
> > +       fi
> > +       exit 0
> > +}
> > +
> >  btrfs_fs_field()
> >  {
> >         lines=$(btrfs filesystem show 2> /dev/null | grep -A 1 "^Label:.*$1" || true)
> > diff --git a/tests/unittests/test_btrfs.py b/tests/unittests/test_btrfs.py
> > index a73420f..1e8d7e1 100644
> > --- a/tests/unittests/test_btrfs.py
> > +++ b/tests/unittests/test_btrfs.py
> > @@ -20,6 +20,7 @@
> >
> >  import unittest
> >  from ssmlib import main
> > +from ssmlib import problem
> >  from ssmlib.backends import btrfs
> >  from tests.unittests.common import *
> >
> > @@ -461,8 +462,10 @@ class BtrfsFunctionCheck(MockSystemDataSource):
> >          self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
> >          self._addDevice('/dev/sde', 11489037516)
> >
> > -        self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> > -        "btrfs device delete /dev/sdc1 /tmp/mount")
> > +        try:
> > +            main.main("ssm migrate /dev/sdc1 /dev/sdc2")
> > +        except problem.DeviceUsed:
> > +            pass
> >
> >          self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
> >          "btrfs replace start -B /dev/sdc2 /dev/sde /tmp/mount")
> > diff --git a/tests/unittests/test_lvm.py b/tests/unittests/test_lvm.py
> > index fe87c4c..ec565e0 100644
> > --- a/tests/unittests/test_lvm.py
> > +++ b/tests/unittests/test_lvm.py
> > @@ -257,21 +257,8 @@ class LvmFunctionCheck(MockSystemDataSource):
> >          self._addDevice('/dev/sde', 11489037516)
> >
> >          self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> > -        "lvm pvremove /dev/sdc1")
> > -        self.assertEqual(self.run_data[-2],
> > -            "lvm vgreduce my_pool /dev/sdc1")
> > -        self.assertEqual(self.run_data[-3],
> >              "lvm pvmove --atomic /dev/sdc1 /dev/sdc2")
> >
> > -        self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
> > -        "lvm pvremove /dev/sdc2")
> > -        self.assertEqual(self.run_data[-2],
> > -            "lvm vgreduce my_pool /dev/sdc2")
> > -        self.assertEqual(self.run_data[-3],
> > -            "lvm pvmove --atomic /dev/sdc2 /dev/sde")
> > -        self.assertEqual(self.run_data[-4],
> > -            "lvm vgextend my_pool /dev/sde")
> > -
> >      def test_lvm_resize(self):
> >          # Generate some storage data
> >          self._addPool('default_pool', ['/dev/sda', '/dev/sdb'])
> > --
> > 2.17.1
> >
> >
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > storagemanager-devel mailing list
> > storagemanager-devel@...
> > https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> > System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)
> 
> 
> 
> -- 
> Jan Tulak
> jtulak@... / jan@...

Re: [ssm-devel] [PATCH 1/4] lvm: fix backtrace when vg/lv is inactive or broken

[Lukas Czerner <lczerner@re...>]

On Tue, Aug 07, 2018 at 11:26:37AM +0200, Jan Tulak wrote:
> On Thu, Aug 2, 2018 at 4:59 PM Lukas Czerner <lczerner@...> wrote:
> >
> > When lv/vg is inactive/broken and we have a vm snapshot, it's possible to
> 
> Just two nitpicks... Here I think that we are not speaking about VM
> snapshots. :-)

Yes, it's lvm snapshots.

> 
> > end up crashing because 'snap_size' may not be defined:
> >
> > size = float(snap['vol_size']) * float(snap['snap_size'])
> > ValueError: could not convert string to float:
> >
> > Fix this be removing the snap_size field altogether. Add tests.
> >
> > Signed-off-by: Lukas Czerner <lczerner@...>
> > ---
> >  ssmlib/backends/lvm.py              | 9 +++++++--
> >  tests/bashtests/005-lvm-snapshot.sh | 9 +++++++++
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
> > index e9be366..7b5b3c8 100644
> > --- a/ssmlib/backends/lvm.py
> > +++ b/ssmlib/backends/lvm.py
> > @@ -552,8 +552,13 @@ class SnapInfo(LvmInfo):
> >          snap['snap_path'] = snap['dev_name']
> >
> >          if snap['type'] != "thin":
> > -            size = float(snap['vol_size']) * float(snap['snap_size'])
> > -            snap['snap_size'] = str(size / 100.00)
> > +            # It's possible that snap_percent is not defined. For example
> 
> And here s/snap_percent/snap_size/

Actually I ment snap_percent. It's the lvm field ssm takes snap_size
from, so both are fine.

-Lukas

> 
> Other than that it looks good.
> 
> > +            # on inactive volume, so just remove it
> > +            if snap['snap_size']:
> > +                size = float(snap['vol_size']) * float(snap['snap_size'])
> > +                snap['snap_size'] = str(size / 100.00)
> > +            else:
> > +                del snap['snap_size']
> >          else:
> >              # Show thin-pool as a pool name in case of thin volumes
> >              snap['parent_pool'] = snap['pool_name']
> > diff --git a/tests/bashtests/005-lvm-snapshot.sh b/tests/bashtests/005-lvm-snapshot.sh
> > index a353562..b34c3f8 100755
> > --- a/tests/bashtests/005-lvm-snapshot.sh
> > +++ b/tests/bashtests/005-lvm-snapshot.sh
> > @@ -175,6 +175,15 @@ not ssm snapthot -s $((DEV_SIZE*2)) -n $snap1 $SSM_LVM_DEFAULT_POOL/$lvol1
> >  check vg_field $SSM_LVM_DEFAULT_POOL lv_count 1
> >  ssm -f remove --all
> >
> > +# Test for problem when ssm list crashed if snapshot volume is inactive
> > +ssm create $dev1 $dev2
> > +ssm add $dev3
> > +ssm snapshot -s $((DEV_SIZE/2)) -n $snap1 $SSM_LVM_DEFAULT_POOL/$lvol1
> > +check lv_field $SSM_LVM_DEFAULT_POOL/$snap1 lv_name $snap1
> > +echo y | lvchange -f -an $SSM_LVM_DEFAULT_POOL/$snap1
> > +ssm list
> > +ssm -f remove --all
> > +
> >  # Some basic thin tests
> >  export TVOL_PREFIX="tvol"
> >  tvol1=${TVOL_PREFIX}001
> > --
> > 2.17.1
> >
> >
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > storagemanager-devel mailing list
> > storagemanager-devel@...
> > https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> > System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)
> 
> 
> 
> --
> Jan Tulak
> jtulak@... / jan@...

Re: [ssm-devel] [PATCH] migrate: remove fs signatures when using dd

[Jan Tulak <jtulak@re...>]

On Fri, Aug 3, 2018 at 8:33 AM Lukas Czerner <lczerner@...> wrote:
>
> On Tue, Jul 31, 2018 at 01:50:33PM +0200, Jan Tulak wrote:
> > On Tue, Jul 31, 2018 at 11:13 AM Lukas Czerner <lczerner@...> wrote:
> > >
> > > On Mon, Jul 30, 2018 at 04:58:19PM +0200, Jan Tulak wrote:
> > > > Make the migrate command behave the same way in all cases. For btrfs and
> > > > lvm the signatures are removed by the respective utilities, so do the
> > > > same for dd-copied devices as well.
> > > >
> > > > No need of the manual is necessary, it already states that accessing the
> > > > data on the source device can be difficult afterwards.
> > >
> > > I gave it some more though lately and I am starting to think that while
> > > consistency is useful we also need to consider the expectations and
> > > convenience. I am not really sure what's the right call with the plain
> > > file system case, but maybe wiping the signature makes the most sense.
> > >
> > > However as for lvm, I am strongly considering changing it so that the pv
> > > is _not_ removed from the vg after the migrate. For example it would be
> > > a perfectly valid use case to replace a ssd pv to make use of it in a
> > > different lv where we want it to be faster, or we want to use it for
> > > caching. And removing it just complicates it for the user. Unless you
> > > have any strong arguments against it I am going to change it.
> >
> > Yes, I see the case for it. However, in that case, we need to be more
> > explicit about the result, printing out what happened, and maybe what
> > to do next or how to revert it, because while such a change is useful,
> > I don't think that it is an obvious and expected result.
>
> I'd actually argue that this is obvious and expected behavior. In lvm
> the device does not have to be removed from the pool to be unused. It's
> perfectly ok just sitting in the volume group not being used by
> anything. And 'migrate' does not really imply 'remove the volume from
> the pool'.
>
> As for btrfs, it needs to remove it from the filesystem since it does
> not really have any notion of "pool" it's ssm construct. It can't be in
> the file system and still be (and remain) unused.

OK, I have no issue with it. But anyway, is there anything wrong with
this patch were are discussing under (e.g. adding an explicit message
in v2 of this patch?) Or can you add reviewed-by?

Thanks,
Jan

>
> -Lukas
>
> >
> > Jan
> >
> > >
> > > Thanks!
> > > -Lukas
> > >
> > >
> > > >
> > > > Signed-off-by: Jan Tulak <jtulak@...>
> > > > ---
> > > >  ssmlib/main.py | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/ssmlib/main.py b/ssmlib/main.py
> > > > index 51e5033..4011d21 100644
> > > > --- a/ssmlib/main.py
> > > > +++ b/ssmlib/main.py
> > > > @@ -990,8 +990,14 @@ class DeviceItem(Item):
> > > >
> > > >          command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> > > >          misc.run(command)
> > > > +
> > > > +        fs = misc.get_fs_type(source)
> > > > +        if fs:
> > > > +            misc.wipefs(source, fs)
> > > > +
> > > >          misc.send_udev_event(source, "change")
> > > >          misc.send_udev_event(target, "change")
> > > > +        misc.udev_settle()
> > > >
> > > >
> > > >  class VolumeItem(Item):
> > > > --
> > > > 2.16.2
> > > >
> > > >
> > > > ------------------------------------------------------------------------------
> > > > Check out the vibrant tech community on one of the world's most
> > > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > > > _______________________________________________
> > > > storagemanager-devel mailing list
> > > > storagemanager-devel@...
> > > > https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> > > > System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)
> >
> >
> >
> > --
> > Jan Tulak
> > jtulak@... / jan@...



-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH 4/4] ssm: Rework migrate command

[Jan Tulak <jtulak@re...>]

On Thu, Aug 2, 2018 at 5:00 PM Lukas Czerner <lczerner@...> wrote:
>
> Migrate command was not working as expected in many ways so here is a
> rework of it. The 018 bashtest is also reworked and significantly
> extended to cover more possible scenarios and test the actual results of
> the operation.
>
> Lvm migrate now does not remove the pv from the volume group as I feel
> there are perfectly valid scenarios where user would like to migrate
> used pv to a different device and repurpose the source device.
>
> unittests for lvm and btrfs are only changed so that they not fail,
> however the unittests needs to be extended for btrfs and lvm as well.
> Generic migrate unittests (test_ssm.py) are missing entirely.
>
> Also some more fine grained testing on plain devices is completely
> missing at the moment because in this way the testing filter actually
> interfere. Those tests are commented out in the 018.So there is still
> some work to be done.
>
> Signed-off-by: Lukas Czerner <lczerner@...>

What revision is this based against? I couldn't get it apply all the
way to v1.0 tag. I tried to apply it on top of the previous three
patches in this set.
Thanks,

Jan


> ---
>  ssmlib/backends/btrfs.py       |  10 +-
>  ssmlib/backends/lvm.py         |  18 +-
>  ssmlib/backends/template.py    |   2 +-
>  ssmlib/main.py                 | 199 ++++++++++------------
>  ssmlib/problem.py              |  14 +-
>  tests/bashtests/018-migrate.sh | 298 +++++++++++++++++++++++++--------
>  tests/bashtests/lib/check.sh   |  46 +++++
>  tests/unittests/test_btrfs.py  |   7 +-
>  tests/unittests/test_lvm.py    |  13 --
>  9 files changed, 397 insertions(+), 210 deletions(-)
>
> diff --git a/ssmlib/backends/btrfs.py b/ssmlib/backends/btrfs.py
> index bd7db3f..39ebbce 100644
> --- a/ssmlib/backends/btrfs.py
> +++ b/ssmlib/backends/btrfs.py
> @@ -600,12 +600,10 @@ class BtrfsPool(Btrfs, template.BackendPool):
>              pool['mount'] = tmp
>
>          dev = BtrfsDev(options=self.options).data
> -        if dev.get(target) and dev.get(source.name):
> -            # both source and target are already members of the pool
> -            # so we are going to only remove source
> -            command = ['device', 'delete', source.name, pool['mount']]
> -        else:
> -            command = ['replace', 'start', '-B', source.name, target, pool['mount']]
> +        command = ['replace', 'start']
> +        if self.options.force:
> +            command.extend(['-f'])
> +        command.extend(['-B', source.name, target, pool['mount']])
>
>          self.run_btrfs(command)
>          misc.send_udev_event(target, "change")
> diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
> index e726d9c..f1373bd 100644
> --- a/ssmlib/backends/lvm.py
> +++ b/ssmlib/backends/lvm.py
> @@ -348,23 +348,17 @@ class VgsInfo(LvmInfo, template.BackendPool):
>          target : [str]
>              Path to the target device.
>          """
> -        pvsinfo = PvsInfo(options=self.options).data
> -        if target in pvsinfo:
> -            tgt = pvsinfo[target]
> -            if tgt['pool_name'] != vg:
> -                self.extend(vg, [target])
> -        else:
> -            self.extend(vg, [target])
> +        # We're ready to do pvmove source and target must be already in the
> +        # pool
>
>          # pvmove does not accept -f, temporarily unset it
>          force = self.options.force
>          self.options.force = False
>
> -        command = ['pvmove', '--atomic', source.name, target ]
> -        self.run_lvm(command)
> -        self.reduce(vg, source.name)
> -        command = ['pvremove', source.name]
> -        self.run_lvm(command)
> +        # If the source is not used we do not have to do anything
> +        if float(source['dev_used']) > 0.0:
> +            command = ['pvmove', '--atomic', source.name, target ]
> +            self.run_lvm(command)
>
>          misc.send_udev_event(source.name, "change")
>          misc.send_udev_event(target, "change")
> diff --git a/ssmlib/backends/template.py b/ssmlib/backends/template.py
> index 8fbb8d2..a17cbb7 100644
> --- a/ssmlib/backends/template.py
> +++ b/ssmlib/backends/template.py
> @@ -76,7 +76,7 @@ class BackendPool(Backend):
>
>      def migrate(self, pool, sources, targets=None):
>          self.problem.check(self.problem.NOT_IMPLEMENTED,
> -                        ["Migrating data in pools", "{} backend".format(self.type)])
> +                        ["Migrating devices", "{} backend".format(self.type)])
>
>
>  class BackendVolume(Backend):
> diff --git a/ssmlib/main.py b/ssmlib/main.py
> index 9a0a2f5..1841edd 100644
> --- a/ssmlib/main.py
> +++ b/ssmlib/main.py
> @@ -443,6 +443,41 @@ class DeviceInfo(object):
>                   "to achieve by removing " +
>                   "{0}".format(device))
>
> +    def migrate(self, vg, source_dev, target_dev):
> +        """ Migrate (in this case clone) the device onto another one.
> +            Pure dd is used.
> +        """
> +        # TODO some possibilities:
> +        # - error handling: conv=noerror,sync (continue on error and pad
> +        #   with zeros), or better to fail on error?
> +        source = self.data[source_dev]
> +        target = self.data[target_dev]
> +
> +        if misc.get_signature(source_dev) == 'btrfs':
> +            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> +
> +        if source['dev_size'] > target['dev_size']:
> +            raise problem.GeneralError("Target device is smaller than source device")
> +
> +        if 'mount' in target:
> +            if PR.check(PR.FS_MOUNTED, [target_dev, target['mount']]):
> +                misc.do_umount(target_dev)
> +
> +        if 'mount' in source:
> +            if PR.check(PR.FS_MOUNTED, [source_dev, source['mount']]):
> +                misc.do_umount(source_dev)
> +
> +        print("Migrating {s} ({size}) to {t} using dd . This may take a long time...".format(
> +            s=source_dev,
> +            t=target_dev,
> +            size=misc.humanize_size(source['dev_size'])))
> +
> +        command = ['dd', 'if={}'.format(source_dev), 'of={}'. \
> +                   format(target_dev), 'conv=fsync']
> +        misc.run(command)
> +        misc.send_udev_event(source_dev, "change")
> +        misc.send_udev_event(target_dev, "change")
> +
>      def set_globals(self, options):
>          self.options = options
>
> @@ -962,36 +997,6 @@ class DeviceItem(Item):
>                      return True
>          return False
>
> -    @classmethod
> -    def migrate(cls, source, target):
> -        """ Migrate (in this case clone) the device onto another one.
> -            Pure dd is used.
> -        """
> -        # TODO some possibilities:
> -        # - error handling: conv=noerror,sync (continue on error and pad
> -        #   with zeros), or better to fail on error?
> -
> -        if misc.get_signature(source) == 'btrfs':
> -            raise problem.ProgrammingError("BTRFS SHOULD NOT BE CLONED WITH DD")
> -
> -        if misc.get_device_size(source) > misc.get_device_size(target):
> -            raise problem.GeneralError("Target device is smaller than source device")
> -
> -        if misc.get_mountinfo(source):
> -            raise problem.GeneralError("You have to umount the source device first")
> -        if misc.get_mountinfo(target):
> -            raise problem.GeneralError("You have to umount the target device first")
> -
> -        print("Migrating {s} ({size}) to {t}. This may take a long time...".format(
> -            s=source,
> -            t=target,
> -            size=misc.humanize_size(misc.get_device_size(source))))
> -
> -        command = ['dd', 'if={}'.format(source), 'of={}'.format(target)]
> -        misc.run(command)
> -        misc.send_udev_event(source, "change")
> -        misc.send_udev_event(target, "change")
> -
>
>  class VolumeItem(Item):
>      def __init__(self, *args, **kwargs):
> @@ -2111,88 +2116,70 @@ class StorageHandle(object):
>                                                         args.options))
>
>      def migrate(self, args):
> -        """ Migrate data from a device to a device"""
> -        source = self._find_device_record(args.source)
> -        target = self._find_device_record(args.target)
> -
> -        # decide if we have PVs or non-PV block devices
> -        found_pv = False
> -        pool = None
> -        check_fs = True
> -        signature = misc.get_signature(target)
> -
> -        # get source dev and try to detect if it is a member of a pool
> -        source_dev = self.dev[source]
> -        if source_dev and 'pool_name' in source_dev:
> -            found_pv = True
> -            pool = self.pool[source_dev['pool_name']]
> +        """
> +        Migrate data from a device to a device
> +        """
> +
> +        source_pool = None
> +        target_pool = None
> +        changed = False
> +        source = self.dev[args.source]
> +        if source and 'pool_name' in source:
> +            source_pool = self.pool[source['pool_name']]
> +        target = self.dev[args.target]
> +        if target and 'pool_name' in target:
> +            target_pool = self.pool[target['pool_name']]
> +
> +        if target_pool:
> +            if not source_pool or (target_pool.name != source_pool.name):
> +                if not PR.check(PR.DEVICE_USED, [target.name, target['pool_name']]):
> +                    raise problem.UserInterrupted("Terminated by user!")
> +                target_pool.reduce(target.name)
> +                target_pool = None
> +                changed = True
> +            elif target_pool.type == "btrfs":
> +                raise problem.DeviceUsed(
> +                    "Target and source devices are in the same btrfs pool!")
>          else:
> -            pass
> -            # This seems to be only a case for some tests where the /tmp/<TEMP>/dev/...
> -            # devices gets incorrectly identified (sometimes they do, sometimes they don't).
> -            # Keep it commented out for now, because it seems to be rather an issue with
> -            # the tests
> -            # source_signature = misc.get_signature(source)
> -            # if source_signature == 'btrfs':
> -            #    print("found btrfs")
> -            #    found_pv = True
> -            #    import pdb; pdb.set_trace()
> -
> -        target_dev = self.dev[target]
> -        if target_dev:
> -            if 'pool_name' in target_dev and\
> -                (signature != 'btrfs' or (pool and int(pool['dev_count']) > 1)):
> -                check_fs = False
> -
> -                if target_dev['pool_name'] != '':
> -                    if pool and target_dev['pool_name'] != pool.name:
> -                        if PR.check(PR.DEVICE_USED,
> -                            [target_dev.name, target_dev['pool_name']]):
> -                            remove_args = Struct()
> -                            remove_args.all = False
> -                            remove_args.items = [target_dev]
> -                            if not self.remove(remove_args):
> -                                raise problem.CommandFailed("The device {} could not be removed from the pool {}".format(
> -                                    target,
> -                                    target_dev['pool_name']
> -                                ))
> -                            misc.wipefs(target_dev.name, signature)
> -                        else:
> -                            raise problem.UserInterrupted("Aborted")
> +            # Check signature of existing file system on the device
> +            # and ask user whether to use it or not.
> +            if target and 'mount' in target:
> +                if PR.check(PR.FS_MOUNTED, [args.target, target['mount']]):
> +                    misc.do_umount(target.name)
>              else:
> -                check_fs = True
> +                signature = misc.get_signature(args.target)
> +                if signature and \
> +                   PR.check(PR.EXISTING_SIGNATURE,
> +                            [signature, args.target]):
> +                    misc.wipefs(args.target, signature)
> +                    changed = True
> +                elif signature:
> +                    raise problem.UserInterrupted("Terminated by user!")
>
> +        # TODO: If the source is in lvm pool and the target is not in any pool
> +        # just add it to the source pool. This should not be here because
> +        # we try to be as generic as possible. However untill we have all the
> +        # data accessible in the backend we need do it here to avoid
> +        # re-initializing information unnecessarily
> +        if source_pool and source_pool.type == "lvm" and not target_pool:
> +            source_pool.extend(args.target)
>
>
> -        if check_fs:
> -            mounts = misc.get_mountinfo(target)
> -            if mounts:
> -                if PR.check(PR.FS_MOUNTED, [target, mounts[target]['mp']]):
> -                    misc.do_umount(target)
> -                else:
> -                    raise problem.CommandFailed("Umount of {} was not possible".format(target))
> -
> -            if signature and not self.options.force and\
> -                PR.check(PR.EXISTING_FILESYSTEM,
> -                        [signature, target]):
> -                misc.wipefs(target, signature)
> -            elif signature and not self.options.force:
> -                # user wants to skip
> -                raise problem.UserInterrupted("Aborted")
> -            elif signature and self.options.force:
> -                misc.wipefs(target, signature)
> -
> -        PR.info("Migrating from device {} to {}. This may take a long time...".format(
> -            source,
> -            target
> -        ))
> -        if not found_pv:
> -            DeviceItem.migrate(source, target)
> -
> -        elif found_pv:
> -            pool.migrate(source_dev, target)
> +        if changed:
> +            self.reinit_dev()
> +            source = self.dev[args.source]
> +            target = self.dev[args.target]
> +
> +        if source_pool:
> +            PR.info("Migrating from device {} to {}. This may take a while...".format(
> +                args.source,
> +                args.target
> +            ))
> +            source_pool.migrate(source, args.target)
>          else:
> -            raise problem.ProgrammingError("Neither PV or non-PV was found. This is a bug")
> +            target = self.dev[args.target]
> +            source = self.dev[args.source]
> +            source.migrate(args.source, args.target)
>
>      def can_check(self, device):
>          fs = self.is_fs(device)
> diff --git a/ssmlib/problem.py b/ssmlib/problem.py
> index 1148ca5..b76d738 100644
> --- a/ssmlib/problem.py
> +++ b/ssmlib/problem.py
> @@ -22,7 +22,7 @@ __all__ = ["ProblemSet", "SsmError", "GeneralError", "ProgrammingError",
>             "BadEnvVariable", "NotEnoughSpace", "ResizeMatch", "FsNotSpecified",
>             "DeviceUsed", "ExistingFilesystem", "NoDevices", "ToolMissing",
>             "CanNotRun", "CommandFailed", "UserInterrupted", "NotSupported",
> -           "NotImplemented"]
> +           "NotImplemented", "ExistingSignature"]
>
>  # Define prompt codes
>  PROMPT_NONE =           0
> @@ -148,11 +148,17 @@ class ExistingFilesystem(SsmError):
>      def __init__(self, msg, errcode=2015):
>          super(ExistingFilesystem, self).__init__(msg, errcode)
>
> +
>  class NotImplemented(SsmError):
>      def __init__(self, msg, errcode=2016):
>          super(NotImplemented, self).__init__(msg, errcode)
>
>
> +class ExistingSignature(SsmError):
> +    def __init__(self, msg, errcode=2017):
> +        super(ExistingSignature, self).__init__(msg, errcode)
> +
> +
>  class ProblemSet(object):
>
>      def __init__(self, options):
> @@ -212,6 +218,10 @@ class ProblemSet(object):
>              ['Filesystem \'{0}\' detected on the device \'{1}\'!',
>               PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingFilesystem]
>
> +        self.EXISTING_SIGNATURE = \
> +            ['Signature \'{0}\' detected on the device \'{1}\'!',
> +             PROMPT_USE, FL_DEFAULT_NO | FL_FORCE_YES, ExistingSignature]
> +
>          self.NO_DEVICES = \
>              ['No devices available to use for the \'{0}\' pool!',
>               PROMPT_NONE, FL_FATAL, NoDevices]
> @@ -233,7 +243,7 @@ class ProblemSet(object):
>               PROMPT_NONE, FL_FATAL, NotSupported]
>
>          self.NOT_IMPLEMENTED = \
> -            ['\'{0}\' function is not implemented by {1}!',
> +            ['\'{0}\' functionality is not implemented by {1}!',
>              PROMPT_NONE, FL_FATAL, NotImplemented]
>
>          self.CREATE_DIRECTORY = \
> diff --git a/tests/bashtests/018-migrate.sh b/tests/bashtests/018-migrate.sh
> index 98b57f0..44e8c2d 100755
> --- a/tests/bashtests/018-migrate.sh
> +++ b/tests/bashtests/018-migrate.sh
> @@ -21,7 +21,7 @@ export test_description='Test the migrate command.'
>  . lib/test
>
>  export COLUMNS=1024
> -DEV_COUNT=6
> +DEV_COUNT=8
>  DEV_SIZE=200
>  aux prepare_devs $DEV_COUNT $DEV_SIZE
>  aux prepare_mnts 2
> @@ -31,6 +31,8 @@ export SSM_DEFAULT_BACKEND='lvm'
>  export SSM_LVM_DEFAULT_POOL=$vg1
>  export SSM_BTRFS_DEFAULT_POOL=$vg1
>  export SSM_NONINTERACTIVE='1'
> +lvol1=${LVOL_PREFIX}001
> +lvol2=${LVOL_PREFIX}002
>
>  cleanup() {
>         aux teardown
> @@ -38,102 +40,262 @@ cleanup() {
>
>  trap cleanup EXIT
>
> -
>  function compare_hash()
>  {
> -       a=$(sha1sum $1 | cut -f1 -d' ')
> -       b=$(sha1sum $2 | cut -f1 -d' ')
> +       a=$(cksum $1 | awk '{print $1 $2}')
> +       b=$(cksum $2 | awk '{print $1 $2}')
>         test "$a" = "$b"
>  }
>
> -function reset_devs()
> -{
> -       # try to clean anything that could linger on any test dev
> -       yes 'c' | ssm -f remove $SSM_LVM_DEFAULT_POOL || true
> -       for dev in $TEST_DEVS; do
> -               pvremove $dev || umount $dev || true
> -               pvcreate -f $dev
> -       done
> -
> -}
> -
> -function test_mount()
> +function test_volume()
>  {
> -       mount $1 $mnt1
> +       ssm check $1
> +       ssm mount $1 $mnt1
>         umount $mnt1
>  }
>
> -mkfs.ext4 $dev1
> -mkfs.ext4 $dev2
> -ssm migrate $dev1 $dev3
> -! ssm migrate $dev2 $dev3
> -ssm -f migrate $dev2 $dev3
> +# Test some nonsense
> +not ssm migrate foobar1 foobar2
> +not ssm migrate foobar1
> +not ssm migrate $dev1 $mnt1
> +not ssm migrate $mnt1 $dev1
>
> -reset_devs
> +# Migrate plain device
> +#dd if=/dev/urandom of=$dev1 bs=1k count=1
> +#dd if=/dev/urandom of=$dev2 bs=1k count=1
> +#sync
> +#ssm -f migrate $dev1 $dev2
> +#compare_hash $dev1 $dev2
> +#
> +# Migrate filesystem on plain device
> +#mkfs.ext4 $dev1
> +#mkfs.ext4 $dev2
> +#ssm migrate $dev1 $dev3
> +#not ssm migrate $dev2 $dev3
> +#ssm -f migrate $dev2 $dev3
> +#wipefs -a $dev1
> +#wipefs -a $dev2
>
>  # BTRFS
>  export SSM_DEFAULT_BACKEND='btrfs'
>
> -ssm create $dev1
> -ssm -f migrate $dev1 $dev2
> -test_mount $dev2
> -ssm -f migrate $dev2 $dev3
> -test_mount $dev3
> -reset_devs
> +# Simple btrfs migrate to plain device
> +#ssm create $dev1
> +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> +#ssm migrate $dev1 $dev2
> +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> +#test_volume $SSM_BTRFS_DEFAULT_POOL
> +# Btrfs migrate on device with signature (ext4)
> +#not ssm migrate $dev2 $dev3
> +#ssm -f migrate $dev2 $dev3
> +#check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev3
> +#test_volume $SSM_BTRFS_DEFAULT_POOL
> +#ssm -f remove --all
>
> +# Simple btrfs migrate of mounted fs
> +ssm create $dev1 $mnt1
> +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1
> +not ssm migrate $mnt1 $dev2
> +ssm migrate $dev1 $dev2
> +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev2
> +umount $mnt1
> +ssm check $SSM_BTRFS_DEFAULT_POOL
> +ssm -f remove --all
> +
> +# Migrate device from multi-device btrfs
>  ssm create -p $vg1 $dev1 $dev2
>  ssm create -p $vg2 $dev3 $dev4 $dev5
> -ssm list
>  ssm -f migrate $dev2 $dev6
> -test_mount $dev1
> -test_mount $dev6
> +check btrfs_devices $vg1 $dev1 $dev6
> +check btrfs_devices $vg2 $dev3 $dev4 $dev5
> +test_volume $vg1
> +test_volume $vg2
>
> -! ssm migrate $dev1 $dev3
> +# Migrate to a device already used in btrfs pool
> +not ssm migrate $dev1 $dev3
>  ssm -f migrate $dev1 $dev3
> -test_mount $dev3
> -test_mount $dev4
> -reset_devs
> +check btrfs_devices $vg1 $dev3 $dev6
> +check btrfs_devices $vg2 $dev4 $dev5
> +test_volume $vg1
> +test_volume $vg2
> +ssm -f remove --all
>
> -ssm create $dev1 $dev2
> -ssm -f migrate $dev1 $dev3
> -test_mount $dev2
> -test_mount $dev3
> -reset_devs
> +# Do not allow migrate between devices within a single pool in btrfs
> +ssm create $dev1 $dev2 $dev3 $dev4
> +not ssm migrate $dev1 $dev2
> +not ssm -f migrate $dev1 $dev2
> +check btrfs_devices $SSM_BTRFS_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> +ssm -f remove --all
>
> -reset_devs
> -ssm -b btrfs create $dev1 $dev2 $dev3
> -ssm list
> -ssm migrate $dev1 $dev2
> +# migrate plain device to a device in btrfs pool - not used
> +#ssm create -p $vg1 $dev1 $dev2 $dev3
> +#mkfs.ext4 -F $dev4
> +#not ssm migrate $dev4 $dev3
> +#ssm -f migrate $dev4 $dev3
> +#check btrfs_devices $vg1 $dev1 $dev2
> +#test_volume $vg1
> +#fsck.ext4 -fn $dev3
> +
> +# migrate plain device to a device in btrfs pool - used completely
> +#ssm -f add -p $vg1 $dev3
> +#ssm mount $vg1 $mnt1
> +# Fill it up as much as we can, we do not care if dd fails
> +#! dd if=/dev/zero of=$mnt1/file bs=1M
> +#sync
> +#umount $mnt1
> +#not ssm migrate $dev4 $dev1
> +#not ssm -f migrate $dev4 $dev1
> +#check btrfs_devices $vg1 $dev1 $dev2 $dev3
> +#test_volume $vg1
> +#ssm -f remove --all
>
>  export SSM_DEFAULT_BACKEND='lvm'
>
>  # LVM
> -! ssm migrate foobar1 foobar2
> -ssm -f migrate $dev1 $dev2
> -reset_devs
>
> -# should not pass because there is already a signature and -f is not used
> -! ssm migrate $dev3 $dev4
> -! ssm migrate $dev5 foobar2
> -! ssm migrate foobar2 $dev5
> -reset_devs
> +# Simple lvm migrate to plain device
> +#ssm create --fs ext4 $dev1
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1
> +#ssm migrate $dev1 $dev2
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> +#mkfs.ext4 $dev3
> +# lvm migrate to a device with signature (ext4)
> +#not ssm migrate $dev2 $dev3
> +#ssm -f migrate $dev2 $dev3
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> +#test_volume $SSM_LVM_DEFAULT_POOL/$lvol1
> +#ssm -f remove --all
>
> -ssm create --size $DEV_SIZE $dev1 $dev2
> -ssm migrate $dev1 $dev2
> -ssm -f migrate $dev2 $dev3
> -reset_devs
> +# Simple lvm migrate without pv being used
> +ssm add $dev1 $dev2 $dev3
> +ssm migrate $dev1 $dev5
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> +# Migrate to a device in the same pool
> +ssm migrate $dev2 $dev3
> +ssm -f remove --all
>
> -ssm create --size $DEV_SIZE $dev1 $dev2 $dev3
> -! ssm -n migrate $dev3 $dev3
> -reset_devs
> +# Migrate used device to free that's already in the pool
> +ssm create $dev1 $dev2
> +ssm -f add $dev3 $dev4
> +ssm migrate $dev1 $dev3
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev4
> +ssm -f remove --all
>
> -# should fail - the target is already mounted or used in another pool
> -mkfs.ext4 $dev1
> -mount $dev1 $mnt1
> +# All devices are used
> +ssm create $dev1
>  ssm create $dev2
> -! ssm migrate $dev2 $dev1
> -! ssm migrate $dev1 $dev2
> -ssm -f migrate $dev2 $dev1
> -reset_devs
> +not ssm migrate $dev1 $dev2
> +# Only specified devices are used
> +ssm add $dev3
> +not ssm migrate $dev1 $dev2
> +ssm migrate $dev2 $dev3
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> +ssm -f remove --all
> +
> +# Simple lvm migrate of mounted fs
> +ssm create --fs ext4 $dev1 $mnt1
> +wipefs -a $dev2
> +not ssm migrate $mnt1 $dev2
> +ssm migrate $dev1 $dev2
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2
> +umount $mnt1
> +ssm check $SSM_LVM_DEFAULT_POOL/$lvol1
> +ssm -f remove --all
> +
> +# Migrate to a device from different pool
> +ssm create -p $vg1 $dev1 $dev2 $dev3
> +ssm add -p $vg1 $dev7
> +ssm create -p $vg2 $dev4 $dev5 $dev6
> +ssm add -p $vg2 $dev8
> +not ssm migrate $dev1 $dev8
> +ssm -f migrate $dev1 $dev8
> +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3 $dev7
> +check vg_devices $vg2 $dev4 $dev5 $dev6
> +# Migrate device that is not used
> +not ssm migrate $dev4 $dev7
> +ssm -f migrate $dev4 $dev7
> +check vg_devices $vg1 $dev1 $dev8 $dev2 $dev3
> +check vg_devices $vg2 $dev4 $dev5 $dev6 $dev7
> +# Migrate device that is used
> +not ssm add -p $vg1 $dev1 # We're not removing the pv after migrate
> +not ssm migrate $dev5 $dev1
> +ssm -f migrate $dev5 $dev1
> +ssm -f remove --all
> +
> +# Migrate plain device to a device in lvm
> +ssm create $dev1 $dev2 $dev3
> +ssm add -p $SSM_LVM_DEFAULT_POOL $dev4 $dev5
> +# target device used
> +not ssm migrate $dev6 $dev3
> +not ssm -f migrate $dev6 $dev3
> +# target device not used
> +not ssm migrate $dev6 $dev4
> +ssm -f migrate $dev6 $dev4
> +check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3 $dev5
> +# Migrate plain device with a file system
> +#mkfs.ext4 -F $dev4
> +# target device used
> +#not ssm migrate $dev4 $dev1
> +#not ssm -f migrate $dev4 $dev1
> +# target not used
> +#not ssm migrate $dev4 $dev5
> +#ssm -f migrate $dev4 $dev5
> +#check vg_devices $SSM_LVM_DEFAULT_POOL $dev1 $dev2 $dev3
> +#fsck.ext4 -fn $dev5
> +#test_volume $dev5
> +ssm -f remove --all
> +
> +# migrate from btrfs pool to lvm
> +wipefs -a $dev4 $dev5
> +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> +not ssm migrate $dev5 $dev3
> +ssm -f migrate $dev5 $dev3
> +check btrfs_devices $vg1 $dev1 $dev2
> +check vg_devices $vg2 $dev4 $dev5 $dev3
> +test_volume $vg1
> +test_volume $vg2/$lvol1
> +ssm -f remove --all
> +
> +# migrate from lvm to btrfs pool
> +ssm -b btrfs create -p $vg1 $dev1 $dev2 $dev3
> +ssm -b lvm create --fs ext4 -p $vg2 $dev4 $dev5
> +ssm add -p $vg2 $dev6
> +not ssm migrate $dev3 $dev6
> +ssm -f migrate $dev3 $dev6
> +check btrfs_devices $vg1 $dev1 $dev2 $dev6
> +check vg_devices $vg2 $dev4 $dev5
> +test_volume $vg1
> +test_volume $vg2/$lvol1
> +ssm -f remove --all
> +
> +# CRYPT
> +
> +# cryptsetup will ask for password. If luks extension is used
> +# it will ask when creating header and then when opening the device
> +pass() {
> +       echo -e "${passwd}\n${passwd}"
> +}
> +
> +export SSM_DEFAULT_BACKEND='crypt'
> +export SSM_CRYPT_DEFAULT_POOL=$vg3
> +export CRYPT_VOL_PREFIX="${SSM_PREFIX_FILTER}enc"
> +export SSM_CRYPT_DEFAULT_VOL_PREFIX=$CRYPT_VOL_PREFIX
> +crypt_vol1=${CRYPT_VOL_PREFIX}001
> +
> +passwd="cai0ohMo8M"
> +pass | ssm create $dev1
> +check crypt_vol_field $crypt_vol1 type LUKS1
> +check crypt_vol_field $crypt_vol1 device $dev1
> +check list_table "$(ssm list vol)" $crypt_vol1 $SSM_CRYPT_DEFAULT_POOL none crypt
> +
> +# Migrate in crypt is not currently supported so this whould both fail
> +not ssm migrate $dev1 $dev2
> +not ssm -f migrate $dev1 $dev2
> +
> +# Device can't be simply removed from the crypt backend so this should fail
> +not ssm migrate $dev2 $dev1
> +not ssm -f migrate $dev2 $dev1
>
> -exit 0
> \ No newline at end of file
> +exit 0
> diff --git a/tests/bashtests/lib/check.sh b/tests/bashtests/lib/check.sh
> index 4dad7c4..2bdc790 100644
> --- a/tests/bashtests/lib/check.sh
> +++ b/tests/bashtests/lib/check.sh
> @@ -257,6 +257,29 @@ lv_field()
>         fi
>  }
>
> +vg_devices()
> +{
> +       # Check the expected list of devices in the fs with the reality
> +       # $1 is the vg we're inspecting
> +       # the rest is list of devices
> +       tmp=`mktemp`
> +
> +       devices=`vgs --no-headings -o pv_name $1 | awk {'print $1'}`
> +       shift
> +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> +
> +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> +       if [ $? -eq 0 ]; then
> +               echo "ACTUAL"
> +               cat ${tmp}.actual
> +               echo "EXPECTED"
> +               cat ${tmp}.expected
> +               exit 1
> +       fi
> +       exit 0
> +}
> +
>  compare_fields()
>  {
>         local cmd1=$1;
> @@ -334,6 +357,29 @@ crypt_vol_field()
>         fi
>  }
>
> +btrfs_devices()
> +{
> +       # Check the expected list of devices in the fs with the reality
> +       # $1 is label for the file system we want to test
> +       # the rest is list of devices
> +       tmp=`mktemp`
> +
> +       devices=`btrfs filesystem show $1| grep devid | awk {'print $NF'}`
> +       shift
> +       for i in $devices; do stat -L -c '%t%T' $i; done | sort > ${tmp}.actual
> +       for i in $@; do stat -L -c '%t%T' $i; done | sort > ${tmp}.expected
> +
> +       ! diff ${tmp}.actual ${tmp}.expected 2>&1> /dev/null
> +       if [ $? -eq 0 ]; then
> +               echo "ACTUAL"
> +               cat ${tmp}.actual
> +               echo "EXPECTED"
> +               cat ${tmp}.expected
> +               exit 1
> +       fi
> +       exit 0
> +}
> +
>  btrfs_fs_field()
>  {
>         lines=$(btrfs filesystem show 2> /dev/null | grep -A 1 "^Label:.*$1" || true)
> diff --git a/tests/unittests/test_btrfs.py b/tests/unittests/test_btrfs.py
> index a73420f..1e8d7e1 100644
> --- a/tests/unittests/test_btrfs.py
> +++ b/tests/unittests/test_btrfs.py
> @@ -20,6 +20,7 @@
>
>  import unittest
>  from ssmlib import main
> +from ssmlib import problem
>  from ssmlib.backends import btrfs
>  from tests.unittests.common import *
>
> @@ -461,8 +462,10 @@ class BtrfsFunctionCheck(MockSystemDataSource):
>          self._addVol('vol002', 237284225, 1, 'my_pool', ['/dev/sdc1'])
>          self._addDevice('/dev/sde', 11489037516)
>
> -        self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> -        "btrfs device delete /dev/sdc1 /tmp/mount")
> +        try:
> +            main.main("ssm migrate /dev/sdc1 /dev/sdc2")
> +        except problem.DeviceUsed:
> +            pass
>
>          self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
>          "btrfs replace start -B /dev/sdc2 /dev/sde /tmp/mount")
> diff --git a/tests/unittests/test_lvm.py b/tests/unittests/test_lvm.py
> index fe87c4c..ec565e0 100644
> --- a/tests/unittests/test_lvm.py
> +++ b/tests/unittests/test_lvm.py
> @@ -257,21 +257,8 @@ class LvmFunctionCheck(MockSystemDataSource):
>          self._addDevice('/dev/sde', 11489037516)
>
>          self._checkCmd("ssm migrate /dev/sdc1 /dev/sdc2", [],
> -        "lvm pvremove /dev/sdc1")
> -        self.assertEqual(self.run_data[-2],
> -            "lvm vgreduce my_pool /dev/sdc1")
> -        self.assertEqual(self.run_data[-3],
>              "lvm pvmove --atomic /dev/sdc1 /dev/sdc2")
>
> -        self._checkCmd("ssm migrate /dev/sdc2 /dev/sde", [],
> -        "lvm pvremove /dev/sdc2")
> -        self.assertEqual(self.run_data[-2],
> -            "lvm vgreduce my_pool /dev/sdc2")
> -        self.assertEqual(self.run_data[-3],
> -            "lvm pvmove --atomic /dev/sdc2 /dev/sde")
> -        self.assertEqual(self.run_data[-4],
> -            "lvm vgextend my_pool /dev/sde")
> -
>      def test_lvm_resize(self):
>          # Generate some storage data
>          self._addPool('default_pool', ['/dev/sda', '/dev/sdb'])
> --
> 2.17.1
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> storagemanager-devel mailing list
> storagemanager-devel@...
> https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)



-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH 2/4] lvm: Remove pool_name key if pv not in pool

[Jan Tulak <jtulak@re...>]

On Thu, Aug 2, 2018 at 4:59 PM Lukas Czerner <lczerner@...> wrote:
>
> When pv is not in volume group the pool_name will still be present in
> dictionary and it's value is going to be empty. Howeve this might
> be confusing because in several places we check for existing pool simply
> by checking for existence of 'pool_name' key. So remove it.
>
> Signed-off-by: Lukas Czerner <lczerner@...>

Reviewed-by: Jan Tulak <jtulak@...>

> ---
>  ssmlib/backends/lvm.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ssmlib/backends/lvm.py b/ssmlib/backends/lvm.py
> index 7b5b3c8..e726d9c 100644
> --- a/ssmlib/backends/lvm.py
> +++ b/ssmlib/backends/lvm.py
> @@ -395,6 +395,7 @@ class PvsInfo(LvmInfo, template.BackendDevice):
>          if not pv['pool_name']:
>              pv['dev_used'] = ''
>              pv['dev_free'] = ''
> +            del pv['pool_name']
>
>      def remove(self, devices):
>          if len(devices) == 0:
> --
> 2.17.1
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> storagemanager-devel mailing list
> storagemanager-devel@...
> https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)



-- 
Jan Tulak
jtulak@... / jan@...

Re: [ssm-devel] [PATCH 3/4] crypt: crypt backend need to have a pool in database

[Jan Tulak <jtulak@re...>]

On Thu, Aug 2, 2018 at 4:59 PM Lukas Czerner <lczerner@...> wrote:
>
> Usually pool is the main handle for the backend, however crypt backned
> has no pool, even though the volumes do reference pool_name.
>
> So far we did not need it, however with some operations such as migrate
> we need to recognize that the device belongs to a valid pool and call
> into the backend to do some work. For that we need to have a valid pool.
> Create it.
>
> Signed-off-by: Lukas Czerner <lczerner@...>

Reviewed-by: Jan Tulak <jtulak@...>

> ---
>  ssmlib/backends/crypt.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/ssmlib/backends/crypt.py b/ssmlib/backends/crypt.py
> index 917ab5b..903d296 100644
> --- a/ssmlib/backends/crypt.py
> +++ b/ssmlib/backends/crypt.py
> @@ -94,6 +94,14 @@ class DmCryptPool(DmObject, template.BackendPool):
>                  'hide': True}
>          self.data[self.default_pool_name] = pool
>          '''
> +        pool = {}
> +        pool['type'] = 'crypt'
> +        pool['pool_name'] = self.default_pool_name
> +        pool['hide'] = True
> +        pool['pool_free'] = 0
> +        pool['pool_size'] = 0
> +        pool['pool_used'] = 0
> +        self.data[pool['pool_name']] = pool
>          self.passphrase = None
>
>      def set_passphrase(self, passphrase):
> --
> 2.17.1
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> storagemanager-devel mailing list
> storagemanager-devel@...
> https://lists.sourceforge.net/lists/listinfo/storagemanager-devel
> System Storage Manager (http://sourceforge.net/p/storagemanager/home/Home/)



-- 
Jan Tulak
jtulak@... / jan@...