Merge ~jasonzio/cloud-init:fixmounterrormatch into cloud-init:master

[Jason Zions]

azure: Accept variation in error msg from mount for ntfs volumes

If Azure detects an ntfs filesystem type during mount attempt, it should
still report the resource device as reformattable. There are slight
differences in error message format on RedHat and SuSE. This patch
simplifies the expected error match to work on both distributions.

LP: #1799338

Fixes lp:1799338 by omitting the prefix of the error message from the text-to-match. This skips the part of the error message that differs between RHEL and SUSE without introducing a regexp match, which would be needlessly expensive in this case.

[Chad Smith (chad.smith)]

Thank you for this branch.

 I'm trying to decide if it's worth noting the SLES/RHEL differences by extending the existing unit test test_ntfs_mount_errors_true in tests/unittests/test_datasource/test_azure.py.

I like getting test coverage to explain what 'mocked' expectation are and why to give future-us context on what to worry about with a change.

How's this patch look?

diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 4c5c6c1..b4bc48d 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -1510,21 +1510,20 @@ class TestCanDevBeReformatted(CiTestCase):
                     '/dev/sda1': {'num': 1, 'fs': 'ntfs', 'files': []}
                 }}})

- err = ("Unexpected error while running command.\n",
- "Command: ['mount', '-o', 'ro,sync', '-t', 'auto', ",
- "'/dev/sda1', '/fake-tmp/dir']\n"
- "Exit code: 32\n"
- "Reason: -\n"
- "Stdout: -\n"
- "Stderr: mount: unknown filesystem type 'ntfs'")
- self.m_mount_cb.side_effect = MountFailedError(
- 'Failed mounting %s to %s due to: %s' %
- ('/dev/sda', '/fake-tmp/dir', err))
-
- value, msg = dsaz.can_dev_be_reformatted('/dev/sda',
- preserve_ntfs=False)
- self.assertTrue(value)
- self.assertIn('cannot mount NTFS, assuming', msg)
+ error_msgs = [
+ "Stderr: mount: unknown filesystem type 'ntfs'", # RHEL
+ "Stderr: mount: /dev/sdb1: unknown filesystem type 'ntfs'" # SLES
+ ]
+
+ for err_msg in error_msgs:
+ self.m_mount_cb.side_effect = MountFailedError(
+ 'Failed mounting %s to %s due to: %s' %
+ ('/dev/sda', '/fake-tmp/dir', err_msg))
+
+ value, msg = dsaz.can_dev_be_reformatted(
+ '/dev/sda', preserve_ntfs=False)
+ self.assertTrue(value)
+ self.assertIn('cannot mount NTFS, assuming', msg)

     def test_never_destroy_ntfs_config_false(self):
         """Normally formattable situation with never_destroy_ntfs set."""

[Chad Smith (chad.smith)]

Jason, I also reformatted the commit message on this branch to align a bit more with our typical git log format. If that message makes sense or needs correction please feel free to "wordsmith" it.

[Jason Zions (jasonzio)]

> I like getting test coverage to explain what 'mocked' expectation are and why
> to give future-us context on what to worry about with a change.
>
>
> How's this patch look?

That looks pretty good to me. It trades off mocking the entire body of the error message in favor of mocking the only part that gets looked at. As long as that last statement is true (i.e. no code examining a failed mount attempt looks at anything but the captured Stderr part), you nicely capture the platform distinction and make sure we don't screw it up in the future.

[Jason Zions (jasonzio)]

> Jason, I also reformatted the commit message on this branch to align a bit
> more with our typical git log format. If that message makes sense or needs
> correction please feel free to "wordsmith" it.

LGTM. I couldn't find a doc describing the preferred git log format for this project, but I admit I failed to read a half-dozen merge requests to see what previous changes looked like, so that's my error. :-)

[Chad Smith (chad.smith)]

no worries Jason, thanks for the feedback. I'll add something to the hacking doc @ https://cloudinit.readthedocs.io/en/latest/topics/hacking.html to describe our formats (as we've recently changed them).

[Chad Smith (chad.smith)]

Jason, if you get a chance to apply the patch I suggested and push it back up, we'll land this branch.

Much thanks

[Jason Zions (jasonzio)]

I hadn't realized the ball was in my court; I'll take care of this as quickly as I can.

4d23c58... by Jason Zions <email address hidden> on 2018-11-06

Check both RHEL and SLES error behavior for ntfs mounts

[Jason Zions (jasonzio)]

> Jason, if you get a chance to apply the patch I suggested and push it back up,
> we'll land this branch.
>
> Much thanks

I have made the changes and pushed them; just waiting on you now.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:4d23c5887d5d146f431057d67251929607ede9fe
https://jenkins.ubuntu.com/server/job/cloud-init-ci/431/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/431/rebuild

review: Needs Fixing (continuous-integration)

Unmerged commits

4d23c58... by Jason Zions <email address hidden> on 2018-11-06

Check both RHEL and SLES error behavior for ntfs mounts

b8b0db1... by Jason Zions <email address hidden> on 2018-10-18

Accept variation in error msg from mount for ntfs volumes

diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index 7bdd43d..19e6d6d 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -716,7 +716,7 @@ def can_dev_be_reformatted(devpath, preserve_ntfs):
         file_count = util.mount_cb(cand_path, count_files, mtype="ntfs",
                                    update_env_for_mount={'LANG': 'C'})
     except util.MountFailedError as e:
-        if "mount: unknown filesystem type 'ntfs'" in str(e):
+        if "unknown filesystem type 'ntfs'" in str(e):
             return True, (bmsg + ' but this system cannot mount NTFS,'
                           ' assuming there are no important files.'
                           ' Formatting allowed.')
diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
index 4c5c6c1..f8c6566 100644
--- a/tests/unittests/test_datasource/test_azure.py
+++ b/tests/unittests/test_datasource/test_azure.py
@@ -1510,21 +1510,20 @@ class TestCanDevBeReformatted(CiTestCase):
                     '/dev/sda1': {'num': 1, 'fs': 'ntfs', 'files': []}
                 }}})
 
-        err = ("Unexpected error while running command.\n",
-               "Command: ['mount', '-o', 'ro,sync', '-t', 'auto', ",
-               "'/dev/sda1', '/fake-tmp/dir']\n"
-               "Exit code: 32\n"
-               "Reason: -\n"
-               "Stdout: -\n"
-               "Stderr: mount: unknown filesystem type 'ntfs'")
-        self.m_mount_cb.side_effect = MountFailedError(
-            'Failed mounting %s to %s due to: %s' %
-            ('/dev/sda', '/fake-tmp/dir', err))
-
-        value, msg = dsaz.can_dev_be_reformatted('/dev/sda',
-                                                 preserve_ntfs=False)
-        self.assertTrue(value)
-        self.assertIn('cannot mount NTFS, assuming', msg)
+        error_msgs = [
+            "Stderr: mount: unknown filesystem type 'ntfs'", # RHEL
+            "Stderr: mount: /dev/sdb1: unknown filesystem type 'ntfs'" # SLES
+        ]
+
+        for err_msg in error_msgs:
+            self.m_mount_cb.side_effect = MountFailedError(
+                "Failed mounting %s to %s due to: \nUnexpected.\n%s" %
+                ('/dev/sda', '/fake-tmp/dir', err_msg))
+
+            value, msg = dsaz.can_dev_be_reformatted('/dev/sda',
+                                                     preserve_ntfs=False)
+            self.assertTrue(value)
+            self.assertIn('cannot mount NTFS, assuming', msg)
 
     def test_never_destroy_ntfs_config_false(self):
         """Normally formattable situation with never_destroy_ntfs set."""