From b995b0ade83b5be73e1c0bece5c2fd468c9fd113 Mon Sep 17 00:00:00 2001 From: Bogdan Dobrelya Date: Wed, 12 Aug 2020 12:22:19 +0200 Subject: [PATCH] [USSURI-ONLY] Do not manage masked service units When a service unit for a containerized service has been manually masked, only ensure its container created, but not started. Also do not manage its service unit and timer. Partial-Bug: #1890789 Change-Id: I571918f37f41f03cde390e94c9bacc873909f92f Signed-off-by: Bogdan Dobrelya --- paunch/builder/base.py | 32 ++++--- paunch/tests/test_utils_systemctl.py | 122 ++++++++++++++++++++------- paunch/tests/test_utils_systemd.py | 72 +++++++++++----- paunch/utils/systemctl.py | 30 +++++-- paunch/utils/systemd.py | 6 +- 5 files changed, 190 insertions(+), 72 deletions(-) diff --git a/paunch/builder/base.py b/paunch/builder/base.py index 35440f1..aeb8b62 100644 --- a/paunch/builder/base.py +++ b/paunch/builder/base.py @@ -21,6 +21,7 @@ import tenacity import yaml from paunch.utils import common +from paunch.utils import systemctl from paunch.utils import systemd @@ -164,18 +165,25 @@ class BaseBuilder(object): self.log.info("stdout: %s" % cmd_stdout) self.log.info("stderr: %s" % cmd_stderr) if systemd_managed: - systemd.service_create(container=container_name, - cconfig=cconfig, - log=self.log) - if (not self.healthcheck_disabled and - 'healthcheck' in cconfig): - check = cconfig.get('healthcheck')['test'] - systemd.healthcheck_create(container=container_name, - log=self.log, test=check) - systemd.healthcheck_timer_create( - container=container_name, - cconfig=cconfig, - log=self.log) + try: + systemd.service_create(container=container_name, + cconfig=cconfig, + log=self.log) + if (not self.healthcheck_disabled and + 'healthcheck' in cconfig): + check = cconfig.get('healthcheck')['test'] + systemd.healthcheck_create( + container=container_name, + log=self.log, + test=check) + systemd.healthcheck_timer_create( + container=container_name, + cconfig=cconfig, + log=self.log) + except systemctl.SystemctlMaskedException: + self.log.warning('Masked service for container %s is ' + 'not managed here' % container_name) + pass if failed_containers: message = ( diff --git a/paunch/tests/test_utils_systemctl.py b/paunch/tests/test_utils_systemctl.py index 67bc496..ac438b0 100644 --- a/paunch/tests/test_utils_systemctl.py +++ b/paunch/tests/test_utils_systemctl.py @@ -20,68 +20,130 @@ from paunch.utils import systemctl class TestUtilsSystemctl(base.TestCase): + def setUp(self): + super(TestUtilsSystemctl, self).setUp() + self.r = mock.MagicMock() + self.r.returncode = 0 + self.r.stdout = '' + self.r.stderr = '' def test_format_service_name(self): expected = 'test.service' self.assertEqual(expected, systemctl.format_name('test')) self.assertEqual(expected, systemctl.format_name(expected)) - @mock.patch('subprocess.check_call', autospec=True) - def test_stop(self, mock_subprocess_check_call): + @mock.patch('subprocess.run', autospec=True) + def test_stop(self, mock_subprocess_run): + mock_subprocess_run.return_value = self.r test = 'test' systemctl.stop(test) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'stop', test]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'stop', test], + stderr=-1, stdout=-1, universal_newlines=True) ]) - @mock.patch('subprocess.check_call', autospec=True) - def test_daemon_reload(self, mock_subprocess_check_call): + @mock.patch('subprocess.run', autospec=True) + def test_daemon_reload(self, mock_subprocess_run): + mock_subprocess_run.return_value = self.r systemctl.daemon_reload() - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'daemon-reload']), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'daemon-reload'], + stderr=-1, stdout=-1, universal_newlines=True) ]) - @mock.patch('subprocess.check_call', autospec=True) - def test_is_active(self, mock_subprocess_check_call): + @mock.patch('subprocess.run', autospec=True) + def test_is_active(self, mock_subprocess_run): + mock_subprocess_run.return_value = self.r systemctl.is_active('foo') - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'is-active', '-q', 'foo']), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-active', '-q', 'foo'], + stderr=-1, stdout=-1, universal_newlines=True) ]) - @mock.patch('subprocess.check_call', autospec=True) - def test_enable(self, mock_subprocess_check_call): + @mock.patch('subprocess.run', autospec=True) + def test_enable(self, mock_subprocess_run): + mock_subprocess_run.return_value = self.r test = 'test' systemctl.enable(test, now=True) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'enable', '--now', test]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', 'test'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'enable', '--now', test], + stderr=-1, stdout=-1, universal_newlines=True) ]) + mock_subprocess_run.reset_mock() systemctl.enable(test) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'enable', '--now', test]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', 'test'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'enable', '--now', test], + stderr=-1, stdout=-1, universal_newlines=True), ]) + mock_subprocess_run.reset_mock() systemctl.enable(test, now=False) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'enable', test]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', 'test'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'enable', test], + stderr=-1, stdout=-1, universal_newlines=True), ]) - @mock.patch('subprocess.check_call', autospec=True) - def test_disable(self, mock_subprocess_check_call): + @mock.patch('subprocess.run', autospec=True) + def test_enable_masked(self, mock_subprocess_run): + f = mock.MagicMock() + f.returncode = 42 + f.stdout = 'masked-runtime' + f.stderr = '' + mock_subprocess_run.return_value = f + test = 'test' + self.assertRaises(systemctl.SystemctlMaskedException, + systemctl.enable, test, True) + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', 'test'], + stderr=-1, stdout=-1, universal_newlines=True) + ]) + + @mock.patch('subprocess.run', autospec=True) + @mock.patch('tenacity.wait.wait_random_exponential.__call__', + return_value=0) + def test_enable_failed(self, mock_wait, mock_subprocess_run): + f = mock.MagicMock() + f.returncode = 42 + f.stdout = 'foo' + f.stderr = 'fail fail fail' + mock_subprocess_run.side_effect = f + test = 'test' + self.assertRaises(systemctl.SystemctlException, + systemctl.enable, test) + + @mock.patch('subprocess.run', autospec=True) + def test_disable(self, mock_subprocess_run): + mock_subprocess_run.return_value = self.r test = 'test' systemctl.disable(test) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'disable', test]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'disable', test], + stderr=-1, stdout=-1, universal_newlines=True), ]) - @mock.patch('subprocess.check_call', autospec=True) - def test_add_requires(self, mock_subprocess_check_call): + @mock.patch('subprocess.run', autospec=True) + def test_add_requires(self, mock_subprocess_run): + mock_subprocess_run.return_value = self.r test = 'test' requires = "foo" systemctl.add_requires(test, requires) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'add-requires', test, requires]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', 'test'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'add-requires', test, requires], + stderr=-1, stdout=-1, universal_newlines=True), ]) requires = ["foo", "bar"] + mock_subprocess_run.reset_mock() systemctl.add_requires(test, requires) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'add-requires', test, "foo", "bar"]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', 'test'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'add-requires', test, "foo", "bar"], + stderr=-1, stdout=-1, universal_newlines=True) ]) diff --git a/paunch/tests/test_utils_systemd.py b/paunch/tests/test_utils_systemd.py index 766ab56..78e6bf3 100644 --- a/paunch/tests/test_utils_systemd.py +++ b/paunch/tests/test_utils_systemd.py @@ -23,12 +23,20 @@ from paunch.utils import systemd class TestUtilsSystemd(base.TestCase): + def setUp(self): + super(TestUtilsSystemd, self).setUp() + self.r = mock.MagicMock() + self.r.returncode = 0 + self.r.stdout = '' + self.r.stderr = '' + @mock.patch('shutil.rmtree', autospec=True) @mock.patch('os.path.exists', autospec=True) - @mock.patch('subprocess.check_call', autospec=True) + @mock.patch('subprocess.run', autospec=True) @mock.patch('os.chmod') - def test_service_create(self, mock_chmod, mock_subprocess_check_call, + def test_service_create(self, mock_chmod, mock_subprocess_run, mock_exists, mock_rmtree): + mock_subprocess_run.return_value = self.r container = 'my_app' service = 'tripleo_' + container cconfig = {'depends_on': ['something'], 'restart': 'unless-stopped', @@ -45,9 +53,13 @@ class TestUtilsSystemd(base.TestCase): self.assertIn('PIDFile=/var/run/my_app.pid', unit) mock_chmod.assert_has_calls([mock.call(sysd_unit_f, 420)]) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'daemon-reload']), - mock.call(['systemctl', 'enable', '--now', service]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'daemon-reload'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'is-enabled', service], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'enable', '--now', service], + stderr=-1, stdout=-1, universal_newlines=True), ]) mock_rmtree.assert_has_calls([ @@ -56,9 +68,10 @@ class TestUtilsSystemd(base.TestCase): os.rmdir(tempdir) - @mock.patch('subprocess.check_call', autospec=True) + @mock.patch('subprocess.run', autospec=True) @mock.patch('os.chmod') - def test_svc_extended_create(self, mock_chmod, mock_subprocess_check_call): + def test_svc_extended_create(self, mock_chmod, mock_subprocess_run): + mock_subprocess_run.return_value = self.r container = 'my_app' service = 'tripleo_' + container cconfig = {'depends_on': ['something'], 'restart': 'unless-stopped', @@ -82,9 +95,10 @@ class TestUtilsSystemd(base.TestCase): @mock.patch('os.remove', autospec=True) @mock.patch('os.path.exists', autospec=True) @mock.patch('os.path.isfile', autospec=True) - @mock.patch('subprocess.check_call', autospec=True) - def test_service_delete(self, mock_subprocess_check_call, mock_isfile, + @mock.patch('subprocess.run', autospec=True) + def test_service_delete(self, mock_subprocess_run, mock_isfile, mock_exists, mock_rm, mock_rmtree): + mock_subprocess_run.return_value = self.r mock_isfile.return_value = True container = 'my_app' service = 'tripleo_' + container @@ -97,15 +111,21 @@ class TestUtilsSystemd(base.TestCase): mock.call(tempdir + service + '_healthcheck.service'), mock.call(tempdir + service + '_healthcheck.timer'), ]) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'stop', service + '.service']), - mock.call(['systemctl', 'disable', service + '.service']), - mock.call(['systemctl', 'stop', service + '_healthcheck.service']), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'stop', service + '.service'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'disable', service + '.service'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'stop', service + '_healthcheck.service'], + stderr=-1, stdout=-1, universal_newlines=True), mock.call(['systemctl', 'disable', service + - '_healthcheck.service']), - mock.call(['systemctl', 'stop', service + '_healthcheck.timer']), + '_healthcheck.service'], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'stop', service + '_healthcheck.timer'], + stderr=-1, stdout=-1, universal_newlines=True), mock.call(['systemctl', 'disable', service + - '_healthcheck.timer']), + '_healthcheck.timer'], + stderr=-1, stdout=-1, universal_newlines=True), ]) mock_rmtree.assert_has_calls([ mock.call(os.path.join(tempdir, service_requires_d)), @@ -141,10 +161,11 @@ class TestUtilsSystemd(base.TestCase): self.assertIn('ExecStart=/usr/bin/podman exec --user root my_app ' '/foo/bar baz', unit) - @mock.patch('subprocess.check_call', autospec=True) + @mock.patch('subprocess.run', autospec=True) @mock.patch('os.chmod') def test_healthcheck_timer_create(self, mock_chmod, - mock_subprocess_check_call): + mock_subprocess_run): + mock_subprocess_run.return_value = self.r container = 'my_app' service = 'tripleo_' + container cconfig = {'check_interval': '15'} @@ -159,9 +180,16 @@ class TestUtilsSystemd(base.TestCase): self.assertIn('OnActiveSec=120', unit) self.assertIn('OnUnitActiveSec=15', unit) mock_chmod.assert_has_calls([mock.call(sysd_unit_f, 420)]) - mock_subprocess_check_call.assert_has_calls([ - mock.call(['systemctl', 'enable', '--now', healthcheck_timer]), + mock_subprocess_run.assert_has_calls([ + mock.call(['systemctl', 'is-enabled', healthcheck_timer], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'enable', '--now', healthcheck_timer], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'is-enabled', service + '.service'], + stderr=-1, stdout=-1, universal_newlines=True), mock.call(['systemctl', 'add-requires', service + '.service', - healthcheck_timer]), - mock.call(['systemctl', 'daemon-reload']), + healthcheck_timer], + stderr=-1, stdout=-1, universal_newlines=True), + mock.call(['systemctl', 'daemon-reload'], + stderr=-1, stdout=-1, universal_newlines=True), ]) diff --git a/paunch/utils/systemctl.py b/paunch/utils/systemctl.py index 21bff1e..6c6c510 100644 --- a/paunch/utils/systemctl.py +++ b/paunch/utils/systemctl.py @@ -22,16 +22,23 @@ class SystemctlException(Exception): pass -def systemctl(cmd, log=None): +class SystemctlMaskedException(Exception): + pass + + +def systemctl(cmd, log=None, ignore_errors=False): log = log or common.configure_logging(__name__) if not isinstance(cmd, list): raise SystemctlException("systemctl cmd passed must be a list") cmd.insert(0, 'systemctl') log.debug("Executing: {}".format(" ".join(cmd))) - try: - subprocess.check_call(cmd) - except subprocess.CalledProcessError as err: - raise SystemctlException(str(err)) + r = subprocess.run(cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + universal_newlines=True) + if not ignore_errors and r.returncode != 0: + raise SystemctlException(r.stderr.rstrip()) + return r.stdout.rstrip() def format_name(name): @@ -54,6 +61,11 @@ def is_active(service, log=None): systemctl(['is-active', '-q', service], log) +def is_masked(service, log=None): + out = systemctl(['is-enabled', service], log, ignore_errors=True) + return 'masked' in out + + # NOTE(bogdando): this implements a crash-loop with reset-failed # counters approach that provides an efficient feature parity to the # classic rate limiting, shall we want to implement that for the @@ -67,6 +79,10 @@ def is_active(service, log=None): stop=tenacity.stop_after_attempt(5) ) def enable(service, now=True, log=None): + if is_masked(service, log): + if log: + log.warning('Not enabling masked service %s' % service) + raise SystemctlMaskedException('Service %s is masked' % service) cmd = ['enable'] if now: cmd.append('--now') @@ -84,6 +100,10 @@ def disable(service, log=None): def add_requires(target, units, log=None): + if is_masked(target, log): + if log: + log.debug('Ignoring masked service target %s' % target) + raise SystemctlMaskedException('Service %s is masked' % target) cmd = ['add-requires', target] if isinstance(units, list): cmd.extend(units) diff --git a/paunch/utils/systemd.py b/paunch/utils/systemd.py index 4c70171..325e3d8 100644 --- a/paunch/utils/systemd.py +++ b/paunch/utils/systemd.py @@ -110,7 +110,7 @@ PIDFile=/var/run/%(name)s.pid WantedBy=multi-user.target""" % s_config) try: systemctl.daemon_reload() - systemctl.enable(service, now=True) + systemctl.enable(service, now=True, log=log) except systemctl.SystemctlException: log.exception("systemctl failed") raise @@ -244,9 +244,9 @@ RandomizedDelaySec=%(randomize)s [Install] WantedBy=timers.target""" % s_config) try: - systemctl.enable(healthcheck_timer, now=True) + systemctl.enable(healthcheck_timer, now=True, log=log) systemctl.add_requires(systemctl.format_name(service), - healthcheck_timer) + healthcheck_timer, log=log) systemctl.daemon_reload() except systemctl.SystemctlException: log.exception("systemctl failed")