From 1525b678ab842fe82449bdf7076296980a90dbcd Mon Sep 17 00:00:00 2001 From: Carlos Date: Wed, 5 Jul 2023 18:38:50 +0100 Subject: [PATCH] Add sanity-check for `BatchExecuteRequest`s (#331) * utils: move batch utils to different source file * util: fix compilation errors after moving batchExec to a different header file * tests: add regression tests * dist-tests: fix compilation --- include/faabric/scheduler/Scheduler.h | 1 - include/faabric/util/batch.h | 14 +++++ include/faabric/util/func.h | 7 --- src/endpoint/FaabricEndpointHandler.cpp | 1 + src/mpi/MpiWorld.cpp | 2 +- src/scheduler/Scheduler.cpp | 2 +- src/util/CMakeLists.txt | 1 + src/util/batch.cpp | 55 ++++++++++++++++++ src/util/func.cpp | 24 -------- tests/dist/mpi/mpi_native.cpp | 1 + tests/dist/scheduler/functions.cpp | 2 +- tests/dist/transport/functions.cpp | 2 +- tests/test/util/test_batch.cpp | 76 +++++++++++++++++++++++++ tests/test/util/test_func.cpp | 21 ------- tests/utils/fixtures.h | 2 +- 15 files changed, 153 insertions(+), 58 deletions(-) create mode 100644 include/faabric/util/batch.h create mode 100644 src/util/batch.cpp create mode 100644 tests/test/util/test_batch.cpp diff --git a/include/faabric/scheduler/Scheduler.h b/include/faabric/scheduler/Scheduler.h index 404a3e5c4..efd8ef142 100644 --- a/include/faabric/scheduler/Scheduler.h +++ b/include/faabric/scheduler/Scheduler.h @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include diff --git a/include/faabric/util/batch.h b/include/faabric/util/batch.h new file mode 100644 index 000000000..813ac62a9 --- /dev/null +++ b/include/faabric/util/batch.h @@ -0,0 +1,14 @@ +#pragma once + +#include + +namespace faabric::util { +std::shared_ptr batchExecFactory(); + +std::shared_ptr batchExecFactory( + const std::string& user, + const std::string& function, + int count = 1); + +bool isBatchExecRequestValid(std::shared_ptr ber); +} diff --git a/include/faabric/util/func.h b/include/faabric/util/func.h index b8cbd3812..7408d8b18 100644 --- a/include/faabric/util/func.h +++ b/include/faabric/util/func.h @@ -31,13 +31,6 @@ std::shared_ptr messageFactoryShared( faabric::Message messageFactory(const std::string& user, const std::string& function); -std::shared_ptr batchExecFactory(); - -std::shared_ptr batchExecFactory( - const std::string& user, - const std::string& function, - int count = 1); - std::string resultKeyFromMessageId(unsigned int mid); std::string statusKeyFromMessageId(unsigned int mid); diff --git a/src/endpoint/FaabricEndpointHandler.cpp b/src/endpoint/FaabricEndpointHandler.cpp index 7a978e889..04d9ca573 100644 --- a/src/endpoint/FaabricEndpointHandler.cpp +++ b/src/endpoint/FaabricEndpointHandler.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include diff --git a/src/mpi/MpiWorld.cpp b/src/mpi/MpiWorld.cpp index 93ac619d4..cac384008 100644 --- a/src/mpi/MpiWorld.cpp +++ b/src/mpi/MpiWorld.cpp @@ -3,8 +3,8 @@ #include #include #include +#include #include -#include #include #include #include diff --git a/src/scheduler/Scheduler.cpp b/src/scheduler/Scheduler.cpp index c26a91c7a..c2641f13d 100644 --- a/src/scheduler/Scheduler.cpp +++ b/src/scheduler/Scheduler.cpp @@ -9,9 +9,9 @@ #include #include #include +#include #include #include -#include #include #include #include diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 8e42b9004..d9bf7d07e 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -3,6 +3,7 @@ faabric_lib(util ExecGraph.cpp PeriodicBackgroundThread.cpp barrier.cpp + batch.cpp bytes.cpp config.cpp clock.cpp diff --git a/src/util/batch.cpp b/src/util/batch.cpp new file mode 100644 index 000000000..3699f2338 --- /dev/null +++ b/src/util/batch.cpp @@ -0,0 +1,55 @@ +#include +#include +#include + +namespace faabric::util { +std::shared_ptr batchExecFactory() +{ + auto req = std::make_shared(); + req->set_id(generateGid()); + return req; +} + +std::shared_ptr batchExecFactory( + const std::string& user, + const std::string& function, + int count) +{ + auto req = batchExecFactory(); + + // Force the messages to have the same app ID than the BER + int appId = req->id(); + for (int i = 0; i < count; i++) { + *req->add_messages() = messageFactory(user, function); + req->mutable_messages()->at(i).set_appid(appId); + } + + return req; +} + +bool isBatchExecRequestValid(std::shared_ptr ber) +{ + if (ber == nullptr) { + return false; + } + + // An empty BER (thus invalid) will have 0 messages and an id of 0 + if (ber->messages_size() <= 0 && ber->id() == 0) { + return false; + } + + std::string user = ber->messages(0).user(); + std::string func = ber->messages(0).function(); + int appId = ber->messages(0).appid(); + + for (int i = 1; i < ber->messages_size(); i++) { + auto msg = ber->messages(i); + if (msg.user() != user || msg.function() != func || + msg.appid() != appId) { + return false; + } + } + + return true; +} +} diff --git a/src/util/func.cpp b/src/util/func.cpp index 9dbb61869..ea958c3d8 100644 --- a/src/util/func.cpp +++ b/src/util/func.cpp @@ -47,30 +47,6 @@ std::string buildAsyncResponse(const faabric::Message& msg) return std::to_string(msg.id()); } -std::shared_ptr batchExecFactory() -{ - auto req = std::make_shared(); - req->set_id(faabric::util::generateGid()); - return req; -} - -std::shared_ptr batchExecFactory( - const std::string& user, - const std::string& function, - int count) -{ - auto req = batchExecFactory(); - - // Force the messages to have the same app ID - uint32_t appId = faabric::util::generateGid(); - for (int i = 0; i < count; i++) { - *req->add_messages() = messageFactory(user, function); - req->mutable_messages()->at(i).set_appid(appId); - } - - return req; -} - std::shared_ptr messageFactoryShared( const std::string& user, const std::string& function) diff --git a/tests/dist/mpi/mpi_native.cpp b/tests/dist/mpi/mpi_native.cpp index 8d048bbd1..5a8813992 100644 --- a/tests/dist/mpi/mpi_native.cpp +++ b/tests/dist/mpi/mpi_native.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include diff --git a/tests/dist/scheduler/functions.cpp b/tests/dist/scheduler/functions.cpp index daa06a6fc..176e95360 100644 --- a/tests/dist/scheduler/functions.cpp +++ b/tests/dist/scheduler/functions.cpp @@ -9,9 +9,9 @@ #include #include #include +#include #include #include -#include #include #include #include diff --git a/tests/dist/transport/functions.cpp b/tests/dist/transport/functions.cpp index cc3cb2e02..2f547fe98 100644 --- a/tests/dist/transport/functions.cpp +++ b/tests/dist/transport/functions.cpp @@ -7,8 +7,8 @@ #include #include #include +#include #include -#include #include #include #include diff --git a/tests/test/util/test_batch.cpp b/tests/test/util/test_batch.cpp new file mode 100644 index 000000000..e7dff5034 --- /dev/null +++ b/tests/test/util/test_batch.cpp @@ -0,0 +1,76 @@ +#include + +#include + +using namespace faabric::util; + +namespace tests { +TEST_CASE("Test batch exec factory", "[util]") +{ + int nMessages = 4; + std::shared_ptr req = + batchExecFactory("demo", "echo", nMessages); + + REQUIRE(req->messages().size() == nMessages); + + REQUIRE(req->id() > 0); + + // Expect all messages to have the same app ID by default + int appId = req->messages().at(0).appid(); + REQUIRE(appId > 0); + + for (const auto& m : req->messages()) { + REQUIRE(m.appid() == appId); + REQUIRE(m.user() == "demo"); + REQUIRE(m.function() == "echo"); + } +} + +TEST_CASE("Test batch. exec request sanity checks") +{ + int nMessages = 4; + std::shared_ptr ber = + batchExecFactory("demo", "echo", nMessages); + bool isBerValid; + + // A null BER is invalid + SECTION("Null BER") + { + isBerValid = false; + ber = nullptr; + } + + // An empty BER is invalid + SECTION("Empty BER") + { + isBerValid = false; + ber = std::make_shared(); + } + + // An appId mismatch between the messages deems a BER invalid + SECTION("App ID mismatch") + { + isBerValid = false; + ber->mutable_messages(1)->set_appid(1337); + } + + // A user mismatch between the messages deems a BER invalid + SECTION("User mismatch") + { + isBerValid = false; + ber->mutable_messages(1)->set_user("foo"); + } + + // A function mismatch between the messages deems a BER invalid + SECTION("Function mismatch") + { + isBerValid = false; + ber->mutable_messages(1)->set_function("foo"); + } + + // BERs constructed with the default factory are valid + SECTION("Valid BER") { isBerValid = true; } + + REQUIRE(isBerValid == isBatchExecRequestValid(ber)); +} +} diff --git a/tests/test/util/test_func.cpp b/tests/test/util/test_func.cpp index 8b1580917..85ebae388 100644 --- a/tests/test/util/test_func.cpp +++ b/tests/test/util/test_func.cpp @@ -30,27 +30,6 @@ TEST_CASE("Test message factory shared", "[util]") REQUIRE(!msg->resultkey().empty()); } -TEST_CASE("Test batch exec factory", "[util]") -{ - int nMessages = 4; - std::shared_ptr req = - faabric::util::batchExecFactory("demo", "echo", nMessages); - - REQUIRE(req->messages().size() == nMessages); - - REQUIRE(req->id() > 0); - - // Expect all messages to have the same app ID by default - int appId = req->messages().at(0).appid(); - REQUIRE(appId > 0); - - for (const auto& m : req->messages()) { - REQUIRE(m.appid() == appId); - REQUIRE(m.user() == "demo"); - REQUIRE(m.function() == "echo"); - } -} - TEST_CASE("Test adding ids to message", "[util]") { faabric::Message msgA; diff --git a/tests/utils/fixtures.h b/tests/utils/fixtures.h index 28d42c0a8..6797df5ea 100644 --- a/tests/utils/fixtures.h +++ b/tests/utils/fixtures.h @@ -22,9 +22,9 @@ #include #include #include +#include #include #include -#include #include #include #include