Decouple bare metal systemd target naming from file path suffixes. * Split `instanceString` into `systemdInstanceString` and `fileInstanceString` in `BaremetalTransferServiceImpl`. * Decouple systemd target instantiation (`@0.target`) from flag file paths and BM instance properties (`""` for single-host platforms). * Update `baremetal_test.cc` to remove artificial `"0"` suffixes from mock flag files and property paths for single-host testing. Velorum Bare Metal Test: https://paste.googleplex.com/5442812391849984 PiperOrigin-RevId: 929829759 Change-Id: Ia54e8b28474751916289658d73c802e83d702ed5
diff --git a/src/baremetal_callback.cc b/src/baremetal_callback.cc index 7592189..213619a 100644 --- a/src/baremetal_callback.cc +++ b/src/baremetal_callback.cc
@@ -12,6 +12,7 @@ #include <thread> #include "absl/log/log.h" +#include "absl/strings/str_cat.h" #include "grpcpp/server_context.h" #include "grpcpp/support/status.h" @@ -32,26 +33,28 @@ std::error_code ec; if (std::filesystem::exists( - std::string(bmDriveCleaningDoneAckFlagPath) + instanceString, ec)) { + absl::StrCat(bmDriveCleaningDoneAckFlagPath, fileInstanceString), + ec)) { LOG(INFO) << "We acked cleaning done and must be in BM mode"; response->set_bm_mode(phosphor::baremetal::BmMode::BMMODE_BM); return grpc::Status::OK; } if (std::filesystem::exists( - std::string(bmDriveCleaningDoneFlagPath) + instanceString, ec)) { + absl::StrCat(bmDriveCleaningDoneFlagPath, fileInstanceString), ec)) { std::filesystem::rename( - std::string(bmDriveCleaningDoneFlagPath) + instanceString, - std::string(bmDriveCleaningDoneAckFlagPath) + instanceString, ec); + absl::StrCat(bmDriveCleaningDoneFlagPath, fileInstanceString), + absl::StrCat(bmDriveCleaningDoneAckFlagPath, fileInstanceString), ec); LOG(INFO) << "We just finished cleaning and must be in BM mode"; response->set_bm_mode(phosphor::baremetal::BmMode::BMMODE_BM); return grpc::Status::OK; } - if (std::filesystem::exists(std::string(bmSignalPath), ec)) { + if (std::filesystem::exists(bmSignalPath, ec)) { if (!std::filesystem::exists( - std::string(bmDriveCleaningFlagPath) + instanceString, ec)) { - std::ofstream file(std::string(bmDriveCleaningFlagPath) + instanceString); + absl::StrCat(bmDriveCleaningFlagPath, fileInstanceString), ec)) { + std::ofstream file( + absl::StrCat(bmDriveCleaningFlagPath, fileInstanceString)); } LOG(INFO) << "We must be in BM cleaning mode"; @@ -74,19 +77,16 @@ // Asynchronously start the systemd targets that will disable communication // with the host. std::thread systemd_thread([this]() { - std::stringstream bm_prep_stream; - // Use instanceString instead of instanceNumber because the instanceString - // variables are "1" and "2", whereas instanceNumber is 0 and 1 - bm_prep_stream << "gbmc-bare-metal-prep@" << this->instanceString - << ".target"; - std::string bm_prep_target = bm_prep_stream.str(); + std::string bm_prep_target = absl::StrCat( + "gbmc-bare-metal-prep@", this->systemdInstanceString, ".target"); // Start the bare metal prep target. This can be used to monitor the // ethernet connection to the host and wait until the CN shuts down the PCIe // connection. // Note: Use "systemctl restart" in case the target was already started on a // previous CN boot. - int ret = system(("systemctl restart " + bm_prep_target).c_str()); + int ret = + system(absl::StrCat("systemctl restart ", bm_prep_target).c_str()); if (ret != 0) { LOG(ERROR) << "Failed to start target: " << bm_prep_target << " error: " << ret; @@ -95,11 +95,9 @@ } // Now we'll start the gbmc-bare-metal-active target. - std::stringstream bm_active_stream; - bm_active_stream << "gbmc-bare-metal-active@" << this->instanceString - << ".target"; - std::string bm_active_target = bm_active_stream.str(); - ret = system(("systemctl start " + bm_active_target).c_str()); + std::string bm_active_target = absl::StrCat( + "gbmc-bare-metal-active@", this->systemdInstanceString, ".target"); + ret = system(absl::StrCat("systemctl start ", bm_active_target).c_str()); if (ret != 0) { LOG(ERROR) << "Failed to start target: " << bm_active_target << " error: " << ret; @@ -121,21 +119,19 @@ << instanceNumber; response->set_asset_tag(readBMInstanceFromFile( - std::string(bmInstanceBasePath) + "asset-tag" + instanceString)); - response->set_board_serial_number( - readBMInstanceFromFile(std::string(bmInstanceBasePath) + - "board-serial-number" + instanceString)); - response->set_family(readBMInstanceFromFile(std::string(bmInstanceBasePath) + - "family" + instanceString)); + absl::StrCat(bmInstanceBasePath, "asset-tag", fileInstanceString))); + response->set_board_serial_number(readBMInstanceFromFile(absl::StrCat( + bmInstanceBasePath, "board-serial-number", fileInstanceString))); + response->set_family(readBMInstanceFromFile( + absl::StrCat(bmInstanceBasePath, "family", fileInstanceString))); response->set_product_name(readBMInstanceFromFile( - std::string(bmInstanceBasePath) + "product-name" + instanceString)); - response->set_sku(readBMInstanceFromFile(std::string(bmInstanceBasePath) + - "sku" + instanceString)); - response->set_system_serial_number( - readBMInstanceFromFile(std::string(bmInstanceBasePath) + - "system-serial-number" + instanceString)); - response->set_uuid(readBMInstanceFromFile(std::string(bmInstanceBasePath) + - "uuid" + instanceString)); + absl::StrCat(bmInstanceBasePath, "product-name", fileInstanceString))); + response->set_sku(readBMInstanceFromFile( + absl::StrCat(bmInstanceBasePath, "sku", fileInstanceString))); + response->set_system_serial_number(readBMInstanceFromFile(absl::StrCat( + bmInstanceBasePath, "system-serial-number", fileInstanceString))); + response->set_uuid(readBMInstanceFromFile( + absl::StrCat(bmInstanceBasePath, "uuid", fileInstanceString))); return grpc::Status::OK; }
diff --git a/src/baremetal_callback.h b/src/baremetal_callback.h index 903749e..f4633ae 100644 --- a/src/baremetal_callback.h +++ b/src/baremetal_callback.h
@@ -3,6 +3,7 @@ #include <string> #include "baremetal.grpc.pb.h" +#include "absl/strings/str_cat.h" #include "grpcblob_defs.h" #include "grpcpp/server_context.h" #include "grpcpp/support/status.h" @@ -15,22 +16,25 @@ const std::string& basePath) : phosphor::baremetal::grpc_gen::BaremetalTransferService::Service(), instanceNumber(instance), - instanceString(std::to_string(instance + 1)), + systemdInstanceString(std::to_string(instance + 1)), + fileInstanceString(std::to_string(instance + 1)), basePath_(basePath), - bmSignalPath(basePath + "/run/bm-ready.flag"), - linuxBootDonePath(basePath + "/run/linux-boot-done.flag"), - bmInstanceBasePath(basePath + "/run/bm-instance/"), - bmDriveCleaningFlagPath(basePath + "/run/bm-drive-cleaning.flag"), - bmDriveCleaningDoneFlagPath(basePath + - "/run/bm-drive-cleaning-done.flag"), - bmDriveCleaningDoneAckFlagPath(basePath + - "/run/bm-drive-cleaning-done-ack.flag") + bmSignalPath(absl::StrCat(basePath, "/run/bm-ready.flag")), + linuxBootDonePath(absl::StrCat(basePath, "/run/linux-boot-done.flag")), + bmInstanceBasePath(absl::StrCat(basePath, "/run/bm-instance/")), + bmDriveCleaningFlagPath( + absl::StrCat(basePath, "/run/bm-drive-cleaning.flag")), + bmDriveCleaningDoneFlagPath( + absl::StrCat(basePath, "/run/bm-drive-cleaning-done.flag")), + bmDriveCleaningDoneAckFlagPath( + absl::StrCat(basePath, "/run/bm-drive-cleaning-done-ack.flag")) // Instance ID will be 1-indexed to better match the redfish Systems { - // For the last instance, use 0 as instanceString as last instance is used - // only in single-host machines. + // For the last instance (single-host machines), systemd targets expect "0", + // while file paths and Redfish expect no suffix (""). if ((instanceNumber + 1) == numInstances) { - instanceString = "0"; + systemdInstanceString = "0"; + fileInstanceString = ""; } } @@ -52,7 +56,8 @@ private: int instanceNumber; - std::string instanceString; + std::string systemdInstanceString; + std::string fileInstanceString; std::string basePath_; std::string bmSignalPath; std::string linuxBootDonePath;
diff --git a/src/test/baremetal_test.cc b/src/test/baremetal_test.cc index e878ca6..1a1938d 100644 --- a/src/test/baremetal_test.cc +++ b/src/test/baremetal_test.cc
@@ -8,6 +8,7 @@ #include <gmock/gmock.h> #include <gtest/gtest.h> #include "absl/log/log.h" +#include "absl/strings/str_cat.h" #include "absl/time/clock.h" #include "absl/time/time.h" #include "grpcblob_server.h" @@ -35,27 +36,36 @@ #define EXPECT_OK(x) EXPECT_TRUE(x.ok()) #endif -class BaremetalTest : public ::testing::Test { +struct BaremetalTestParam { + std::string port; + std::string instance_suffix; +}; + +class BaremetalTest : public ::testing::TestWithParam<BaremetalTestParam> { protected: static void SetUpTestSuite() { server_ = std::make_unique<GrpcBlobServer>("/tmp"); server_->start(); - - // 10166: (insecure gRPC), default case - used for tests here - // 10167: (insecure gRPC), CN 0 of multi-CN system - // 10168: (insecure gRPC), CN 1 of multi-CN system - std::shared_ptr<Channel> channel = grpc::CreateChannel( - "localhost:10166", grpc::InsecureChannelCredentials()); - baremetal_stub_ = BaremetalTransferService::NewStub(channel); } static void TearDownTestSuite() { absl::SleepFor(absl::Seconds(5)); server_->stop(); server_.reset(); - baremetal_stub_.reset(); } + void SetUp() override { + // 10166: (insecure gRPC), default case - single-CN system + // 10167: (insecure gRPC), CN 0 of multi-CN system + // 10168: (insecure gRPC), CN 1 of multi-CN system + std::shared_ptr<Channel> channel = + grpc::CreateChannel(absl::StrCat("localhost:", GetParam().port), + grpc::InsecureChannelCredentials()); + baremetal_stub_ = BaremetalTransferService::NewStub(channel); + } + + void TearDown() override { baremetal_stub_.reset(); } + static void createFile(const std::filesystem::path& path) { std::error_code ec; std::filesystem::path dirPath = path.parent_path(); @@ -80,15 +90,14 @@ } static std::unique_ptr<GrpcBlobServer> server_; - static std::unique_ptr<BaremetalTransferService::Stub> baremetal_stub_; + std::unique_ptr<BaremetalTransferService::Stub> baremetal_stub_; }; std::unique_ptr<GrpcBlobServer> BaremetalTest::server_; -std::unique_ptr<BaremetalTransferService::Stub> BaremetalTest::baremetal_stub_; -TEST_F(BaremetalTest, ServerStartsAndStops) { EXPECT_NE(server_, nullptr); } +TEST_P(BaremetalTest, ServerStartsAndStops) { EXPECT_NE(server_, nullptr); } -TEST_F(BaremetalTest, BmModeTransferNonBm) { +TEST_P(BaremetalTest, BmModeTransferNonBm) { BmModeRequest request; BmModeResponse response; ClientContext context; @@ -97,7 +106,7 @@ EXPECT_EQ(phosphor::baremetal::BmMode::BMMODE_NON_BM, response.bm_mode()); } -TEST_F(BaremetalTest, BmModeTransferBmSteadyState) { +TEST_P(BaremetalTest, BmModeTransferBmSteadyState) { createFile("/tmp/run/bm-ready.flag"); BmModeRequest request; BmModeResponse response; @@ -108,11 +117,13 @@ response.bm_mode()); removeFile("/tmp/run/bm-ready.flag"); - removeFile("/tmp/run/bm-drive-cleaning.flag0"); + removeFile(absl::StrCat("/tmp/run/bm-drive-cleaning.flag", + GetParam().instance_suffix)); } -TEST_F(BaremetalTest, BmModeTransferBmCleaningDone) { - createFile("/tmp/run/bm-drive-cleaning-done.flag0"); +TEST_P(BaremetalTest, BmModeTransferBmCleaningDone) { + createFile(absl::StrCat("/tmp/run/bm-drive-cleaning-done.flag", + GetParam().instance_suffix)); BmModeRequest request; BmModeResponse response; ClientContext context; @@ -120,12 +131,15 @@ EXPECT_OK(baremetal_stub_->BmModeTransfer(&context, request, &response)); EXPECT_EQ(phosphor::baremetal::BmMode::BMMODE_BM, response.bm_mode()); - removeFile("/tmp/run/bm-drive-cleaning-done.flag0"); - removeFile("/tmp/run/bm-drive-cleaning-done-ack.flag0"); + removeFile(absl::StrCat("/tmp/run/bm-drive-cleaning-done.flag", + GetParam().instance_suffix)); + removeFile(absl::StrCat("/tmp/run/bm-drive-cleaning-done-ack.flag", + GetParam().instance_suffix)); } -TEST_F(BaremetalTest, BmModeTransferBmCleaningDoneAck) { - createFile("/tmp/run/bm-drive-cleaning-done-ack.flag0"); +TEST_P(BaremetalTest, BmModeTransferBmCleaningDoneAck) { + createFile(absl::StrCat("/tmp/run/bm-drive-cleaning-done-ack.flag", + GetParam().instance_suffix)); BmModeRequest request; BmModeResponse response; ClientContext context; @@ -133,10 +147,11 @@ EXPECT_OK(baremetal_stub_->BmModeTransfer(&context, request, &response)); EXPECT_EQ(phosphor::baremetal::BmMode::BMMODE_BM, response.bm_mode()); - removeFile("/tmp/run/bm-drive-cleaning-done-ack.flag0"); + removeFile(absl::StrCat("/tmp/run/bm-drive-cleaning-done-ack.flag", + GetParam().instance_suffix)); } -TEST_F(BaremetalTest, LinuxBootDone) { +TEST_P(BaremetalTest, LinuxBootDone) { LinuxBootDoneRequest request; LinuxBootDoneResponse response; ClientContext context; @@ -144,21 +159,22 @@ EXPECT_OK(baremetal_stub_->LinuxBootDone(&context, request, &response)); } -TEST_F(BaremetalTest, BMInstancePropertiesTransfer) { +TEST_P(BaremetalTest, BMInstancePropertiesTransfer) { const std::string base_path = "/tmp/run/bm-instance/"; - const int instance = 0; + const std::string instance_suffix = GetParam().instance_suffix; const std::string asset_tag_path = - base_path + "asset-tag" + std::to_string(instance); + absl::StrCat(base_path, "asset-tag", instance_suffix); const std::string board_serial_number_path = - base_path + "board-serial-number" + std::to_string(instance); + absl::StrCat(base_path, "board-serial-number", instance_suffix); const std::string family_path = - base_path + "family" + std::to_string(instance); + absl::StrCat(base_path, "family", instance_suffix); const std::string product_name_path = - base_path + "product-name" + std::to_string(instance); - const std::string sku_path = base_path + "sku" + std::to_string(instance); + absl::StrCat(base_path, "product-name", instance_suffix); + const std::string sku_path = absl::StrCat(base_path, "sku", instance_suffix); const std::string system_serial_number_path = - base_path + "system-serial-number" + std::to_string(instance); - const std::string uuid_path = base_path + "uuid" + std::to_string(instance); + absl::StrCat(base_path, "system-serial-number", instance_suffix); + const std::string uuid_path = + absl::StrCat(base_path, "uuid", instance_suffix); createFile(asset_tag_path); createFile(board_serial_number_path); @@ -214,5 +230,12 @@ removeFile(uuid_path); } +INSTANTIATE_TEST_SUITE_P( + BaremetalTests, BaremetalTest, + ::testing::Values(BaremetalTestParam{"10166", ""}, // Single-host case + BaremetalTestParam{"10167", "1"}, // Multi-host CN 0 + BaremetalTestParam{"10168", "2"} // Multi-host CN 1 + )); + } // namespace } // namespace blobs