Merge lp:~trapnine/maas/fix-1562214 into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

The proxy service status will not be failed when the internal proxy server has been disabled in MAAS Settings.

The proxy service status will not be failed when the internal proxy server has been disabled in MAAS Settings.

When this is the case, the proxy status_info will read 'disabled, alternate proxy is configured in settings' so the user knows why proxy is off, and where to change it.

[Blake Rouse (blake-rouse)]

Looks really good. Just one comment that needs to be fixed before landing.

review: Approve

=== modified file 'src/maasserver/enum.py'
--- src/maasserver/enum.py	2016-03-26 00:36:43 +0000
+++ src/maasserver/enum.py	2016-03-31 16:08:17 +0000
@@ -609,6 +609,8 @@
     DEAD = 'dead'
     #: Service is off. (Should be off and is off).
     OFF = 'off'
+    #: Service is on.
+    ON = 'on'
 
 
 SERVICE_STATUS_CHOICES = (

=== modified file 'src/maasserver/service_monitor.py'
--- src/maasserver/service_monitor.py	2016-03-11 02:42:18 +0000
+++ src/maasserver/service_monitor.py	2016-03-31 16:08:17 +0000
@@ -8,14 +8,17 @@
 
 from datetime import timedelta
 
+from maasserver.enum import SERVICE_STATUS
+from maasserver.models.config import Config
 from maasserver.models.regioncontrollerprocess import RegionControllerProcess
-from maasserver.models.service import Service
+from maasserver.models.service import Service as ServiceModel
 from maasserver.utils.orm import transactional
 from maasserver.utils.threads import deferToDatabase
 from provisioningserver.config import is_dev_environment
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.utils.service_monitor import (
     AlwaysOnService,
+    Service,
     ServiceMonitor,
 )
 from twisted.application.internet import TimerService
@@ -34,12 +37,25 @@
     service_name = "bind9"
 
 
-class ProxyService(AlwaysOnService):
+class ProxyService(Service):
     """Monitored proxy service."""
 
     name = "proxy"
     service_name = "maas-proxy"
 
+    def get_expected_state(self):
+
+        @transactional
+        def db_get_expected_state():
+            if (Config.objects.get_config("enable_http_proxy") and
+                    Config.objects.get_config("http_proxy")):
+                return (SERVICE_STATUS.OFF,
+                        "disabled, alternate proxy is configured in settings.")
+            else:
+                return (SERVICE_STATUS.ON, None)
+
+        return deferToDatabase(db_get_expected_state)
+
 
 # Global service monitor for regiond.
 service_monitor = ServiceMonitor(
@@ -88,7 +104,7 @@
         """
         process = RegionControllerProcess.objects.get(id=processId)
         for service in services:
-            Service.objects.update_service_for(
+            ServiceModel.objects.update_service_for(
                 process.region, service["name"],
                 service["status"], service["status_info"])
 

=== modified file 'src/maasserver/tests/test_service_monitor.py'
--- src/maasserver/tests/test_service_monitor.py	2016-03-08 21:35:13 +0000
+++ src/maasserver/tests/test_service_monitor.py	2016-03-31 16:08:17 +0000
@@ -10,8 +10,10 @@
 from crochet import wait_for
 from maasserver import service_monitor as service_monitor_module
 from maasserver.enum import SERVICE_STATUS
+from maasserver.models.config import Config
 from maasserver.models.service import Service
 from maasserver.service_monitor import (
+    ProxyService,
     service_monitor,
     ServiceMonitorService,
 )
@@ -38,6 +40,7 @@
 from twisted.internet.defer import (
     fail,
     inlineCallbacks,
+    maybeDeferred,
     succeed,
 )
 from twisted.internet.task import Clock
@@ -157,4 +160,48 @@
             "status": "running",
             "status_info": "",
         }]
-        self.assertEquals(expected_services, observed_services)
+        self.assertEqual(expected_services, observed_services)
+
+
+class TestProxyService(MAASTransactionServerTestCase):
+
+    def make_proxy_service(self):
+        class FakeProxyService(ProxyService):
+            name = factory.make_name("name")
+            service_name = factory.make_name("service")
+        return FakeProxyService()
+
+    @inlineCallbacks
+    def test_get_expected_state_returns_on_for_proxy_off_and_unset(self):
+        service = self.make_proxy_service()
+        Config.objects.set_config("enable_http_proxy", False)
+        Config.objects.set_config("http_proxy", "")
+        expected_state = yield maybeDeferred(service.get_expected_state)
+        self.assertEqual((SERVICE_STATUS.ON, None), expected_state)
+
+    @inlineCallbacks
+    def test_get_expected_state_returns_on_for_proxy_off_and_set(self):
+        service = self.make_proxy_service()
+        Config.objects.set_config("enable_http_proxy", False)
+        Config.objects.set_config("http_proxy", factory.make_url())
+        expected_state = yield maybeDeferred(service.get_expected_state)
+        self.assertEqual((SERVICE_STATUS.ON, None), expected_state)
+
+    @inlineCallbacks
+    def test_get_expected_state_returns_on_for_proxy_on_but_unset(self):
+        service = self.make_proxy_service()
+        Config.objects.set_config("enable_http_proxy", True)
+        Config.objects.set_config("http_proxy", "")
+        expected_state = yield maybeDeferred(service.get_expected_state)
+        self.assertEqual((SERVICE_STATUS.ON, None), expected_state)
+
+    @inlineCallbacks
+    def test_get_expected_state_returns_off_for_proxy_on_and_set(self):
+        service = self.make_proxy_service()
+        Config.objects.set_config("enable_http_proxy", True)
+        Config.objects.set_config("http_proxy", factory.make_url())
+        expected_state = yield maybeDeferred(service.get_expected_state)
+        self.assertEqual(
+            (SERVICE_STATUS.OFF,
+             'disabled, alternate proxy is configured in settings.'),
+            expected_state)

=== modified file 'src/provisioningserver/service_monitor.py'
--- src/provisioningserver/service_monitor.py	2016-03-25 14:52:23 +0000
+++ src/provisioningserver/service_monitor.py	2016-03-31 16:08:17 +0000
@@ -27,7 +27,7 @@
         The dhcp service always starts as off. Once the rackd starts dhcp
         `expected_state` will be set to ON.
         """
-        return self.expected_state
+        return (self.expected_state, None)
 
     def is_on(self):
         """Return true if the service should be on."""

=== modified file 'src/provisioningserver/tests/test_service_monitor.py'
--- src/provisioningserver/tests/test_service_monitor.py	2016-03-25 14:52:23 +0000
+++ src/provisioningserver/tests/test_service_monitor.py	2016-03-31 16:08:17 +0000
@@ -36,7 +36,7 @@
     def test_get_expected_state_returns_from_expected_state(self):
         service = self.make_dhcp_service()
         service.expected_state = sentinel.state
-        self.assertEqual(sentinel.state, service.get_expected_state())
+        self.assertEqual((sentinel.state, None), service.get_expected_state())
 
     def test_is_on_returns_True_when_expected_state_on(self):
         service = self.make_dhcp_service()
@@ -95,7 +95,7 @@
 
     def test_get_expected_state(self):
         tgt = TGTService()
-        self.assertEqual(SERVICE_STATE.ON, tgt.get_expected_state())
+        self.assertEqual((SERVICE_STATE.ON, None), tgt.get_expected_state())
 
 
 class TestGlobalServiceMonitor(MAASTestCase):

=== modified file 'src/provisioningserver/utils/service_monitor.py'
--- src/provisioningserver/utils/service_monitor.py	2016-03-25 14:52:23 +0000
+++ src/provisioningserver/utils/service_monitor.py	2016-03-31 16:08:17 +0000
@@ -71,28 +71,26 @@
     @inlineCallbacks
     def get_status_and_status_info_for(self, service):
         """Return the status and status_info for the state of `service`."""
+        status = "off"
+        status_info = ""
+        expected_state, service_status_info = yield maybeDeferred(
+            service.get_expected_state)
         if self.active_state == SERVICE_STATE.UNKNOWN:
             status = "unknown"
-            status_info = ""
         elif self.active_state == SERVICE_STATE.ON:
             status = "running"
-            status_info = ""
-        else:
-            expected_state = yield maybeDeferred(
-                service.get_expected_state)
-            if expected_state == SERVICE_STATE.ON:
-                status = "dead"
-                if self.active_state == SERVICE_STATE.OFF:
-                    status_info = "%s is currently stopped." % (
-                        service.service_name)
-                else:
-                    status_info = (
-                        "%s failed to start, process result: (%s)" % (
-                            service.service_name, self.process_state))
+        elif expected_state == SERVICE_STATE.ON:
+            status = "dead"
+            if self.active_state == SERVICE_STATE.OFF:
+                status_info = "%s is currently stopped." % (
+                    service.service_name)
             else:
-                status = "off"
-                status_info = ""
-        returnValue((status, status_info))
+                status_info = (
+                    "%s failed to start, process result: (%s)" % (
+                        service.service_name, self.process_state))
+        returnValue((
+            status,
+            service_status_info if service_status_info else status_info))
 
 
 class Service(metaclass=ABCMeta):
@@ -108,15 +106,15 @@
 
     @abstractmethod
     def get_expected_state(self):
-        """Return a the expected state for the service."""
+        """Returns (expected state, status_info) for the service."""
 
 
 class AlwaysOnService(Service):
     """Service that should always be on."""
 
     def get_expected_state(self):
-        """Service is should always on."""
-        return SERVICE_STATE.ON
+        """AlwaysOnService should always be on."""
+        return (SERVICE_STATE.ON, None)
 
 
 class ServiceUnknownError(Exception):
@@ -252,7 +250,7 @@
         services expected state is not ON.
         """
         service = self.getServiceByName(name)
-        expected_state = yield maybeDeferred(service.get_expected_state)
+        expected_state, _ = yield maybeDeferred(service.get_expected_state)
         if expected_state != SERVICE_STATE.ON:
             raise ServiceNotOnError(
                 "Service '%s' is not expected to be on, unable to restart." % (
@@ -374,7 +372,7 @@
         reach its expected process state based on the service's current
         active state.
         """
-        expected_state = yield maybeDeferred(service.get_expected_state)
+        expected_state, _ = yield maybeDeferred(service.get_expected_state)
         state = yield self.getServiceState(service.name, now=True)
         if state.active_state == expected_state:
             expected_process_state = (

=== modified file 'src/provisioningserver/utils/tests/test_service_monitor.py'
--- src/provisioningserver/utils/tests/test_service_monitor.py	2016-03-25 14:52:23 +0000
+++ src/provisioningserver/utils/tests/test_service_monitor.py	2016-03-31 16:08:17 +0000
@@ -46,7 +46,7 @@
 from twisted.internet.threads import deferToThread
 
 
-def make_fake_service(expected_state=None):
+def make_fake_service(expected_state=None, status_info=None):
     fake_name = factory.make_name("name")
     fake_service_name = factory.make_name("service")
     if expected_state is None:
@@ -60,7 +60,7 @@
         service_name = fake_service_name
 
         def get_expected_state(self):
-            return succeed(expected_state)
+            return succeed((expected_state, status_info))
 
     return FakeService()
 
@@ -117,6 +117,18 @@
         self.assertEquals(
             ("off", ""), observed_status)
 
+    @inlineCallbacks
+    def test_get_status_and_status_info_for_returns_service_service_info(self):
+        # Make sure any service_info given by a service gets passed through.
+        status_info = factory.make_string(60, True)
+        active_state = factory.pick_enum(SERVICE_STATE)
+        service = make_fake_service(active_state, status_info)
+        state = ServiceState(active_state, None)
+        observed_status = yield state.get_status_and_status_info_for(
+            service)
+        # Only check status_info - tests above have tested state.
+        self.assertEquals(status_info, observed_status[1])
+
 
 class TestServiceMonitor(MAASTestCase):
     """Tests for `ServiceMonitor`."""