Add timeout to image operations in the direct deploy

Currently they may hang when the remote server is not responding.

Change-Id: I1de17fed3b43a3d16795dc614ce76e2cfe1faca0

ironic/common/image_service.py

@@ -98,7 +98,8 @@ class HttpImageService(BaseImageService):
             verify = CONF.webserver_verify_ca
 
         try:
-            response = requests.head(image_href, verify=verify)
+            response = requests.head(image_href, verify=verify,
+                                     timeout=CONF.webserver_connection_timeout)
             if response.status_code != http_client.OK:
                 raise exception.ImageRefValidationFailed(
                     image_href=output_url,
@@ -130,8 +131,8 @@ class HttpImageService(BaseImageService):
             verify = CONF.webserver_verify_ca
 
         try:
-            response = requests.get(image_href, stream=True,
+            response = requests.get(image_href, stream=True, verify=verify,
-                                    verify=verify)
+                                    timeout=CONF.webserver_connection_timeout)
             if response.status_code != http_client.OK:
                 raise exception.ImageRefValidationFailed(
                     image_href=image_href,

ironic/conf/default.py

@@ -362,7 +362,7 @@ utils_opts = [
                       'dir.')),
 ]
 
-cert_verify_opts = [
+webserver_opts = [
     cfg.StrOpt('webserver_verify_ca',
                default='True',
                mutable=True,
@@ -380,6 +380,10 @@ cert_verify_opts = [
                       'is set to True i.e the certificates present in the '
                       'standard path are used for SSL verification.'
                       'Defaults to True.')),
+    cfg.IntOpt('webserver_connection_timeout',
+               default=60,
+               help=_('Connection timeout when accessing remote web servers '
+                      'with images.')),
 ]
 
 
@@ -396,4 +400,4 @@ def register_opts(conf):
     conf.register_opts(portgroup_opts)
     conf.register_opts(service_opts)
     conf.register_opts(utils_opts)
-    conf.register_opts(cert_verify_opts)
+    conf.register_opts(webserver_opts)

ironic/tests/unit/common/test_image_service.py

@@ -43,7 +43,8 @@ class HttpImageServiceTestCase(base.TestCase):
         response.status_code = http_client.OK
         self.service.validate_href(self.href)
         path_mock.assert_not_called()
-        head_mock.assert_called_once_with(self.href, verify=True)
+        head_mock.assert_called_once_with(self.href, verify=True,
+                                          timeout=60)
         response.status_code = http_client.NO_CONTENT
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href,
@@ -60,7 +61,8 @@ class HttpImageServiceTestCase(base.TestCase):
         response = head_mock.return_value
         response.status_code = http_client.OK
         self.service.validate_href(self.href)
-        head_mock.assert_called_once_with(self.href, verify=False)
+        head_mock.assert_called_once_with(self.href, verify=False,
+                                          timeout=60)
         response.status_code = http_client.NO_CONTENT
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href,
@@ -76,7 +78,8 @@ class HttpImageServiceTestCase(base.TestCase):
         head_mock.side_effect = requests.ConnectionError()
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
-        head_mock.assert_called_once_with(self.href, verify=False)
+        head_mock.assert_called_once_with(self.href, verify=False,
+                                          timeout=60)
         head_mock.side_effect = requests.RequestException()
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
@@ -88,7 +91,8 @@ class HttpImageServiceTestCase(base.TestCase):
         response = head_mock.return_value
         response.status_code = http_client.OK
         self.service.validate_href(self.href)
-        head_mock.assert_called_once_with(self.href, verify=True)
+        head_mock.assert_called_once_with(self.href, verify=True,
+                                          timeout=60)
         response.status_code = http_client.NO_CONTENT
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href,
@@ -105,7 +109,8 @@ class HttpImageServiceTestCase(base.TestCase):
         head_mock.side_effect = requests.ConnectionError()
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
-        head_mock.assert_called_once_with(self.href, verify=True)
+        head_mock.assert_called_once_with(self.href, verify=True,
+                                          timeout=60)
         head_mock.side_effect = requests.RequestException()
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
@@ -118,7 +123,26 @@ class HttpImageServiceTestCase(base.TestCase):
         response.status_code = http_client.OK
 
         self.service.validate_href(self.href)
-        head_mock.assert_called_once_with(self.href, verify='/some/path')
+        head_mock.assert_called_once_with(self.href, verify='/some/path',
+                                          timeout=60)
+        response.status_code = http_client.NO_CONTENT
+        self.assertRaises(exception.ImageRefValidationFailed,
+                          self.service.validate_href,
+                          self.href)
+        response.status_code = http_client.BAD_REQUEST
+        self.assertRaises(exception.ImageRefValidationFailed,
+                          self.service.validate_href,
+                          self.href)
+
+    @mock.patch.object(requests, 'head', autospec=True)
+    def test_validate_href_custom_timeout(self, head_mock):
+        cfg.CONF.set_override('webserver_connection_timeout', 15)
+
+        response = head_mock.return_value
+        response.status_code = http_client.OK
+        self.service.validate_href(self.href)
+        head_mock.assert_called_once_with(self.href, verify=True,
+                                          timeout=15)
         response.status_code = http_client.NO_CONTENT
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href,
@@ -137,7 +161,8 @@ class HttpImageServiceTestCase(base.TestCase):
 
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
-        head_mock.assert_called_once_with(self.href, verify='/some/path')
+        head_mock.assert_called_once_with(self.href, verify='/some/path',
+                                          timeout=60)
 
     @mock.patch.object(requests, 'head', autospec=True)
     def test_validate_href_verify_error(self, head_mock):
@@ -145,7 +170,8 @@ class HttpImageServiceTestCase(base.TestCase):
         head_mock.side_effect = requests.RequestException()
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
-        head_mock.assert_called_once_with(self.href, verify='/some/path')
+        head_mock.assert_called_once_with(self.href, verify='/some/path',
+                                          timeout=60)
 
     @mock.patch.object(requests, 'head', autospec=True)
     def test_validate_href_verify_os_error(self, head_mock):
@@ -153,7 +179,8 @@ class HttpImageServiceTestCase(base.TestCase):
         head_mock.side_effect = OSError()
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.validate_href, self.href)
-        head_mock.assert_called_once_with(self.href, verify='/some/path')
+        head_mock.assert_called_once_with(self.href, verify='/some/path',
+                                          timeout=60)
 
     @mock.patch.object(requests, 'head', autospec=True)
     def test_validate_href_error_with_secret_parameter(self, head_mock):
@@ -165,7 +192,8 @@ class HttpImageServiceTestCase(base.TestCase):
                               True)
         self.assertIn('secreturl', str(e))
         self.assertNotIn(self.href, str(e))
-        head_mock.assert_called_once_with(self.href, verify=False)
+        head_mock.assert_called_once_with(self.href, verify=False,
+                                          timeout=60)
 
     @mock.patch.object(requests, 'head', autospec=True)
     def _test_show(self, head_mock, mtime, mtime_date):
@@ -175,7 +203,8 @@ class HttpImageServiceTestCase(base.TestCase):
             'Last-Modified': mtime
         }
         result = self.service.show(self.href)
-        head_mock.assert_called_once_with(self.href, verify=True)
+        head_mock.assert_called_once_with(self.href, verify=True,
+                                          timeout=60)
         self.assertEqual({'size': 100, 'updated_at': mtime_date,
                           'properties': {}}, result)
 
@@ -197,7 +226,8 @@ class HttpImageServiceTestCase(base.TestCase):
         head_mock.return_value.headers = {}
         self.assertRaises(exception.ImageRefValidationFailed,
                           self.service.show, self.href)
-        head_mock.assert_called_with(self.href, verify=True)
+        head_mock.assert_called_with(self.href, verify=True,
+                                     timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -213,7 +243,8 @@ class HttpImageServiceTestCase(base.TestCase):
             image_service.IMAGE_CHUNK_SIZE
         )
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify=True)
+                                             verify=True,
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -230,7 +261,8 @@ class HttpImageServiceTestCase(base.TestCase):
             image_service.IMAGE_CHUNK_SIZE
         )
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify=False)
+                                             verify=False,
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -247,7 +279,8 @@ class HttpImageServiceTestCase(base.TestCase):
             image_service.IMAGE_CHUNK_SIZE
         )
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify=True)
+                                             verify=True,
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -264,7 +297,8 @@ class HttpImageServiceTestCase(base.TestCase):
             image_service.IMAGE_CHUNK_SIZE
         )
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify='/some/path')
+                                             verify='/some/path',
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -289,7 +323,8 @@ class HttpImageServiceTestCase(base.TestCase):
         self.assertRaises(exception.ImageDownloadFailed,
                           self.service.download, self.href, file_mock)
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify=False)
+                                             verify=False,
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -305,7 +340,8 @@ class HttpImageServiceTestCase(base.TestCase):
         self.assertRaises(exception.ImageDownloadFailed,
                           self.service.download, self.href, file_mock)
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify='/some/path')
+                                             verify='/some/path',
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -320,7 +356,8 @@ class HttpImageServiceTestCase(base.TestCase):
         self.assertRaises(exception.ImageDownloadFailed,
                           self.service.download, self.href, file_mock)
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify='/some/path')
+                                             verify='/some/path',
+                                             timeout=60)
 
     @mock.patch.object(shutil, 'copyfileobj', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
@@ -335,7 +372,26 @@ class HttpImageServiceTestCase(base.TestCase):
         self.assertRaises(exception.ImageDownloadFailed,
                           self.service.download, self.href, file_mock)
         req_get_mock.assert_called_once_with(self.href, stream=True,
-                                             verify='/some/path')
+                                             verify='/some/path',
+                                             timeout=60)
+
+    @mock.patch.object(shutil, 'copyfileobj', autospec=True)
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_download_success_custom_timeout(
+            self, req_get_mock, shutil_mock):
+        cfg.CONF.set_override('webserver_connection_timeout', 15)
+        response_mock = req_get_mock.return_value
+        response_mock.status_code = http_client.OK
+        response_mock.raw = mock.MagicMock(spec=io.BytesIO)
+        file_mock = mock.Mock(spec=io.BytesIO)
+        self.service.download(self.href, file_mock)
+        shutil_mock.assert_called_once_with(
+            response_mock.raw.__enter__(), file_mock,
+            image_service.IMAGE_CHUNK_SIZE
+        )
+        req_get_mock.assert_called_once_with(self.href, stream=True,
+                                             verify=True,
+                                             timeout=15)
 
 
 class FileImageServiceTestCase(base.TestCase):

releasenotes/notes/webserver-timeout-d85781bf634cef39.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Adds timeout to HTTP image validation and downloading operations, so that
+    the ``direct`` deploy does not hang when the remote server is not
+    responsive. The default timeout is 60 seconds and can be changed via the
+    new ``webserver_connection_timeout`` option.