summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--ironic_python_agent/config.py15
-rw-r--r--ironic_python_agent/extensions/standby.py28
-rw-r--r--ironic_python_agent/tests/unit/extensions/test_standby.py82
-rw-r--r--ironic_python_agent/tests/unit/test_utils.py105
-rw-r--r--ironic_python_agent/utils.py44
-rw-r--r--releasenotes/notes/image-download-retries-65ac31fe4328e438.yaml11
-rw-r--r--releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml5
7 files changed, 215 insertions, 75 deletions
diff --git a/ironic_python_agent/config.py b/ironic_python_agent/config.py
index 9122fb75..9c3ade97 100644
--- a/ironic_python_agent/config.py
+++ b/ironic_python_agent/config.py
@@ -229,6 +229,21 @@ cli_opts = [
'the bare metal introspection service when the '
'``ironic-collect-introspection-data`` program is '
'executing in daemon mode.'),
+ cfg.IntOpt('image_download_connection_timeout', min=1,
+ default=APARAMS.get(
+ 'ipa-image-download-connection-timeout', 60),
+ help='The connection timeout (in seconds) when downloading '
+ 'an image. Does not affect the whole download.'),
+ cfg.IntOpt('image_download_connection_retries', min=0,
+ default=APARAMS.get('ipa-image-download-connection-retries', 2),
+ help='How many times to retry the connection when downloading '
+ 'an image. Also retries on failure HTTP statuses.'),
+ cfg.IntOpt('image_download_connection_retry_interval', min=0,
+ default=APARAMS.get(
+ 'ipa-image-download-connection-retry-interval', 5),
+ help='Interval (in seconds) between two attempts to establish '
+ 'connection when downloading an image.'),
+
]
CONF.register_cli_opts(cli_opts)
diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py
index e252aaed..1add183d 100644
--- a/ironic_python_agent/extensions/standby.py
+++ b/ironic_python_agent/extensions/standby.py
@@ -71,12 +71,28 @@ def _download_with_proxy(image_info, url, image_id):
os.environ['no_proxy'] = no_proxy
proxies = image_info.get('proxies', {})
verify, cert = utils.get_ssl_client_options(CONF)
- resp = requests.get(url, stream=True, proxies=proxies,
- verify=verify, cert=cert)
- if resp.status_code != 200:
- msg = ('Received status code {} from {}, expected 200. Response '
- 'body: {}').format(resp.status_code, url, resp.text)
- raise errors.ImageDownloadError(image_id, msg)
+ resp = None
+ for attempt in range(CONF.image_download_connection_retries + 1):
+ try:
+ resp = requests.get(url, stream=True, proxies=proxies,
+ verify=verify, cert=cert,
+ timeout=CONF.image_download_connection_timeout)
+ if resp.status_code != 200:
+ msg = ('Received status code {} from {}, expected 200. '
+ 'Response body: {}').format(resp.status_code, url,
+ resp.text)
+ raise errors.ImageDownloadError(image_id, msg)
+ except (errors.ImageDownloadError, requests.RequestException) as e:
+ if (attempt == CONF.image_download_connection_retries
+ # NOTE(dtantsur): do not retry 4xx status codes
+ or (resp and resp.status_code < 500)):
+ raise
+ else:
+ LOG.warning('Unable to connect to %s, retrying. Error: %s',
+ url, e)
+ time.sleep(CONF.image_download_connection_retry_interval)
+ else:
+ break
return resp
diff --git a/ironic_python_agent/tests/unit/extensions/test_standby.py b/ironic_python_agent/tests/unit/extensions/test_standby.py
index 9682aef5..e9bd9c13 100644
--- a/ironic_python_agent/tests/unit/extensions/test_standby.py
+++ b/ironic_python_agent/tests/unit/extensions/test_standby.py
@@ -17,6 +17,7 @@ import tempfile
import mock
from oslo_concurrency import processutils
+import requests
from ironic_python_agent import errors
from ironic_python_agent.extensions import standby
@@ -356,7 +357,8 @@ class TestStandbyExtension(base.IronicAgentTest):
standby._download_image(image_info)
requests_mock.assert_called_once_with(image_info['urls'][0],
cert=None, verify=True,
- stream=True, proxies={})
+ stream=True, proxies={},
+ timeout=60)
write = file_mock.write
write.assert_any_call('some')
write.assert_any_call('content')
@@ -387,7 +389,8 @@ class TestStandbyExtension(base.IronicAgentTest):
self.assertEqual(no_proxy, os.environ['no_proxy'])
requests_mock.assert_called_once_with(image_info['urls'][0],
cert=None, verify=True,
- stream=True, proxies=proxies)
+ stream=True, proxies=proxies,
+ timeout=60)
write = file_mock.write
write.assert_any_call('some')
write.assert_any_call('content')
@@ -1065,7 +1068,8 @@ class TestStandbyExtension(base.IronicAgentTest):
'/dev/foo')
requests_mock.assert_called_once_with(image_info['urls'][0],
cert=None, verify=True,
- stream=True, proxies={})
+ stream=True, proxies={},
+ timeout=60)
expected_calls = [mock.call('some'), mock.call('content')]
file_mock.write.assert_has_calls(expected_calls)
@@ -1089,7 +1093,8 @@ class TestStandbyExtension(base.IronicAgentTest):
image_info, '/dev/foo')
requests_mock.assert_called_once_with(image_info['urls'][0],
cert=None, verify=True,
- stream=True, proxies={})
+ stream=True, proxies={},
+ timeout=60)
# Assert write was only called once and failed!
file_mock.write.assert_called_once_with('some')
@@ -1180,11 +1185,13 @@ class TestImageDownload(base.IronicAgentTest):
self.assertEqual(content, list(image_download))
requests_mock.assert_called_once_with(image_info['urls'][0],
cert=None, verify=True,
- stream=True, proxies={})
+ stream=True, proxies={},
+ timeout=60)
self.assertEqual(image_info['checksum'],
image_download._hash_algo.hexdigest())
- def test_download_image_fail(self, requests_mock, time_mock):
+ @mock.patch('time.sleep', autospec=True)
+ def test_download_image_fail(self, sleep_mock, requests_mock, time_mock):
response = requests_mock.return_value
response.status_code = 401
response.text = 'Unauthorized'
@@ -1198,7 +1205,56 @@ class TestImageDownload(base.IronicAgentTest):
standby.ImageDownload, image_info)
requests_mock.assert_called_once_with(image_info['urls'][0],
cert=None, verify=True,
- stream=True, proxies={})
+ stream=True, proxies={},
+ timeout=60)
+ self.assertFalse(sleep_mock.called)
+
+ @mock.patch('time.sleep', autospec=True)
+ def test_download_image_retries(self, sleep_mock, requests_mock,
+ time_mock):
+ response = requests_mock.return_value
+ response.status_code = 500
+ response.text = 'Oops'
+ time_mock.return_value = 0.0
+ image_info = _build_fake_image_info()
+ msg = ('Error downloading image: Download of image fake_id failed: '
+ 'URL: http://example.org; time: .* seconds. Error: '
+ 'Received status code 500 from http://example.org, expected '
+ '200. Response body: Oops')
+ self.assertRaisesRegex(errors.ImageDownloadError, msg,
+ standby.ImageDownload, image_info)
+ requests_mock.assert_called_with(image_info['urls'][0],
+ cert=None, verify=True,
+ stream=True, proxies={},
+ timeout=60)
+ self.assertEqual(3, requests_mock.call_count)
+ sleep_mock.assert_called_with(5)
+ self.assertEqual(2, sleep_mock.call_count)
+
+ @mock.patch('time.sleep', autospec=True)
+ def test_download_image_retries_success(self, sleep_mock, requests_mock,
+ md5_mock):
+ content = ['SpongeBob', 'SquarePants']
+ fail_response = mock.Mock()
+ fail_response.status_code = 500
+ fail_response.text = " "
+ response = mock.Mock()
+ response.status_code = 200
+ response.iter_content.return_value = content
+ requests_mock.side_effect = [requests.Timeout, fail_response, response]
+
+ image_info = _build_fake_image_info()
+ md5_mock.return_value.hexdigest.return_value = image_info['checksum']
+ image_download = standby.ImageDownload(image_info)
+
+ self.assertEqual(content, list(image_download))
+ requests_mock.assert_called_with(image_info['urls'][0],
+ cert=None, verify=True,
+ stream=True, proxies={},
+ timeout=60)
+ self.assertEqual(3, requests_mock.call_count)
+ sleep_mock.assert_called_with(5)
+ self.assertEqual(2, sleep_mock.call_count)
def test_download_image_and_checksum(self, requests_mock, md5_mock):
content = ['SpongeBob', 'SquarePants']
@@ -1219,9 +1275,9 @@ class TestImageDownload(base.IronicAgentTest):
self.assertEqual(content, list(image_download))
requests_mock.assert_has_calls([
mock.call('http://example.com/checksum', cert=None, verify=True,
- stream=True, proxies={}),
+ stream=True, proxies={}, timeout=60),
mock.call(image_info['urls'][0], cert=None, verify=True,
- stream=True, proxies={}),
+ stream=True, proxies={}, timeout=60),
])
self.assertEqual(fake_cs, image_download._hash_algo.hexdigest())
@@ -1249,9 +1305,9 @@ foobar irrelevant file.img
self.assertEqual(content, list(image_download))
requests_mock.assert_has_calls([
mock.call('http://example.com/checksum', cert=None, verify=True,
- stream=True, proxies={}),
+ stream=True, proxies={}, timeout=60),
mock.call(image_info['urls'][0], cert=None, verify=True,
- stream=True, proxies={}),
+ stream=True, proxies={}, timeout=60),
])
self.assertEqual(fake_cs, image_download._hash_algo.hexdigest())
@@ -1304,7 +1360,9 @@ foobar irrelevant file.img
response = mock.Mock()
response.status_code = 200
response.iter_content.return_value = content
- requests_mock.side_effect = [cs_response, response]
+ # 3 retries on status code
+ requests_mock.side_effect = [cs_response, cs_response, cs_response,
+ response]
image_info = _build_fake_image_info(
'http://example.com/path/image.img')
diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py
index 07192033..08d5f1a0 100644
--- a/ironic_python_agent/tests/unit/test_utils.py
+++ b/ironic_python_agent/tests/unit/test_utils.py
@@ -22,6 +22,7 @@ import subprocess
import tarfile
import tempfile
+from ironic_lib import disk_utils
from ironic_lib import utils as ironic_utils
import mock
from oslo_concurrency import processutils
@@ -46,30 +47,6 @@ Number Start End Size File system Name Flags
1 116MB 2361MB 2245MB ext4
'''
-PARTED_OUTPUT_UNFORMATTED_NOFS = '''Model: whatever
-Disk /dev/sda: 480GB
-Sector size (logical/physical): 512B/512B
-Partition Table: gpt
-Disk Flags:
-
-Number Start End Size File system Name Flags
-1 1049kB 9437kB 8389kB ESP boot, esp
-2 9437kB 17.8MB 8389kB BSP bios_grub
-3 17.8MB 40.0GB 40.0GB
-4 479GB 480GB 68.1MB
-'''
-
-PARTED_OUTPUT_NO_EFI = '''Model: whatever
-Disk /dev/sda: 450GB
-Sector size (logical/physical): 512B/512B
-Partition Table: gpt
-Disk Flags:
-
-Number Start End Size File system Name Flags
-14 1049kB 5243kB 4194kB bios_grub
- 1 116MB 2361MB 2245MB ext4
-'''
-
class ExecuteTestCase(ironic_agent_base.IronicAgentTest):
# This test case does call utils.execute(), so don't block access to the
@@ -630,35 +607,73 @@ class TestUtils(testtools.TestCase):
)
@mock.patch.object(utils, 'execute', autospec=True)
- def test_get_efi_part_on_device(self, mocked_execute):
- parted_ret = PARTED_OUTPUT_UNFORMATTED.format('gpt')
+ def test_scan_partition_table_type_gpt(self, mocked_execute):
+ self._test_scan_partition_table_by_type(mocked_execute, 'gpt', 'gpt')
+
+ @mock.patch.object(utils, 'execute', autospec=True)
+ def test_scan_partition_table_type_msdos(self, mocked_execute):
+ self._test_scan_partition_table_by_type(mocked_execute, 'msdos',
+ 'msdos')
+
+ @mock.patch.object(utils, 'execute', autospec=True)
+ def test_scan_partition_table_type_unknown(self, mocked_execute):
+ self._test_scan_partition_table_by_type(mocked_execute, 'whatever',
+ 'unknown')
+
+ def _test_scan_partition_table_by_type(self, mocked_execute,
+ table_type_output,
+ expected_table_type):
+
+ parted_ret = PARTED_OUTPUT_UNFORMATTED.format(table_type_output)
+
mocked_execute.side_effect = [
- (parted_ret, None)
+ (parted_ret, None),
]
- ret = utils.get_efi_part_on_device('/dev/sda')
+
+ ret = utils.scan_partition_table_type('hello')
mocked_execute.assert_has_calls(
- [mock.call('parted', '-s', '/dev/sda', '--', 'print')]
+ [mock.call('parted', '-s', 'hello', '--', 'print')]
)
+ self.assertEqual(expected_table_type, ret)
+
+
+@mock.patch.object(disk_utils, 'list_partitions', autospec=True)
+@mock.patch.object(utils, 'scan_partition_table_type', autospec=True)
+class TestGetEfiPart(testtools.TestCase):
+
+ def test_get_efi_part_on_device(self, mocked_type, mocked_parts):
+ mocked_parts.return_value = [
+ {'number': '1', 'flags': ''},
+ {'number': '14', 'flags': 'bios_grub'},
+ {'number': '15', 'flags': 'esp, boot'},
+ ]
+ ret = utils.get_efi_part_on_device('/dev/sda')
self.assertEqual('15', ret)
- @mock.patch.object(utils, 'execute', autospec=True)
- def test_get_efi_part_on_device_without_fs(self, mocked_execute):
- parted_ret = PARTED_OUTPUT_UNFORMATTED_NOFS.format('gpt')
- mocked_execute.side_effect = [
- (parted_ret, None)
+ def test_get_efi_part_on_device_only_boot_flag_gpt(self, mocked_type,
+ mocked_parts):
+ mocked_type.return_value = 'gpt'
+ mocked_parts.return_value = [
+ {'number': '1', 'flags': ''},
+ {'number': '14', 'flags': 'bios_grub'},
+ {'number': '15', 'flags': 'boot'},
]
ret = utils.get_efi_part_on_device('/dev/sda')
- mocked_execute.assert_has_calls(
- [mock.call('parted', '-s', '/dev/sda', '--', 'print')]
- )
- self.assertEqual('1', ret)
+ self.assertEqual('15', ret)
- @mock.patch.object(utils, 'execute', autospec=True)
- def test_get_efi_part_on_device_not_found(self, mocked_execute):
- mocked_execute.side_effect = [
- (PARTED_OUTPUT_NO_EFI, None)
+ def test_get_efi_part_on_device_only_boot_flag_mbr(self, mocked_type,
+ mocked_parts):
+ mocked_type.return_value = 'msdos'
+ mocked_parts.return_value = [
+ {'number': '1', 'flags': ''},
+ {'number': '14', 'flags': 'bios_grub'},
+ {'number': '15', 'flags': 'boot'},
+ ]
+ self.assertIsNone(utils.get_efi_part_on_device('/dev/sda'))
+
+ def test_get_efi_part_on_device_not_found(self, mocked_type, mocked_parts):
+ mocked_parts.return_value = [
+ {'number': '1', 'flags': ''},
+ {'number': '14', 'flags': 'bios_grub'},
]
self.assertIsNone(utils.get_efi_part_on_device('/dev/sda'))
- mocked_execute.assert_has_calls(
- [mock.call('parted', '-s', '/dev/sda', '--', 'print')]
- )
diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py
index 7c4a2468..cb29fcd0 100644
--- a/ironic_python_agent/utils.py
+++ b/ironic_python_agent/utils.py
@@ -24,6 +24,7 @@ import tarfile
import tempfile
import time
+from ironic_lib import disk_utils
from ironic_lib import utils as ironic_utils
from oslo_concurrency import processutils
from oslo_log import log as logging
@@ -63,7 +64,8 @@ COLLECT_LOGS_COMMANDS = {
DEVICE_EXTRACTOR = re.compile(r'^(?:(.*\d)p|(.*\D))(?:\d+)$')
-PARTED_ESP_PATTERN = re.compile(r'^\s*(\d+)\s.*\s\s.*\s.*esp(,|\s|$).*$')
+PARTED_TABLE_TYPE_REGEX = re.compile(r'^.*partition\s+table\s*:\s*(gpt|msdos)',
+ re.IGNORECASE)
def execute(*cmd, **kwargs):
@@ -457,21 +459,39 @@ def extract_device(part):
return (m.group(1) or m.group(2))
-def get_efi_part_on_device(device):
- """Looks for the efi partition on a given device
+def scan_partition_table_type(device):
+ """Get partition table type, msdos or gpt.
- :param device: lock device upon which to check for the efi partition
- :return: the efi partition or None
+ :param device_name: the name of the device
+ :return: msdos, gpt or unknown
"""
- efi_part = None
out, _u = execute('parted', '-s', device, '--', 'print')
- for line in out.splitlines():
- m = PARTED_ESP_PATTERN.match(line)
+ out = out.splitlines()
+
+ for line in out:
+ m = PARTED_TABLE_TYPE_REGEX.match(line)
if m:
- efi_part = m.group(1)
+ return m.group(1)
+
+ LOG.warning("Unable to get partition table type for device %s.",
+ device)
+
+ return 'unknown'
+
- LOG.debug("Found efi partition %s on device %s.", efi_part, device)
- break
+def get_efi_part_on_device(device):
+ """Looks for the efi partition on a given device.
+
+ A boot partition on a GPT disk is assumed to be an EFI partition as well.
+
+ :param device: lock device upon which to check for the efi partition
+ :return: the efi partition or None
+ """
+ is_gpt = scan_partition_table_type(device) == 'gpt'
+ for part in disk_utils.list_partitions(device):
+ flags = {x.strip() for x in part['flags'].split(',')}
+ if 'esp' in flags or ('boot' in flags and is_gpt):
+ LOG.debug("Found EFI partition %s on device %s.", part, device)
+ return part['number']
else:
LOG.debug("No efi partition found on device %s", device)
- return efi_part
diff --git a/releasenotes/notes/image-download-retries-65ac31fe4328e438.yaml b/releasenotes/notes/image-download-retries-65ac31fe4328e438.yaml
new file mode 100644
index 00000000..7f787a56
--- /dev/null
+++ b/releasenotes/notes/image-download-retries-65ac31fe4328e438.yaml
@@ -0,0 +1,11 @@
+---
+fixes:
+ - |
+ Provides timeout and retries when establishing a connection to download
+ an image in the ``standby`` extension. Reduces probability of an image
+ download getting stuck in the event of network problems.
+
+ The default timeout is 60 seconds and can be set via the
+ ``ipa-image-download-connection-timeout`` kernel parameter. The default
+ number of retries is 2 and can be set via the
+ ``ipa-image-download-connection-retries`` parameter.
diff --git a/releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml b/releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml
new file mode 100644
index 00000000..8701019e
--- /dev/null
+++ b/releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+ - |
+ No longer tries to use GRUB2 for configuring boot for whole disk images
+ with an EFI partition present but only marked as ``boot`` (not ``esp``).
View Lorry Controller Status