Aggregation: Better handle dropped requests
It's possible for HTTP client's request buffer to become full
(especially when $expand is used). Instead of ignoring the requests
we should provide a 429 Too Many Requests response for the provided
callback to process.
The aggregator's response handling also needs to account for this
possibility so that it only completely overwrites the asyncResp
object when it receives a response from a satellite.
Also added more test cases for the response processing functions.
Tested:
Unit tests passed
Flooded aggregator with requests for satellite resources. Requests
began returning 429 Too Many Requests errors after the request buffer
became full.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ib052dc0454d759de7fae761977ca26d6b8d208e5
diff --git a/http/http_client.hpp b/http/http_client.hpp
index 48e8cfd..a2cd62e 100644
--- a/http/http_client.hpp
+++ b/http/http_client.hpp
@@ -775,8 +775,13 @@
}
else
{
+ // If we can't buffer the request then we should let the callback
+ // handle a 429 Too Many Requests dummy response
BMCWEB_LOG_ERROR << destIP << ":" << std::to_string(destPort)
<< " request queue full. Dropping request.";
+ Response dummyRes;
+ dummyRes.result(boost::beast::http::status::too_many_requests);
+ resHandler(dummyRes);
}
}
diff --git a/redfish-core/include/redfish_aggregator.hpp b/redfish-core/include/redfish_aggregator.hpp
index bc384f8..f097ac6 100644
--- a/redfish-core/include/redfish_aggregator.hpp
+++ b/redfish-core/include/redfish_aggregator.hpp
@@ -602,6 +602,14 @@
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
crow::Response& resp)
{
+ // 429 and 502 mean we didn't actually send the request so don't
+ // overwrite the response headers in that case
+ if ((resp.resultInt() == 429) || (resp.resultInt() == 502))
+ {
+ asyncResp->res.result(resp.result());
+ return;
+ }
+
// We want to attempt prefix fixing regardless of response code
// The resp will not have a json component
// We need to create a json from resp's stringResponse
@@ -647,6 +655,13 @@
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
crow::Response& resp)
{
+ // 429 and 502 mean we didn't actually send the request so don't
+ // overwrite the response headers in that case
+ if ((resp.resultInt() == 429) || (resp.resultInt() == 502))
+ {
+ return;
+ }
+
if (resp.resultInt() != 200)
{
BMCWEB_LOG_DEBUG
@@ -672,6 +687,7 @@
// Notify the user if doing so won't overwrite a valid response
if ((asyncResp->res.resultInt() != 200) &&
+ (asyncResp->res.resultInt() != 429) &&
(asyncResp->res.resultInt() != 502))
{
messages::operationFailed(asyncResp->res);
@@ -707,6 +723,7 @@
<< "Collection does not exist, overwriting asyncResp";
asyncResp->res.result(resp.result());
asyncResp->res.jsonValue = std::move(jsonVal);
+ asyncResp->res.addHeader("Content-Type", "application/json");
BMCWEB_LOG_DEBUG << "Finished overwriting asyncResp";
}
@@ -750,12 +767,13 @@
{
BMCWEB_LOG_ERROR << "Received unparsable response from \"" << prefix
<< "\"";
- // We received as response that was not a json
+ // We received a response that was not a json.
// Notify the user only if we did not receive any valid responses,
// if the resource collection does not already exist on the
// aggregating BMC, and if we did not already set this warning due
// to a failure from a different satellite
if ((asyncResp->res.resultInt() != 200) &&
+ (asyncResp->res.resultInt() != 429) &&
(asyncResp->res.resultInt() != 502))
{
messages::operationFailed(asyncResp->res);
diff --git a/test/redfish-core/include/redfish_aggregator_test.cpp b/test/redfish-core/include/redfish_aggregator_test.cpp
index 665446c..5a7d2d0 100644
--- a/test/redfish-core/include/redfish_aggregator_test.cpp
+++ b/test/redfish-core/include/redfish_aggregator_test.cpp
@@ -240,5 +240,211 @@
assertProcessResponse(507);
}
+TEST(processResponse, preserveHeaders)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ asyncResp->res.addHeader("OData-Version", "4.0");
+ asyncResp->res.result(boost::beast::http::status::ok);
+
+ crow::Response resp;
+ resp.addHeader("OData-Version", "3.0");
+ resp.addHeader(boost::beast::http::field::location,
+ "/redfish/v1/Chassis/Test");
+ resp.result(boost::beast::http::status::too_many_requests); // 429
+
+ RedfishAggregator::processResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.resultInt(), 429);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Location"), "");
+
+ asyncResp->res.result(boost::beast::http::status::ok);
+ resp.result(boost::beast::http::status::bad_gateway); // 502
+
+ RedfishAggregator::processResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.resultInt(), 502);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Location"), "");
+}
+
+// Helper function to correctly populate a ComputerSystem collection response
+void populateCollectionResponse(crow::Response& resp)
+{
+ nlohmann::json jsonResp = nlohmann::json::parse(R"(
+ {
+ "@odata.id": "/redfish/v1/Systems",
+ "@odata.type": "#ComputerSystemCollection.ComputerSystemCollection",
+ "Members": [
+ {
+ "@odata.id": "/redfish/v1/Systems/system"
+ }
+ ],
+ "Members@odata.count": 1,
+ "Name": "Computer System Collection"
+ }
+ )",
+ nullptr, false);
+
+ resp.clear();
+ // resp.body() =
+ // jsonResp.dump(2, ' ', true,
+ // nlohmann::json::error_handler_t::replace);
+ resp.jsonValue = std::move(jsonResp);
+ resp.addHeader("OData-Version", "4.0");
+ resp.addHeader("Content-Type", "application/json");
+ resp.result(boost::beast::http::status::ok);
+}
+
+void populateCollectionNotFound(crow::Response& resp)
+{
+ resp.clear();
+ resp.addHeader("OData-Version", "4.0");
+ resp.result(boost::beast::http::status::not_found);
+}
+
+// Used with the above functions to convert the response to appear like it's
+// from a satellite which will not have a json component
+void convertToSat(crow::Response& resp)
+{
+ resp.body() = resp.jsonValue.dump(2, ' ', true,
+ nlohmann::json::error_handler_t::replace);
+ resp.jsonValue.clear();
+}
+
+TEST(processCollectionResponse, localOnly)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ crow::Response resp;
+ populateCollectionResponse(asyncResp->res);
+ populateCollectionNotFound(resp);
+
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.resultInt(), 200);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Content-Type"),
+ "application/json");
+ EXPECT_EQ(asyncResp->res.jsonValue["Members@odata.count"], 1);
+ for (auto& member : asyncResp->res.jsonValue["Members"])
+ {
+ // There should only be one member
+ EXPECT_EQ(member["@odata.id"], "/redfish/v1/Systems/system");
+ }
+}
+
+TEST(processCollectionResponse, satelliteOnly)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ crow::Response resp;
+ populateCollectionNotFound(asyncResp->res);
+ populateCollectionResponse(resp);
+ convertToSat(resp);
+
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.resultInt(), 200);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Content-Type"),
+ "application/json");
+ EXPECT_EQ(asyncResp->res.jsonValue["Members@odata.count"], 1);
+ for (auto& member : asyncResp->res.jsonValue["Members"])
+ {
+ // There should only be one member
+ EXPECT_EQ(member["@odata.id"], "/redfish/v1/Systems/prefix_system");
+ }
+}
+
+TEST(processCollectionResponse, bothExist)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ crow::Response resp;
+ populateCollectionResponse(asyncResp->res);
+ populateCollectionResponse(resp);
+ convertToSat(resp);
+
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.resultInt(), 200);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Content-Type"),
+ "application/json");
+ EXPECT_EQ(asyncResp->res.jsonValue["Members@odata.count"], 2);
+
+ bool foundLocal = false;
+ bool foundSat = false;
+ for (auto& member : asyncResp->res.jsonValue["Members"])
+ {
+ if (member["@odata.id"] == "/redfish/v1/Systems/system")
+ {
+ foundLocal = true;
+ }
+ else if (member["@odata.id"] == "/redfish/v1/Systems/prefix_system")
+ {
+ foundSat = true;
+ }
+ }
+ EXPECT_TRUE(foundLocal);
+ EXPECT_TRUE(foundSat);
+}
+
+TEST(processCollectionResponse, satelliteWrongContentHeader)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ crow::Response resp;
+ populateCollectionResponse(asyncResp->res);
+ populateCollectionResponse(resp);
+ convertToSat(resp);
+
+ // Ignore the satellite even though otherwise valid
+ resp.addHeader("Content-Type", "");
+
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.resultInt(), 200);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Content-Type"),
+ "application/json");
+ EXPECT_EQ(asyncResp->res.jsonValue["Members@odata.count"], 1);
+ for (auto& member : asyncResp->res.jsonValue["Members"])
+ {
+ EXPECT_EQ(member["@odata.id"], "/redfish/v1/Systems/system");
+ }
+}
+
+TEST(processCollectionResponse, neitherExist)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ crow::Response resp;
+ populateCollectionNotFound(asyncResp->res);
+ populateCollectionNotFound(resp);
+ convertToSat(resp);
+
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.resultInt(), 404);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Content-Type"), "");
+}
+
+TEST(processCollectionResponse, preserveHeaders)
+{
+ auto asyncResp = std::make_shared<bmcweb::AsyncResp>();
+ crow::Response resp;
+ populateCollectionNotFound(asyncResp->res);
+ populateCollectionResponse(resp);
+ convertToSat(resp);
+
+ resp.addHeader("OData-Version", "3.0");
+ resp.addHeader(boost::beast::http::field::location,
+ "/redfish/v1/Chassis/Test");
+
+ // We skip processing collection responses that have a 429 or 502 code
+ resp.result(boost::beast::http::status::too_many_requests); // 429
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.resultInt(), 404);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Location"), "");
+
+ resp.result(boost::beast::http::status::bad_gateway); // 502
+ RedfishAggregator::processCollectionResponse("prefix", asyncResp, resp);
+ EXPECT_EQ(asyncResp->res.resultInt(), 404);
+ EXPECT_EQ(asyncResp->res.getHeaderValue("OData-Version"), "4.0");
+ EXPECT_EQ(asyncResp->res.getHeaderValue("Location"), "");
+}
+
} // namespace
} // namespace redfish