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

Proposed by Jason Zions
Status: Superseded
Proposed branch: ~jasonzio/cloud-init:fixmounterrormatch
Merge into: cloud-init:master
Diff against target: 53 lines (+15/-16)
2 files modified
cloudinit/sources/DataSourceAzure.py (+1/-1)
tests/unittests/test_datasource/test_azure.py (+14/-15)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Needs Fixing
Review via email: mp+357669@code.launchpad.net

This proposal has been superseded by a proposal from 2018-11-12.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Chad Smith (chad.smith) wrote :

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."""

Revision history for this message
Chad Smith (chad.smith) wrote :

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.

Revision history for this message
Jason Zions (jasonzio) wrote :

> 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.

Revision history for this message
Jason Zions (jasonzio) wrote :

> 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. :-)

Revision history for this message
Chad Smith (chad.smith) wrote :

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).

Revision history for this message
Chad Smith (chad.smith) wrote :

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

Much thanks

Revision history for this message
Jason Zions (jasonzio) wrote :

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

Revision history for this message
Jason Zions (jasonzio) wrote :

> 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.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

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

Update scan failed

At least one of the branches involved have failed to scan. You can manually schedule a rescan if required.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
2index 7bdd43d..19e6d6d 100644
3--- a/cloudinit/sources/DataSourceAzure.py
4+++ b/cloudinit/sources/DataSourceAzure.py
5@@ -716,7 +716,7 @@ def can_dev_be_reformatted(devpath, preserve_ntfs):
6 file_count = util.mount_cb(cand_path, count_files, mtype="ntfs",
7 update_env_for_mount={'LANG': 'C'})
8 except util.MountFailedError as e:
9- if "mount: unknown filesystem type 'ntfs'" in str(e):
10+ if "unknown filesystem type 'ntfs'" in str(e):
11 return True, (bmsg + ' but this system cannot mount NTFS,'
12 ' assuming there are no important files.'
13 ' Formatting allowed.')
14diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
15index 4c5c6c1..f8c6566 100644
16--- a/tests/unittests/test_datasource/test_azure.py
17+++ b/tests/unittests/test_datasource/test_azure.py
18@@ -1510,21 +1510,20 @@ class TestCanDevBeReformatted(CiTestCase):
19 '/dev/sda1': {'num': 1, 'fs': 'ntfs', 'files': []}
20 }}})
21
22- err = ("Unexpected error while running command.\n",
23- "Command: ['mount', '-o', 'ro,sync', '-t', 'auto', ",
24- "'/dev/sda1', '/fake-tmp/dir']\n"
25- "Exit code: 32\n"
26- "Reason: -\n"
27- "Stdout: -\n"
28- "Stderr: mount: unknown filesystem type 'ntfs'")
29- self.m_mount_cb.side_effect = MountFailedError(
30- 'Failed mounting %s to %s due to: %s' %
31- ('/dev/sda', '/fake-tmp/dir', err))
32-
33- value, msg = dsaz.can_dev_be_reformatted('/dev/sda',
34- preserve_ntfs=False)
35- self.assertTrue(value)
36- self.assertIn('cannot mount NTFS, assuming', msg)
37+ error_msgs = [
38+ "Stderr: mount: unknown filesystem type 'ntfs'", # RHEL
39+ "Stderr: mount: /dev/sdb1: unknown filesystem type 'ntfs'" # SLES
40+ ]
41+
42+ for err_msg in error_msgs:
43+ self.m_mount_cb.side_effect = MountFailedError(
44+ "Failed mounting %s to %s due to: \nUnexpected.\n%s" %
45+ ('/dev/sda', '/fake-tmp/dir', err_msg))
46+
47+ value, msg = dsaz.can_dev_be_reformatted('/dev/sda',
48+ preserve_ntfs=False)
49+ self.assertTrue(value)
50+ self.assertIn('cannot mount NTFS, assuming', msg)
51
52 def test_never_destroy_ntfs_config_false(self):
53 """Normally formattable situation with never_destroy_ntfs set."""

Subscribers

People subscribed via source and target branches