auto_updater_transfer: remove fallback devserver calls
Remove the fallback calls to devserver that used to check for payloads'
availability and transfer. Also remove the metrics that measure those
fallback calls and related unittests.
BUG=chromium:1059008
TEST=./run_pytest
TEST=cros flash --debug chromeos6-row1-rack23-host15.cros
xbuddy://remote/caroline/latest
TEST=test_that --args="value='caroline-release/R86-13355.0.0'"
chromeos6-row1-rack23-host15.cros provision_AutoUpdate.double
TEST=test_that --autotest_dir ~/trunk/src/third_party/
autotest/files/ --args="target_release=13020.98.0
target_payload_uri='gs://chromeos-releases/stable-channel/
caroline/13020.98.0/payloads/chromeos_13020.98.0_caroline
_stable-channel_full_test.bin-gvtdcnzqgbtdn3xqudirkd5grrvwbhfo'
source_release=13020.52.0 source_payload_uri='gs://chromeos
-releases/stable-channel/caroline/13020.52.0/payloads
/chromeos_13020.52.0_caroline_stable-channel_full_test.bin-
gvswgm3dhe4dhbbln3c6fa2lgp4fzbc6' update_type=full"
chromeos6-row1-rack23-host15.cros autoupdate_EndToEndTest
Change-Id: Ic298197bda7c43e8ab1493d90b11ef193e341f6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/chromite/+/2337218
Tested-by: Sanika Kulkarni <sanikak@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Auto-Submit: Sanika Kulkarni <sanikak@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
diff --git a/lib/auto_updater_transfer.py b/lib/auto_updater_transfer.py
index a5555a7..4a01592 100644
--- a/lib/auto_updater_transfer.py
+++ b/lib/auto_updater_transfer.py
@@ -48,7 +48,6 @@
from chromite.lib import cros_build_lib
from chromite.lib import cros_logging as logging
-from chromite.lib import metrics
from chromite.lib import nebraska_wrapper
from chromite.lib import osutils
from chromite.lib import retry_util
@@ -336,8 +335,6 @@
complete list of accepted keyword arguments.
"""
self._staging_server = staging_server
- self._fallback_metrics = metrics.Counter(
- 'chromeos/autotest/devserver/remote_devserver_call')
super(LabTransfer, self).__init__(*args, **kwargs)
@property
@@ -361,22 +358,6 @@
"""
return r'.*/(R[0-9]+-)(?P<image_version>.+)'
- def _UpdateFallbackMetrics(self, ssh_used, success, hostname=None):
- """Updates metrics that track the fallback devserver calls.
-
- Args:
- ssh_used: Indicates whether the devserver was ssh-ed into first.
- success: Whether the command completed successfully or not.
- hostname: Hostname of the devserver on which payloads are staged. If None,
- then the hostname is derived from the self._staging_server URL.
- """
- if not hostname:
- hostname = urllib.parse.urlparse(self._staging_server).hostname
-
- self._fallback_metrics.increment(fields={'dev_server': hostname,
- 'ssh_used': ssh_used,
- 'success': success})
-
def _RemoteDevserverCall(self, cmd, stdout=False):
"""Runs a command on a remote devserver by sshing into it.
@@ -388,16 +369,7 @@
stdout: True if the stdout of the command should be captured.
"""
ip = urllib.parse.urlparse(self._staging_server).hostname
- try:
- success = True
- return cros_build_lib.run(['ssh', ip] + cmd, log_output=True,
- stdout=stdout)
- except cros_build_lib.RunCommandError:
- success = False
- logging.error('Remote devserver call failed.')
- raise
- finally:
- self._UpdateFallbackMetrics(ssh_used=True, success=success, hostname=ip)
+ return cros_build_lib.run(['ssh', ip] + cmd, log_output=True, stdout=stdout)
def _CheckPayloads(self, payload_name):
"""Runs the curl command that checks if payloads have been staged."""
@@ -407,27 +379,9 @@
try:
self._RemoteDevserverCall(cmd)
except cros_build_lib.RunCommandError as e:
- logging.error('Could not verify if %s was staged at %s. Received '
- 'exception: %s', payload_name, payload_url, e)
- # TODO(crbug.com/1059008): The following code block is a fallback. It
- # should be removed once _RemoteDevserverCall has been proven to be
- # reliable.
- try:
- success = True
- # TODO(crbug.com/1033187): Remove log_output parameter passed to
- # retry_util.RunCurl after the bug is fixed. The log_output=True option
- # has been added to correct what seems to be a timing issue in
- # retry_util.RunCurl. The error ((23) Failed writing body) is usually
- # observed when a piped program closes the read pipe before the curl
- # command has finished writing. log_output forces the read pipe to stay
- # open, thus avoiding the failure.
- retry_util.RunCurl(curl_args=cmd[1:], log_output=True)
- except retry_util.DownloadError as e:
- success = False
- raise ChromiumOSTransferError('Payload %s does not exist at %s: %s' %
- (payload_name, payload_url, e))
- finally:
- self._UpdateFallbackMetrics(ssh_used=False, success=success)
+ raise ChromiumOSTransferError(
+ 'Could not verify if %s was staged at %s. Received exception: %s' %
+ (payload_name, payload_url, e))
def CheckPayloads(self):
"""Verify that all required payloads are staged on staging server."""
@@ -553,7 +507,6 @@
if self._local_payload_props_path is None:
payload_props_filename = GetPayloadPropertiesFileName(self._payload_name)
payload_props_path = os.path.join(self._tempdir, payload_props_filename)
- fallback = True
# Get command to retrieve contents of the properties file.
cmd = ['curl',
@@ -561,44 +514,16 @@
try:
result = self._RemoteDevserverCall(cmd, stdout=True)
json.loads(result.output)
- except cros_build_lib.RunCommandError as e:
- logging.error('Unable to get payload properties file by running %s due '
- 'to exception: %s.', ' '.join(cmd), e)
- except ValueError:
- logging.error('Could not create %s as %s not valid json.',
- payload_props_path, result.output)
- else:
- fallback = False
osutils.WriteFile(payload_props_path, result.output, 'wb',
makedirs=True)
-
- if fallback:
- # TODO(crbug.com/1059008): Fallback in case the try block above fails.
- # Should be removed once reliable.
-
- # Get command to download the properties file.
- cmd = self._GetCurlCmdForPayloadDownload(
- payload_dir=self._tempdir, build_id=self._payload_dir,
- payload_filename=payload_props_filename)
-
- # _GetCurlCmdForPayloadDownload removes the 'payloads/' prefix from the
- # beginning of the payload_props_filename before creating the
- # command. So here we need to remove that too.
- prefix = 'payloads/'
- if payload_props_filename.startswith(prefix):
- payload_props_path = os.path.join(
- self._tempdir,
- payload_props_filename[len(prefix):])
-
- try:
- success = True
- retry_util.RunCurl(cmd[1:])
- except retry_util.DownloadError as e:
- success = False
- raise ChromiumOSTransferError('Unable to download %s: %s' %
- (payload_props_filename, e))
- finally:
- self._UpdateFallbackMetrics(ssh_used=False, success=success)
+ except cros_build_lib.RunCommandError as e:
+ raise ChromiumOSTransferError(
+ 'Unable to get payload properties file by running %s due to '
+ 'exception: %s.' % (' '.join(cmd), e))
+ except ValueError:
+ raise ChromiumOSTransferError(
+ 'Could not create %s as %s not valid json.' %
+ (payload_props_path, result.output))
self._local_payload_props_path = payload_props_path
return self._local_payload_props_path
@@ -615,18 +540,9 @@
try:
proc = self._RemoteDevserverCall(cmd, stdout=True)
except cros_build_lib.RunCommandError as e:
- logging.error('Unable to get payload size by running command %s due '
- 'to exception: %s.', ' '.join(cmd), e)
- # TODO(crbug.com/1059008): Fallback in case the try block above fails.
- # Should be removed once reliable.
- try:
- success = True
- proc = retry_util.RunCurl(cmd[1:], stdout=True)
- except cros_build_lib.RunCommandError as e:
- success = False
- raise ChromiumOSTransferError('Unable to get payload size: %s' % e)
- finally:
- self._UpdateFallbackMetrics(ssh_used=False, success=success)
+ raise ChromiumOSTransferError(
+ 'Unable to get payload size by running command %s due to exception: '
+ '%s.' % (' '.join(cmd), e))
pattern = re.compile(r'Content-Length: [0-9]+', re.I)
match = pattern.findall(proc.output)
diff --git a/lib/auto_updater_transfer_unittest.py b/lib/auto_updater_transfer_unittest.py
index aeb027e..ac919a1 100644
--- a/lib/auto_updater_transfer_unittest.py
+++ b/lib/auto_updater_transfer_unittest.py
@@ -26,7 +26,6 @@
from chromite.lib import osutils
from chromite.lib import partial_mock
from chromite.lib import remote_access
-from chromite.lib import retry_util
_DEFAULT_ARGS = {
@@ -622,8 +621,8 @@
def testCheckPayloadsAllInRemoteDevserverCallError(self):
"""Test auto_updater_transfer.CheckPayloads.
- Test CheckPayloads() method's fallback when transfer_rootfs_update and
- transfer_stateful_update are both set to True and
+ Test the exception thrown by CheckPayloads() method when
+ transfer_rootfs_update and transfer_stateful_update are both set to True and
LabTransfer._RemoteDevserver() throws an error.
"""
with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
@@ -636,22 +635,9 @@
self.PatchObject(auto_updater_transfer.LabTransfer,
'_RemoteDevserverCall',
side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl')
- expected = [
- {'curl_args': ['-I', 'http://0.0.0.0:8000/static/board-release/'
- '12345.6.7/test_update.gz', '--fail'],
- 'log_output': True},
- {'curl_args': ['-I', 'http://0.0.0.0:8000/static/board-release/'
- '12345.6.7/test_update.gz.json', '--fail'],
- 'log_output': True},
- {'curl_args': ['-I', 'http://0.0.0.0:8000/static/board-release/'
- '12345.6.7/test_stateful.tgz', '--fail'],
- 'log_output': True}]
-
- CrOS_LabTransfer.CheckPayloads()
- self.assertListEqual(retry_util.RunCurl.call_args_list,
- [mock.call(**x) for x in expected])
+ self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
+ CrOS_LabTransfer.CheckPayloads)
def testCheckPayloadsNoStatefulTransfer(self):
"""Test auto_updater_transfer.CheckPayloads.
@@ -680,9 +666,9 @@
def testCheckPayloadsNoStatefulTransferRemoteDevserverCallError(self):
"""Test auto_updater_transfer.CheckPayloads.
- Test CheckPayloads() method's fallback when transfer_rootfs_update is True
- and transfer_stateful_update is set to False and
- LabTransfer._RemoteDevserver() throws an error.
+ Test the exception thrown by CheckPayloads() method when
+ transfer_rootfs_update is True and transfer_stateful_update is False
+ and LabTransfer._RemoteDevserver() throws an error.
"""
with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
CrOS_LabTransfer = CreateLabTransferInstance(
@@ -692,19 +678,9 @@
self.PatchObject(auto_updater_transfer.LabTransfer,
'_RemoteDevserverCall',
side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl')
- expected = [
- {'curl_args': ['-I', 'http://0.0.0.0:8000/static/board-release/'
- '12345.6.7/test_update.gz', '--fail'],
- 'log_output': True},
- {'curl_args': ['-I', 'http://0.0.0.0:8000/static/board-release/'
- '12345.6.7/test_update.gz.json', '--fail'],
- 'log_output': True}]
-
- CrOS_LabTransfer.CheckPayloads()
- self.assertListEqual(retry_util.RunCurl.call_args_list,
- [mock.call(**x) for x in expected])
+ self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
+ CrOS_LabTransfer.CheckPayloads)
def testCheckPayloadsNoRootfsTransfer(self):
"""Test auto_updater_transfer.CheckPayloads.
@@ -730,9 +706,9 @@
def testCheckPayloadsNoRootfsTransferRemoteDevserverCallError(self):
"""Test auto_updater_transfer.CheckPayloads.
- Test CheckPayloads() method's fallback when transfer_rootfs_update is False
- and transfer_stateful_update is set to True and
- LabTransfer._RemoteDevserver() throws an error.
+ Test exception thrown by CheckPayloads() method when
+ transfer_rootfs_update is False and transfer_stateful_update is set to True
+ and LabTransfer._RemoteDevserver() throws an error.
"""
with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
CrOS_LabTransfer = CreateLabTransferInstance(
@@ -743,16 +719,9 @@
self.PatchObject(auto_updater_transfer.LabTransfer,
'_RemoteDevserverCall',
side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl')
- expected = [
- {'curl_args': ['-I', 'http://0.0.0.0:8000/static/board-release/'
- '12345.6.7/test_stateful.tgz', '--fail'],
- 'log_output': True}]
-
- CrOS_LabTransfer.CheckPayloads()
- self.assertListEqual(retry_util.RunCurl.call_args_list,
- [mock.call(**x) for x in expected])
+ self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
+ CrOS_LabTransfer.CheckPayloads)
def testCheckPayloadsNoPayloadError(self):
"""Test auto_updater_transfer.CheckPayloads.
@@ -766,8 +735,6 @@
self.PatchObject(auto_updater_transfer.LabTransfer,
'_RemoteDevserverCall',
side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl',
- side_effect=retry_util.DownloadError())
self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
CrOS_LabTransfer.CheckPayloads)
@@ -825,12 +792,10 @@
def testGetPayloadPropsFileWrongFormat(self):
"""Test LabTransfer.GetPayloadPropsFile().
- Test when the payload is not in the correct json format. Also, test if the
- fallback is being called.
+ Test exception thrown when the payload is not in the correct json format.
"""
with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
lab_xfer = auto_updater_transfer.LabTransfer
- payload_props_path = os.path.join(self.tempdir, 'test_update.gz.json')
output = 'Not in Json format'
CrOS_LabTransfer = CreateLabTransferInstance(
@@ -838,36 +803,9 @@
self.PatchObject(lab_xfer, '_RemoteDevserverCall',
return_value=cros_build_lib.CommandResult(stdout=output))
- self.PatchObject(retry_util, 'RunCurl',
- return_value=cros_build_lib.CommandResult(stdout=''))
- CrOS_LabTransfer.GetPayloadPropsFile()
- self.assertEqual(CrOS_LabTransfer._local_payload_props_path,
- payload_props_path)
-
- def testGetPayloadPropsFileRemoteDevserverCallError(self):
- """Test LabTransfer.GetPayloadPropsFile().
-
- Test when the LabTransfer._RemoteDevserverCall() method throws an error.
- """
- with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
-
- lab_xfer = auto_updater_transfer.LabTransfer
- payload_props_path = os.path.join(self.tempdir, 'test_update.gz.json')
- output = ''
-
- CrOS_LabTransfer = CreateLabTransferInstance(
- device, tempdir=self.tempdir, payload_name='test_update.gz')
-
- self.PatchObject(lab_xfer, '_RemoteDevserverCall',
- side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl',
- return_value=cros_build_lib.CommandResult(stdout=output))
-
- CrOS_LabTransfer.GetPayloadPropsFile()
-
- self.assertEqual(CrOS_LabTransfer._local_payload_props_path,
- payload_props_path)
+ self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
+ CrOS_LabTransfer.GetPayloadPropsFile)
def testGetPayloadPropsFileError(self):
"""Test LabTransfer.GetPayloadPropsFile().
@@ -881,8 +819,6 @@
payload_dir='/test/dir')
self.PatchObject(lab_xfer, '_RemoteDevserverCall',
side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl',
- side_effect=retry_util.DownloadError())
self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
CrOS_LabTransfer.GetPayloadPropsFile)
self.assertIsNone(CrOS_LabTransfer._local_payload_props_path)
@@ -911,14 +847,10 @@
CrOS_LabTransfer = CreateLabTransferInstance(
device, payload_name='test_update.gz',
payload_dir='/test/payload/dir')
- expected_size = 75810234
- output = 'Content-Length: %s' % str(expected_size)
self.PatchObject(lab_xfer, '_RemoteDevserverCall',
side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl',
- return_value=cros_build_lib.CommandResult(stdout=output))
- size = CrOS_LabTransfer._GetPayloadSize()
- self.assertEqual(size, expected_size)
+ self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
+ CrOS_LabTransfer._GetPayloadSize)
def test_GetPayloadSizeNoRegexMatchError(self):
"""Test errors thrown by auto_updater_transfer._GetPayloadSize().
@@ -937,43 +869,6 @@
self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
CrOS_LabTransfer._GetPayloadSize)
- def test_GetPayloadSizeNoRegexMatchRemoteDevserverCallError(self):
- """Test errors thrown by auto_updater_transfer._GetPayloadSize().
-
- Test error thrown when the output received from the curl command does not
- contain expected fields and LabTransfer._RemoteDevserverCall method throws
- an error.
- """
- with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
- lab_xfer = auto_updater_transfer.LabTransfer
- CrOS_LabTransfer = CreateLabTransferInstance(
- device, payload_name='test_update.gz',
- payload_dir='/test/payload/dir')
- output = 'No Match Output'
- self.PatchObject(lab_xfer, '_RemoteDevserverCall',
- side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl',
- return_value=cros_build_lib.CommandResult(stdout=output))
- self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
- CrOS_LabTransfer._GetPayloadSize)
-
- def test_GetPayloadSizeCurlRunCommandError(self):
- """Test errors thrown by auto_updater_transfer.GetPayloadSize().
-
- Test error thrown when cros_build_lib.run raises a RunCommandError.
- """
- with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
- lab_xfer = auto_updater_transfer.LabTransfer
- CrOS_LabTransfer = CreateLabTransferInstance(
- device, payload_name='test_update.gz',
- payload_dir='/test/payload/dir')
- self.PatchObject(lab_xfer, '_RemoteDevserverCall',
- side_effect=cros_build_lib.RunCommandError(msg=''))
- self.PatchObject(retry_util, 'RunCurl',
- side_effect=cros_build_lib.RunCommandError(msg=''))
- self.assertRaises(auto_updater_transfer.ChromiumOSTransferError,
- CrOS_LabTransfer._GetPayloadSize)
-
def testGetPayloadPropsLab(self):
"""Test LabTransfer.GetPayloadProps()."""
with remote_access.ChromiumOSDeviceHandler(remote_access.TEST_IP) as device:
