openstack
/
sushy
Code Issues Proposed changes

Merge "Improve resiliency of eTag handling"

This commit is contained in:
Zuul 2022-10-27 11:58:47 +00:00 committed by Gerrit Code Review
parent 1c0a35a8ec 3e69fce709
commit 5d6e5486fa
7 changed files with 287 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
releasenotes/notes/releasenote-d7138d1e1d414632.yaml Normal file
View File

@ -0,0 +1,10 @@
---
fixes:
- |
Alters eTag handling in PATCH requests: First, the original eTag is used.
In case of a failure, if the eTag provided was weak, it is converted to
a strong format by removing the weak prefix. If this approach is not
applicable or fails, the final attempt is made omitting the eTag entirely.
By taking this approach, no workarounds are applied if BMC is handling
eTags as expected and in case of failures, known workarounds are
attempted, improving overall resiliency.

95
sushy/connector.py
View File

@ -13,7 +13,9 @@
# License for the specific language governing permissions and limitations
# under the License.
from http import client as http_client
import logging
import re
import time
from urllib import parse as urlparse
@ -268,13 +270,14 @@ class Connector(object):
blocking=blocking, timeout=timeout,
**extra_session_req_kwargs)
def patch(self, path='', data=None, headers=None, blocking=False,
timeout=60, **extra_session_req_kwargs):
"""HTTP PATCH method.
def _etag_handler(self, path='', data=None, headers=None, etag=None,
blocking=False, timeout=60, **extra_session_req_kwargs):
"""eTag handler containing workarounds for PATCH requests with eTags.
:param path: Optional sub-URI path to the resource.
:param data: Optional JSON data.
:param headers: Optional dictionary of headers.
:param etag: eTag string.
:param blocking: Whether to block for asynchronous operations.
:param timeout: Max time in seconds to wait for blocking async call.
:param extra_session_req_kwargs: Optional keyword argument to pass
@ -284,9 +287,89 @@ class Connector(object):
:raises: ConnectionError
:raises: HTTPError
"""
return self._op('PATCH', path, data=data, headers=headers,
blocking=blocking, timeout=timeout,
**extra_session_req_kwargs)
if etag is not None:
if not headers:
headers = {}
headers['If-Match'] = etag
try:
response = self._op('PATCH', path, data=data,
headers=headers,
blocking=blocking, timeout=timeout,
**extra_session_req_kwargs)
except exceptions.HTTPError as resp:
LOG.warning("Initial request with eTag failed: %s",
resp)
if resp.status_code == http_client.PRECONDITION_FAILED:
# NOTE(janders) if there was no eTag provided AND the response
# is HTTP 412 Precondition Failed, raise the exception
if not etag:
raise
# checking for weak eTag
pattern = re.compile(r'^(W\/)("\w*")$')
match = pattern.match(etag)
if match:
LOG.info("Weak eTag provided with original request to "
"%s. Attempting to conversion to strong eTag "
"and re-trying.", path)
# trying weak eTag converted to strong
headers['If-Match'] = match.group(2)
try:
response = self._op('PATCH', path, data=data,
headers=headers,
blocking=blocking,
timeout=timeout,
**extra_session_req_kwargs)
except exceptions.HTTPError as resp:
if (resp.status_code == http_client.
PRECONDITION_FAILED):
LOG.warning("Request to %s with weak eTag "
"converted to strong eTag also "
"failed. Making the final attempt "
"with no eTag specified.", path)
response = None
if response:
return response
else:
# eTag is strong, if it failed the only other thing
# to try is removing it entirely
# info
LOG.warning("Strong eTag provided - retrying request to "
"%s with eTag removed.", path)
del headers['If-Match']
try:
response = self._op('PATCH', path, data=data,
headers=headers,
blocking=blocking, timeout=timeout,
**extra_session_req_kwargs)
except exceptions.HTTPError as resp:
LOG.error("Final re-try with eTag removed has failed, "
"raising exception %s", resp)
raise
else:
raise
return response
def patch(self, path='', data=None, headers=None, etag=None,
blocking=False, timeout=60, **extra_session_req_kwargs):
"""HTTP PATCH method.
:param path: Optional sub-URI path to the resource.
:param data: Optional JSON data.
:param headers: Optional dictionary of headers.
:param etag: Optional eTag string.
:param blocking: Whether to block for asynchronous operations.
:param timeout: Max time in seconds to wait for blocking async call.
:param extra_session_req_kwargs: Optional keyword argument to pass
requests library arguments which would pass on to requests session
object.
:returns: The response object from the requests library.
:raises: ConnectionError
:raises: HTTPError
"""
return self._etag_handler(path, data,
headers, etag,
blocking, timeout,
**extra_session_req_kwargs)
def put(self, path='', data=None, headers=None, blocking=False,
timeout=60, **extra_session_req_kwargs):

17
sushy/resources/base.py
View File

@ -20,7 +20,6 @@ import enum
import io
import json
import logging
import re
import zipfile
import pkg_resources
@ -638,21 +637,7 @@ class ResourceBase(object, metaclass=abc.ABCMeta):
:returns ETag or None
"""
etag = self._get_headers().get('ETag')
if not etag:
return None
# Note (arne_wiebalck): in case there is a weak Etag,
# return it with and without the qualifier to handle
# vendor implementations which may require one or the
# other (but should actually not use weak tags in the
# first place).
pattern = re.compile(r'^(W\/)("\w*")$')
match = pattern.match(etag)
if match:
return etag + ',' + match.group(2)
return etag
return self._get_headers().get('ETag')
def _get_headers(self):
"""Returns the HTTP headers of the request for the resource.

5
sushy/resources/system/system.py
View File

@ -276,7 +276,6 @@ class System(base.ResourceBase):
data['Boot']['BootSourceOverrideMode'] = fishy_mode
headers = None
etag = self._get_etag()
path = self.path
@ -290,9 +289,7 @@ class System(base.ResourceBase):
# TODO(lucasagomes): Check the return code and response body ?
# Probably we should call refresh() as well.
if etag is not None:
headers = {'If-Match': etag}
self._conn.patch(path, data=data, headers=headers)
self._conn.patch(path, data=data, etag=etag)
# TODO(etingof): we should remove this method, eventually
def set_system_boot_source(

4
sushy/tests/unit/resources/manager/test_virtual_media.py
View File

@ -175,7 +175,7 @@ class VirtualMediaTestCase(base.TestCase):
("/redfish/v1/Managers/BMC/VirtualMedia/Floppy1"),
data={"Image": "https://www.dmtf.org/freeImages/Sardine.img",
"Inserted": True, "WriteProtected": False},
headers={"If-Match": 'W/"3d7b8a7360bf2941d","3d7b8a7360bf2941d"'})
headers={"If-Match": 'W/"3d7b8a7360bf2941d"'})
self.assertTrue(self.sys_virtual_media._is_stale)
@mock.patch.object(requests, 'post', autospec=True)
@ -239,7 +239,7 @@ class VirtualMediaTestCase(base.TestCase):
self.sys_virtual_media._conn.patch.assert_called_once_with(
("/redfish/v1/Managers/BMC/VirtualMedia/Floppy1"),
data={"Image": None, "Inserted": False},
headers={"If-Match": 'W/"3d7b8a7360bf2941d","3d7b8a7360bf2941d"'})
headers={"If-Match": 'W/"3d7b8a7360bf2941d"'})
self.assertTrue(self.sys_virtual_media._is_stale)
def test_eject_media_pass_empty_dict_415(self):

24
sushy/tests/unit/resources/system/test_system.py
View File

@ -283,7 +283,7 @@ class SystemTestCase(base.TestCase):
data={'Boot': {'BootSourceOverrideEnabled': 'Continuous',
'BootSourceOverrideTarget': 'Pxe',
'BootSourceOverrideMode': 'UEFI'}},
headers=None)
etag=None)
def test_set_system_boot_options_no_mode_specified(self):
self.sys_inst.set_system_boot_options(
@ -293,7 +293,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2',
data={'Boot': {'BootSourceOverrideEnabled': 'Once',
'BootSourceOverrideTarget': 'Hdd'}},
headers=None)
etag=None)
def test_set_system_boot_options_no_target_specified(self):
self.sys_inst.set_system_boot_options(
@ -303,7 +303,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2',
data={'Boot': {'BootSourceOverrideEnabled': 'Continuous',
'BootSourceOverrideMode': 'UEFI'}},
headers=None)
etag=None)
def test_set_system_boot_options_no_freq_specified(self):
self.sys_inst.set_system_boot_options(
@ -313,13 +313,13 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2',
data={'Boot': {'BootSourceOverrideTarget': 'Pxe',
'BootSourceOverrideMode': 'UEFI'}},
headers=None)
etag=None)
def test_set_system_boot_options_nothing_specified(self):
self.sys_inst.set_system_boot_options()
self.sys_inst._conn.patch.assert_called_once_with(
'/redfish/v1/Systems/437XR1138R2', data={},
headers=None)
etag=None)
def test_set_system_boot_options_invalid_target(self):
self.assertRaises(exceptions.InvalidParameterValueError,
@ -350,7 +350,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2',
data={'Boot': {'BootSourceOverrideEnabled': 'Once',
'BootSourceOverrideTarget': 'UsbCd'}},
headers=None)
etag=None)
def test_set_system_boot_options_supermicro_no_usb_cd_boot(self):
@ -363,7 +363,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2',
data={'Boot': {'BootSourceOverrideEnabled': 'Once',
'BootSourceOverrideTarget': 'Cd'}},
headers=None)
etag=None)
def test_set_system_boot_options_settings_resource_nokia(self):
self.conn.get.return_value.headers = {'ETag': '"3d7b838291941d"'}
@ -381,7 +381,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/Self/SD',
data={'Boot': {'BootSourceOverrideEnabled': 'Once',
'BootSourceOverrideTarget': 'Cd'}},
headers={'If-Match': '"3d7b838291941d"'})
etag='"3d7b838291941d"')
def test_set_system_boot_options_settings_resource(self):
self.conn.get.return_value.headers = {'ETag': '"3d7b838291941d"'}
@ -399,7 +399,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2/BIOS/Settings',
data={'Boot': {'BootSourceOverrideEnabled': 'Once',
'BootSourceOverrideTarget': 'Cd'}},
headers={'If-Match': '"3d7b838291941d"'})
etag='"3d7b838291941d"')
def test_set_system_boot_source(self):
self.sys_inst.set_system_boot_source(
@ -411,7 +411,7 @@ class SystemTestCase(base.TestCase):
data={'Boot': {'BootSourceOverrideEnabled': 'Continuous',
'BootSourceOverrideTarget': 'Pxe',
'BootSourceOverrideMode': 'UEFI'}},
headers=None)
etag=None)
def test_set_system_boot_source_with_etag(self):
self.conn.get.return_value.headers = {'ETag': '"3d7b838291941d"'}
@ -424,7 +424,7 @@ class SystemTestCase(base.TestCase):
data={'Boot': {'BootSourceOverrideEnabled': 'Continuous',
'BootSourceOverrideTarget': 'Pxe',
'BootSourceOverrideMode': 'UEFI'}},
headers={'If-Match': '"3d7b838291941d"'})
etag='"3d7b838291941d"')
def test_set_system_boot_source_no_mode_specified(self):
self.sys_inst.set_system_boot_source(
@ -434,7 +434,7 @@ class SystemTestCase(base.TestCase):
'/redfish/v1/Systems/437XR1138R2',
data={'Boot': {'BootSourceOverrideEnabled': 'Once',
'BootSourceOverrideTarget': 'Hdd'}},
headers=None)
etag=None)
def test_set_system_boot_source_invalid_target(self):
self.assertRaises(exceptions.InvalidParameterValueError,

172
sushy/tests/unit/test_connector.py
View File

@ -577,3 +577,175 @@ class ConnectorOpTestCase(base.TestCase):
'POST',
'http://foo.bar',
blocking=True)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_strong_etag_ok(self, mock__op):
self.headers['If-Match'] = '"3d7b8a7360bf2941d"'
response = mock.MagicMock(spec=requests.Response)
response.status_code = http_client.OK
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
mock__op.return_value = response
self.conn._etag_handler(path='/redfish/v1/Systems/1',
headers=self.headers, data=data,
etag=self.headers['If-Match'],
blocking=False, timeout=60)
mock__op.assert_called_once_with(
self.conn,
"PATCH",
'/redfish/v1/Systems/1',
data={'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}},
headers={'X-Fake': 'header',
'If-Match': '"3d7b8a7360bf2941d"'},
blocking=False,
timeout=60)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_weak_etag_ok(self, mock__op):
self.headers['If-Match'] = 'W/"3d7b8a7360bf2941d"'
response = mock.MagicMock(spec=requests.Response)
response.status_code = http_client.OK
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
mock__op.return_value = response
self.conn._etag_handler(path='/redfish/v1/Systems/1',
headers=self.headers, data=data,
etag=self.headers['If-Match'],
blocking=False, timeout=60)
mock__op.assert_called_once_with(
self.conn,
"PATCH",
'/redfish/v1/Systems/1',
data={'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}},
headers={'X-Fake': 'header',
'If-Match': 'W/"3d7b8a7360bf2941d"'},
blocking=False,
timeout=60)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_no_etag_ok(self, mock__op):
response = mock.MagicMock(spec=requests.Response)
response.status_code = http_client.OK
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
mock__op.return_value = response
self.conn._etag_handler(path='/redfish/v1/Systems/1',
headers=self.headers, data=data,
blocking=False, timeout=60)
mock__op.assert_called_once_with(
self.conn,
"PATCH",
'/redfish/v1/Systems/1',
data={'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}},
headers={'X-Fake': 'header'},
blocking=False,
timeout=60)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_weak_etag_fallback_to_strong(self, mock__op):
self.headers['If-Match'] = 'W/"3d7b8a7360bf2941d"'
target_uri = '/redfish/v1/Systems/1'
response_412 = mock.MagicMock(spec=requests.Response)
response_412.status_code = http_client.PRECONDITION_FAILED
response_200 = mock.MagicMock(spec=requests.Response)
response_200.status_code = http_client.OK
mock__op.side_effect = [
exceptions.HTTPError(method='PATCH', url=target_uri,
response=response_412),
response_200]
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
self.conn._etag_handler(path=target_uri, headers=self.headers,
data=data, etag=self.headers['If-Match'],
blocking=False, timeout=60)
mock__op.assert_called_with(
self.conn,
"PATCH",
'/redfish/v1/Systems/1',
data={'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}},
headers={'X-Fake': 'header',
'If-Match': '"3d7b8a7360bf2941d"'},
blocking=False,
timeout=60)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_weak_etag_fallback_to_no_etag(self, mock__op):
self.headers['If-Match'] = 'W/"3d7b8a7360bf2941d"'
target_uri = '/redfish/v1/Systems/1'
response_412 = mock.MagicMock(spec=requests.Response)
response_412.status_code = http_client.PRECONDITION_FAILED
response_200 = mock.MagicMock(spec=requests.Response)
response_200.status_code = http_client.OK
mock__op.side_effect = [
exceptions.HTTPError(method='PATCH', url=target_uri,
response=response_412),
exceptions.HTTPError(method='PATCH', url=target_uri,
response=response_412),
response_200]
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
self.conn._etag_handler(path=target_uri, headers=self.headers,
data=data, etag=self.headers['If-Match'],
blocking=False, timeout=60)
mock__op.assert_called_with(
self.conn,
"PATCH",
'/redfish/v1/Systems/1',
data={'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}},
headers={'X-Fake': 'header'},
blocking=False,
timeout=60)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_strong_etag_fallback_to_no_etag(self, mock__op):
self.headers['If-Match'] = '"3d7b8a7360bf2941d"'
target_uri = '/redfish/v1/Systems/1'
response_412 = mock.MagicMock(spec=requests.Response)
response_412.status_code = http_client.PRECONDITION_FAILED
response_200 = mock.MagicMock(spec=requests.Response)
response_200.status_code = http_client.OK
mock__op.side_effect = [
exceptions.HTTPError(method='PATCH', url=target_uri,
response=response_412),
response_200]
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
self.conn._etag_handler(path=target_uri, headers=self.headers,
data=data, etag=self.headers['If-Match'],
blocking=False, timeout=60)
mock__op.assert_called_with(
self.conn,
"PATCH",
'/redfish/v1/Systems/1',
data={'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}},
headers={'X-Fake': 'header'},
blocking=False,
timeout=60)
@mock.patch.object(connector.Connector, '_op', autospec=True)
def test_etag_handler_fail_other_exception(self, mock__op):
self.headers['If-Match'] = 'W/"3d7b8a7360bf2941d"'
target_uri = '/redfish/v1/Systems/1'
data = {'Boot': {'BootSourceOverrideTarget': 'Cd',
'BootSourceOverrideEnabled': 'Once'}}
response = mock.MagicMock(spec=requests.Response)
response.status_code = http_client.FORBIDDEN
response.message = "boom"
mock__op.return_value = response
mock__op.side_effect = \
exceptions.HTTPError(method='PATCH',
url=target_uri,
response=response)
self.assertRaises(exceptions.HTTPError, self.conn._etag_handler,
'PATCH', target_uri, data,
self.headers,
blocking=False, timeout=60)