From d6c3a5833d19bee4043168016ef0504b88cbfb9a Mon Sep 17 00:00:00 2001
From: Lasse Tjernaes Wardenaer <lasse.tjernaes.wardenaer@cern.ch>
Date: Thu, 23 Feb 2023 16:35:00 +0100
Subject: [PATCH] Resolve "Improve error message for grpc calls"
---
ReleaseNotes.md | 1 +
.../ChangeStorageClass.cpp | 32 +++++++++++------
.../EosNamespaceInjection.cpp | 31 ++++++++--------
common/CMakeLists.txt | 1 +
common/exception/GrpcError.cpp | 26 ++++++++++++++
common/exception/GrpcError.hpp | 35 +++++++++++++++++++
eos_grpc_client/GrpcClient.cpp | 26 +++++++-------
eos_grpc_client/GrpcClient.hpp | 3 +-
eos_grpc_client/GrpcEndpoint.cpp | 4 +--
eos_grpc_client/GrpcEndpoint.hpp | 5 ++-
10 files changed, 118 insertions(+), 46 deletions(-)
create mode 100644 common/exception/GrpcError.cpp
create mode 100644 common/exception/GrpcError.hpp
diff --git a/ReleaseNotes.md b/ReleaseNotes.md
index d089b97823..bab354d983 100644
--- a/ReleaseNotes.md
+++ b/ReleaseNotes.md
@@ -2,6 +2,7 @@
### Features
- cta/CTA#213 - Improve error messages for `cta-eos-namespace-inject`
- cta/CTA#308 - Remove catalogue autogenerated files
+- cta/CTA#294 - Improve error messages for 'Exec' in the gRPC client
### Bug Fixes
- cta/CTA#305 - AllCatalogueSchema file not created when updating schema
diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
index b79cff7040..573b49e5c2 100644
--- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
+++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp
@@ -35,6 +35,7 @@
#include "cmdline/standalone_cli_tools/common/ConnectionConfiguration.hpp"
#include "common/checksum/ChecksumBlob.hpp"
#include "common/exception/CommandLineNotParsed.hpp"
+#include "common/exception/GrpcError.hpp"
#include "common/exception/UserError.hpp"
#include "common/log/StdoutLogger.hpp"
#include "common/utils/utils.hpp"
@@ -50,6 +51,11 @@ extern std::list<std::string> g_storageClasses;
namespace cta {
namespace cliTool {
+class ChangeStorageClassError : public std::runtime_error {
+public:
+ using runtime_error::runtime_error;
+}; // class ChangeStorageClassError
+
//------------------------------------------------------------------------------
// constructor
//------------------------------------------------------------------------------
@@ -147,13 +153,17 @@ void ChangeStorageClass::updateStorageClassInEosNamespace() {
const auto [diskInstance, diskFileId] = CatalogueFetch::getInstanceAndFid(archiveFileId, m_serviceProviderPtr, m_log);
// No files in flight should change storage class
- if (const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, cta::utils::toUint64(diskFileId), "", false);
+ const bool showJson = false;
+ if(const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, cta::utils::toUint64(diskFileId), "", showJson);
fileInFlight(md_response.fmd().locations())) {
- m_archiveIdsNotUpdatedInEos.push_back(archiveFileId);
- std::list<cta::log::Param> params;
- params.push_back(cta::log::Param("archiveFileId", archiveFileId));
- m_log(cta::log::WARNING, "File did not change storage class because the file was in flight", params);
- continue;
+ if (md_response.fmd().locations().empty()){
+ throw ChangeStorageClassError("Metadata from EOS could not be fetched: disk file id " + diskFileId + " does not exist or authentication is failing");
+ }
+ m_archiveIdsNotUpdatedInEos.push_back(archiveFileId);
+ std::list<cta::log::Param> params;
+ params.push_back(cta::log::Param("archiveFileId", archiveFileId));
+ m_log(cta::log::WARNING, "File did not change storage class because the file was in flight", params);
+ continue;
}
const auto path = m_endpointMapPtr->getPath(diskInstance, diskFileId);
@@ -162,16 +172,16 @@ void ChangeStorageClass::updateStorageClassInEosNamespace() {
params.push_back(cta::log::Param("path", path));
m_log(cta::log::INFO, "Path found", params);
}
- const auto status = m_endpointMapPtr->setXAttr(diskInstance, path, "sys.archive.storage_class", m_storageClassName);
- if (status != 0) {
+
+ if(auto status = m_endpointMapPtr->setXAttr(diskInstance, path, "sys.archive.storage_class", m_storageClassName); status.ok()) {
+ m_archiveIdsUpdatedInEos.push_back(archiveFileId);
+ } else {
m_archiveIdsNotUpdatedInEos.push_back(archiveFileId);
std::list<cta::log::Param> params;
params.push_back(cta::log::Param("archiveFileId", archiveFileId));
+ params.push_back(cta::log::Param("error", status.error_message()));
m_log(cta::log::WARNING, "File did not change storage class because query to EOS failed", params);
}
- else {
- m_archiveIdsUpdatedInEos.push_back(archiveFileId);
- }
}
}
diff --git a/cmdline/standalone_cli_tools/eos_namespace_injection/EosNamespaceInjection.cpp b/cmdline/standalone_cli_tools/eos_namespace_injection/EosNamespaceInjection.cpp
index e58dbe0093..91b468885d 100644
--- a/cmdline/standalone_cli_tools/eos_namespace_injection/EosNamespaceInjection.cpp
+++ b/cmdline/standalone_cli_tools/eos_namespace_injection/EosNamespaceInjection.cpp
@@ -94,8 +94,12 @@ void IStreamBuffer<cta::xrd::Data>::DataCallback(cta::xrd::Data record) const {
}
-namespace cta {
-namespace cliTool {
+namespace cta::cliTool {
+
+class EosNameSpaceInjectionError : public std::runtime_error {
+public:
+ using runtime_error::runtime_error;
+}; // class EosNameSpaceInjectionError
//------------------------------------------------------------------------------
// constructor
@@ -157,12 +161,18 @@ int EosNamespaceInjection::exceptionThrowingMain(const int argc, char *const *co
}
if(const auto fid = getFileIdEos(metaDataObject.diskInstance, metaDataObject.eosPath); fid != 0) {
- throw std::runtime_error("The file with path " + metaDataObject.eosPath + " already exists for instance " + metaDataObject.diskInstance + ". This tool does not overwrite existing files");
+ throw cta::cliTool::EosNameSpaceInjectionError("The file with path " + metaDataObject.eosPath + " already exists for instance " + metaDataObject.diskInstance + ". This tool does not overwrite existing files");
}
const auto newFid = createFileInEos(metaDataObject, parentId, uid, gid);
- if(newFid == 0) {
- throw std::runtime_error("The file with path " + metaDataObject.eosPath + " was not created");
+ if (newFid != 0) {
+ std::list<cta::log::Param> params;
+ params.push_back(cta::log::Param("diskFileId", newFid));
+ m_log(cta::log::INFO, "File was created in the EOS namespace", params);
+ } else {
+ std::list<cta::log::Param> params;
+ params.push_back(cta::log::Param("diskFileId", newFid));
+ m_log(cta::log::WARNING, "Could not find file in the EOS namespace. Check that gRPC authentication is set up correctly, and that the path exists", params);
}
auto decimalToHexadecimal = [](const std::string &decimalNumber) {
@@ -191,7 +201,7 @@ int EosNamespaceInjection::exceptionThrowingMain(const int argc, char *const *co
params.push_back(cta::log::Param("eosArchiveFileId", eosArchiveFileId));
params.push_back(cta::log::Param("eosChecksum", eosChecksum));
m_log(cta::log::WARNING, "File metadata in EOS and CTA does not match", params);
- throw std::runtime_error("Sanity check failed.");
+ throw cta::cliTool::EosNameSpaceInjectionError("Sanity check failed.");
}
}
return 0;
@@ -347,12 +357,6 @@ uint64_t EosNamespaceInjection::getFileIdEos(const std::string &diskInstance, co
m_log(cta::log::DEBUG, "Querying for file metadata in the EOS namespace", params);
const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, 0, path, false);
const auto fid = md_response.fmd().id();
- params.push_back(cta::log::Param("diskFileId", fid));
- if (fid != 0) {
- m_log(cta::log::INFO, "File path exists in the EOS namespace", params);
- } else {
- m_log(cta::log::WARNING, "Could not find path in the EOS namespace. Check that gRPC authentication is set up correctly, and that the path exists", params);
- }
return fid;
}
@@ -453,5 +457,4 @@ std::pair<ArchiveId, Checksum> EosNamespaceInjection::getArchiveFileIdAndChecksu
return std::make_pair(archiveFileId,checksumValue);
}
-} // namespace cliTool
-} // namespace cta
\ No newline at end of file
+} // namespace cta::cliTool
\ No newline at end of file
diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index c9be052e0b..14f8766a22 100644
--- a/common/CMakeLists.txt
+++ b/common/CMakeLists.txt
@@ -96,6 +96,7 @@ set (COMMON_LIB_SRC_FILES
exception/Errnum.cpp
exception/Exception.cpp
exception/ForceDismountFailed.cpp
+ exception/GrpcError.cpp
exception/InvalidArgument.cpp
exception/InvalidConfigEntry.cpp
exception/LostDatabaseConnection.cpp
diff --git a/common/exception/GrpcError.cpp b/common/exception/GrpcError.cpp
new file mode 100644
index 0000000000..7b797678d3
--- /dev/null
+++ b/common/exception/GrpcError.cpp
@@ -0,0 +1,26 @@
+/*
+ * @project The CERN Tape Archive (CTA)
+ * @copyright Copyright © 2021-2022 CERN
+ * @license This program is free software, distributed under the terms of the GNU General Public
+ * Licence version 3 (GPL Version 3), copied verbatim in the file "COPYING". You can
+ * redistribute it and/or modify it under the terms of the GPL Version 3, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
+ * PARTICULAR PURPOSE. See the GNU General Public License for more details.
+ *
+ * In applying this licence, CERN does not waive the privileges and immunities
+ * granted to it by virtue of its status as an Intergovernmental Organization or
+ * submit itself to any jurisdiction.
+ */
+
+#include "common/exception/GrpcError.hpp"
+
+
+// -----------------------------------------------------------------------------
+// Constructor
+// -----------------------------------------------------------------------------
+cta::exception::GrpcError::GrpcError(const std::string &context, const bool embedBacktrace):
+ cta::exception::Exception(context, embedBacktrace) {
+}
diff --git a/common/exception/GrpcError.hpp b/common/exception/GrpcError.hpp
new file mode 100644
index 0000000000..398541ea84
--- /dev/null
+++ b/common/exception/GrpcError.hpp
@@ -0,0 +1,35 @@
+/*
+ * @project The CERN Tape Archive (CTA)
+ * @copyright Copyright © 2021-2022 CERN
+ * @license This program is free software, distributed under the terms of the GNU General Public
+ * Licence version 3 (GPL Version 3), copied verbatim in the file "COPYING". You can
+ * redistribute it and/or modify it under the terms of the GPL Version 3, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
+ * PARTICULAR PURPOSE. See the GNU General Public License for more details.
+ *
+ * In applying this licence, CERN does not waive the privileges and immunities
+ * granted to it by virtue of its status as an Intergovernmental Organization or
+ * submit itself to any jurisdiction.
+ */
+
+#pragma once
+
+#include "common/exception/Exception.hpp"
+
+namespace cta::exception {
+
+ class GrpcError : public cta::exception::Exception {
+
+ public:
+
+ /**
+ * Constructor
+ */
+ GrpcError(const std::string &context, const bool embedBacktrace = true);
+
+ }; // class GrpcError
+
+} // namespace cta exception
\ No newline at end of file
diff --git a/eos_grpc_client/GrpcClient.cpp b/eos_grpc_client/GrpcClient.cpp
index 87ca3f8f2c..9512905c59 100644
--- a/eos_grpc_client/GrpcClient.cpp
+++ b/eos_grpc_client/GrpcClient.cpp
@@ -18,6 +18,7 @@
#include <sys/stat.h>
#include <sstream>
#include "GrpcClient.hpp"
+#include "common/exception/GrpcError.hpp"
namespace eos {
namespace client {
@@ -73,7 +74,7 @@ std::string GrpcClient::ping(const std::string& payload)
if(status.ok()) {
return reply.message();
} else {
- throw std::runtime_error("Ping failed with error: " + status.error_message());
+ throw cta::exception::GrpcError("Ping failed with error: " + status.error_message());
}
}
@@ -119,7 +120,7 @@ int GrpcClient::FileInsert(const std::vector<eos::rpc::FileMdProto> &files, eos:
}
return num_errors;
} else {
- throw std::runtime_error("FileInsert failed with error: " + status.error_message());
+ throw cta::exception::GrpcError("FileInsert failed with error: " + status.error_message());
}
}
@@ -175,7 +176,7 @@ int GrpcClient::ContainerInsert(const std::vector<eos::rpc::ContainerMdProto> &d
}
return num_errors;
} else {
- throw std::runtime_error("ContainerInsert failed with error: " + status.error_message());
+ throw cta::exception::GrpcError("ContainerInsert failed with error: " + status.error_message());
}
}
@@ -207,7 +208,7 @@ void GrpcClient::GetCurrentIds(uint64_t &cid, uint64_t &fid)
cid = m_eos_cid = response.current_cid();
fid = m_eos_fid = response.current_fid();
} else {
- throw std::runtime_error("EOS namespace query failed with error: " + status.error_message());
+ throw cta::exception::GrpcError("GetCurrentIds namespace query failed with error: " + status.error_message());
}
}
@@ -257,30 +258,27 @@ eos::rpc::MDResponse GrpcClient::GetMD(eos::rpc::TYPE type, uint64_t id, const s
return response;
}
-using QueryStatus = int;
-QueryStatus GrpcClient::Exec(eos::rpc::NSRequest& request, eos::rpc::NSResponse& reply) const {
+grpc::Status GrpcClient::Exec(eos::rpc::NSRequest& request) {
request.set_authkey(token());
+ auto tag = nextTag();
+
+ eos::rpc::NSResponse response;
grpc::ClientContext context;
grpc::CompletionQueue cq;
grpc::Status status;
std::unique_ptr<grpc::ClientAsyncResponseReader<eos::rpc::NSResponse> > rpc(
stub_->AsyncExec(&context, request, &cq));
- rpc->Finish(&reply, &status, (void*) 1);
+ rpc->Finish(&response, &status, tag);
void* got_tag;
bool ok = false;
GPR_ASSERT(cq.Next(&got_tag, &ok));
- GPR_ASSERT(got_tag == (void*) 1);
+ GPR_ASSERT(got_tag == tag);
GPR_ASSERT(ok);
- // Act upon the status of the actual RPC.
- if (status.ok()) {
- return reply.error().code();
- } else {
- return -1;
- }
+ return status;
}
}} // namespace eos::client
diff --git a/eos_grpc_client/GrpcClient.hpp b/eos_grpc_client/GrpcClient.hpp
index 79c315181f..c4e16aff8c 100644
--- a/eos_grpc_client/GrpcClient.hpp
+++ b/eos_grpc_client/GrpcClient.hpp
@@ -49,8 +49,7 @@ public:
// Obtain container or file metadata
eos::rpc::MDResponse GetMD(eos::rpc::TYPE type, uint64_t id, const std::string &path, bool showJson = false);
- using QueryStatus = int;
- QueryStatus Exec(eos::rpc::NSRequest& request, eos::rpc::NSResponse& reply) const;
+ grpc::Status Exec(eos::rpc::NSRequest& request);
void set_ssl(bool onoff) {
m_SSL = onoff;
diff --git a/eos_grpc_client/GrpcEndpoint.cpp b/eos_grpc_client/GrpcEndpoint.cpp
index 10878ebf9a..d1be73fd24 100644
--- a/eos_grpc_client/GrpcEndpoint.cpp
+++ b/eos_grpc_client/GrpcEndpoint.cpp
@@ -62,13 +62,13 @@ eos::rpc::MDResponse eos::client::Endpoint::getMD(eos::rpc::TYPE type, uint64_t
return m_grpcClient->GetMD(type, id, path, showJson);
}
-int eos::client::Endpoint::setXAttr(const std::string &path, const std::string &attrKey, const std::string &attrValue) const {
+grpc::Status eos::client::Endpoint::setXAttr(const std::string &path, const std::string &attrKey, const std::string &attrValue) const {
eos::rpc::NSRequest request;
eos::rpc::NSResponse reply;
request.mutable_xattr()->mutable_id()->set_path(path);
(*(request.mutable_xattr()->mutable_xattrs()))[attrKey] = attrValue;
- return m_grpcClient->Exec(request, reply);
+ return m_grpcClient->Exec(request);
}
diff --git a/eos_grpc_client/GrpcEndpoint.hpp b/eos_grpc_client/GrpcEndpoint.hpp
index 88a2263d8f..08fe1a91fe 100644
--- a/eos_grpc_client/GrpcEndpoint.hpp
+++ b/eos_grpc_client/GrpcEndpoint.hpp
@@ -37,7 +37,7 @@ public:
void getCurrentIds(uint64_t &cid, uint64_t &fid) const;
::eos::rpc::MDResponse getMD(::eos::rpc::TYPE type, uint64_t id, const std::string &path, bool showJson) const;
using QueryStatus = int;
- [[nodiscard]] QueryStatus setXAttr(const std::string &path, const std::string &attrKey, const std::string &attrValue) const;
+ grpc::Status setXAttr(const std::string &path, const std::string &attrKey, const std::string &attrValue) const;
private:
std::unique_ptr<::eos::client::GrpcClient> m_grpcClient;
@@ -99,8 +99,7 @@ public:
}
}
- using QueryStatus = int;
- [[nodiscard]] QueryStatus setXAttr(const std::string &diskInstance, const std::string &path, const std::string &attrKey, const std::string &attrValue) const {
+ grpc::Status setXAttr(const std::string &diskInstance, const std::string &path, const std::string &attrKey, const std::string &attrValue) const {
auto ep_it = m_endpointMap.find(diskInstance);
if(ep_it == m_endpointMap.end()) {
throw cta::exception::UserError("Namespace for disk instance \"" + diskInstance + "\" is not configured in the CTA Frontend");
--
GitLab