Skip to content

Commit

Permalink
Remove procmon checkin from Hal (#841)
Browse files Browse the repository at this point in the history 
Procmon is not used anymore and the failed checkin causes unnecessary error log messages. The Procmon code itself is not removed by this change, just the checkin from `Hal`.
Also cleans references to `cmal`, another google-only service, which we never had.

Progress towards #492
  • Loading branch information
@pudelkoM
pudelkoM committed Oct 15, 2021
1 parent d2b6442 commit 09ddb5a
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 126 deletions.
2 changes: 0 additions & 2 deletions stratum/hal/lib/common/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,6 @@ stratum_cc_library(
"//stratum/lib:utils",
"//stratum/lib/security:auth_policy_checker",
"//stratum/lib/security:credentials_manager",
"//stratum/procmon:procmon_cc_grpc",
],
)

Expand All @@ -546,7 +545,6 @@ stratum_cc_test(
"//stratum/lib/security:credentials_manager_mock",
"//stratum/lib/test_utils:matchers",
"//stratum/public/lib:error",
"//stratum/procmon:procmon_cc_grpc",
],
)

Expand Down
52 changes: 4 additions & 48 deletions stratum/hal/lib/common/hal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "stratum/lib/constants.h"
#include "stratum/lib/macros.h"
#include "stratum/lib/utils.h"
#include "stratum/procmon/procmon.grpc.pb.h"

// TODO(unknown): Use FLAG_DEFINE for all flags.
DEFINE_string(external_stratum_urls, stratum::kExternalStratumUrls,
Expand All @@ -27,8 +26,6 @@ DEFINE_string(external_stratum_urls, stratum::kExternalStratumUrls,
DEFINE_string(local_stratum_url, stratum::kLocalStratumUrl,
"URL for listening to local calls from stratum stub.");
DEFINE_bool(warmboot, false, "Determines whether HAL is in warmboot stage.");
DEFINE_string(procmon_service_addr, ::stratum::kProcmonServiceUrl,
"URL of the procmon service to connect to.");
DEFINE_string(persistent_config_dir, "/etc/stratum/",
"The persistent dir where all the config files will be stored.");
DEFINE_int32(grpc_keepalive_time_ms, 600000, "grpc keep alive time");
Expand Down Expand Up @@ -111,20 +108,12 @@ ::util::Status Hal::SanityCheck() {
CHECK_RETURN_IF_FALSE(!external_stratum_urls.empty())
<< "No external URL was given. This is invalid.";

auto it =
std::find_if(external_stratum_urls.begin(), external_stratum_urls.end(),
[](const std::string& url) {
return (url == FLAGS_local_stratum_url ||
// FIXME(boc) google only url ==
// FLAGS_cmal_service_url ||
url == FLAGS_procmon_service_addr);
});
auto it = std::find_if(
external_stratum_urls.begin(), external_stratum_urls.end(),
[](const std::string& url) { return url == FLAGS_local_stratum_url; });
CHECK_RETURN_IF_FALSE(it == external_stratum_urls.end())
<< "You used one of these reserved local URLs as your external URLs: "
<< FLAGS_local_stratum_url
<< ", "
/*FIXME(boc) google only << FLAGS_cmal_service_url */
<< ", " << FLAGS_procmon_service_addr << ".";
<< FLAGS_local_stratum_url << ".";

CHECK_RETURN_IF_FALSE(!FLAGS_persistent_config_dir.empty())
<< "persistent_config_dir flag needs to be explicitly given.";
Expand Down Expand Up @@ -233,16 +222,6 @@ ::util::Status Hal::Run() {
<< FLAGS_local_stratum_url << "...";
}

if (mode_ != OPERATION_MODE_SIM) {
// Try checking in with Procmon if we are not running in sim mode. Continue
// if checkin fails.
::util::Status status = ProcmonCheckin();
if (!status.ok()) {
LOG(ERROR) << "Error when checking in with procmon: "
<< status.error_message() << ".";
}
}

external_server_->Wait(); // blocking until external_server_->Shutdown()
// is called. We dont wait on internal_service.
return Teardown();
Expand Down Expand Up @@ -367,29 +346,6 @@ ::util::Status Hal::UnregisterSignalHandlers() {
return ::util::OkStatus();
}

::util::Status Hal::ProcmonCheckin() {
// FIXME replace Procmon with gNOI
std::unique_ptr<procmon::ProcmonService::Stub> stub =
procmon::ProcmonService::NewStub(::grpc::CreateChannel(
FLAGS_procmon_service_addr, ::grpc::InsecureChannelCredentials()));
if (stub == nullptr) {
return MAKE_ERROR(ERR_INTERNAL)
<< "Could not create stub for procmon gRPC service.";
}

procmon::CheckinRequest req;
procmon::CheckinResponse resp;
::grpc::ClientContext context;
req.set_checkin_key(getpid());
::grpc::Status status = stub->Checkin(&context, req, &resp);
if (!status.ok()) {
return MAKE_ERROR(ERR_INTERNAL)
<< "Failed to check in with procmon: " << status.error_message();
}

return ::util::OkStatus();
}

void* Hal::SignalWaiterThreadFunc(void*) {
int signal_value;
int ret = read(Hal::pipe_read_fd_, &signal_value, sizeof(signal_value));
Expand Down
5 changes: 0 additions & 5 deletions stratum/hal/lib/common/hal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "grpcpp/grpcpp.h"
#include "stratum/hal/lib/common/admin_service.h"
#include "stratum/hal/lib/common/certificate_management_service.h"
// #include "stratum/hal/lib/common/cmal_service.h"
#include "stratum/hal/lib/common/common.pb.h"
#include "stratum/hal/lib/common/config_monitoring_service.h"
#include "stratum/hal/lib/common/diag_service.h"
Expand Down Expand Up @@ -111,10 +110,6 @@ class Hal final {
::util::Status RegisterSignalHandlers();
::util::Status UnregisterSignalHandlers();

// Sends an RPC to procmon gRPC service to checkin. To be called before
// ::grpc::Server::Wait().
::util::Status ProcmonCheckin();

// Thread function waiting for a signal in the pipe and then initialting the
// HAL shutdown.
static void* SignalWaiterThreadFunc(void*);
Expand Down
72 changes: 1 addition & 71 deletions stratum/hal/lib/common/hal_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include "stratum/lib/security/auth_policy_checker_mock.h"
#include "stratum/lib/security/credentials_manager_mock.h"
#include "stratum/lib/utils.h"
#include "stratum/procmon/procmon.grpc.pb.h"
#include "stratum/public/lib/error.h"

DECLARE_string(external_stratum_urls);
Expand All @@ -24,7 +23,6 @@ DECLARE_string(chassis_config_file);
DECLARE_string(forwarding_pipeline_configs_file);
DECLARE_string(test_tmpdir);
DECLARE_string(local_stratum_url);
DECLARE_string(procmon_service_addr);
DECLARE_string(persistent_config_dir);

namespace stratum {
Expand All @@ -36,33 +34,6 @@ using ::testing::Return;

MATCHER_P(EqualsProto, proto, "") { return ProtoEqual(arg, proto); }

// A fake implementation of ProcmonService for unit testing.
class FakeProcmonService final : public procmon::ProcmonService::Service {
public:
FakeProcmonService() : pid_(-1) {}
~FakeProcmonService() override {}

void SetPid(int pid) { pid_ = pid; }

::grpc::Status Checkin(::grpc::ServerContext* context,
const procmon::CheckinRequest* req,
procmon::CheckinResponse* resp) override {
// Fake a behavior where for checkin_key = kGoodPid we return OK and for
// other checkin_keys we return error.
if (req->checkin_key() == pid_) {
return ::grpc::Status::OK;
}
return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, "Invalid pid.");
}

// FakeProcmonService is neither copyable nor movable.
FakeProcmonService(const FakeProcmonService&) = delete;
FakeProcmonService& operator=(const FakeProcmonService&) = delete;

private:
int pid_;
};

class HalTest : public ::testing::Test {
protected:
static const std::string RandomURL() {
Expand All @@ -81,31 +52,16 @@ class HalTest : public ::testing::Test {
hal_ = Hal::CreateSingleton(kMode, switch_mock_, auth_policy_checker_mock_,
credentials_manager_mock_);
ASSERT_NE(hal_, nullptr);

// Create and start a FakeProcmonService instance for testing purpose.
FLAGS_procmon_service_addr = RandomURL();
procmon_service_ = new FakeProcmonService();
::grpc::ServerBuilder builder;
builder.AddListeningPort(FLAGS_procmon_service_addr,
::grpc::InsecureServerCredentials());
builder.RegisterService(procmon_service_);
procmon_server_ = builder.BuildAndStart().release();
ASSERT_NE(procmon_server_, nullptr);
}

// Per-test-case tear-down.
static void TearDownTestCase() {
procmon_server_->Shutdown(std::chrono::system_clock::now());
delete switch_mock_;
delete auth_policy_checker_mock_;
delete credentials_manager_mock_;
delete procmon_service_;
delete procmon_server_;
switch_mock_ = nullptr;
auth_policy_checker_mock_ = nullptr;
credentials_manager_mock_ = nullptr;
procmon_service_ = nullptr;
procmon_server_ = nullptr;
}

void SetUp() override {
Expand All @@ -116,7 +72,6 @@ class HalTest : public ::testing::Test {
FLAGS_external_stratum_urls =
absl::StrJoin({RandomURL(), RandomURL()}, ",");
FLAGS_local_stratum_url = RandomURL();
// FLAGS_cmal_service_url = RandomURL(); // google only
ASSERT_OK(hal_->SanityCheck());
hal_->ClearErrors();
}
Expand Down Expand Up @@ -193,8 +148,6 @@ class HalTest : public ::testing::Test {
static ::testing::StrictMock<CredentialsManagerMock>*
credentials_manager_mock_;
static Hal* hal_; // pointer which points to the singleton instance
static FakeProcmonService* procmon_service_;
static ::grpc::Server* procmon_server_;
};

constexpr char HalTest::kChassisConfigTemplate[];
Expand All @@ -211,8 +164,6 @@ ::testing::StrictMock<AuthPolicyCheckerMock>*
::testing::StrictMock<CredentialsManagerMock>*
HalTest::credentials_manager_mock_ = nullptr;
Hal* HalTest::hal_ = nullptr;
FakeProcmonService* HalTest::procmon_service_ = nullptr;
::grpc::Server* HalTest::procmon_server_ = nullptr;

TEST_F(HalTest, SanityCheckFailureWhenExtURLsNotGiven) {
FLAGS_external_stratum_urls = "";
Expand Down Expand Up @@ -543,14 +494,13 @@ void* TestShutdownThread(void* arg) {

} // namespace

TEST_F(HalTest, StartAndShutdownServerWhenProcmonCheckinSucceeds) {
TEST_F(HalTest, StartAndShutdownServerSucceeds) {
EXPECT_CALL(*switch_mock_, Shutdown()).WillOnce(Return(::util::OkStatus()));
EXPECT_CALL(*auth_policy_checker_mock_, Shutdown())
.WillOnce(Return(::util::OkStatus()));
EXPECT_CALL(*credentials_manager_mock_,
GenerateExternalFacingServerCredentials())
.WillOnce(Return(::grpc::InsecureServerCredentials()));
procmon_service_->SetPid(getpid());

pthread_t tid;
ASSERT_EQ(0, pthread_create(&tid, nullptr, &TestShutdownThread, hal_));
Expand All @@ -562,25 +512,5 @@ TEST_F(HalTest, StartAndShutdownServerWhenProcmonCheckinSucceeds) {
ASSERT_EQ(0, pthread_join(tid, nullptr));
}

TEST_F(HalTest, StartAndShutdownServerWhenProcmonCheckinFails) {
EXPECT_CALL(*switch_mock_, Shutdown()).WillOnce(Return(::util::OkStatus()));
EXPECT_CALL(*auth_policy_checker_mock_, Shutdown())
.WillOnce(Return(::util::OkStatus()));
EXPECT_CALL(*credentials_manager_mock_,
GenerateExternalFacingServerCredentials())
.WillOnce(Return(::grpc::InsecureServerCredentials()));
procmon_service_->SetPid(getpid() + 1);

pthread_t tid;
ASSERT_EQ(0, pthread_create(&tid, nullptr, &TestShutdownThread, hal_));

// Call and validate results. Even if Checkin is false, we still do not return
// any error. We just log an error.
FLAGS_warmboot = false;
ASSERT_OK(hal_->Run()); // blocking until HandleSignal() is called in
// TestShutdownThread()
ASSERT_EQ(0, pthread_join(tid, nullptr));
}

} // namespace hal
} // namespace stratum

0 comments on commit 09ddb5a

Please sign in to comment.