handler: Support more retry logic
Add retry logic for more methods that may fail due to unstable
ipmi interfaces. Also added one seconds sleep in between retries to
allow some time for the interface to be initialized.
Tested: Updated BMC firmware with the new tool and didn't have any
regression.
Change-Id: Ifc4155dd895f1a654da9e03f17e4c1e8613c9133
Signed-off-by: Willy Tu <wltu@google.com>
diff --git a/tools/handler.cpp b/tools/handler.cpp
index d241b9f..c3fa30c 100644
--- a/tools/handler.cpp
+++ b/tools/handler.cpp
@@ -23,6 +23,7 @@
#include "util.hpp"
#include <ipmiblob/blob_errors.hpp>
+#include <stdplus/function_view.hpp>
#include <stdplus/handle/managed.hpp>
#include <algorithm>
@@ -70,6 +71,38 @@
return true;
}
+std::vector<uint8_t> UpdateHandler::retryIfFailed(
+ stdplus::function_view<std::vector<uint8_t>()> callback)
+{
+ constexpr uint8_t retryCount = 3;
+ uint8_t i = 1;
+ while (true)
+ {
+ try
+ {
+ return callback();
+ }
+ catch (const ipmiblob::BlobException& b)
+ {
+ throw ToolException("blob exception received: " +
+ std::string(b.what()));
+ }
+ catch (const ToolException& t)
+ {
+ uint8_t remains = retryCount - i;
+ std::fprintf(
+ stderr,
+ "tool exception received: %s: Retrying it %u more times\n",
+ t.what(), remains);
+ if (remains == 0)
+ throw;
+ }
+ ++i;
+ handler->waitForRetry();
+ }
+ return {};
+}
+
void UpdateHandler::retrySendFile(const std::string& target,
const std::string& path)
{
@@ -88,92 +121,72 @@
void UpdateHandler::sendFile(const std::string& target, const std::string& path)
{
- const uint8_t retryCount = 3;
- uint8_t i = 1;
- while (true)
+ retryIfFailed([this, target, path]() {
+ this->retrySendFile(target, path);
+ return std::vector<uint8_t>{};
+ });
+}
+
+void UpdateHandler::retryVerifyFile(const std::string& target,
+ bool ignoreStatus)
+{
+ auto session =
+ openBlob(blob, target,
+ static_cast<std::uint16_t>(
+ ipmi_flash::FirmwareFlags::UpdateFlags::openWrite));
+
+ std::fprintf(stderr, "Committing to %s to trigger service\n",
+ target.c_str());
+ blob->commit(*session, {});
+
+ if (ignoreStatus)
{
- try
- {
- retrySendFile(target, path);
- return;
- }
- catch (const ipmiblob::BlobException& b)
- {
- throw ToolException("blob exception received: " +
- std::string(b.what()));
- }
- catch (const ToolException& t)
- {
- uint8_t remains = retryCount - i;
- std::fprintf(
- stderr,
- "tool exception received: %s: Retrying it %u more times\n",
- t.what(), remains);
- if (remains == 0)
- throw;
- }
- ++i;
+ // Skip checking the blob for status if ignoreStatus is enabled
+ return;
}
+
+ std::fprintf(stderr, "Calling stat on %s session to check status\n",
+ target.c_str());
+ pollStatus(*session, blob);
+ return;
}
bool UpdateHandler::verifyFile(const std::string& target, bool ignoreStatus)
{
- try
+ retryIfFailed([this, target, ignoreStatus]() {
+ this->retryVerifyFile(target, ignoreStatus);
+ return std::vector<uint8_t>{};
+ });
+
+ return true;
+}
+
+std::vector<uint8_t>
+ UpdateHandler::retryReadVersion(const std::string& versionBlob)
+{
+ auto session =
+ openBlob(blob, versionBlob,
+ static_cast<std::uint16_t>(
+ ipmi_flash::FirmwareFlags::UpdateFlags::openRead));
+
+ std::fprintf(stderr, "Calling stat on %s session to check status\n",
+ versionBlob.c_str());
+
+ /* TODO: call readBytes multiple times in case IPMI message length
+ * exceeds IPMI_MAX_MSG_LENGTH.
+ */
+ auto size = pollReadReady(*session, blob);
+ if (size > 0)
{
- auto session =
- openBlob(blob, target,
- static_cast<std::uint16_t>(
- ipmi_flash::FirmwareFlags::UpdateFlags::openWrite));
-
- std::fprintf(stderr, "Committing to %s to trigger service\n",
- target.c_str());
- blob->commit(*session, {});
-
- if (ignoreStatus)
- {
- // Skip checking the blob for status if ignoreStatus is enabled
- return true;
- }
-
- std::fprintf(stderr, "Calling stat on %s session to check status\n",
- target.c_str());
- pollStatus(*session, blob);
- return true;
+ return blob->readBytes(*session, 0, size);
}
- catch (const ipmiblob::BlobException& b)
- {
- throw ToolException("blob exception received: " +
- std::string(b.what()));
- }
+ return {};
}
std::vector<uint8_t> UpdateHandler::readVersion(const std::string& versionBlob)
{
- try
- {
- auto session =
- openBlob(blob, versionBlob,
- static_cast<std::uint16_t>(
- ipmi_flash::FirmwareFlags::UpdateFlags::openRead));
-
- std::fprintf(stderr, "Calling stat on %s session to check status\n",
- versionBlob.c_str());
-
- /* TODO: call readBytes multiple times in case IPMI message length
- * exceeds IPMI_MAX_MSG_LENGTH.
- */
- auto size = pollReadReady(*session, blob);
- if (size > 0)
- {
- return blob->readBytes(*session, 0, size);
- }
- return {};
- }
- catch (const ipmiblob::BlobException& b)
- {
- throw ToolException("blob exception received: " +
- std::string(b.what()));
- }
+ return retryIfFailed(
+ [this, versionBlob]() { return retryReadVersion(versionBlob); });
}
void UpdateHandler::cleanArtifacts()
diff --git a/tools/handler.hpp b/tools/handler.hpp
index 4d2fe50..25dc2f5 100644
--- a/tools/handler.hpp
+++ b/tools/handler.hpp
@@ -3,6 +3,7 @@
#include "interface.hpp"
#include <ipmiblob/blob_interface.hpp>
+#include <stdplus/function_view.hpp>
#include <string>
@@ -76,11 +77,6 @@
void sendFile(const std::string& target, const std::string& path) override;
/**
- * @throw ToolException on failure.
- */
- void retrySendFile(const std::string& target, const std::string& path);
-
- /**
* @throw ToolException on failure (TODO: throw on timeout.)
*/
bool verifyFile(const std::string& target, bool ignoreStatus) override;
@@ -92,6 +88,27 @@
private:
ipmiblob::BlobInterface* blob;
DataInterface* handler;
+
+ /**
+ * @throw ToolException on failure.
+ */
+ void retrySendFile(const std::string& target, const std::string& path);
+
+ /**
+ * @throw ToolException on failure (TODO: throw on timeout.)
+ */
+ void retryVerifyFile(const std::string& target, bool ignoreStatus);
+
+ /**
+ * @throw ToolException on failure.
+ */
+ std::vector<uint8_t> retryReadVersion(const std::string& versionBlob);
+
+ /**
+ * @throw ToolException on failure.
+ */
+ std::vector<uint8_t>
+ retryIfFailed(stdplus::function_view<std::vector<uint8_t>()> callback);
};
} // namespace host_tool
diff --git a/tools/interface.hpp b/tools/interface.hpp
index c613f0e..bd19484 100644
--- a/tools/interface.hpp
+++ b/tools/interface.hpp
@@ -3,8 +3,10 @@
#include "flags.hpp"
#include "progress.hpp"
+#include <chrono>
#include <cstdint>
#include <string>
+#include <thread>
namespace host_tool
{
@@ -26,6 +28,11 @@
virtual bool sendContents(const std::string& input,
std::uint16_t session) = 0;
+ virtual void waitForRetry()
+ {
+ std::this_thread::sleep_for(std::chrono::seconds(1));
+ }
+
/**
* Return the supported data interface for this.
*
diff --git a/tools/test/data_interface_mock.hpp b/tools/test/data_interface_mock.hpp
index a533a32..4b7479b 100644
--- a/tools/test/data_interface_mock.hpp
+++ b/tools/test/data_interface_mock.hpp
@@ -14,6 +14,7 @@
MOCK_METHOD(bool, sendContents, (const std::string&, std::uint16_t),
(override));
+ MOCK_METHOD(void, waitForRetry, (), (override));
MOCK_METHOD(ipmi_flash::FirmwareFlags::UpdateFlags, supportedType, (),
(const, override));
};
diff --git a/tools/test/tools_updater_unittest.cpp b/tools/test/tools_updater_unittest.cpp
index ac21a1e..acaaee7 100644
--- a/tools/test/tools_updater_unittest.cpp
+++ b/tools/test/tools_updater_unittest.cpp
@@ -187,6 +187,37 @@
ToolException);
}
+TEST_F(UpdateHandlerTest, VerifyFileToolException)
+{
+ /* On open, it can except and this is converted to a ToolException. */
+ EXPECT_CALL(blobMock, openBlob(ipmi_flash::verifyBlobId, _))
+ .Times(3)
+ .WillRepeatedly(Throw(ToolException("asdf")));
+ EXPECT_THROW(updater.verifyFile(ipmi_flash::verifyBlobId, false),
+ ToolException);
+}
+
+TEST_F(UpdateHandlerTest, VerifyFileHandleReturnsTrueOnSuccessWithRetries)
+{
+ EXPECT_CALL(blobMock, openBlob(ipmi_flash::verifyBlobId, _))
+ .Times(3)
+ .WillRepeatedly(Return(session));
+ EXPECT_CALL(blobMock, commit(session, _))
+ .WillOnce(Throw(ToolException("asdf")))
+ .WillOnce(Throw(ToolException("asdf")))
+ .WillOnce(Return());
+ ipmiblob::StatResponse verificationResponse = {};
+ /* the other details of the response are ignored, and should be. */
+ verificationResponse.metadata.push_back(
+ static_cast<std::uint8_t>(ipmi_flash::ActionStatus::success));
+
+ EXPECT_CALL(blobMock, getStat(TypedEq<std::uint16_t>(session)))
+ .WillOnce(Return(verificationResponse));
+ EXPECT_CALL(blobMock, closeBlob(session)).Times(3).WillRepeatedly(Return());
+
+ EXPECT_TRUE(updater.verifyFile(ipmi_flash::verifyBlobId, false));
+}
+
TEST_F(UpdateHandlerTest, VerifyFileCommitExceptionForwards)
{
/* On commit, it can except. */
@@ -238,16 +269,41 @@
{
/* It can throw an error, when polling fails. */
EXPECT_CALL(blobMock, openBlob(ipmi_flash::biosVersionBlobId, _))
- .WillOnce(Return(session));
+ .Times(3)
+ .WillRepeatedly(Return(session));
ipmiblob::StatResponse readVersionResponse = {};
readVersionResponse.blob_state = blobs::StateFlags::commit_error;
EXPECT_CALL(blobMock, getStat(TypedEq<std::uint16_t>(session)))
- .WillOnce(Return(readVersionResponse));
- EXPECT_CALL(blobMock, closeBlob(session)).WillOnce(Return());
+ .Times(3)
+ .WillRepeatedly(Return(readVersionResponse));
+ EXPECT_CALL(blobMock, closeBlob(session)).Times(3).WillRepeatedly(Return());
EXPECT_THROW(updater.readVersion(ipmi_flash::biosVersionBlobId),
ToolException);
}
+TEST_F(UpdateHandlerTest, ReadVersionReturnExpectedWithRetries)
+{
+ /* It can return as expected, when polling and readBytes succeeds. */
+ EXPECT_CALL(blobMock, openBlob(ipmi_flash::biosVersionBlobId, _))
+ .Times(3)
+ .WillRepeatedly(Return(session));
+ ipmiblob::StatResponse readVersionResponse = {};
+ readVersionResponse.blob_state = blobs::StateFlags::open_read |
+ blobs::StateFlags::committed;
+ readVersionResponse.size = 10;
+ EXPECT_CALL(blobMock, getStat(TypedEq<std::uint16_t>(session)))
+ .Times(3)
+ .WillRepeatedly(Return(readVersionResponse));
+ std::vector<uint8_t> resp = {0x2d, 0xfe};
+ EXPECT_CALL(blobMock, readBytes(session, 0, _))
+ .WillOnce(Throw(ToolException("asdf")))
+ .WillOnce(Throw(ToolException("asdf")))
+ .WillOnce(Return(resp));
+
+ EXPECT_CALL(blobMock, closeBlob(session)).Times(3).WillRepeatedly(Return());
+ EXPECT_EQ(resp, updater.readVersion(ipmi_flash::biosVersionBlobId));
+}
+
TEST_F(UpdateHandlerTest, ReadVersionCovertsOpenBlobExceptionToToolException)
{
/* On open, it can except and this is converted to a ToolException. */