From a962fb08f1960ace152822e54f70a92041127af3 Mon Sep 17 00:00:00 2001 From: Andrei Strelkovskii Date: Sat, 12 Oct 2024 11:34:50 +0300 Subject: [PATCH] issue-1928: filestore-client: using ISession instead of IClient for the Session-aware methods to properly process E_FS_INVALID_SESSION errors; existing tests are more or less sufficient to ensure that this code is correct (#2265) --- cloud/filestore/apps/client/lib/command.cpp | 53 ++++++++----------- cloud/filestore/apps/client/lib/command.h | 30 +++++++---- .../apps/client/lib/find_garbage.cpp | 29 +++++++--- cloud/filestore/apps/client/lib/ls.cpp | 28 +++++----- cloud/filestore/apps/client/lib/mkdir.cpp | 5 +- cloud/filestore/apps/client/lib/read.cpp | 7 +-- .../apps/client/lib/reset_session.cpp | 8 +-- cloud/filestore/apps/client/lib/rm.cpp | 5 +- .../apps/client/lib/set_node_attr.cpp | 3 +- cloud/filestore/apps/client/lib/stat.cpp | 5 +- cloud/filestore/apps/client/lib/touch.cpp | 11 ++-- cloud/filestore/apps/client/lib/write.cpp | 7 +-- 12 files changed, 110 insertions(+), 81 deletions(-) diff --git a/cloud/filestore/apps/client/lib/command.cpp b/cloud/filestore/apps/client/lib/command.cpp index 40fb6add4a..231922c3f6 100644 --- a/cloud/filestore/apps/client/lib/command.cpp +++ b/cloud/filestore/apps/client/lib/command.cpp @@ -311,46 +311,38 @@ void TFileStoreCommand::Stop() TFileStoreServiceCommand::Stop(); } -TFileStoreCommand::TSessionGuard TFileStoreCommand::CreateSession() +TFileStoreCommand::TSessionGuard TFileStoreCommand::CreateCustomSession( + TString fsId, + TString clientId) { - // TODO use ISession instead - auto request = std::make_shared(); - request->SetFileSystemId(FileSystemId); - request->MutableHeaders()->SetClientId(ClientId); - request->SetRestoreClientSession(true); - - TCallContextPtr ctx = MakeIntrusive(); - auto response = WaitFor(Client->CreateSession(ctx, std::move(request))); + NProto::TSessionConfig protoConfig; + protoConfig.SetFileSystemId(std::move(fsId)); + protoConfig.SetClientId(std::move(clientId)); + auto config = std::make_shared(protoConfig); + auto session = + NClient::CreateSession(Logging, Timer, Scheduler, Client, config); + // extracting the value will break the internal state of this session object + auto response = WaitFor(session->CreateSession(), /* extract */ false); CheckResponse(response); - auto sessionId = response.GetSession().GetSessionId(); - - Headers.SetClientId(ClientId); - Headers.SetSessionId(sessionId); - Headers.SetDisableMultiTabletForwarding(DisableMultiTabletForwarding); - - return TSessionGuard(*this); + return TSessionGuard(*this, std::move(session)); } -void TFileStoreCommand::DestroySession() +TFileStoreCommand::TSessionGuard TFileStoreCommand::CreateSession() { - if (Headers.GetSessionId().Empty()) { - return; - } - - auto request = std::make_shared(); - request->SetFileSystemId(FileSystemId); - request->MutableHeaders()->SetSessionId(Headers.GetSessionId()); - request->MutableHeaders()->SetClientId(Headers.GetClientId()); + return CreateCustomSession(FileSystemId, ClientId); +} - TCallContextPtr ctx = MakeIntrusive(); - auto response = WaitFor(Client->DestroySession(ctx, std::move(request))); +void TFileStoreCommand::DestroySession(ISession& session) +{ + auto response = WaitFor(session.DestroySession()); CheckResponse(response); } //////////////////////////////////////////////////////////////////////////////// NProto::TNodeAttr TFileStoreCommand::ResolveNode( + ISession& session, ui64 parentNodeId, TString name, bool ignoreMissing) @@ -372,7 +364,7 @@ NProto::TNodeAttr TFileStoreCommand::ResolveNode( request->SetNodeId(parentNodeId); request->SetName(std::move(name)); - auto response = WaitFor(Client->GetNodeAttr( + auto response = WaitFor(session.GetNodeAttr( PrepareCallContext(), std::move(request))); @@ -387,6 +379,7 @@ NProto::TNodeAttr TFileStoreCommand::ResolveNode( } TVector TFileStoreCommand::ResolvePath( + ISession& session, TStringBuf path, bool ignoreMissing) { @@ -401,10 +394,10 @@ TVector TFileStoreCommand::ResolvePath( while (it.NextTok('/', tok)) { if (tok) { auto node = ResolveNode( + session, result.back().Node.GetId(), ToString(tok), - ignoreMissing - ); + ignoreMissing); result.push_back({node, tok}); } } diff --git a/cloud/filestore/apps/client/lib/command.h b/cloud/filestore/apps/client/lib/command.h index 968448a0a2..d3509dcccb 100644 --- a/cloud/filestore/apps/client/lib/command.h +++ b/cloud/filestore/apps/client/lib/command.h @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -96,7 +97,7 @@ class TCommand } template - T WaitFor(NThreading::TFuture future) + T WaitFor(NThreading::TFuture future, bool extract = true) { if (!future.HasValue()) { auto* ptr = reinterpret_cast*>(&future); @@ -104,7 +105,7 @@ class TCommand return TErrorResponse(E_REJECTED, "request cancelled"); } } - return future.ExtractValue(); + return extract ? future.ExtractValue() : future.GetValue(); } private: @@ -123,8 +124,6 @@ class TFileStoreServiceCommand IFileStoreServicePtr Client; - NProto::THeaders Headers; - public: TFileStoreServiceCommand() = default; @@ -152,13 +151,14 @@ class TFileStoreCommand { auto request = std::make_shared(); request->SetFileSystemId(FileSystemId); - request->MutableHeaders()->CopyFrom(Headers); + request->MutableHeaders()->SetDisableMultiTabletForwarding( + DisableMultiTabletForwarding); return request; } template - void CheckResponse(const T& response) + static void CheckResponse(const T& response) { if (HasError(response)) { throw TServiceError(response.GetError()); @@ -172,21 +172,25 @@ class TFileStoreCommand }; NProto::TNodeAttr ResolveNode( + ISession& session, ui64 parentNodeId, TString name, bool ignoreMissing); TVector ResolvePath( + ISession& session, TStringBuf path, bool ignoreMissing); class TSessionGuard final { TFileStoreCommand& FileStoreCmd; + ISessionPtr Session; TLog& Log; public: - explicit TSessionGuard(TFileStoreCommand& fileStoreCmd) + TSessionGuard(TFileStoreCommand& fileStoreCmd, ISessionPtr session) : FileStoreCmd(fileStoreCmd) + , Session(std::move(session)) , Log(FileStoreCmd.AccessLog()) { } @@ -194,17 +198,25 @@ class TFileStoreCommand ~TSessionGuard() { try { - FileStoreCmd.DestroySession(); + FileStoreCmd.DestroySession(*Session); } catch (...) { STORAGE_ERROR("~TSessionGuard: " << CurrentExceptionMessage()); } } + + ISession& AccessSession() + { + return *Session; + } }; [[nodiscard]] TSessionGuard CreateSession(); + [[nodiscard]] TSessionGuard CreateCustomSession( + TString fsId, + TString clientId); private: - void DestroySession(); + void DestroySession(ISession& session); }; //////////////////////////////////////////////////////////////////////////////// diff --git a/cloud/filestore/apps/client/lib/find_garbage.cpp b/cloud/filestore/apps/client/lib/find_garbage.cpp index 53c5bb2757..5b95bb4b66 100644 --- a/cloud/filestore/apps/client/lib/find_garbage.cpp +++ b/cloud/filestore/apps/client/lib/find_garbage.cpp @@ -41,7 +41,10 @@ class TFindGarbageCommand final .StoreResult(&PageSize); } - NProto::TListNodesResponse ListAll(const TString& fsId, ui64 parentId) + NProto::TListNodesResponse ListAll( + ISession& session, + const TString& fsId, + ui64 parentId) { NProto::TListNodesResponse fullResult; TString cookie; @@ -56,7 +59,7 @@ class TFindGarbageCommand final request->SetCookie(cookie); // TODO: async listing - auto response = WaitFor(Client->ListNodes( + auto response = WaitFor(session.ListNodes( PrepareCallContext(), std::move(request))); @@ -82,18 +85,19 @@ class TFindGarbageCommand final } void FetchAll( + ISession& session, const TString& fsId, ui64 parentId, TVector* nodes) { // TODO: async listing - auto response = ListAll(fsId, parentId); + auto response = ListAll(session, fsId, parentId); for (ui32 i = 0; i < response.NodesSize(); ++i) { const auto& node = response.GetNodes(i); const auto& name = response.GetNames(i); if (node.GetType() == NProto::E_DIRECTORY_NODE) { - FetchAll(fsId, node.GetId(), nodes); + FetchAll(session, fsId, node.GetId(), nodes); } else { nodes->push_back({ node.GetId(), @@ -106,6 +110,7 @@ class TFindGarbageCommand final } TMaybe Stat( + ISession& session, const TString& fsId, ui64 parentId, const TString& name) @@ -115,7 +120,7 @@ class TFindGarbageCommand final request->SetNodeId(parentId); request->SetName(name); - auto response = WaitFor(Client->GetNodeAttr( + auto response = WaitFor(session.GetNodeAttr( PrepareCallContext(), std::move(request))); @@ -134,13 +139,17 @@ class TFindGarbageCommand final bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); TMap> shard2Nodes; for (const auto& shard: Shards) { - FetchAll(shard, RootNodeId, &shard2Nodes[shard]); + auto shardSessionGuard = + CreateCustomSession(shard, shard + "::" + ClientId); + auto& shardSession = shardSessionGuard.AccessSession(); + FetchAll(shardSession, shard, RootNodeId, &shard2Nodes[shard]); } TVector leaderNodes; - FetchAll(FileSystemId, RootNodeId, &leaderNodes); + FetchAll(session, FileSystemId, RootNodeId, &leaderNodes); THashSet shardNames; for (const auto& node: leaderNodes) { @@ -169,9 +178,13 @@ class TFindGarbageCommand final TVector results; for (const auto& [shard, nodes]: shard2Nodes) { + auto shardSessionGuard = + CreateCustomSession(shard, shard + "::" + ClientId); + auto& shardSession = shardSessionGuard.AccessSession(); for (const auto& node: nodes) { if (!shardNames.contains(node.Name)) { - auto stat = Stat(shard, RootNodeId, node.Name); + auto stat = + Stat(shardSession, shard, RootNodeId, node.Name); if (stat) { results.push_back({shard, node.Name, stat->GetSize()}); diff --git a/cloud/filestore/apps/client/lib/ls.cpp b/cloud/filestore/apps/client/lib/ls.cpp index a29360e647..6f749b5020 100644 --- a/cloud/filestore/apps/client/lib/ls.cpp +++ b/cloud/filestore/apps/client/lib/ls.cpp @@ -225,7 +225,10 @@ class TLsCommand final Opts.MutuallyExclusive(LimitOptionName, AllOptionName); } - TPageInfo FetchNodesInfo(const NProto::TNodeAttr& node, TStringBuf name = TStringBuf()) + TPageInfo FetchNodesInfo( + ISession& session, + const NProto::TNodeAttr& node, + TStringBuf name = TStringBuf()) { if (node.GetType() != NProto::E_DIRECTORY_NODE) { return TPageInfo{ @@ -233,10 +236,10 @@ class TLsCommand final .Cookie = {}, }; } - return FetchNodesInfo(node.GetId()); + return FetchNodesInfo(session, node.GetId()); } - TPageInfo FetchNodesInfo(TNodeId nodeId) + TPageInfo FetchNodesInfo(ISession& session, TNodeId nodeId) { TPageInfo page; page.Cookie = CliArgs.Cookie; @@ -246,7 +249,7 @@ class TLsCommand final request->SetNodeId(nodeId); request->SetCookie(page.Cookie); - auto response = WaitFor(Client->ListNodes( + auto response = WaitFor(session.ListNodes( PrepareCallContext(), std::move(request))); @@ -267,16 +270,16 @@ class TLsCommand final return page; } - TPageInfo HandlePathMode(TStringBuf path) + TPageInfo HandlePathMode(ISession& session, TStringBuf path) { Y_ENSURE(!path.empty(), "Path must be set"); - const auto resolvedPath = ResolvePath(path, false).back(); + const auto resolvedPath = ResolvePath(session, path, false).back(); - return FetchNodesInfo(resolvedPath.Node, resolvedPath.Name); + return FetchNodesInfo(session, resolvedPath.Node, resolvedPath.Name); } - TPageInfo HandleNodeMode(TNodeId nodeId) + TPageInfo HandleNodeMode(ISession& session, TNodeId nodeId) { Y_ENSURE(nodeId != NProto::E_INVALID_NODE_ID, "Path must be set"); @@ -284,11 +287,11 @@ class TLsCommand final request->SetNodeId(nodeId); request->SetName(TString{}); - auto node = WaitFor(Client->GetNodeAttr( + auto node = WaitFor(session.GetNodeAttr( PrepareCallContext(), std::move(request))).GetNode(); - return FetchNodesInfo(node); + return FetchNodesInfo(session, node); } bool Execute() override @@ -296,14 +299,15 @@ class TLsCommand final Y_ENSURE(CliArgs.Mode != EModeType::Unknown, "Mode type is not set"); auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); TPageInfo page; switch (CliArgs.Mode) { case EModeType::Path: - page = HandlePathMode(CliArgs.Path); + page = HandlePathMode(session, CliArgs.Path); break; case EModeType::Node: - page = HandleNodeMode(CliArgs.NodeId); + page = HandleNodeMode(session, CliArgs.NodeId); break; case EModeType::Unknown: break; diff --git a/cloud/filestore/apps/client/lib/mkdir.cpp b/cloud/filestore/apps/client/lib/mkdir.cpp index c2ed7d44a5..d1c1faaf53 100644 --- a/cloud/filestore/apps/client/lib/mkdir.cpp +++ b/cloud/filestore/apps/client/lib/mkdir.cpp @@ -34,6 +34,7 @@ class TMkDirCommand final bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); auto makeDir = [&] (ui64 nodeId, TStringBuf name) { auto request = CreateRequest(); @@ -41,7 +42,7 @@ class TMkDirCommand final request->SetName(ToString(name)); request->MutableDirectory()->SetMode(MODE0777); - auto response = WaitFor(Client->CreateNode( + auto response = WaitFor(session.CreateNode( PrepareCallContext(), std::move(request))); @@ -50,7 +51,7 @@ class TMkDirCommand final return response.GetNode(); }; - auto resolved = ResolvePath(Path, true); + auto resolved = ResolvePath(session, Path, true); if (resolved.size() == 1) { return true; diff --git a/cloud/filestore/apps/client/lib/read.cpp b/cloud/filestore/apps/client/lib/read.cpp index fc69174ef2..88566ab02f 100644 --- a/cloud/filestore/apps/client/lib/read.cpp +++ b/cloud/filestore/apps/client/lib/read.cpp @@ -39,8 +39,9 @@ class TReadCommand final bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); - const auto resolved = ResolvePath(Path, false); + const auto resolved = ResolvePath(session, Path, false); Y_ENSURE( resolved.back().Node.GetType() != NProto::E_DIRECTORY_NODE, @@ -58,7 +59,7 @@ class TReadCommand final createRequest->SetName(ToString(resolved.back().Name)); createRequest->SetFlags(flags); - auto createResponse = WaitFor(Client->CreateHandle( + auto createResponse = WaitFor(session.CreateHandle( PrepareCallContext(), std::move(createRequest))); @@ -76,7 +77,7 @@ class TReadCommand final readRequest->SetOffset(Offset); readRequest->SetLength(Length); - auto readResponse = WaitFor(Client->ReadData( + auto readResponse = WaitFor(session.ReadData( PrepareCallContext(), std::move(readRequest) )); diff --git a/cloud/filestore/apps/client/lib/reset_session.cpp b/cloud/filestore/apps/client/lib/reset_session.cpp index 7740a78934..cefee16584 100644 --- a/cloud/filestore/apps/client/lib/reset_session.cpp +++ b/cloud/filestore/apps/client/lib/reset_session.cpp @@ -42,11 +42,11 @@ class TResetSessionCommand final bool Execute() override { - Headers.SetSessionId(SessionId); - Headers.SetClientId(ClientId); - Headers.SetSessionSeqNo(SeqNo); - auto request = CreateRequest(); + auto& headers = *request->MutableHeaders(); + headers.SetSessionId(SessionId); + headers.SetClientId(ClientId); + headers.SetSessionSeqNo(SeqNo); if (SessionState) { request->SetSessionState(Base64Decode(SessionState)); } diff --git a/cloud/filestore/apps/client/lib/rm.cpp b/cloud/filestore/apps/client/lib/rm.cpp index edc9c1fb6c..f1d2494829 100644 --- a/cloud/filestore/apps/client/lib/rm.cpp +++ b/cloud/filestore/apps/client/lib/rm.cpp @@ -33,8 +33,9 @@ class TRmCommand final bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); - const auto resolved = ResolvePath(Path, false); + const auto resolved = ResolvePath(session, Path, false); Y_ENSURE(resolved.size() >= 2, "can't rm root node"); @@ -44,7 +45,7 @@ class TRmCommand final request->SetName(ToString(resolved.back().Name)); request->SetUnlinkDirectory(RemoveDir); - auto response = WaitFor(Client->UnlinkNode( + auto response = WaitFor(session.UnlinkNode( PrepareCallContext(), std::move(request))); diff --git a/cloud/filestore/apps/client/lib/set_node_attr.cpp b/cloud/filestore/apps/client/lib/set_node_attr.cpp index 5f28628b55..6b5cd88e71 100644 --- a/cloud/filestore/apps/client/lib/set_node_attr.cpp +++ b/cloud/filestore/apps/client/lib/set_node_attr.cpp @@ -65,6 +65,7 @@ class TSetNodeAttrCommand final: public TFileStoreCommand bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); auto request = CreateRequest(); request->SetNodeId(NodeId); @@ -112,7 +113,7 @@ class TSetNodeAttrCommand final: public TFileStoreCommand } auto response = WaitFor( - Client->SetNodeAttr(PrepareCallContext(), std::move(request))); + session.SetNodeAttr(PrepareCallContext(), std::move(request))); CheckResponse(response); return true; diff --git a/cloud/filestore/apps/client/lib/stat.cpp b/cloud/filestore/apps/client/lib/stat.cpp index 33e53cc0c7..600a3e4e26 100644 --- a/cloud/filestore/apps/client/lib/stat.cpp +++ b/cloud/filestore/apps/client/lib/stat.cpp @@ -40,15 +40,16 @@ class TStatCommand final: public TFileStoreCommand bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); - const auto resolved = ResolvePath(Path, false); + const auto resolved = ResolvePath(session, Path, false); Y_ABORT_UNLESS(resolved.size() >= 2); auto request = CreateRequest(); request->SetNodeId(resolved[resolved.size() - 2].Node.GetId()); request->SetName(ToString(resolved.back().Name)); auto response = WaitFor( - Client->GetNodeAttr(PrepareCallContext(), std::move(request))); + session.GetNodeAttr(PrepareCallContext(), std::move(request))); CheckResponse(response); Print(response.GetNode(), JsonOutput); diff --git a/cloud/filestore/apps/client/lib/touch.cpp b/cloud/filestore/apps/client/lib/touch.cpp index 3a443d3642..9e08d3decb 100644 --- a/cloud/filestore/apps/client/lib/touch.cpp +++ b/cloud/filestore/apps/client/lib/touch.cpp @@ -29,8 +29,9 @@ class TTouchCommand final bool Execute() override { auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); - auto resolved = ResolvePath(Path, true); + auto resolved = ResolvePath(session, Path, true); if (resolved.size() == 1) { return true; @@ -47,9 +48,9 @@ class TTouchCommand final update->SetMTime(now); request->SetNodeId(node.GetId()); - auto response = WaitFor(Client->SetNodeAttr( - PrepareCallContext(), - std::move(request))); + auto response = WaitFor(session.SetNodeAttr( + PrepareCallContext(), + std::move(request))); CheckResponse(response); return true; @@ -66,7 +67,7 @@ class TTouchCommand final request->SetName(ToString(resolved.back().Name)); request->MutableFile()->SetMode(0644); - auto response = WaitFor(Client->CreateNode( + auto response = WaitFor(session.CreateNode( PrepareCallContext(), std::move(request))); diff --git a/cloud/filestore/apps/client/lib/write.cpp b/cloud/filestore/apps/client/lib/write.cpp index ed61bd34be..1e7520ac18 100644 --- a/cloud/filestore/apps/client/lib/write.cpp +++ b/cloud/filestore/apps/client/lib/write.cpp @@ -41,8 +41,9 @@ class TWriteCommand final TString data = TIFStream(DataPath).ReadAll(); auto sessionGuard = CreateSession(); + auto& session = sessionGuard.AccessSession(); - const auto resolved = ResolvePath(Path, true); + const auto resolved = ResolvePath(session, Path, true); Y_ENSURE( resolved.back().Node.GetType() != NProto::E_DIRECTORY_NODE, @@ -67,7 +68,7 @@ class TWriteCommand final createRequest->SetName(ToString(resolved.back().Name)); createRequest->SetFlags(flags); - auto createResponse = WaitFor(Client->CreateHandle( + auto createResponse = WaitFor(session.CreateHandle( PrepareCallContext(), std::move(createRequest))); @@ -80,7 +81,7 @@ class TWriteCommand final writeRequest->SetOffset(Offset); *writeRequest->MutableBuffer() = data; - auto writeResponse = WaitFor(Client->WriteData( + auto writeResponse = WaitFor(session.WriteData( PrepareCallContext(), std::move(writeRequest) ));