From c78112f5c5e621ccfda2d6946482cc554c1b98d2 Mon Sep 17 00:00:00 2001 From: lwardena <lasse.tjernaes.wardenaer@cern.ch> Date: Mon, 12 Dec 2022 15:41:09 +0100 Subject: [PATCH 1/4] Add small code quality changes --- .../ChangeStorageClass.cpp | 25 ++++++++----------- .../ChangeStorageClass.hpp | 8 +++--- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp index a8ff83a083..ba5a535c75 100644 --- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp +++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp @@ -85,7 +85,7 @@ ChangeStorageClass::ChangeStorageClass( } if (cmdLineArgs.m_archiveFileIds) { - for (const auto archiveFileId : cmdLineArgs.m_archiveFileIds.value() ) { + for (const auto& archiveFileId : cmdLineArgs.m_archiveFileIds.value() ) { m_archiveFileIds.push_back(archiveFileId); } } @@ -104,12 +104,9 @@ ChangeStorageClass::ChangeStorageClass( //------------------------------------------------------------------------------ // fileInFlight //------------------------------------------------------------------------------ -bool ChangeStorageClass::fileInFlight(const google::protobuf::RepeatedField<uint32_t> &locations) { - for (auto location : locations) { - // file is not in flight if fsid==65535 - if (location == 65535) { return false; } - } - return true; +bool ChangeStorageClass::fileInFlight(const google::protobuf::RepeatedField<uint32_t> &locations) const { + // file is not in flight if fsid==65535 + return std::all_of(std::begin(locations), std::end(locations), [](const int &location) { return location != 65535; }); } @@ -128,8 +125,8 @@ void ChangeStorageClass::updateStorageClassInEosNamespace() { const auto [diskInstance, diskFileId] = CatalogueFetch::getInstanceAndFid(archiveFileId, m_serviceProviderPtr, m_log); // No files in flight should change storage class - const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, cta::utils::toUint64(diskFileId), "", false); - if (fileInFlight(md_response.fmd().locations())) { + if (const auto md_response = m_endpointMapPtr->getMD(diskInstance, ::eos::rpc::FILE, cta::utils::toUint64(diskFileId), "", false); + fileInFlight(md_response.fmd().locations())) { m_archiveIdsNotUpdatedInEos.push_back(archiveFileId); std::list<cta::log::Param> params; params.push_back(cta::log::Param("archiveFileId", archiveFileId)); @@ -154,7 +151,7 @@ void ChangeStorageClass::updateStorageClassInEosNamespace() { //------------------------------------------------------------------------------ // storageClassExists //------------------------------------------------------------------------------ -void ChangeStorageClass::storageClassExists() { +void ChangeStorageClass::storageClassExists() const { cta::xrd::Request request; const auto admincmd = request.mutable_admincmd(); @@ -185,7 +182,7 @@ void ChangeStorageClass::storageClassExists() { // wait until the data stream has been processed before exiting stream_future.wait(); - std::list<std::string>::iterator storageClassFound = std::find(g_storageClasses.begin(), g_storageClasses.end(), m_storageClassName); + auto storageClassFound = std::find(g_storageClasses.begin(), g_storageClasses.end(), m_storageClassName); if ( g_storageClasses.end() == storageClassFound ){ throw(exception::UserError("The storage class provided has not been defined.")); } @@ -194,7 +191,7 @@ void ChangeStorageClass::storageClassExists() { //------------------------------------------------------------------------------ // updateCatalogue //------------------------------------------------------------------------------ -void ChangeStorageClass::updateStorageClassInCatalogue() { +void ChangeStorageClass::updateStorageClassInCatalogue() const { cta::xrd::Request request; const auto admincmd = request.mutable_admincmd(); @@ -249,7 +246,7 @@ void ChangeStorageClass::updateStorageClassInCatalogue() { //------------------------------------------------------------------------------ // writeSkippedArchiveIdsToFile //------------------------------------------------------------------------------ -void ChangeStorageClass::writeSkippedArchiveIdsToFile() { +void ChangeStorageClass::writeSkippedArchiveIdsToFile() const { const std::filesystem::path filePath = "/var/log/skippedArchiveIds.txt"; std::ofstream archiveIdFile(filePath); @@ -258,7 +255,7 @@ void ChangeStorageClass::writeSkippedArchiveIdsToFile() { } if (archiveIdFile.is_open()) { - for (const auto archiveId : m_archiveIdsNotUpdatedInEos) { + for (const auto& archiveId : m_archiveIdsNotUpdatedInEos) { archiveIdFile << archiveId << std::endl; } archiveIdFile.close(); diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp index 87a5b6187a..4fe030f8c8 100644 --- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp +++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp @@ -105,22 +105,22 @@ private: * Checks if the storage class provided to the tool is defined, * and throws an exception::UserError if it is not found */ - void storageClassExists(); + void storageClassExists() const; /** * Updates the storage class name in the catalogue */ - void updateStorageClassInCatalogue(); + void updateStorageClassInCatalogue() const; /** * Writes the skipped archive ids to file */ - void writeSkippedArchiveIdsToFile(); + void writeSkippedArchiveIdsToFile() const; /** * Checks if a file is in flight */ - bool fileInFlight(const google::protobuf::RepeatedField<unsigned int> &locations); + bool fileInFlight(const google::protobuf::RepeatedField<unsigned int> &locations) const; /** * An exception throwing version of main(). -- GitLab From 3c241240c0ac139fa60e0c4bbd43a085a3ccc2d2 Mon Sep 17 00:00:00 2001 From: lwardena <lasse.tjernaes.wardenaer@cern.ch> Date: Mon, 12 Dec 2022 15:58:17 +0100 Subject: [PATCH 2/4] Move code out of constructor --- .../ChangeStorageClass.cpp | 42 +++++++++---------- .../ChangeStorageClass.hpp | 4 +- .../ChangeStorageClassMain.cpp | 2 +- 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp index ba5a535c75..b82cd778d5 100644 --- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp +++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp @@ -34,9 +34,9 @@ #include "cmdline/standalone_cli_tools/common/ConnectionConfiguration.hpp" #include "common/checksum/ChecksumBlob.hpp" #include "common/exception/CommandLineNotParsed.hpp" -#include "common/utils/utils.hpp" +#include "common/exception/UserError.hpp" #include "common/log/StdoutLogger.hpp" - +#include "common/utils/utils.hpp" #include "CtaFrontendApi.hpp" // GLOBAL VARIABLES : used to pass information between main thread and stream handler thread @@ -55,27 +55,29 @@ ChangeStorageClass::ChangeStorageClass( std::istream &inStream, std::ostream &outStream, std::ostream &errStream, - cta::log::StdoutLogger &log, - const int& argc, - char *const *const argv): + cta::log::StdoutLogger &log): CmdLineTool(inStream, outStream, errStream), - m_log(log) { + m_log(log) {} - CmdLineArgs cmdLineArgs(argc, argv, StandaloneCliTool::CTA_CHANGE_STORAGE_CLASS); +//------------------------------------------------------------------------------ +// exceptionThrowingMain +//------------------------------------------------------------------------------ +int ChangeStorageClass::exceptionThrowingMain(const int argc, char *const *const argv) { + CmdLineArgs cmdLineArgs(argc, argv, StandaloneCliTool::CTA_CHANGE_STORAGE_CLASS); if (cmdLineArgs.m_help) { cmdLineArgs.printUsage(std::cout); - exit(0); + throw exception::UserError(""); } if (!cmdLineArgs.m_storageClassName) { cmdLineArgs.printUsage(std::cout); - exit(1); + throw exception::UserError("Missing requried option: storage.class.name"); } if (!cmdLineArgs.m_archiveFileId && !cmdLineArgs.m_archiveFileIds) { cmdLineArgs.printUsage(std::cout); - exit(1); + throw exception::UserError("filename or id must be provided"); } m_storageClassName = cmdLineArgs.m_storageClassName.value(); @@ -96,9 +98,15 @@ ChangeStorageClass::ChangeStorageClass( m_eosUpdateFrequency = 100; } - auto [serviceProvider, endpointmap] = ConnConfiguration::readAndSetConfiguration(log, getUsername(), cmdLineArgs); + auto [serviceProvider, endpointmap] = ConnConfiguration::readAndSetConfiguration(m_log, getUsername(), cmdLineArgs); m_serviceProviderPtr = std::move(serviceProvider); m_endpointMapPtr = std::move(endpointmap); + + storageClassExists(); + updateStorageClassInEosNamespace(); + updateStorageClassInCatalogue(); + writeSkippedArchiveIdsToFile(); + return 0; } //------------------------------------------------------------------------------ @@ -264,17 +272,5 @@ void ChangeStorageClass::writeSkippedArchiveIdsToFile() const { } } -//------------------------------------------------------------------------------ -// exceptionThrowingMain -//------------------------------------------------------------------------------ -int ChangeStorageClass::exceptionThrowingMain(const int argc, char *const *const argv) { - storageClassExists(); - updateStorageClassInEosNamespace(); - updateStorageClassInCatalogue(); - writeSkippedArchiveIdsToFile(); - return 0; -} - - } // namespace admin } // namespace cta diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp index 4fe030f8c8..64246a2b0c 100644 --- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp +++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.hpp @@ -50,9 +50,7 @@ public: std::istream &inStream, std::ostream &outStream, std::ostream &errStream, - cta::log::StdoutLogger &log, - const int& argc, - char *const *const argv); + cta::log::StdoutLogger &log); private: diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp index 5175ab2e58..c0aedff3d1 100644 --- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp +++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClassMain.cpp @@ -35,7 +35,7 @@ int main(const int argc, char *const *const argv) { cta::log::StdoutLogger log(hostName, "cta-change-storage-class"); - cta::cliTool::ChangeStorageClass cmd(std::cin, std::cout, std::cerr, log, argc, argv); + cta::cliTool::ChangeStorageClass cmd(std::cin, std::cout, std::cerr, log); int ret = cmd.main(argc, argv); // Delete all global objects allocated by libprotobuf google::protobuf::ShutdownProtobufLibrary(); -- GitLab From 5ef5925928efc43b7036f2bba287ddfed2232b29 Mon Sep 17 00:00:00 2001 From: lwardena <lasse.tjernaes.wardenaer@cern.ch> Date: Mon, 12 Dec 2022 17:22:41 +0100 Subject: [PATCH 3/4] Add search option for storage class check --- cmdline/CtaAdminCmdParse.hpp | 2 +- .../change_storage_class/ChangeStorageClass.cpp | 10 ++++++++-- xroot_plugins/XrdCtaStorageClassLs.hpp | 17 ++++++++++++++--- xroot_plugins/XrdSsiCtaRequestMessage.cpp | 4 +++- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/cmdline/CtaAdminCmdParse.hpp b/cmdline/CtaAdminCmdParse.hpp index 7ceba7d4df..e7eac64628 100644 --- a/cmdline/CtaAdminCmdParse.hpp +++ b/cmdline/CtaAdminCmdParse.hpp @@ -634,7 +634,7 @@ const std::map<cmd_key_t, cmd_val_t> cmdOptions = { {{ AdminCmd::CMD_STORAGECLASS, AdminCmd::SUBCMD_CH }, { opt_storageclass_alias, opt_copynb_alias.optional(), opt_vo.optional(), opt_comment.optional() }}, {{ AdminCmd::CMD_STORAGECLASS, AdminCmd::SUBCMD_RM }, { opt_storageclass_alias }}, - {{ AdminCmd::CMD_STORAGECLASS, AdminCmd::SUBCMD_LS }, { }}, + {{ AdminCmd::CMD_STORAGECLASS, AdminCmd::SUBCMD_LS }, { opt_storageclass_alias.optional() }}, /*----------------------------------------------------------------------------------------------------*/ {{ AdminCmd::CMD_TAPE, AdminCmd::SUBCMD_ADD }, { opt_vid, opt_mediatype, opt_vendor, opt_logicallibrary, opt_tapepool, opt_full, diff --git a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp index b82cd778d5..fba8a08df1 100644 --- a/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp +++ b/cmdline/standalone_cli_tools/change_storage_class/ChangeStorageClass.cpp @@ -168,6 +168,13 @@ void ChangeStorageClass::storageClassExists() const { admincmd->set_cmd(cta::admin::AdminCmd::CMD_STORAGECLASS); admincmd->set_subcmd(cta::admin::AdminCmd::SUBCMD_LS); + { + const auto key = cta::admin::OptionString::STORAGE_CLASS; + const auto new_opt = admincmd->add_option_str(); + new_opt->set_key(key); + new_opt->set_value(m_storageClassName); + } + cta::xrd::Response response; auto stream_future = m_serviceProviderPtr->SendAsync(request, response); @@ -190,8 +197,7 @@ void ChangeStorageClass::storageClassExists() const { // wait until the data stream has been processed before exiting stream_future.wait(); - auto storageClassFound = std::find(g_storageClasses.begin(), g_storageClasses.end(), m_storageClassName); - if ( g_storageClasses.end() == storageClassFound ){ + if (g_storageClasses.empty()){ throw(exception::UserError("The storage class provided has not been defined.")); } } diff --git a/xroot_plugins/XrdCtaStorageClassLs.hpp b/xroot_plugins/XrdCtaStorageClassLs.hpp index ff5cdba7c0..a881f809fa 100644 --- a/xroot_plugins/XrdCtaStorageClassLs.hpp +++ b/xroot_plugins/XrdCtaStorageClassLs.hpp @@ -19,6 +19,7 @@ #include <xroot_plugins/XrdCtaStream.hpp> #include <xroot_plugins/XrdSsiCtaRequestMessage.hpp> +#include <optional> namespace cta { namespace xrd { @@ -35,7 +36,7 @@ public: * @param[in] catalogue CTA Catalogue * @param[in] scheduler CTA Scheduler */ - StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler); + StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler, const std::optional<std::string> storageClassName); private: /*! @@ -52,17 +53,27 @@ private: std::list<cta::common::dataStructures::StorageClass> m_storageClassList; //!< List of storage classes from the catalogue + std::optional<std::string> m_storageClassName; + static constexpr const char* const LOG_SUFFIX = "StorageClassLsStream"; //!< Identifier for log messages }; -StorageClassLsStream::StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler) : +StorageClassLsStream::StorageClassLsStream(const RequestMessage &requestMsg, cta::catalogue::Catalogue &catalogue, cta::Scheduler &scheduler, const std::optional<std::string> storageClassName) : XrdCtaStream(catalogue, scheduler), - m_storageClassList(catalogue.getStorageClasses()) + m_storageClassName(storageClassName) { using namespace cta::admin; XrdSsiPb::Log::Msg(XrdSsiPb::Log::DEBUG, LOG_SUFFIX, "StorageClassLsStream() constructor"); + + if(m_storageClassName) { + m_storageClassList.push_back(m_catalogue.getStorageClass(m_storageClassName.value())); + } else { + for(const auto &storageClass : m_catalogue.getStorageClasses()) { + m_storageClassList.push_back(storageClass); + } + } } int StorageClassLsStream::fillBuffer(XrdSsiPb::OStreamBuffer<Data> *streambuf) { diff --git a/xroot_plugins/XrdSsiCtaRequestMessage.cpp b/xroot_plugins/XrdSsiCtaRequestMessage.cpp index f48b47247b..d81226f41a 100644 --- a/xroot_plugins/XrdSsiCtaRequestMessage.cpp +++ b/xroot_plugins/XrdSsiCtaRequestMessage.cpp @@ -1872,8 +1872,10 @@ void RequestMessage::processStorageClass_Ls(cta::xrd::Response &response, XrdSsi { using namespace cta::admin; + std::optional<std::string> scn = getOptional(OptionString::STORAGE_CLASS); + // Create a XrdSsi stream object to return the results - stream = new StorageClassLsStream(*this, m_catalogue, m_scheduler); + stream = new StorageClassLsStream(*this, m_catalogue, m_scheduler, scn); response.set_show_header(HeaderType::STORAGECLASS_LS); response.set_type(cta::xrd::Response::RSP_SUCCESS); -- GitLab From dc7c37490f4cb5d9c899c2b38f96cfb5c1413fc7 Mon Sep 17 00:00:00 2001 From: lwardena <lasse.tjernaes.wardenaer@cern.ch> Date: Thu, 15 Dec 2022 13:38:00 +0100 Subject: [PATCH 4/4] Update release notes --- ReleaseNotes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/ReleaseNotes.md b/ReleaseNotes.md index 9b4c9ed5fe..ad6aeda97d 100644 --- a/ReleaseNotes.md +++ b/ReleaseNotes.md @@ -7,6 +7,7 @@ - cta/CTA#213 - Add tool for injecting file into eos - cta/CTA#224 - Improve error message for cta-verify-file whn VID does not exist - cta/CTA#230 - Modify CTA code to enforce VID uppercase +- cta/CTA#239 - Add improvments to the cta-change-storage-class tool ### Bug Fixes - cta/CTA#234 - Replace stoi with toUint64 in standalone cli tool - cta/CTA#238 - Compilation fails when using cta::common::Configuration::getConfEntInt(...) -- GitLab