Merge lp:~chad.smith/charm-helpers/resume-pause-idempotent into lp:charm-helpers

[Chad Smith]

Minor tweak to service_resume and service_pause functions to allow them be idempotent. Supports possibility of successful repeated pause or resume retries if needed.

[Stuart Bishop (stub)]

I don't think we need to worry about backwards compatibility here, which is good, since existing charms that would trigger the new behaviour are currently ending up in an error state.

Is service_running(foo) guaranteed to return true immediately after a start or false immediately after a stop? I don't think sysv makes any such guarantee (not sure about upstart), so I think service_start() will fail with daemons that take a while to startup and write their pid file.

I'd personally trust look-before-you-leap more, only calling stop if service_running() returns true and only calling start if service_running returns false etc. But I don't know what upstart guarantees (nor systemd, when we add xenial support).

review: Needs Information

[Chad Smith (chad.smith)]

Thanks for the review, I've updated the functions to check service_running first before calling start or stop appropriately.

Yeah, we'll have to add systemd support to this in short order.

[lp:~chad.smith/charm-helpers/resume-pause-idempotent]

483. By Chad Smith on 2015-11-17

update service_resume and service_pause to check if service_running before calling stop or start

484. By Chad Smith on 2015-11-17

fix logic on service_pause to call stop only when service is *not* running. Add/fix unit tests

[Stuart Bishop (stub)]

These changes look good.

I've made a couple of subjective stylistic comments inline, none of which are important enough to stop landing this as is.

review: Approve

=== modified file 'VERSION'
--- VERSION	2015-08-04 22:20:26 +0000
+++ VERSION	2015-11-17 20:21:43 +0000
@@ -1,1 +1,1 @@
-0.5.0
+0.5.1

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2015-10-26 09:53:06 +0000
+++ charmhelpers/core/host.py	2015-11-17 20:21:43 +0000
@@ -67,7 +67,9 @@
     """Pause a system service.
 
     Stop it, and prevent it from starting again at boot."""
-    stopped = service_stop(service_name)
+    stopped = True 
+    if service_running(service_name):
+        stopped = service_stop(service_name)
     upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
     sysv_file = os.path.join(initd_dir, service_name)
     if os.path.exists(upstart_file):
@@ -105,7 +107,9 @@
             "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
                 service_name, upstart_file, sysv_file))
 
-    started = service_start(service_name)
+    started = service_running(service_name)
+    if not started:
+        started = service_start(service_name)
     return started
 
 

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2015-10-26 09:53:06 +0000
+++ tests/core/test_host.py	2015-11-17 20:21:43 +0000
@@ -108,40 +108,94 @@
 
         service.assert_called_with('restart', service_name)
 
-    @patch.object(host, 'service')
-    def test_pauses_an_upstart_service(self, service):
-        service_name = 'foo-service'
-        service.side_effect = [True]
-        tempdir = mkdtemp(prefix="test_pauses_an_upstart_service")
-        conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
-        # Just needs to exist
-        with open(conf_path, "w") as fh:
-            fh.write("")
-        self.addCleanup(rmtree, tempdir)
-        self.assertTrue(host.service_pause(service_name, init_dir=tempdir))
-
-        service.assert_called_with('stop', service_name)
-        override_path = os.path.join(
-            tempdir, "{}.override".format(service_name))
-        with open(override_path, "r") as fh:
-            override_contents = fh.read()
-        self.assertEqual("manual\n", override_contents)
-
-    @patch('subprocess.check_call')
-    @patch.object(host, 'service')
-    def test_pauses_a_sysv_service(self, service, check_call):
-        service_name = 'foo-service'
-        service.side_effect = [True]
-        tempdir = mkdtemp(prefix="test_pauses_a_sysv_service")
-        sysv_path = os.path.join(tempdir, service_name)
-        # Just needs to exist
-        with open(sysv_path, "w") as fh:
-            fh.write("")
-        self.addCleanup(rmtree, tempdir)
-        self.assertTrue(host.service_pause(
-            service_name, init_dir=tempdir, initd_dir=tempdir))
-
-        service.assert_called_with('stop', service_name)
+    @patch('subprocess.check_output')
+    @patch.object(host, 'service')
+    def test_pauses_a_running_upstart_service(self, service, check_output):
+        """Pause on a running service will call service stop."""
+        check_output.return_value = b'foo-service start/running, process 123'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_pauses_an_upstart_service")
+        conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
+        # Just needs to exist
+        with open(conf_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_pause(service_name, init_dir=tempdir))
+
+        service.assert_called_with('stop', service_name)
+        override_path = os.path.join(
+            tempdir, "{}.override".format(service_name))
+        with open(override_path, "r") as fh:
+            override_contents = fh.read()
+        self.assertEqual("manual\n", override_contents)
+
+    @patch('subprocess.check_output')
+    @patch.object(host, 'service')
+    def test_pauses_a_stopped_upstart_service(self, service, check_output):
+        """Pause on a stopped service will not call service stop."""
+        check_output.return_value = b'foo-service stop/waiting'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_pauses_an_upstart_service")
+        conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
+        # Just needs to exist
+        with open(conf_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_pause(service_name, init_dir=tempdir))
+
+        # Stop isn't called because service is already stopped
+        self.assertRaises(
+            AssertionError, service.assert_called_with, 'stop', service_name)
+        override_path = os.path.join(
+            tempdir, "{}.override".format(service_name))
+        with open(override_path, "r") as fh:
+            override_contents = fh.read()
+        self.assertEqual("manual\n", override_contents)
+
+    @patch('subprocess.check_output')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'service')
+    def test_pauses_a_running_sysv_service(self, service, check_call,
+                                           check_output):
+        """Pause calls service stop on a running sysv service."""
+        check_output.return_value = b'foo-service start/running, process 123'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_pauses_a_sysv_service")
+        sysv_path = os.path.join(tempdir, service_name)
+        # Just needs to exist
+        with open(sysv_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_pause(
+            service_name, init_dir=tempdir, initd_dir=tempdir))
+
+        service.assert_called_with('stop', service_name)
+        check_call.assert_called_with(["update-rc.d", service_name, "disable"])
+
+    @patch('subprocess.check_output')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'service')
+    def test_pauses_a_stopped_sysv_service(self, service, check_call,
+                                           check_output):
+        """Pause does not call service stop on a stopped sysv service."""
+        check_output.return_value = b'foo-service stop/waiting'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_pauses_a_sysv_service")
+        sysv_path = os.path.join(tempdir, service_name)
+        # Just needs to exist
+        with open(sysv_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_pause(
+            service_name, init_dir=tempdir, initd_dir=tempdir))
+
+        # Stop isn't called because service is already stopped
+        self.assertRaises(
+            AssertionError, service.assert_called_with, 'stop', service_name)
         check_call.assert_called_with(["update-rc.d", service_name, "disable"])
 
     @patch.object(host, 'service')
@@ -157,37 +211,87 @@
             "Unable to detect {0}".format(service_name), str(exception))
         self.assertIn(tempdir, str(exception))
 
-    @patch.object(host, 'service')
-    def test_resumes_an_upstart_service(self, service):
-        service_name = 'foo-service'
-        service.side_effect = [True]
-        tempdir = mkdtemp(prefix="test_resumes_an_upstart_service")
-        conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
-        with open(conf_path, "w") as fh:
-            fh.write("")
-        self.addCleanup(rmtree, tempdir)
-        self.assertTrue(host.service_resume(service_name, init_dir=tempdir))
-
-        service.assert_called_with('start', service_name)
-        override_path = os.path.join(
-            tempdir, "{}.override".format(service_name))
-        self.assertFalse(os.path.exists(override_path))
-
-    @patch('subprocess.check_call')
-    @patch.object(host, 'service')
-    def test_resumes_a_sysv_service(self, service, check_call):
-        service_name = 'foo-service'
-        service.side_effect = [True]
-        tempdir = mkdtemp(prefix="test_resumes_a_sysv_service")
-        sysv_path = os.path.join(tempdir, service_name)
-        # Just needs to exist
-        with open(sysv_path, "w") as fh:
-            fh.write("")
-        self.addCleanup(rmtree, tempdir)
-        self.assertTrue(host.service_resume(
-            service_name, init_dir=tempdir, initd_dir=tempdir))
-
-        service.assert_called_with('start', service_name)
+    @patch('subprocess.check_output')
+    @patch.object(host, 'service')
+    def test_resumes_a_running_upstart_service(self, service, check_output):
+        """When the service is already running, service start isn't called."""
+        check_output.return_value = b'foo-service start/running, process 111'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_resumes_an_upstart_service")
+        conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
+        with open(conf_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_resume(service_name, init_dir=tempdir))
+
+        # Start isn't called because service is already running
+        self.assertRaises(
+            AssertionError, service.assert_called_with, 'start', service_name)
+        override_path = os.path.join(
+            tempdir, "{}.override".format(service_name))
+        self.assertFalse(os.path.exists(override_path))
+
+    @patch('subprocess.check_output')
+    @patch.object(host, 'service')
+    def test_resumes_a_stopped_upstart_service(self, service, check_output):
+        """When the service is stopped, service start is called."""
+        check_output.return_value = b'foo-service stop/waiting'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_resumes_an_upstart_service")
+        conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
+        with open(conf_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_resume(service_name, init_dir=tempdir))
+
+        service.assert_called_with('start', service_name)
+        override_path = os.path.join(
+            tempdir, "{}.override".format(service_name))
+        self.assertFalse(os.path.exists(override_path))
+
+    @patch('subprocess.check_output')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'service')
+    def test_resumes_a_sysv_service(self, service, check_call, check_output):
+        """When process is in a stop/waiting state, service start is called."""
+        check_output.return_value = b'foo-service stop/waiting'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_resumes_a_sysv_service")
+        sysv_path = os.path.join(tempdir, service_name)
+        # Just needs to exist
+        with open(sysv_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_resume(
+            service_name, init_dir=tempdir, initd_dir=tempdir))
+
+        service.assert_called_with('start', service_name)
+        check_call.assert_called_with(["update-rc.d", service_name, "enable"])
+
+    @patch('subprocess.check_output')
+    @patch('subprocess.check_call')
+    @patch.object(host, 'service')
+    def test_resume_a_running_sysv_service(self, service, check_call,
+                                           check_output):
+        """When process is already running, service start isn't called."""
+        check_output.return_value = b'foo-service start/running, process 123'
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        tempdir = mkdtemp(prefix="test_resumes_a_sysv_service")
+        sysv_path = os.path.join(tempdir, service_name)
+        # Just needs to exist
+        with open(sysv_path, "w") as fh:
+            fh.write("")
+        self.addCleanup(rmtree, tempdir)
+        self.assertTrue(host.service_resume(
+            service_name, init_dir=tempdir, initd_dir=tempdir))
+
+        # Start isn't called because service is already running
+        self.assertRaises(
+            AssertionError, service.assert_called_with, 'start', service_name)
         check_call.assert_called_with(["update-rc.d", service_name, "enable"])
 
     @patch.object(host, 'service')