From 30b4d15764a1a9644379cf95770e8b2480856882 Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Tue, 5 Dec 2017 16:25:11 -0700 Subject: cli: Add clean and status subcommands The 'cloud-init clean' command allows a user or script to clear cloud-init artifacts from the system so that cloud-init sees the system as unconfigured upon reboot. Optional parameters can be provided to remove cloud-init logs and reboot after clean. The 'cloud-init status' command allows the user or script to check whether cloud-init has finished all configuration stages and whether errors occurred. An optional --wait argument will poll on a 0.25 second interval until cloud-init configuration is complete. The benefit here is scripts can block on cloud-init completion before performing post-config tasks. --- cloudinit/cmd/clean.py | 102 +++++++++++ cloudinit/cmd/main.py | 18 ++ cloudinit/cmd/status.py | 157 +++++++++++++++++ cloudinit/cmd/tests/__init__.py | 0 cloudinit/cmd/tests/test_clean.py | 159 +++++++++++++++++ cloudinit/cmd/tests/test_status.py | 353 +++++++++++++++++++++++++++++++++++++ 6 files changed, 789 insertions(+) create mode 100644 cloudinit/cmd/clean.py create mode 100644 cloudinit/cmd/status.py create mode 100644 cloudinit/cmd/tests/__init__.py create mode 100644 cloudinit/cmd/tests/test_clean.py create mode 100644 cloudinit/cmd/tests/test_status.py (limited to 'cloudinit/cmd') diff --git a/cloudinit/cmd/clean.py b/cloudinit/cmd/clean.py new file mode 100644 index 00000000..81797b1c --- /dev/null +++ b/cloudinit/cmd/clean.py @@ -0,0 +1,102 @@ +# Copyright (C) 2017 Canonical Ltd. +# +# This file is part of cloud-init. See LICENSE file for license information. + +"""Define 'clean' utility and handler as part of cloud-init commandline.""" + +import argparse +import os +import sys + +from cloudinit.stages import Init +from cloudinit.util import ( + ProcessExecutionError, chdir, del_dir, del_file, get_config_logfiles, subp) + + +def error(msg): + sys.stderr.write("ERROR: " + msg + "\n") + + +def get_parser(parser=None): + """Build or extend an arg parser for clean utility. + + @param parser: Optional existing ArgumentParser instance representing the + clean subcommand which will be extended to support the args of + this utility. + + @returns: ArgumentParser with proper argument configuration. + """ + if not parser: + parser = argparse.ArgumentParser( + prog='clean', + description=('Remove logs and artifacts so cloud-init re-runs on ' + 'a clean system')) + parser.add_argument( + '-l', '--logs', action='store_true', default=False, dest='remove_logs', + help='Remove cloud-init logs.') + parser.add_argument( + '-r', '--reboot', action='store_true', default=False, + help='Reboot system after logs are cleaned so cloud-init re-runs.') + parser.add_argument( + '-s', '--seed', action='store_true', default=False, dest='remove_seed', + help='Remove cloud-init seed directory /var/lib/cloud/seed.') + return parser + + +def remove_artifacts(remove_logs, remove_seed=False): + """Helper which removes artifacts dir and optionally log files. + + @param: remove_logs: Boolean. Set True to delete the cloud_dir path. False + preserves them. + @param: remove_seed: Boolean. Set True to also delete seed subdir in + paths.cloud_dir. + @returns: 0 on success, 1 otherwise. + """ + init = Init(ds_deps=[]) + init.read_cfg() + if remove_logs: + for log_file in get_config_logfiles(init.cfg): + del_file(log_file) + + if not os.path.isdir(init.paths.cloud_dir): + return 0 # Artifacts dir already cleaned + with chdir(init.paths.cloud_dir): + for path in os.listdir('.'): + if path == 'seed' and not remove_seed: + continue + try: + if os.path.isdir(path): + del_dir(path) + else: + del_file(path) + except OSError as e: + error('Could not remove {0}: {1}'.format(path, str(e))) + return 1 + return 0 + + +def handle_clean_args(name, args): + """Handle calls to 'cloud-init clean' as a subcommand.""" + exit_code = remove_artifacts(args.remove_logs, args.remove_seed) + if exit_code == 0 and args.reboot: + cmd = ['shutdown', '-r', 'now'] + try: + subp(cmd, capture=False) + except ProcessExecutionError as e: + error( + 'Could not reboot this system using "{0}": {1}'.format( + cmd, str(e))) + exit_code = 1 + return exit_code + + +def main(): + """Tool to collect and tar all cloud-init related logs.""" + parser = get_parser() + sys.exit(handle_clean_args('clean', parser.parse_args())) + + +if __name__ == '__main__': + main() + +# vi: ts=4 expandtab diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py index 6fb9d9e7..aa56225d 100644 --- a/cloudinit/cmd/main.py +++ b/cloudinit/cmd/main.py @@ -767,6 +767,12 @@ def main(sysv_args=None): parser_collect_logs = subparsers.add_parser( 'collect-logs', help='Collect and tar all cloud-init debug info') + parser_clean = subparsers.add_parser( + 'clean', help='Remove logs and artifacts so cloud-init can re-run.') + + parser_status = subparsers.add_parser( + 'status', help='Report cloud-init status or wait on completion.') + if sysv_args: # Only load subparsers if subcommand is specified to avoid load cost if sysv_args[0] == 'analyze': @@ -783,6 +789,18 @@ def main(sysv_args=None): logs_parser(parser_collect_logs) parser_collect_logs.set_defaults( action=('collect-logs', handle_collect_logs_args)) + elif sysv_args[0] == 'clean': + from cloudinit.cmd.clean import ( + get_parser as clean_parser, handle_clean_args) + clean_parser(parser_clean) + parser_clean.set_defaults( + action=('clean', handle_clean_args)) + elif sysv_args[0] == 'status': + from cloudinit.cmd.status import ( + get_parser as status_parser, handle_status_args) + status_parser(parser_status) + parser_status.set_defaults( + action=('status', handle_status_args)) args = parser.parse_args(args=sysv_args) diff --git a/cloudinit/cmd/status.py b/cloudinit/cmd/status.py new file mode 100644 index 00000000..3e5d0d07 --- /dev/null +++ b/cloudinit/cmd/status.py @@ -0,0 +1,157 @@ +# Copyright (C) 2017 Canonical Ltd. +# +# This file is part of cloud-init. See LICENSE file for license information. + +"""Define 'status' utility and handler as part of cloud-init commandline.""" + +import argparse +import os +import sys +from time import gmtime, strftime, sleep + +from cloudinit.distros import uses_systemd +from cloudinit.stages import Init +from cloudinit.util import get_cmdline, load_file, load_json + +CLOUDINIT_DISABLED_FILE = '/etc/cloud/cloud-init.disabled' + +# customer visible status messages +STATUS_ENABLED_NOT_RUN = 'not run' +STATUS_RUNNING = 'running' +STATUS_DONE = 'done' +STATUS_ERROR = 'error' +STATUS_DISABLED = 'disabled' + + +def get_parser(parser=None): + """Build or extend an arg parser for status utility. + + @param parser: Optional existing ArgumentParser instance representing the + status subcommand which will be extended to support the args of + this utility. + + @returns: ArgumentParser with proper argument configuration. + """ + if not parser: + parser = argparse.ArgumentParser( + prog='status', + description='Report run status of cloud init') + parser.add_argument( + '-l', '--long', action='store_true', default=False, + help=('Report long format of statuses including run stage name and' + ' error messages')) + parser.add_argument( + '-w', '--wait', action='store_true', default=False, + help='Block waiting on cloud-init to complete') + return parser + + +def handle_status_args(name, args): + """Handle calls to 'cloud-init status' as a subcommand.""" + # Read configured paths + init = Init(ds_deps=[]) + init.read_cfg() + + status, status_detail, time = _get_status_details(init.paths) + if args.wait: + while status in (STATUS_ENABLED_NOT_RUN, STATUS_RUNNING): + sys.stdout.write('.') + sys.stdout.flush() + status, status_detail, time = _get_status_details(init.paths) + sleep(0.25) + sys.stdout.write('\n') + if args.long: + print('status: {0}'.format(status)) + if time: + print('time: {0}'.format(time)) + print('detail:\n{0}'.format(status_detail)) + else: + print('status: {0}'.format(status)) + return 1 if status == STATUS_ERROR else 0 + + +def _is_cloudinit_disabled(disable_file, paths): + """Report whether cloud-init is disabled. + + @param disable_file: The path to the cloud-init disable file. + @param paths: An initialized cloudinit.helpers.Paths object. + @returns: A tuple containing (bool, reason) about cloud-init's status and + why. + """ + is_disabled = False + cmdline_parts = get_cmdline().split() + if not uses_systemd(): + reason = 'Cloud-init enabled on sysvinit' + elif 'cloud-init=enabled' in cmdline_parts: + reason = 'Cloud-init enabled by kernel command line cloud-init=enabled' + elif os.path.exists(disable_file): + is_disabled = True + reason = 'Cloud-init disabled by {0}'.format(disable_file) + elif 'cloud-init=disabled' in cmdline_parts: + is_disabled = True + reason = 'Cloud-init disabled by kernel parameter cloud-init=disabled' + elif not os.path.exists(os.path.join(paths.run_dir, 'enabled')): + is_disabled = True + reason = 'Cloud-init disabled by cloud-init-generator' + return (is_disabled, reason) + + +def _get_status_details(paths): + """Return a 3-tuple of status, status_details and time of last event. + + @param paths: An initialized cloudinit.helpers.paths object. + + Values are obtained from parsing paths.run_dir/status.json. + """ + + status = STATUS_ENABLED_NOT_RUN + status_detail = '' + status_v1 = {} + + status_file = os.path.join(paths.run_dir, 'status.json') + + (is_disabled, reason) = _is_cloudinit_disabled( + CLOUDINIT_DISABLED_FILE, paths) + if is_disabled: + status = STATUS_DISABLED + status_detail = reason + if os.path.exists(status_file): + status_v1 = load_json(load_file(status_file)).get('v1', {}) + errors = [] + latest_event = 0 + for key, value in sorted(status_v1.items()): + if key == 'stage': + if value: + status_detail = 'Running in stage: {0}'.format(value) + elif key == 'datasource': + status_detail = value + elif isinstance(value, dict): + errors.extend(value.get('errors', [])) + finished = value.get('finished') or 0 + if finished == 0: + status = STATUS_RUNNING + event_time = max(value.get('start', 0), finished) + if event_time > latest_event: + latest_event = event_time + if errors: + status = STATUS_ERROR + status_detail = '\n'.join(errors) + elif status == STATUS_ENABLED_NOT_RUN and latest_event > 0: + status = STATUS_DONE + if latest_event: + time = strftime('%a, %d %b %Y %H:%M:%S %z', gmtime(latest_event)) + else: + time = '' + return status, status_detail, time + + +def main(): + """Tool to report status of cloud-init.""" + parser = get_parser() + sys.exit(handle_status_args('status', parser.parse_args())) + + +if __name__ == '__main__': + main() + +# vi: ts=4 expandtab diff --git a/cloudinit/cmd/tests/__init__.py b/cloudinit/cmd/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cloudinit/cmd/tests/test_clean.py b/cloudinit/cmd/tests/test_clean.py new file mode 100644 index 00000000..af438aab --- /dev/null +++ b/cloudinit/cmd/tests/test_clean.py @@ -0,0 +1,159 @@ +# This file is part of cloud-init. See LICENSE file for license information. + +from cloudinit.cmd import clean +from cloudinit.util import ensure_dir, write_file +from cloudinit.tests.helpers import CiTestCase, wrap_and_call, mock +from collections import namedtuple +import os +from six import StringIO + +mypaths = namedtuple('MyPaths', 'cloud_dir') + + +class TestClean(CiTestCase): + + def setUp(self): + super(TestClean, self).setUp() + self.new_root = self.tmp_dir() + self.artifact_dir = self.tmp_path('artifacts', self.new_root) + self.log1 = self.tmp_path('cloud-init.log', self.new_root) + self.log2 = self.tmp_path('cloud-init-output.log', self.new_root) + + class FakeInit(object): + cfg = {'def_log_file': self.log1, + 'output': {'all': '|tee -a {0}'.format(self.log2)}} + paths = mypaths(cloud_dir=self.artifact_dir) + + def __init__(self, ds_deps): + pass + + def read_cfg(self): + pass + + self.init_class = FakeInit + + def test_remove_artifacts_removes_logs(self): + """remove_artifacts removes logs when remove_logs is True.""" + write_file(self.log1, 'cloud-init-log') + write_file(self.log2, 'cloud-init-output-log') + + self.assertFalse( + os.path.exists(self.artifact_dir), 'Unexpected artifacts dir') + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'Init': {'side_effect': self.init_class}}, + clean.remove_artifacts, remove_logs=True) + self.assertFalse(os.path.exists(self.log1), 'Unexpected file') + self.assertFalse(os.path.exists(self.log2), 'Unexpected file') + self.assertEqual(0, retcode) + + def test_remove_artifacts_preserves_logs(self): + """remove_artifacts leaves logs when remove_logs is False.""" + write_file(self.log1, 'cloud-init-log') + write_file(self.log2, 'cloud-init-output-log') + + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'Init': {'side_effect': self.init_class}}, + clean.remove_artifacts, remove_logs=False) + self.assertTrue(os.path.exists(self.log1), 'Missing expected file') + self.assertTrue(os.path.exists(self.log2), 'Missing expected file') + self.assertEqual(0, retcode) + + def test_remove_artifacts_removes_artifacts_skipping_seed(self): + """remove_artifacts cleans artifacts dir with exception of seed dir.""" + dirs = [ + self.artifact_dir, + os.path.join(self.artifact_dir, 'seed'), + os.path.join(self.artifact_dir, 'dir1'), + os.path.join(self.artifact_dir, 'dir2')] + for _dir in dirs: + ensure_dir(_dir) + + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'Init': {'side_effect': self.init_class}}, + clean.remove_artifacts, remove_logs=False) + self.assertEqual(0, retcode) + for expected_dir in dirs[:2]: + self.assertTrue( + os.path.exists(expected_dir), + 'Missing {0} dir'.format(expected_dir)) + for deleted_dir in dirs[2:]: + self.assertFalse( + os.path.exists(deleted_dir), + 'Unexpected {0} dir'.format(deleted_dir)) + + def test_remove_artifacts_removes_artifacts_removes_seed(self): + """remove_artifacts removes seed dir when remove_seed is True.""" + dirs = [ + self.artifact_dir, + os.path.join(self.artifact_dir, 'seed'), + os.path.join(self.artifact_dir, 'dir1'), + os.path.join(self.artifact_dir, 'dir2')] + for _dir in dirs: + ensure_dir(_dir) + + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'Init': {'side_effect': self.init_class}}, + clean.remove_artifacts, remove_logs=False, remove_seed=True) + self.assertEqual(0, retcode) + self.assertTrue( + os.path.exists(self.artifact_dir), 'Missing artifact dir') + for deleted_dir in dirs[1:]: + self.assertFalse( + os.path.exists(deleted_dir), + 'Unexpected {0} dir'.format(deleted_dir)) + + def test_remove_artifacts_returns_one_on_errors(self): + """remove_artifacts returns non-zero on failure and prints an error.""" + ensure_dir(self.artifact_dir) + ensure_dir(os.path.join(self.artifact_dir, 'dir1')) + + with mock.patch('sys.stderr', new_callable=StringIO) as m_stderr: + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'del_dir': {'side_effect': OSError('oops')}, + 'Init': {'side_effect': self.init_class}}, + clean.remove_artifacts, remove_logs=False) + self.assertEqual(1, retcode) + self.assertEqual( + 'ERROR: Could not remove dir1: oops\n', m_stderr.getvalue()) + + def test_handle_clean_args_reboots(self): + """handle_clean_args_reboots when reboot arg is provided.""" + + called_cmds = [] + + def fake_subp(cmd, capture): + called_cmds.append((cmd, capture)) + return '', '' + + myargs = namedtuple('MyArgs', 'remove_logs remove_seed reboot') + cmdargs = myargs(remove_logs=False, remove_seed=False, reboot=True) + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'subp': {'side_effect': fake_subp}, + 'Init': {'side_effect': self.init_class}}, + clean.handle_clean_args, name='does not matter', args=cmdargs) + self.assertEqual(0, retcode) + self.assertEqual( + [(['shutdown', '-r', 'now'], False)], called_cmds) + + def test_status_main(self): + '''clean.main can be run as a standalone script.''' + write_file(self.log1, 'cloud-init-log') + with self.assertRaises(SystemExit) as context_manager: + wrap_and_call( + 'cloudinit.cmd.clean', + {'Init': {'side_effect': self.init_class}, + 'sys.argv': {'new': ['clean', '--logs']}}, + clean.main) + + self.assertEqual(0, context_manager.exception.code) + self.assertFalse( + os.path.exists(self.log1), 'Unexpected log {0}'.format(self.log1)) + + +# vi: ts=4 expandtab syntax=python diff --git a/cloudinit/cmd/tests/test_status.py b/cloudinit/cmd/tests/test_status.py new file mode 100644 index 00000000..8ec9b5bc --- /dev/null +++ b/cloudinit/cmd/tests/test_status.py @@ -0,0 +1,353 @@ +# This file is part of cloud-init. See LICENSE file for license information. + +from collections import namedtuple +import os +from six import StringIO +from textwrap import dedent + +from cloudinit.atomic_helper import write_json +from cloudinit.cmd import status +from cloudinit.util import write_file +from cloudinit.tests.helpers import CiTestCase, wrap_and_call, mock + +mypaths = namedtuple('MyPaths', 'run_dir') +myargs = namedtuple('MyArgs', 'long wait') + + +class TestStatus(CiTestCase): + + def setUp(self): + super(TestStatus, self).setUp() + self.new_root = self.tmp_dir() + self.status_file = self.tmp_path('status.json', self.new_root) + self.disable_file = self.tmp_path('cloudinit-disable', self.new_root) + self.paths = mypaths(run_dir=self.new_root) + + class FakeInit(object): + paths = self.paths + + def __init__(self, ds_deps): + pass + + def read_cfg(self): + pass + + self.init_class = FakeInit + + def test__is_cloudinit_disabled_false_on_sysvinit(self): + '''When not in an environment using systemd, return False.''' + write_file(self.disable_file, '') # Create the ignored disable file + (is_disabled, reason) = wrap_and_call( + 'cloudinit.cmd.status', + {'uses_systemd': False}, + status._is_cloudinit_disabled, self.disable_file, self.paths) + self.assertFalse( + is_disabled, 'expected enabled cloud-init on sysvinit') + self.assertEqual('Cloud-init enabled on sysvinit', reason) + + def test__is_cloudinit_disabled_true_on_disable_file(self): + '''When using systemd and disable_file is present return disabled.''' + write_file(self.disable_file, '') # Create observed disable file + (is_disabled, reason) = wrap_and_call( + 'cloudinit.cmd.status', + {'uses_systemd': True}, + status._is_cloudinit_disabled, self.disable_file, self.paths) + self.assertTrue(is_disabled, 'expected disabled cloud-init') + self.assertEqual( + 'Cloud-init disabled by {0}'.format(self.disable_file), reason) + + def test__is_cloudinit_disabled_false_on_kernel_cmdline_enable(self): + '''Not disabled when using systemd and enabled via commandline.''' + write_file(self.disable_file, '') # Create ignored disable file + (is_disabled, reason) = wrap_and_call( + 'cloudinit.cmd.status', + {'uses_systemd': True, + 'get_cmdline': 'something cloud-init=enabled else'}, + status._is_cloudinit_disabled, self.disable_file, self.paths) + self.assertFalse(is_disabled, 'expected enabled cloud-init') + self.assertEqual( + 'Cloud-init enabled by kernel command line cloud-init=enabled', + reason) + + def test__is_cloudinit_disabled_true_on_kernel_cmdline(self): + '''When using systemd and disable_file is present return disabled.''' + (is_disabled, reason) = wrap_and_call( + 'cloudinit.cmd.status', + {'uses_systemd': True, + 'get_cmdline': 'something cloud-init=disabled else'}, + status._is_cloudinit_disabled, self.disable_file, self.paths) + self.assertTrue(is_disabled, 'expected disabled cloud-init') + self.assertEqual( + 'Cloud-init disabled by kernel parameter cloud-init=disabled', + reason) + + def test__is_cloudinit_disabled_true_when_generator_disables(self): + '''When cloud-init-generator doesn't write enabled file return True.''' + enabled_file = os.path.join(self.paths.run_dir, 'enabled') + self.assertFalse(os.path.exists(enabled_file)) + (is_disabled, reason) = wrap_and_call( + 'cloudinit.cmd.status', + {'uses_systemd': True, + 'get_cmdline': 'something'}, + status._is_cloudinit_disabled, self.disable_file, self.paths) + self.assertTrue(is_disabled, 'expected disabled cloud-init') + self.assertEqual('Cloud-init disabled by cloud-init-generator', reason) + + def test_status_returns_not_run(self): + '''When status.json does not exist yet, return 'not run'.''' + self.assertFalse( + os.path.exists(self.status_file), 'Unexpected status.json found') + cmdargs = myargs(long=False, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + self.assertEqual('status: not run\n', m_stdout.getvalue()) + + def test_status_returns_disabled_long_on_presence_of_disable_file(self): + '''When cloudinit is disabled, return disabled reason.''' + + checked_files = [] + + def fakeexists(filepath): + checked_files.append(filepath) + status_file = os.path.join(self.paths.run_dir, 'status.json') + return bool(not filepath == status_file) + + cmdargs = myargs(long=True, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'os.path.exists': {'side_effect': fakeexists}, + '_is_cloudinit_disabled': (True, 'disabled for some reason'), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + self.assertEqual( + [os.path.join(self.paths.run_dir, 'status.json')], + checked_files) + expected = dedent('''\ + status: disabled + detail: + disabled for some reason + ''') + self.assertEqual(expected, m_stdout.getvalue()) + + def test_status_returns_running(self): + '''Report running when status file exists but isn't finished.''' + write_json(self.status_file, {'v1': {'init': {'finished': None}}}) + cmdargs = myargs(long=False, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + self.assertEqual('status: running\n', m_stdout.getvalue()) + + def test_status_returns_done(self): + '''Reports done when stage is None and all stages are finished.''' + write_json( + self.status_file, + {'v1': {'stage': None, + 'datasource': ( + 'DataSourceNoCloud [seed=/var/.../seed/nocloud-net]' + '[dsmode=net]'), + 'blah': {'finished': 123.456}, + 'init': {'errors': [], 'start': 124.567, + 'finished': 125.678}, + 'init-local': {'start': 123.45, 'finished': 123.46}}}) + cmdargs = myargs(long=False, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + self.assertEqual('status: done\n', m_stdout.getvalue()) + + def test_status_returns_done_long(self): + '''Long format of done status includes datasource info.''' + write_json( + self.status_file, + {'v1': {'stage': None, + 'datasource': ( + 'DataSourceNoCloud [seed=/var/.../seed/nocloud-net]' + '[dsmode=net]'), + 'init': {'start': 124.567, 'finished': 125.678}, + 'init-local': {'start': 123.45, 'finished': 123.46}}}) + cmdargs = myargs(long=True, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + expected = dedent('''\ + status: done + time: Thu, 01 Jan 1970 00:02:05 +0000 + detail: + DataSourceNoCloud [seed=/var/.../seed/nocloud-net][dsmode=net] + ''') + self.assertEqual(expected, m_stdout.getvalue()) + + def test_status_on_errors(self): + '''Reports error when any stage has errors.''' + write_json( + self.status_file, + {'v1': {'stage': None, + 'blah': {'errors': [], 'finished': 123.456}, + 'init': {'errors': ['error1'], 'start': 124.567, + 'finished': 125.678}, + 'init-local': {'start': 123.45, 'finished': 123.46}}}) + cmdargs = myargs(long=False, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(1, retcode) + self.assertEqual('status: error\n', m_stdout.getvalue()) + + def test_status_on_errors_long(self): + '''Long format of error status includes all error messages.''' + write_json( + self.status_file, + {'v1': {'stage': None, + 'datasource': ( + 'DataSourceNoCloud [seed=/var/.../seed/nocloud-net]' + '[dsmode=net]'), + 'init': {'errors': ['error1'], 'start': 124.567, + 'finished': 125.678}, + 'init-local': {'errors': ['error2', 'error3'], + 'start': 123.45, 'finished': 123.46}}}) + cmdargs = myargs(long=True, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(1, retcode) + expected = dedent('''\ + status: error + time: Thu, 01 Jan 1970 00:02:05 +0000 + detail: + error1 + error2 + error3 + ''') + self.assertEqual(expected, m_stdout.getvalue()) + + def test_status_returns_running_long_format(self): + '''Long format reports the stage in which we are running.''' + write_json( + self.status_file, + {'v1': {'stage': 'init', + 'init': {'start': 124.456, 'finished': None}, + 'init-local': {'start': 123.45, 'finished': 123.46}}}) + cmdargs = myargs(long=True, wait=False) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + expected = dedent('''\ + status: running + time: Thu, 01 Jan 1970 00:02:04 +0000 + detail: + Running in stage: init + ''') + self.assertEqual(expected, m_stdout.getvalue()) + + def test_status_wait_blocks_until_done(self): + '''Specifying wait will poll every 1/4 second until done state.''' + running_json = { + 'v1': {'stage': 'init', + 'init': {'start': 124.456, 'finished': None}, + 'init-local': {'start': 123.45, 'finished': 123.46}}} + done_json = { + 'v1': {'stage': None, + 'init': {'start': 124.456, 'finished': 125.678}, + 'init-local': {'start': 123.45, 'finished': 123.46}}} + + self.sleep_calls = 0 + + def fake_sleep(interval): + self.assertEqual(0.25, interval) + self.sleep_calls += 1 + if self.sleep_calls == 2: + write_json(self.status_file, running_json) + elif self.sleep_calls == 3: + write_json(self.status_file, done_json) + + cmdargs = myargs(long=False, wait=True) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'sleep': {'side_effect': fake_sleep}, + '_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(0, retcode) + self.assertEqual(4, self.sleep_calls) + self.assertEqual('....\nstatus: done\n', m_stdout.getvalue()) + + def test_status_wait_blocks_until_error(self): + '''Specifying wait will poll every 1/4 second until error state.''' + running_json = { + 'v1': {'stage': 'init', + 'init': {'start': 124.456, 'finished': None}, + 'init-local': {'start': 123.45, 'finished': 123.46}}} + error_json = { + 'v1': {'stage': None, + 'init': {'errors': ['error1'], 'start': 124.456, + 'finished': 125.678}, + 'init-local': {'start': 123.45, 'finished': 123.46}}} + + self.sleep_calls = 0 + + def fake_sleep(interval): + self.assertEqual(0.25, interval) + self.sleep_calls += 1 + if self.sleep_calls == 2: + write_json(self.status_file, running_json) + elif self.sleep_calls == 3: + write_json(self.status_file, error_json) + + cmdargs = myargs(long=False, wait=True) + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + retcode = wrap_and_call( + 'cloudinit.cmd.status', + {'sleep': {'side_effect': fake_sleep}, + '_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.handle_status_args, 'ignored', cmdargs) + self.assertEqual(1, retcode) + self.assertEqual(4, self.sleep_calls) + self.assertEqual('....\nstatus: error\n', m_stdout.getvalue()) + + def test_status_main(self): + '''status.main can be run as a standalone script.''' + write_json(self.status_file, {'v1': {'init': {'finished': None}}}) + with self.assertRaises(SystemExit) as context_manager: + with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: + wrap_and_call( + 'cloudinit.cmd.status', + {'sys.argv': {'new': ['status']}, + '_is_cloudinit_disabled': (False, ''), + 'Init': {'side_effect': self.init_class}}, + status.main) + self.assertEqual(0, context_manager.exception.code) + self.assertEqual('status: running\n', m_stdout.getvalue()) + +# vi: ts=4 expandtab syntax=python -- cgit v1.2.3 From a110e483e8644ab73e69853ea11b6c4c6cfa04b6 Mon Sep 17 00:00:00 2001 From: Ryan Harper Date: Wed, 6 Dec 2017 16:30:22 -0600 Subject: pylint: Update pylint to 1.7.1, run on tests/ and tools and fix complaints. The motivation for this is that a.) 1.7.1 runs with python 3.6 (bionic) b.) we want to run pylint on tests/ and tools for the same reasons that we want to run it on cloudinit/ The changes are described below. - Update tox.ini to invoke pylint v1.7.1. - Modify .pylintrc generated-members ignore mocked object members (m_.*) - Replace "dangerous" params defaulting to {} - Fix up cloud_tests use of platforms - Cast some instance objects to with dict() - Handle python2.7 vs 3+ ConfigParser use of readfp (deprecated) - Update use of assertEqual(, value) to assert(value) - replace depricated assertRegexp -> assertRegex - Remove useless test-class calls to super class - Assign class property accessors a result and use it - Fix missing class member in CepkoResultTests - Fix Cheetah test import --- .pylintrc | 2 +- cloudinit/cmd/tests/test_clean.py | 2 +- cloudinit/cmd/tests/test_status.py | 2 +- cloudinit/tests/helpers.py | 35 +++++ tests/cloud_tests/__init__.py | 6 + tests/cloud_tests/bddeb.py | 9 +- tests/cloud_tests/collect.py | 6 +- tests/cloud_tests/config.py | 4 +- tests/cloud_tests/testcases/base.py | 3 +- .../testcases/modules/set_hostname_fqdn.py | 2 +- tests/cloud_tests/util.py | 2 +- tests/unittests/test_cs_util.py | 1 + tests/unittests/test_datasource/test_azure.py | 31 ++-- .../unittests/test_datasource/test_digitalocean.py | 9 +- tests/unittests/test_datasource/test_ec2.py | 3 +- tests/unittests/test_distros/test_create_users.py | 7 +- tests/unittests/test_distros/test_netconfig.py | 3 - tests/unittests/test_handler/test_handler_lxd.py | 3 - .../test_handler/test_handler_power_state.py | 3 - .../test_handler/test_handler_yum_add_repo.py | 10 +- .../test_handler/test_handler_zypper_add_repo.py | 7 +- tests/unittests/test_reporting.py | 2 +- tests/unittests/test_templating.py | 2 +- tests/unittests/test_util.py | 6 +- tests/unittests/test_vmware_config_file.py | 3 +- tools/hacking.py | 172 --------------------- tools/make-mime.py | 2 +- tools/mock-meta.py | 45 +++--- tox.ini | 5 +- 29 files changed, 121 insertions(+), 266 deletions(-) delete mode 100755 tools/hacking.py (limited to 'cloudinit/cmd') diff --git a/.pylintrc b/.pylintrc index b160ce7b..3ad36924 100644 --- a/.pylintrc +++ b/.pylintrc @@ -56,5 +56,5 @@ ignored-classes=optparse.Values,thread._local # List of members which are set dynamically and missed by pylint inference # system, and so shouldn't trigger E1101 when accessed. Python regular # expressions are accepted. -generated-members=types,http.client,command_handlers +generated-members=types,http.client,command_handlers,m_.* diff --git a/cloudinit/cmd/tests/test_clean.py b/cloudinit/cmd/tests/test_clean.py index af438aab..1379740b 100644 --- a/cloudinit/cmd/tests/test_clean.py +++ b/cloudinit/cmd/tests/test_clean.py @@ -151,7 +151,7 @@ class TestClean(CiTestCase): 'sys.argv': {'new': ['clean', '--logs']}}, clean.main) - self.assertEqual(0, context_manager.exception.code) + self.assertRaisesCodeEqual(0, context_manager.exception.code) self.assertFalse( os.path.exists(self.log1), 'Unexpected log {0}'.format(self.log1)) diff --git a/cloudinit/cmd/tests/test_status.py b/cloudinit/cmd/tests/test_status.py index 8ec9b5bc..6d4a11e8 100644 --- a/cloudinit/cmd/tests/test_status.py +++ b/cloudinit/cmd/tests/test_status.py @@ -347,7 +347,7 @@ class TestStatus(CiTestCase): '_is_cloudinit_disabled': (False, ''), 'Init': {'side_effect': self.init_class}}, status.main) - self.assertEqual(0, context_manager.exception.code) + self.assertRaisesCodeEqual(0, context_manager.exception.code) self.assertEqual('status: running\n', m_stdout.getvalue()) # vi: ts=4 expandtab syntax=python diff --git a/cloudinit/tests/helpers.py b/cloudinit/tests/helpers.py index feb884ab..0080c729 100644 --- a/cloudinit/tests/helpers.py +++ b/cloudinit/tests/helpers.py @@ -19,6 +19,11 @@ try: except ImportError: from contextlib2 import ExitStack +try: + from configparser import ConfigParser +except ImportError: + from ConfigParser import ConfigParser + from cloudinit import helpers as ch from cloudinit import util @@ -113,6 +118,16 @@ class TestCase(unittest2.TestCase): self.addCleanup(m.stop) setattr(self, attr, p) + # prefer python3 read_file over readfp but allow fallback + def parse_and_read(self, contents): + parser = ConfigParser() + if hasattr(parser, 'read_file'): + parser.read_file(contents) + elif hasattr(parser, 'readfp'): + # pylint: disable=W1505 + parser.readfp(contents) + return parser + class CiTestCase(TestCase): """This is the preferred test case base class unless user @@ -158,6 +173,18 @@ class CiTestCase(TestCase): dir = self.tmp_dir() return os.path.normpath(os.path.abspath(os.path.join(dir, path))) + def assertRaisesCodeEqual(self, expected, found): + """Handle centos6 having different context manager for assertRaises. + with assertRaises(Exception) as e: + raise Exception("BOO") + + centos6 will have e.exception as an integer. + anything nwere will have it as something with a '.code'""" + if isinstance(found, int): + self.assertEqual(expected, found) + else: + self.assertEqual(expected, found.code) + class ResourceUsingTestCase(CiTestCase): @@ -395,4 +422,12 @@ if not hasattr(mock.Mock, 'assert_not_called'): mock.Mock.assert_not_called = __mock_assert_not_called +# older unittest2.TestCase (centos6) do not have assertRaisesRegex +# And setting assertRaisesRegex to assertRaisesRegexp causes +# https://github.com/PyCQA/pylint/issues/1653 . So the workaround. +if not hasattr(unittest2.TestCase, 'assertRaisesRegex'): + def _tricky(*args, **kwargs): + return unittest2.TestCase.assertRaisesRegexp + unittest2.TestCase.assertRaisesRegex = _tricky + # vi: ts=4 expandtab diff --git a/tests/cloud_tests/__init__.py b/tests/cloud_tests/__init__.py index 98c1d6c7..dd436989 100644 --- a/tests/cloud_tests/__init__.py +++ b/tests/cloud_tests/__init__.py @@ -10,6 +10,12 @@ TESTCASES_DIR = os.path.join(BASE_DIR, 'testcases') TEST_CONF_DIR = os.path.join(BASE_DIR, 'testcases') TREE_BASE = os.sep.join(BASE_DIR.split(os.sep)[:-2]) +# This domain contains reverse lookups for hostnames that are used. +# The primary reason is so sudo will return quickly when it attempts +# to look up the hostname. i9n is just short for 'integration'. +# see also bug 1730744 for why we had to do this. +CI_DOMAIN = "i9n.cloud-init.io" + def _initialize_logging(): """Configure logging for cloud_tests.""" diff --git a/tests/cloud_tests/bddeb.py b/tests/cloud_tests/bddeb.py index c259dfea..a6d5069f 100644 --- a/tests/cloud_tests/bddeb.py +++ b/tests/cloud_tests/bddeb.py @@ -8,8 +8,7 @@ import tempfile from cloudinit import util as c_util from tests.cloud_tests import (config, LOG) -from tests.cloud_tests.platforms import (platforms, images, snapshots, - instances) +from tests.cloud_tests import platforms from tests.cloud_tests.stage import (PlatformComponent, run_stage, run_single) pre_reqs = ['devscripts', 'equivs', 'git', 'tar'] @@ -85,18 +84,18 @@ def setup_build(args): # set up image LOG.info('acquiring image for os: %s', args.build_os) img_conf = config.load_os_config(platform.platform_name, args.build_os) - image_call = partial(images.get_image, platform, img_conf) + image_call = partial(platforms.get_image, platform, img_conf) with PlatformComponent(image_call) as image: # set up snapshot - snapshot_call = partial(snapshots.get_snapshot, image) + snapshot_call = partial(platforms.get_snapshot, image) with PlatformComponent(snapshot_call) as snapshot: # create instance with cloud-config to set it up LOG.info('creating instance to build deb in') empty_cloud_config = "#cloud-config\n{}" instance_call = partial( - instances.get_instance, snapshot, empty_cloud_config, + platforms.get_instance, snapshot, empty_cloud_config, use_desc='build cloud-init deb') with PlatformComponent(instance_call) as instance: diff --git a/tests/cloud_tests/collect.py b/tests/cloud_tests/collect.py index db5ee99f..4805cea1 100644 --- a/tests/cloud_tests/collect.py +++ b/tests/cloud_tests/collect.py @@ -64,9 +64,9 @@ def collect_test_data(args, snapshot, os_name, test_name): # skip the testcase with a warning req_features = test_config.get('required_features', []) if any(feature not in snapshot.features for feature in req_features): - LOG.warn('test config %s requires features not supported by image, ' - 'skipping.\nrequired features: %s\nsupported features: %s', - test_name, req_features, snapshot.features) + LOG.warning('test config %s requires features not supported by image, ' + 'skipping.\nrequired features: %s\nsupported features: %s', + test_name, req_features, snapshot.features) return ({}, 0) # if there are user data overrides required for this test case, apply them diff --git a/tests/cloud_tests/config.py b/tests/cloud_tests/config.py index 52fc2bda..8bd569fd 100644 --- a/tests/cloud_tests/config.py +++ b/tests/cloud_tests/config.py @@ -92,7 +92,7 @@ def load_platform_config(platform_name, require_enabled=False): def load_os_config(platform_name, os_name, require_enabled=False, - feature_overrides={}): + feature_overrides=None): """Load configuration for os. @param platform_name: platform name to load os config for @@ -101,6 +101,8 @@ def load_os_config(platform_name, os_name, require_enabled=False, @param feature_overrides: feature flag overrides to merge with features @return_value: config dict """ + if feature_overrides is None: + feature_overrides = {} main_conf = c_util.read_conf(RELEASES_CONF) default = main_conf['default_release_config'] image = main_conf['releases'][os_name] diff --git a/tests/cloud_tests/testcases/base.py b/tests/cloud_tests/testcases/base.py index 1706f59b..1c5b5405 100644 --- a/tests/cloud_tests/testcases/base.py +++ b/tests/cloud_tests/testcases/base.py @@ -12,7 +12,8 @@ from cloudinit import util as c_util class CloudTestCase(unittest.TestCase): """Base test class for verifiers.""" - data = None + # data gets populated in get_suite.setUpClass + data = {} conf = None _cloud_config = None diff --git a/tests/cloud_tests/testcases/modules/set_hostname_fqdn.py b/tests/cloud_tests/testcases/modules/set_hostname_fqdn.py index eb6f0650..a405b30b 100644 --- a/tests/cloud_tests/testcases/modules/set_hostname_fqdn.py +++ b/tests/cloud_tests/testcases/modules/set_hostname_fqdn.py @@ -1,7 +1,7 @@ # This file is part of cloud-init. See LICENSE file for license information. """cloud-init Integration Test Verify Script.""" -from tests.cloud_tests.instances.nocloudkvm import CI_DOMAIN +from tests.cloud_tests import CI_DOMAIN from tests.cloud_tests.testcases import base diff --git a/tests/cloud_tests/util.py b/tests/cloud_tests/util.py index c5cd6974..2aedcd0d 100644 --- a/tests/cloud_tests/util.py +++ b/tests/cloud_tests/util.py @@ -262,7 +262,7 @@ def shell_safe(cmd): out = subprocess.check_output( ["getopt", "--shell", "sh", "--options", "", "--", "--"] + list(cmd)) # out contains ' -- \n'. drop the ' -- ' and the '\n' - return out[4:-1].decode() + return out.decode()[4:-1] def shell_pack(cmd): diff --git a/tests/unittests/test_cs_util.py b/tests/unittests/test_cs_util.py index ee88520d..2a1095b9 100644 --- a/tests/unittests/test_cs_util.py +++ b/tests/unittests/test_cs_util.py @@ -35,6 +35,7 @@ class CepkoMock(Cepko): # touched the underlying Cepko class methods. class CepkoResultTests(test_helpers.TestCase): def setUp(self): + self.c = Cepko() raise test_helpers.SkipTest('This test is completely useless') def test_getitem(self): diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py index 226c214a..5ab48897 100644 --- a/tests/unittests/test_datasource/test_azure.py +++ b/tests/unittests/test_datasource/test_azure.py @@ -36,9 +36,9 @@ def construct_valid_ovf_env(data=None, pubkeys=None, userdata=None): """ for key, dval in data.items(): if isinstance(dval, dict): - val = dval.get('text') - attrs = ' ' + ' '.join(["%s='%s'" % (k, v) for k, v in dval.items() - if k != 'text']) + val = dict(dval).get('text') + attrs = ' ' + ' '.join(["%s='%s'" % (k, v) for k, v + in dict(dval).items() if k != 'text']) else: val = dval attrs = "" @@ -897,9 +897,6 @@ class TestCanDevBeReformatted(CiTestCase): setattr(self, sattr, patcher.start()) self.addCleanup(patcher.stop) - def setUp(self): - super(TestCanDevBeReformatted, self).setUp() - def patchup(self, devs): bypath = {} for path, data in devs.items(): @@ -954,14 +951,14 @@ class TestCanDevBeReformatted(CiTestCase): '/dev/sda3': {'num': 3}, }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertFalse(False, value) + self.assertFalse(value) self.assertIn("3 or more", msg.lower()) def test_no_partitions_is_false(self): """A disk with no partitions can not be formatted.""" self.patchup({'/dev/sda': {}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertEqual(False, value) + self.assertFalse(value) self.assertIn("not partitioned", msg.lower()) def test_two_partitions_not_ntfs_false(self): @@ -973,7 +970,7 @@ class TestCanDevBeReformatted(CiTestCase): '/dev/sda2': {'num': 2, 'fs': 'ext4', 'files': []}, }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertFalse(False, value) + self.assertFalse(value) self.assertIn("not ntfs", msg.lower()) def test_two_partitions_ntfs_populated_false(self): @@ -986,7 +983,7 @@ class TestCanDevBeReformatted(CiTestCase): 'files': ['secret.txt']}, }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertFalse(False, value) + self.assertFalse(value) self.assertIn("files on it", msg.lower()) def test_two_partitions_ntfs_empty_is_true(self): @@ -998,7 +995,7 @@ class TestCanDevBeReformatted(CiTestCase): '/dev/sda2': {'num': 2, 'fs': 'ntfs', 'files': []}, }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertEqual(True, value) + self.assertTrue(value) self.assertIn("safe for", msg.lower()) def test_one_partition_not_ntfs_false(self): @@ -1009,7 +1006,7 @@ class TestCanDevBeReformatted(CiTestCase): '/dev/sda1': {'num': 1, 'fs': 'zfs'}, }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertEqual(False, value) + self.assertFalse(value) self.assertIn("not ntfs", msg.lower()) def test_one_partition_ntfs_populated_false(self): @@ -1021,7 +1018,7 @@ class TestCanDevBeReformatted(CiTestCase): 'files': ['file1.txt', 'file2.exe']}, }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertEqual(False, value) + self.assertFalse(value) self.assertIn("files on it", msg.lower()) def test_one_partition_ntfs_empty_is_true(self): @@ -1032,7 +1029,7 @@ class TestCanDevBeReformatted(CiTestCase): '/dev/sda1': {'num': 1, 'fs': 'ntfs', 'files': []} }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertEqual(True, value) + self.assertTrue(value) self.assertIn("safe for", msg.lower()) def test_one_partition_ntfs_empty_with_dataloss_file_is_true(self): @@ -1044,7 +1041,7 @@ class TestCanDevBeReformatted(CiTestCase): 'files': ['dataloss_warning_readme.txt']} }}}) value, msg = dsaz.can_dev_be_reformatted("/dev/sda") - self.assertEqual(True, value) + self.assertTrue(value) self.assertIn("safe for", msg.lower()) def test_one_partition_through_realpath_is_true(self): @@ -1059,7 +1056,7 @@ class TestCanDevBeReformatted(CiTestCase): 'realpath': '/dev/sdb1'} }}}) value, msg = dsaz.can_dev_be_reformatted(epath) - self.assertEqual(True, value) + self.assertTrue(value) self.assertIn("safe for", msg.lower()) def test_three_partition_through_realpath_is_false(self): @@ -1078,7 +1075,7 @@ class TestCanDevBeReformatted(CiTestCase): 'realpath': '/dev/sdb3'} }}}) value, msg = dsaz.can_dev_be_reformatted(epath) - self.assertEqual(False, value) + self.assertFalse(value) self.assertIn("3 or more", msg.lower()) diff --git a/tests/unittests/test_datasource/test_digitalocean.py b/tests/unittests/test_datasource/test_digitalocean.py index ec321733..3127014b 100644 --- a/tests/unittests/test_datasource/test_digitalocean.py +++ b/tests/unittests/test_datasource/test_digitalocean.py @@ -199,9 +199,8 @@ class TestDataSourceDigitalOcean(CiTestCase): class TestNetworkConvert(CiTestCase): - @mock.patch('cloudinit.net.get_interfaces_by_mac') - def _get_networking(self, m_get_by_mac): - m_get_by_mac.return_value = { + def _get_networking(self): + self.m_get_by_mac.return_value = { '04:01:57:d1:9e:01': 'ens1', '04:01:57:d1:9e:02': 'ens2', 'b8:ae:ed:75:5f:9a': 'enp0s25', @@ -211,6 +210,10 @@ class TestNetworkConvert(CiTestCase): self.assertIn('config', netcfg) return netcfg + def setUp(self): + super(TestNetworkConvert, self).setUp() + self.add_patch('cloudinit.net.get_interfaces_by_mac', 'm_get_by_mac') + def test_networking_defined(self): netcfg = self._get_networking() self.assertIsNotNone(netcfg) diff --git a/tests/unittests/test_datasource/test_ec2.py b/tests/unittests/test_datasource/test_ec2.py index ba042eac..f0dc8338 100644 --- a/tests/unittests/test_datasource/test_ec2.py +++ b/tests/unittests/test_datasource/test_ec2.py @@ -330,7 +330,8 @@ class TestEc2(test_helpers.HttprettyTestCase): ds.fallback_nic = 'eth9' with mock.patch(get_interface_mac_path) as m_get_interface_mac: m_get_interface_mac.return_value = mac1 - ds.network_config # Will re-crawl network metadata + nc = ds.network_config # Will re-crawl network metadata + self.assertIsNotNone(nc) self.assertIn('Re-crawl of metadata service', self.logs.getvalue()) expected = {'version': 1, 'config': [ {'mac_address': '06:17:04:d7:26:09', diff --git a/tests/unittests/test_distros/test_create_users.py b/tests/unittests/test_distros/test_create_users.py index aa13670a..5670904a 100644 --- a/tests/unittests/test_distros/test_create_users.py +++ b/tests/unittests/test_distros/test_create_users.py @@ -7,7 +7,11 @@ from cloudinit.tests.helpers import (TestCase, mock) class MyBaseDistro(distros.Distro): # MyBaseDistro is here to test base Distro class implementations - def __init__(self, name="basedistro", cfg={}, paths={}): + def __init__(self, name="basedistro", cfg=None, paths=None): + if not cfg: + cfg = {} + if not paths: + paths = {} super(MyBaseDistro, self).__init__(name, cfg, paths) def install_packages(self, pkglist): @@ -42,7 +46,6 @@ class MyBaseDistro(distros.Distro): @mock.patch("cloudinit.distros.util.subp") class TestCreateUser(TestCase): def setUp(self): - super(TestCase, self).setUp() self.dist = MyBaseDistro() def _useradd2call(self, args): diff --git a/tests/unittests/test_distros/test_netconfig.py b/tests/unittests/test_distros/test_netconfig.py index c4bd11bc..8d0b2634 100644 --- a/tests/unittests/test_distros/test_netconfig.py +++ b/tests/unittests/test_distros/test_netconfig.py @@ -188,9 +188,6 @@ hn0: flags=8843 metric 0 mtu 1500 status: active """ - def setUp(self): - super(TestNetCfgDistro, self).setUp() - def _get_distro(self, dname, renderers=None): cls = distros.fetch(dname) cfg = settings.CFG_BUILTIN diff --git a/tests/unittests/test_handler/test_handler_lxd.py b/tests/unittests/test_handler/test_handler_lxd.py index e0d9ab6c..a2054980 100644 --- a/tests/unittests/test_handler/test_handler_lxd.py +++ b/tests/unittests/test_handler/test_handler_lxd.py @@ -25,9 +25,6 @@ class TestLxd(t_help.CiTestCase): } } - def setUp(self): - super(TestLxd, self).setUp() - def _get_cloud(self, distro): cls = distros.fetch(distro) paths = helpers.Paths({}) diff --git a/tests/unittests/test_handler/test_handler_power_state.py b/tests/unittests/test_handler/test_handler_power_state.py index 85a0fe0a..3c726422 100644 --- a/tests/unittests/test_handler/test_handler_power_state.py +++ b/tests/unittests/test_handler/test_handler_power_state.py @@ -9,9 +9,6 @@ from cloudinit.tests.helpers import mock class TestLoadPowerState(t_help.TestCase): - def setUp(self): - super(self.__class__, self).setUp() - def test_no_config(self): # completely empty config should mean do nothing (cmd, _timeout, _condition) = psc.load_power_state({}) diff --git a/tests/unittests/test_handler/test_handler_yum_add_repo.py b/tests/unittests/test_handler/test_handler_yum_add_repo.py index b7adbe50..b90a3af3 100644 --- a/tests/unittests/test_handler/test_handler_yum_add_repo.py +++ b/tests/unittests/test_handler/test_handler_yum_add_repo.py @@ -5,10 +5,6 @@ from cloudinit import util from cloudinit.tests import helpers -try: - from configparser import ConfigParser -except ImportError: - from ConfigParser import ConfigParser import logging import shutil from six import StringIO @@ -58,8 +54,7 @@ class TestConfig(helpers.FilesystemMockingTestCase): self.patchUtils(self.tmp) cc_yum_add_repo.handle('yum_add_repo', cfg, None, LOG, []) contents = util.load_file("/etc/yum.repos.d/epel_testing.repo") - parser = ConfigParser() - parser.readfp(StringIO(contents)) + parser = self.parse_and_read(StringIO(contents)) expected = { 'epel_testing': { 'name': 'Extra Packages for Enterprise Linux 5 - Testing', @@ -95,8 +90,7 @@ class TestConfig(helpers.FilesystemMockingTestCase): self.patchUtils(self.tmp) cc_yum_add_repo.handle('yum_add_repo', cfg, None, LOG, []) contents = util.load_file("/etc/yum.repos.d/puppetlabs_products.repo") - parser = ConfigParser() - parser.readfp(StringIO(contents)) + parser = self.parse_and_read(StringIO(contents)) expected = { 'puppetlabs_products': { 'name': 'Puppet Labs Products El 6 - $basearch', diff --git a/tests/unittests/test_handler/test_handler_zypper_add_repo.py b/tests/unittests/test_handler/test_handler_zypper_add_repo.py index 315c2a5e..72ab6c08 100644 --- a/tests/unittests/test_handler/test_handler_zypper_add_repo.py +++ b/tests/unittests/test_handler/test_handler_zypper_add_repo.py @@ -9,10 +9,6 @@ from cloudinit import util from cloudinit.tests import helpers from cloudinit.tests.helpers import mock -try: - from configparser import ConfigParser -except ImportError: - from ConfigParser import ConfigParser import logging from six import StringIO @@ -70,8 +66,7 @@ class TestConfig(helpers.FilesystemMockingTestCase): root_d = self.tmp_dir() cc_zypper_add_repo._write_repos(cfg['repos'], root_d) contents = util.load_file("%s/testing-foo.repo" % root_d) - parser = ConfigParser() - parser.readfp(StringIO(contents)) + parser = self.parse_and_read(StringIO(contents)) expected = { 'testing-foo': { 'name': 'test-foo', diff --git a/tests/unittests/test_reporting.py b/tests/unittests/test_reporting.py index 571420ed..e15ba6cf 100644 --- a/tests/unittests/test_reporting.py +++ b/tests/unittests/test_reporting.py @@ -126,7 +126,7 @@ class TestBaseReportingHandler(TestCase): def test_base_reporting_handler_is_abstract(self): regexp = r".*abstract.*publish_event.*" - self.assertRaisesRegexp(TypeError, regexp, handlers.ReportingHandler) + self.assertRaisesRegex(TypeError, regexp, handlers.ReportingHandler) class TestLogHandler(TestCase): diff --git a/tests/unittests/test_templating.py b/tests/unittests/test_templating.py index b911d929..53154d33 100644 --- a/tests/unittests/test_templating.py +++ b/tests/unittests/test_templating.py @@ -14,7 +14,7 @@ from cloudinit import templater try: import Cheetah HAS_CHEETAH = True - Cheetah # make pyflakes happy, as Cheetah is not used here + c = Cheetah # make pyflakes and pylint happy, as Cheetah is not used here except ImportError: HAS_CHEETAH = False diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index 71f59529..787ca208 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -695,9 +695,9 @@ class TestSubp(helpers.CiTestCase): util.write_file(noshebang, 'true\n') os.chmod(noshebang, os.stat(noshebang).st_mode | stat.S_IEXEC) - self.assertRaisesRegexp(util.ProcessExecutionError, - 'Missing #! in script\?', - util.subp, (noshebang,)) + self.assertRaisesRegex(util.ProcessExecutionError, + 'Missing #! in script\?', + util.subp, (noshebang,)) def test_returns_none_if_no_capture(self): (out, err) = util.subp(self.stdin2out, data=b'', capture=False) diff --git a/tests/unittests/test_vmware_config_file.py b/tests/unittests/test_vmware_config_file.py index 808d303a..0f8cda95 100644 --- a/tests/unittests/test_vmware_config_file.py +++ b/tests/unittests/test_vmware_config_file.py @@ -133,7 +133,8 @@ class TestVmwareConfigFile(CiTestCase): conf = Config(cf) with self.assertRaises(ValueError): - conf.reset_password() + pw = conf.reset_password + self.assertIsNone(pw) cf.clear() cf._insertKey("PASSWORD|RESET", "yes") diff --git a/tools/hacking.py b/tools/hacking.py deleted file mode 100755 index e6a05136..00000000 --- a/tools/hacking.py +++ /dev/null @@ -1,172 +0,0 @@ -#!/usr/bin/env python -# Copyright (c) 2012, Cloudscaling -# All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. - -"""cloudinit HACKING file compliance testing (based off of nova hacking.py) - -built on top of pep8.py -""" - -import inspect -import logging -import re -import sys - -import pep8 - -# Don't need this for testing -logging.disable('LOG') - -# N1xx comments -# N2xx except -# N3xx imports -# N4xx docstrings -# N[5-9]XX (future use) - -DOCSTRING_TRIPLE = ['"""', "'''"] -VERBOSE_MISSING_IMPORT = False -_missingImport = set([]) - - -def import_normalize(line): - # convert "from x import y" to "import x.y" - # handle "from x import y as z" to "import x.y as z" - split_line = line.split() - if (line.startswith("from ") and "," not in line and - split_line[2] == "import" and split_line[3] != "*" and - split_line[1] != "__future__" and - (len(split_line) == 4 or (len(split_line) == 6 and - split_line[4] == "as"))): - return "import %s.%s" % (split_line[1], split_line[3]) - else: - return line - - -def cloud_import_alphabetical(physical_line, line_number, lines): - """Check for imports in alphabetical order. - - HACKING guide recommendation for imports: - imports in human alphabetical order - N306 - """ - # handle import x - # use .lower since capitalization shouldn't dictate order - split_line = import_normalize(physical_line.strip()).lower().split() - split_previous = import_normalize(lines[line_number - 2]) - split_previous = split_previous.strip().lower().split() - # with or without "as y" - length = [2, 4] - if (len(split_line) in length and len(split_previous) in length and - split_line[0] == "import" and split_previous[0] == "import"): - if split_line[1] < split_previous[1]: - return (0, "N306: imports not in alphabetical order (%s, %s)" - % (split_previous[1], split_line[1])) - - -def cloud_docstring_start_space(physical_line): - """Check for docstring not start with space. - - HACKING guide recommendation for docstring: - Docstring should not start with space - N401 - """ - pos = max([physical_line.find(i) for i in DOCSTRING_TRIPLE]) # start - if (pos != -1 and len(physical_line) > pos + 1): - if (physical_line[pos + 3] == ' '): - return (pos, - "N401: one line docstring should not start with a space") - - -def cloud_todo_format(physical_line): - """Check for 'TODO()'. - - HACKING guide recommendation for TODO: - Include your name with TODOs as in "#TODO(termie)" - N101 - """ - pos = physical_line.find('TODO') - pos1 = physical_line.find('TODO(') - pos2 = physical_line.find('#') # make sure it's a comment - if (pos != pos1 and pos2 >= 0 and pos2 < pos): - return pos, "N101: Use TODO(NAME)" - - -def cloud_docstring_one_line(physical_line): - """Check one line docstring end. - - HACKING guide recommendation for one line docstring: - A one line docstring looks like this and ends in a period. - N402 - """ - pos = max([physical_line.find(i) for i in DOCSTRING_TRIPLE]) # start - end = max([physical_line[-4:-1] == i for i in DOCSTRING_TRIPLE]) # end - if (pos != -1 and end and len(physical_line) > pos + 4): - if (physical_line[-5] != '.'): - return pos, "N402: one line docstring needs a period" - - -def cloud_docstring_multiline_end(physical_line): - """Check multi line docstring end. - - HACKING guide recommendation for docstring: - Docstring should end on a new line - N403 - """ - pos = max([physical_line.find(i) for i in DOCSTRING_TRIPLE]) # start - if (pos != -1 and len(physical_line) == pos): - print(physical_line) - if (physical_line[pos + 3] == ' '): - return (pos, "N403: multi line docstring end on new line") - - -current_file = "" - - -def readlines(filename): - """Record the current file being tested.""" - pep8.current_file = filename - return open(filename).readlines() - - -def add_cloud(): - """Monkey patch pep8 for cloud-init guidelines. - - Look for functions that start with cloud_ - and add them to pep8 module. - - Assumes you know how to write pep8.py checks - """ - for name, function in globals().items(): - if not inspect.isfunction(function): - continue - if name.startswith("cloud_"): - exec("pep8.%s = %s" % (name, name)) - - -if __name__ == "__main__": - # NOVA based 'hacking.py' error codes start with an N - pep8.ERRORCODE_REGEX = re.compile(r'[EWN]\d{3}') - add_cloud() - pep8.current_file = current_file - pep8.readlines = readlines - try: - pep8._main() - finally: - if len(_missingImport) > 0: - sys.stderr.write( - "%i imports missing in this test environment\n" % - len(_missingImport)) - -# vi: ts=4 expandtab diff --git a/tools/make-mime.py b/tools/make-mime.py index f6a72044..d321479b 100755 --- a/tools/make-mime.py +++ b/tools/make-mime.py @@ -23,7 +23,7 @@ def file_content_type(text): filename, content_type = text.split(":", 1) return (open(filename, 'r'), filename, content_type.strip()) except ValueError: - raise argparse.ArgumentError("Invalid value for %r" % (text)) + raise argparse.ArgumentError(text, "Invalid value for %r" % (text)) def main(): diff --git a/tools/mock-meta.py b/tools/mock-meta.py index a5d14ab7..724f7fc4 100755 --- a/tools/mock-meta.py +++ b/tools/mock-meta.py @@ -17,6 +17,7 @@ Then: ec2metadata --instance-id """ +import argparse import functools import json import logging @@ -27,8 +28,6 @@ import string import sys import yaml -from optparse import OptionParser - try: from BaseHTTPServer import HTTPServer, BaseHTTPRequestHandler import httplib as hclient @@ -415,29 +414,27 @@ def setup_logging(log_level, fmt='%(levelname)s: @%(name)s : %(message)s'): def extract_opts(): - parser = OptionParser() - parser.add_option("-p", "--port", dest="port", action="store", type=int, - default=80, metavar="PORT", - help=("port from which to serve traffic" - " (default: %default)")) - parser.add_option("-a", "--addr", dest="address", action="store", type=str, - default='::', metavar="ADDRESS", - help=("address from which to serve traffic" - " (default: %default)")) - parser.add_option("-f", '--user-data-file', dest='user_data_file', - action='store', metavar='FILE', - help=("user data filename to serve back to" - "incoming requests")) - (options, args) = parser.parse_args() - out = dict() - out['extra'] = args - out['port'] = options.port - out['user_data_file'] = None - out['address'] = options.address - if options.user_data_file: - if not os.path.isfile(options.user_data_file): + parser = argparse.ArgumentParser() + parser.add_argument("-p", "--port", dest="port", action="store", type=int, + default=80, metavar="PORT", + help=("port from which to serve traffic" + " (default: %default)")) + parser.add_argument("-a", "--addr", dest="address", action="store", + type=str, default='::', metavar="ADDRESS", + help=("address from which to serve traffic" + " (default: %default)")) + parser.add_argument("-f", '--user-data-file', dest='user_data_file', + action='store', metavar='FILE', + help=("user data filename to serve back to" + "incoming requests")) + parser.add_argument('extra', nargs='*') + args = parser.parse_args() + out = {'port': args.port, 'address': args.address, 'extra': args.extra, + 'user_data_file': None} + if args.user_data_file: + if not os.path.isfile(args.user_data_file): parser.error("Option -f specified a non-existent file") - with open(options.user_data_file, 'rb') as fh: + with open(args.user_data_file, 'rb') as fh: out['user_data_file'] = fh.read() return out diff --git a/tox.ini b/tox.ini index 92232201..d7316cc2 100644 --- a/tox.ini +++ b/tox.ini @@ -21,12 +21,13 @@ setenv = LC_ALL = en_US.utf-8 [testenv:pylint] +basepython = python3 deps = # requirements pylint==1.7.1 # test-requirements because unit tests are now present in cloudinit tree -r{toxinidir}/test-requirements.txt -commands = {envpython} -m pylint {posargs:cloudinit} +commands = {envpython} -m pylint {posargs:cloudinit tests tools} [testenv:py3] basepython = python3 @@ -119,7 +120,7 @@ commands = {envpython} -m pyflakes {posargs:cloudinit/ tests/ tools/} deps = pyflakes [testenv:tip-pylint] -commands = {envpython} -m pylint {posargs:cloudinit} +commands = {envpython} -m pylint {posargs:cloudinit tests tools} deps = # requirements pylint -- cgit v1.2.3 From 4089e20c0a20bc2ad5c21b106687c4f3faf84b4b Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Thu, 14 Dec 2017 22:06:29 -0700 Subject: cli: Fix error in cloud-init modules --mode=init. The cli help docs and argument parser allow the 'init' mode value which caused a traceback. Fix the cli to support 'init', 'config' and 'final' modes for the cloud-init modules subcommand. Add a check in the cli to raise a ValueError if a new subcommand ends up allowing an unsupported/unimplemented modes. Drive by unit test additions for a bit better coverage of error handling. LP: #1736600 --- cloudinit/cmd/main.py | 18 +++++++---- tests/unittests/test_cli.py | 75 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 6 deletions(-) (limited to 'cloudinit/cmd') diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py index aa56225d..30b37fe1 100644 --- a/cloudinit/cmd/main.py +++ b/cloudinit/cmd/main.py @@ -603,7 +603,11 @@ def status_wrapper(name, args, data_d=None, link_d=None): else: raise ValueError("unknown name: %s" % name) - modes = ('init', 'init-local', 'modules-config', 'modules-final') + modes = ('init', 'init-local', 'modules-init', 'modules-config', + 'modules-final') + if mode not in modes: + raise ValueError( + "Invalid cloud init mode specified '{0}'".format(mode)) status = None if mode == 'init-local': @@ -615,16 +619,18 @@ def status_wrapper(name, args, data_d=None, link_d=None): except Exception: pass + nullstatus = { + 'errors': [], + 'start': None, + 'finished': None, + } if status is None: - nullstatus = { - 'errors': [], - 'start': None, - 'finished': None, - } status = {'v1': {}} for m in modes: status['v1'][m] = nullstatus.copy() status['v1']['datasource'] = None + elif mode not in status['v1']: + status['v1'][mode] = nullstatus.copy() v1 = status['v1'] v1['stage'] = mode diff --git a/tests/unittests/test_cli.py b/tests/unittests/test_cli.py index a8d28ae6..0c0f427a 100644 --- a/tests/unittests/test_cli.py +++ b/tests/unittests/test_cli.py @@ -1,9 +1,12 @@ # This file is part of cloud-init. See LICENSE file for license information. +from collections import namedtuple +import os import six from cloudinit.cmd import main as cli from cloudinit.tests import helpers as test_helpers +from cloudinit.util import load_file, load_json mock = test_helpers.mock @@ -11,6 +14,8 @@ mock = test_helpers.mock class TestCLI(test_helpers.FilesystemMockingTestCase): + with_logs = True + def setUp(self): super(TestCLI, self).setUp() self.stderr = six.StringIO() @@ -24,6 +29,76 @@ class TestCLI(test_helpers.FilesystemMockingTestCase): except SystemExit as e: return e.code + def test_status_wrapper_errors_on_invalid_name(self): + """status_wrapper will error when the name parameter is not valid. + + Valid name values are only init and modules. + """ + tmpd = self.tmp_dir() + data_d = self.tmp_path('data', tmpd) + link_d = self.tmp_path('link', tmpd) + FakeArgs = namedtuple('FakeArgs', ['action', 'local', 'mode']) + + def myaction(): + raise Exception('Should not call myaction') + + myargs = FakeArgs(('doesnotmatter', myaction), False, 'bogusmode') + with self.assertRaises(ValueError) as cm: + cli.status_wrapper('init1', myargs, data_d, link_d) + self.assertEqual('unknown name: init1', str(cm.exception)) + self.assertNotIn('Should not call myaction', self.logs.getvalue()) + + def test_status_wrapper_errors_on_invalid_modes(self): + """status_wrapper will error if a parameter combination is invalid.""" + tmpd = self.tmp_dir() + data_d = self.tmp_path('data', tmpd) + link_d = self.tmp_path('link', tmpd) + FakeArgs = namedtuple('FakeArgs', ['action', 'local', 'mode']) + + def myaction(): + raise Exception('Should not call myaction') + + myargs = FakeArgs(('modules_name', myaction), False, 'bogusmode') + with self.assertRaises(ValueError) as cm: + cli.status_wrapper('modules', myargs, data_d, link_d) + self.assertEqual( + "Invalid cloud init mode specified 'modules-bogusmode'", + str(cm.exception)) + self.assertNotIn('Should not call myaction', self.logs.getvalue()) + + def test_status_wrapper_init_local_writes_fresh_status_info(self): + """When running in init-local mode, status_wrapper writes status.json. + + Old status and results artifacts are also removed. + """ + tmpd = self.tmp_dir() + data_d = self.tmp_path('data', tmpd) + link_d = self.tmp_path('link', tmpd) + status_link = self.tmp_path('status.json', link_d) + # Write old artifacts which will be removed or updated. + for _dir in data_d, link_d: + test_helpers.populate_dir( + _dir, {'status.json': 'old', 'result.json': 'old'}) + + FakeArgs = namedtuple('FakeArgs', ['action', 'local', 'mode']) + + def myaction(name, args): + # Return an error to watch status capture them + return 'SomeDatasource', ['an error'] + + myargs = FakeArgs(('ignored_name', myaction), True, 'bogusmode') + cli.status_wrapper('init', myargs, data_d, link_d) + # No errors reported in status + status_v1 = load_json(load_file(status_link))['v1'] + self.assertEqual(['an error'], status_v1['init-local']['errors']) + self.assertEqual('SomeDatasource', status_v1['datasource']) + self.assertFalse( + os.path.exists(self.tmp_path('result.json', data_d)), + 'unexpected result.json found') + self.assertFalse( + os.path.exists(self.tmp_path('result.json', link_d)), + 'unexpected result.json link found') + def test_no_arguments_shows_usage(self): exit_code = self._call_main() self.assertIn('usage: cloud-init', self.stderr.getvalue()) -- cgit v1.2.3 From 0b5bacb1761aefa74adb79bd1683d614bdf8c998 Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Wed, 3 Jan 2018 12:56:22 -0700 Subject: cli: cloud-init clean handles symlinks Fix cloud-init clean subcommand to unlink symlinks instead of calling del_dir. LP: #1741093 --- cloudinit/cmd/clean.py | 5 +++-- cloudinit/cmd/tests/test_clean.py | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) (limited to 'cloudinit/cmd') diff --git a/cloudinit/cmd/clean.py b/cloudinit/cmd/clean.py index 81797b1c..de22f7f2 100644 --- a/cloudinit/cmd/clean.py +++ b/cloudinit/cmd/clean.py @@ -10,7 +10,8 @@ import sys from cloudinit.stages import Init from cloudinit.util import ( - ProcessExecutionError, chdir, del_dir, del_file, get_config_logfiles, subp) + ProcessExecutionError, chdir, del_dir, del_file, get_config_logfiles, + is_link, subp) def error(msg): @@ -65,7 +66,7 @@ def remove_artifacts(remove_logs, remove_seed=False): if path == 'seed' and not remove_seed: continue try: - if os.path.isdir(path): + if os.path.isdir(path) and not is_link(path): del_dir(path) else: del_file(path) diff --git a/cloudinit/cmd/tests/test_clean.py b/cloudinit/cmd/tests/test_clean.py index 1379740b..6713af4f 100644 --- a/cloudinit/cmd/tests/test_clean.py +++ b/cloudinit/cmd/tests/test_clean.py @@ -1,7 +1,7 @@ # This file is part of cloud-init. See LICENSE file for license information. from cloudinit.cmd import clean -from cloudinit.util import ensure_dir, write_file +from cloudinit.util import ensure_dir, sym_link, write_file from cloudinit.tests.helpers import CiTestCase, wrap_and_call, mock from collections import namedtuple import os @@ -60,6 +60,23 @@ class TestClean(CiTestCase): self.assertTrue(os.path.exists(self.log2), 'Missing expected file') self.assertEqual(0, retcode) + def test_remove_artifacts_removes_unlinks_symlinks(self): + """remove_artifacts cleans artifacts dir unlinking any symlinks.""" + dir1 = os.path.join(self.artifact_dir, 'dir1') + ensure_dir(dir1) + symlink = os.path.join(self.artifact_dir, 'mylink') + sym_link(dir1, symlink) + + retcode = wrap_and_call( + 'cloudinit.cmd.clean', + {'Init': {'side_effect': self.init_class}}, + clean.remove_artifacts, remove_logs=False) + self.assertEqual(0, retcode) + for path in (dir1, symlink): + self.assertFalse( + os.path.exists(path), + 'Unexpected {0} dir'.format(path)) + def test_remove_artifacts_removes_artifacts_skipping_seed(self): """remove_artifacts cleans artifacts dir with exception of seed dir.""" dirs = [ -- cgit v1.2.3 From 6299e8d0cc230b0c9b41a69a5963bcd2c252c337 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 12 Jan 2018 14:23:26 -0700 Subject: Do not log warning on config files that represent None. This issue was first identified when manual_cache_clean was set, as ds-identify would write /run/cloud-init/cloud.cfg with # manual_cache_clean that would generate a warning as cloud-init expected to load a dict. Any other "empty" config would also log such a warning. Also fix reading of di_report to allow it to be None, as ds-identify would write: di_report: # manual_cache_clean which reads as 'di_report: None' rather than di_report: {}. LP: #1742479 --- cloudinit/cmd/main.py | 8 +++++++- cloudinit/util.py | 10 +++++----- tests/unittests/test_util.py | 8 ++++++++ 3 files changed, 20 insertions(+), 6 deletions(-) (limited to 'cloudinit/cmd') diff --git a/cloudinit/cmd/main.py b/cloudinit/cmd/main.py index 30b37fe1..d2f1b778 100644 --- a/cloudinit/cmd/main.py +++ b/cloudinit/cmd/main.py @@ -421,7 +421,13 @@ def di_report_warn(datasource, cfg): LOG.debug("no di_report found in config.") return - dicfg = cfg.get('di_report', {}) + dicfg = cfg['di_report'] + if dicfg is None: + # ds-identify may write 'di_report:\n #comment\n' + # which reads as {'di_report': None} + LOG.debug("di_report was None.") + return + if not isinstance(dicfg, dict): LOG.warning("di_report config not a dictionary: %s", dicfg) return diff --git a/cloudinit/util.py b/cloudinit/util.py index 8a9f1ab2..e42498d9 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -891,17 +891,17 @@ def load_yaml(blob, default=None, allowed=(dict,)): "of length %s with allowed root types %s", len(blob), allowed) converted = safeyaml.load(blob) - if not isinstance(converted, allowed): + if converted is None: + LOG.debug("loaded blob returned None, returning default.") + converted = default + elif not isinstance(converted, allowed): # Yes this will just be caught, but thats ok for now... raise TypeError(("Yaml load allows %s root types," " but got %s instead") % (allowed, type_utils.obj_name(converted))) loaded = converted except (yaml.YAMLError, TypeError, ValueError): - if len(blob) == 0: - LOG.debug("load_yaml given empty string, returning default") - else: - logexc(LOG, "Failed loading yaml blob") + logexc(LOG, "Failed loading yaml blob") return loaded diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index 787ca208..d63b760e 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -299,6 +299,14 @@ class TestLoadYaml(helpers.TestCase): default=self.mydefault), myobj) + def test_none_returns_default(self): + """If yaml.load returns None, then default should be returned.""" + blobs = ("", " ", "# foo\n", "#") + mdef = self.mydefault + self.assertEqual( + [(b, self.mydefault) for b in blobs], + [(b, util.load_yaml(blob=b, default=mdef)) for b in blobs]) + class TestMountinfoParsing(helpers.ResourceUsingTestCase): def test_invalid_mountinfo(self): -- cgit v1.2.3 From 1d8c327139a8c291eeb244ee1a6a8badd83e9e72 Mon Sep 17 00:00:00 2001 From: Chad Smith Date: Fri, 26 Jan 2018 13:36:30 -0700 Subject: Fix potential cases of uninitialized variables. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While addressing undeclared variable in 'cloud-init status', I also fixed the errors raised by automated code reviews against cloud-init master at https://lgtm.com/projects/g/cloud-init/cloud-init/alerts The following items are addressed:  * Fix 'cloud-init status':     * Only report 'running' state when any stage in /run/cloud-init/status.json has a start time but no finished time. Default start time to 0 if null.     * undeclared variable 'reason' now reports 'Cloud-init enabled by systemd cloud-init-generator' when systemd enables cloud-init  * cc_rh_subscription.py util.subp return values aren't set during if an exception is raised, use ProcessExecution as e instead.  * distros/freebsd.py:    * Drop repetitive looping over ipv4 and ipv6 nic lists.    * Initialize bsddev to 'NOTFOUND' in the event that no devs are discovered    * declare nics_with_addresses = set() in broader scope outside check_downable conditional  * cloudinit/util.py: Raise TypeError if mtype parameter isn't string, iterable or None. LP: #1744796 --- cloudinit/cmd/status.py | 7 +++++-- cloudinit/cmd/tests/test_status.py | 21 ++++++++++++++++++--- cloudinit/config/cc_power_state_change.py | 1 + cloudinit/config/cc_rh_subscription.py | 5 ++--- cloudinit/distros/freebsd.py | 11 +++-------- cloudinit/util.py | 4 ++++ 6 files changed, 33 insertions(+), 16 deletions(-) (limited to 'cloudinit/cmd') diff --git a/cloudinit/cmd/status.py b/cloudinit/cmd/status.py index 3e5d0d07..d7aaee9d 100644 --- a/cloudinit/cmd/status.py +++ b/cloudinit/cmd/status.py @@ -93,6 +93,8 @@ def _is_cloudinit_disabled(disable_file, paths): elif not os.path.exists(os.path.join(paths.run_dir, 'enabled')): is_disabled = True reason = 'Cloud-init disabled by cloud-init-generator' + else: + reason = 'Cloud-init enabled by systemd cloud-init-generator' return (is_disabled, reason) @@ -127,10 +129,11 @@ def _get_status_details(paths): status_detail = value elif isinstance(value, dict): errors.extend(value.get('errors', [])) + start = value.get('start') or 0 finished = value.get('finished') or 0 - if finished == 0: + if finished == 0 and start != 0: status = STATUS_RUNNING - event_time = max(value.get('start', 0), finished) + event_time = max(start, finished) if event_time > latest_event: latest_event = event_time if errors: diff --git a/cloudinit/cmd/tests/test_status.py b/cloudinit/cmd/tests/test_status.py index 6d4a11e8..a7c0a91a 100644 --- a/cloudinit/cmd/tests/test_status.py +++ b/cloudinit/cmd/tests/test_status.py @@ -93,6 +93,19 @@ class TestStatus(CiTestCase): self.assertTrue(is_disabled, 'expected disabled cloud-init') self.assertEqual('Cloud-init disabled by cloud-init-generator', reason) + def test__is_cloudinit_disabled_false_when_enabled_in_systemd(self): + '''Report enabled when systemd generator creates the enabled file.''' + enabled_file = os.path.join(self.paths.run_dir, 'enabled') + write_file(enabled_file, '') + (is_disabled, reason) = wrap_and_call( + 'cloudinit.cmd.status', + {'uses_systemd': True, + 'get_cmdline': 'something ignored'}, + status._is_cloudinit_disabled, self.disable_file, self.paths) + self.assertFalse(is_disabled, 'expected enabled cloud-init') + self.assertEqual( + 'Cloud-init enabled by systemd cloud-init-generator', reason) + def test_status_returns_not_run(self): '''When status.json does not exist yet, return 'not run'.''' self.assertFalse( @@ -137,8 +150,9 @@ class TestStatus(CiTestCase): self.assertEqual(expected, m_stdout.getvalue()) def test_status_returns_running(self): - '''Report running when status file exists but isn't finished.''' - write_json(self.status_file, {'v1': {'init': {'finished': None}}}) + '''Report running when status exists with an unfinished stage.''' + write_json(self.status_file, + {'v1': {'init': {'start': 1, 'finished': None}}}) cmdargs = myargs(long=False, wait=False) with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: retcode = wrap_and_call( @@ -338,7 +352,8 @@ class TestStatus(CiTestCase): def test_status_main(self): '''status.main can be run as a standalone script.''' - write_json(self.status_file, {'v1': {'init': {'finished': None}}}) + write_json(self.status_file, + {'v1': {'init': {'start': 1, 'finished': None}}}) with self.assertRaises(SystemExit) as context_manager: with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: wrap_and_call( diff --git a/cloudinit/config/cc_power_state_change.py b/cloudinit/config/cc_power_state_change.py index eba58b02..4da3a588 100644 --- a/cloudinit/config/cc_power_state_change.py +++ b/cloudinit/config/cc_power_state_change.py @@ -194,6 +194,7 @@ def doexit(sysexit): def execmd(exe_args, output=None, data_in=None): + ret = 1 try: proc = subprocess.Popen(exe_args, stdin=subprocess.PIPE, stdout=output, stderr=subprocess.STDOUT) diff --git a/cloudinit/config/cc_rh_subscription.py b/cloudinit/config/cc_rh_subscription.py index a9d21e78..530808ce 100644 --- a/cloudinit/config/cc_rh_subscription.py +++ b/cloudinit/config/cc_rh_subscription.py @@ -276,9 +276,8 @@ class SubscriptionManager(object): cmd = ['attach', '--auto'] try: return_out, return_err = self._sub_man_cli(cmd) - except util.ProcessExecutionError: - self.log_warn("Auto-attach failed with: " - "{0}]".format(return_err.strip())) + except util.ProcessExecutionError as e: + self.log_warn("Auto-attach failed with: {0}".format(e)) return False for line in return_out.split("\n"): if line is not "": diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index bad112fe..aa468bca 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -116,6 +116,7 @@ class Distro(distros.Distro): (out, err) = util.subp(['ifconfig', '-a']) ifconfigoutput = [x for x in (out.strip()).splitlines() if len(x.split()) > 0] + bsddev = 'NOT_FOUND' for line in ifconfigoutput: m = re.match('^\w+', line) if m: @@ -347,15 +348,9 @@ class Distro(distros.Distro): bymac[Distro.get_interface_mac(n)] = { 'name': n, 'up': self.is_up(n), 'downable': None} + nics_with_addresses = set() if check_downable: - nics_with_addresses = set() - ipv6 = self.get_ipv6() - ipv4 = self.get_ipv4() - for bytes_out in (ipv6, ipv4): - for i in ipv6: - nics_with_addresses.update(i) - for i in ipv4: - nics_with_addresses.update(i) + nics_with_addresses = set(self.get_ipv4() + self.get_ipv6()) for d in bymac.values(): d['downable'] = (d['up'] is False or diff --git a/cloudinit/util.py b/cloudinit/util.py index 9976400f..338fb971 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -1587,6 +1587,10 @@ def mount_cb(device, callback, data=None, rw=False, mtype=None, sync=True): mtypes = list(mtype) elif mtype is None: mtypes = None + else: + raise TypeError( + 'Unsupported type provided for mtype parameter: {_type}'.format( + _type=type(mtype))) # clean up 'mtype' input a bit based on platform. platsys = platform.system().lower() -- cgit v1.2.3