Add parameter to override locale to utils.execute

In common.utils.mkfs and in common.disk_partitioner.list_partitions
functions standard locale is required in order to get correct console
output. In this change use_standard_locale flag is added to
common.utils.execute function to avoid code duplication when copying
environment variables and setting correct loacale.

Change-Id: Icd0ceb7b588d435eba9eb30846a9c66565e98a5e

ironic/common/disk_partitioner.py

@@ -13,7 +13,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import os
 import re
 
 from oslo.config import cfg
@@ -187,10 +186,9 @@ def list_partitions(device):
     :returns: list of dictionaries (one per partition) with keys:
               start, end, size (in MiB), filesystem, flags
     """
-    env = os.environ.copy()
+    output = utils.execute(
-    env['LC_ALL'] = 'C'
+        'parted', '-s', '-m', device, 'unit', 'MiB', 'print',
-    output = utils.execute('parted', '-s', '-m', device, 'unit', 'MiB',
+        use_standard_locale=True)[0]
-                           'print', env_variables=env)[0]
     lines = [line for line in output.split('\n') if line.strip()][2:]
     # Example of line: 1:1.00MiB:501MiB:500MiB:ext4::boot
     fields = ('start', 'end', 'size', 'filesystem', 'flags')

ironic/common/utils.py

@@ -61,7 +61,22 @@ def _get_root_helper():
 
 
 def execute(*cmd, **kwargs):
-    """Convenience wrapper around oslo's execute() method."""
+    """Convenience wrapper around oslo's execute() method.
+
+    :param cmd: Passed to processutils.execute.
+    :param use_standard_locale: True | False. Defaults to False. If set to
+                                True, execute command with standard locale
+                                added to environment variables.
+    :returns: (stdout, stderr) from process execution
+    :raises: UnknownArgumentError
+    :raises: ProcessExecutionError
+    """
+
+    use_standard_locale = kwargs.pop('use_standard_locale', False)
+    if use_standard_locale:
+        env = kwargs.pop('env_variables', os.environ.copy())
+        env['LC_ALL'] = 'C'
+        kwargs['env_variables'] = env
     if kwargs.get('run_as_root') and 'root_helper' not in kwargs:
         kwargs['root_helper'] = _get_root_helper()
     result = processutils.execute(*cmd, **kwargs)
@@ -438,9 +453,7 @@ def mkfs(fs, path, label=None):
         args.extend([label_opt, label])
     args.append(path)
     try:
-        env = os.environ.copy()
+        execute(*args, run_as_root=True, use_standard_locale=True)
-        env['LC_ALL'] = 'C'
-        execute(*args, run_as_root=True, env_variables=env)
     except processutils.ProcessExecutionError as e:
         with excutils.save_and_reraise_exception() as ctx:
             if os.strerror(errno.ENOENT) in e.stderr:

ironic/tests/test_disk_partitioner.py

@@ -15,7 +15,6 @@
 
 import fixtures
 import mock
-import os
 from testtools.matchers import HasLength
 
 from ironic.common import disk_partitioner
@@ -167,8 +166,7 @@ class DiskPartitionerTestCase(base.TestCase):
 @mock.patch.object(utils, 'execute')
 class ListPartitionsTestCase(base.TestCase):
 
-    @mock.patch.object(os.environ, 'copy', return_value={})
+    def test_correct(self, execute_mock):
-    def test_correct(self, env_mock, execute_mock):
         output = """
 BYT;
 /dev/sda:500107862016B:scsi:512:4096:msdos:ATA HGST HTS725050A7:;
@@ -182,12 +180,11 @@ BYT;
              'filesystem': '', 'flags': ''},
         ]
         execute_mock.return_value = (output, '')
-        env = {'LC_ALL': 'C'}
         result = disk_partitioner.list_partitions('/dev/fake')
         self.assertEqual(expected, result)
         execute_mock.assert_called_once_with(
             'parted', '-s', '-m', '/dev/fake', 'unit', 'MiB', 'print',
-            env_variables=env)
+            use_standard_locale=True)
 
     @mock.patch.object(disk_partitioner.LOG, 'warn')
     def test_incorrect(self, log_mock, execute_mock):

ironic/tests/test_utils.py

@@ -150,6 +150,30 @@ grep foo
             os.unlink(tmpfilename)
             os.unlink(tmpfilename2)
 
+    @mock.patch.object(processutils, 'execute')
+    @mock.patch.object(os.environ, 'copy', return_value={})
+    def test_execute_use_standard_locale_no_env_variables(self, env_mock,
+                                                          execute_mock):
+        utils.execute('foo', use_standard_locale=True)
+        execute_mock.assert_called_once_with('foo',
+                                             env_variables={'LC_ALL': 'C'})
+
+    @mock.patch.object(processutils, 'execute')
+    def test_execute_use_standard_locale_with_env_variables(self,
+                                                            execute_mock):
+        utils.execute('foo', use_standard_locale=True,
+                      env_variables={'foo': 'bar'})
+        execute_mock.assert_called_once_with('foo',
+                                             env_variables={'LC_ALL': 'C',
+                                                            'foo': 'bar'})
+
+    @mock.patch.object(processutils, 'execute')
+    def test_execute_not_use_standard_locale(self, execute_mock):
+        utils.execute('foo', use_standard_locale=False,
+                      env_variables={'foo': 'bar'})
+        execute_mock.assert_called_once_with('foo',
+                                             env_variables={'foo': 'bar'})
+
     def test_execute_get_root_helper(self):
         with mock.patch.object(processutils, 'execute') as execute_mock:
             helper = utils._get_root_helper()
@@ -349,44 +373,38 @@ class GenericUtilsTestCase(base.TestCase):
 
 class MkfsTestCase(base.TestCase):
 
-    @mock.patch.object(os.environ, 'copy')
     @mock.patch.object(utils, 'execute')
-    def test_mkfs(self, execute_mock, mock_env):
+    def test_mkfs(self, execute_mock):
-        lang_env_variable = {'LC_ALL': 'C'}
-        mock_env.return_value = lang_env_variable
         utils.mkfs('ext4', '/my/block/dev')
         utils.mkfs('msdos', '/my/msdos/block/dev')
         utils.mkfs('swap', '/my/swap/block/dev')
 
         expected = [mock.call('mkfs', '-t', 'ext4', '-F', '/my/block/dev',
                               run_as_root=True,
-                              env_variables=lang_env_variable),
+                              use_standard_locale=True),
                     mock.call('mkfs', '-t', 'msdos', '/my/msdos/block/dev',
                               run_as_root=True,
-                              env_variables=lang_env_variable),
+                              use_standard_locale=True),
                     mock.call('mkswap', '/my/swap/block/dev',
                               run_as_root=True,
-                              env_variables=lang_env_variable)]
+                              use_standard_locale=True)]
         self.assertEqual(expected, execute_mock.call_args_list)
 
-    @mock.patch.object(os.environ, 'copy')
     @mock.patch.object(utils, 'execute')
-    def test_mkfs_with_label(self, execute_mock, mock_env):
+    def test_mkfs_with_label(self, execute_mock):
-        lang_env_variable = {'LC_ALL': 'C'}
-        mock_env.return_value = lang_env_variable
         utils.mkfs('ext4', '/my/block/dev', 'ext4-vol')
         utils.mkfs('msdos', '/my/msdos/block/dev', 'msdos-vol')
         utils.mkfs('swap', '/my/swap/block/dev', 'swap-vol')
 
         expected = [mock.call('mkfs', '-t', 'ext4', '-F', '-L', 'ext4-vol',
                               '/my/block/dev', run_as_root=True,
-                              env_variables=lang_env_variable),
+                              use_standard_locale=True),
                     mock.call('mkfs', '-t', 'msdos', '-n', 'msdos-vol',
                               '/my/msdos/block/dev', run_as_root=True,
-                              env_variables=lang_env_variable),
+                              use_standard_locale=True),
                     mock.call('mkswap', '-L', 'swap-vol',
                               '/my/swap/block/dev', run_as_root=True,
-                              env_variables=lang_env_variable)]
+                              use_standard_locale=True)]
         self.assertEqual(expected, execute_mock.call_args_list)
 
     @mock.patch.object(utils, 'execute',