Merge branch 'master' into bump-version

Author: Matthew Fisher

Makefile

@@ -28,7 +28,7 @@ build: docker-build
 
 docker-build: check-docker
 	docker build --rm -t ${IMAGE} rootfs
-	docker tag -f ${IMAGE} ${MUTABLE_IMAGE}
+	docker tag ${IMAGE} ${MUTABLE_IMAGE}
 
 deploy: docker-build docker-push
 	sed 's#\(image:\) .*#\1 $(IMAGE)#' /tmp/deis-$(COMPONENT) | kubectl apply --validate=true -f -

rootfs/api/exceptions.py

@@ -18,6 +18,10 @@ class AlreadyExists(APIException):
     status_code = 409
 
 
+class UnprocessableEntity(APIException):
+    status_code = 422
+
+
 class ServiceUnavailable(APIException):
     status_code = 503
     default_detail = 'Service temporarily unavailable, try again later.'

rootfs/api/models/config.py

@@ -3,7 +3,8 @@
 from jsonfield import JSONField
 
 from api.models.release import Release
-from api.models import UuidAuditedModel, DeisException
+from api.models import UuidAuditedModel
+from api.exceptions import DeisException, UnprocessableEntity
 
 
 class Config(UuidAuditedModel):
@@ -130,9 +131,16 @@ def save(self, **kwargs):
                 data = getattr(previous_config, attr, {}).copy()
                 new_data = getattr(self, attr, {}).copy()
 
-                data.update(new_data)
-                # remove config keys if we provided a null value
-                [data.pop(k) for k, v in new_data.items() if v is None]
+                # remove config keys if a null value is provided
+                for key, value in new_data.items():
+                    if value is None:
+                        # error if unsetting non-existing key
+                        if key not in data:
+                            raise UnprocessableEntity('{} does not exist under {}'.format(key, attr))  # noqa
+
+                        data.pop(key)
+                    else:
+                        data[key] = value
                 setattr(self, attr, data)
 
             self.set_healthchecks()

rootfs/api/models/release.py

@@ -5,7 +5,8 @@
 
 from registry import publish_release, get_port as docker_get_port, RegistryException
 from api.utils import dict_diff
-from api.models import UuidAuditedModel, DeisException
+from api.models import UuidAuditedModel
+from api.exceptions import DeisException, AlreadyExists
 from scheduler import KubeHTTPException
 
 logger = logging.getLogger(__name__)
@@ -423,5 +424,7 @@ def save(self, *args, **kwargs):  # noqa
                 if self.version == 1:
                     self.summary = "{} created the initial release".format(self.owner)
                 else:
-                    self.summary = "{} changed nothing".format(self.owner)
+                    # There were no changes to this release
+                    raise AlreadyExists("{} changed nothing - release stopped".format(self.owner))
+
         super(Release, self).save(*args, **kwargs)

rootfs/api/tests/test_release.py

@@ -333,3 +333,43 @@ def json(self):
                 body = {'version': 2}
                 response = self.client.post(url, body)
                 self.assertEqual(response.status_code, 400, response.data)
+
+    def test_release_unset_config(self, mock_requests):
+        """
+        Test that a release is created when an app is created, a config can be
+        set and then unset without causing a 409 (conflict)
+        """
+        url = '/v2/apps'
+        response = self.client.post(url)
+        self.assertEqual(response.status_code, 201, response.data)
+        app_id = response.data['id']
+
+        # check that updating config rolls a new release
+        url = '/v2/apps/{app_id}/config'.format(**locals())
+        body = {'cpu': json.dumps({'cmd': None})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 422, response.data)
+
+    def test_release_no_change(self, mock_requests):
+        """
+        Test that a release is created when an app is created, and
+        then has 2 identical config set, causing a 409 as there was
+        no change
+        """
+        url = '/v2/apps'
+        response = self.client.post(url)
+        self.assertEqual(response.status_code, 201, response.data)
+        app_id = response.data['id']
+
+        # check that updating config rolls a new release
+        url = '/v2/apps/{app_id}/config'.format(**locals())
+        body = {'values': json.dumps({'NEW_URL1': 'http://localhost:8080/'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+        self.assertIn('NEW_URL1', response.data['values'])
+
+        # trigger identical release
+        url = '/v2/apps/{app_id}/config'.format(**locals())
+        body = {'values': json.dumps({'NEW_URL1': 'http://localhost:8080/'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 409, response.data)

rootfs/scheduler/__init__.py

@@ -766,8 +766,6 @@ def _wait_until_pods_are_ready(self, namespace, container, labels, desired):  #
 
         logger.info("waiting for {} pods in {} namespace to be in services ({} timeout)".format(desired, namespace, timeout))  # noqa
 
-        # has timeout been increased or not within the loop
-        timeout_padded = False
         # Ensure the minimum desired number of pods are available
         while waited < timeout:
             count = 0  # ready pods
@@ -777,11 +775,7 @@ def _wait_until_pods_are_ready(self, namespace, container, labels, desired):  #
                 if pod['status']['phase'] == 'Pending':
                     reason, message = self._pod_pending_status(pod)
                     # If pulling an image is taking long then increase the timeout
-                    if not timeout_padded:
-                        pull = self._handle_pod_long_image_pulling(pod, reason)
-                        if pull:
-                            timeout_padded = True
-                            timeout += pull
+                    timeout += self._handle_pod_long_image_pulling(pod, reason)
 
                     # handle errors and bubble up if need be
                     self._handle_pod_image_errors(pod, reason, message)
@@ -1411,6 +1405,10 @@ def _handle_pod_long_image_pulling(self, reason, pod):
 
         Return value is an int that represents seconds
         """
+        # only apply once
+        if getattr(self, '_handle_pod_long_image_pulling_applied', False):
+            return 0
+
         if reason is not 'Pulling':
             return 0
 
@@ -1427,6 +1425,9 @@ def _handle_pod_long_image_pulling(self, reason, pod):
             # add 10 minutes to timeout to allow a pull image operation to finish
             logger.info('Kubernetes has been pulling the image for {} seconds'.format(seconds))  # noqa
             logger.info('Increasing timeout by 10 minutes to allow a pull image operation to finish for pods in namespace {}'.format(namespace))  # noqa
+
+            # make it so function doesn't do processing again
+            setattr(self, '_handle_pod_long_image_pulling_applied', True)
             return 600
 
         return 0