issue-2674: cross-shard RenameNode implementation - seems to work now…

…, TODO: uts, TODO: RenameNodeInDestination replay upon tablet start, TODO: DupCache in RenameNodeInDestination
ydb-platform · Jan 11, 2025 · 83fa5e2 · 83fa5e2
1 parent af4dd7c
commit 83fa5e2
Show file tree

Hide file tree

Showing 12 changed files with 453 additions and 20 deletions.
diff --git a/cloud/filestore/libs/diagnostics/critical_events.h b/cloud/filestore/libs/diagnostics/critical_events.h
@@ -52,6 +52,7 @@ namespace NCloud::NFileStore{
     xxx(LocalFsDuplicateFileHandle)                                            \
     xxx(UnexpectedLocalNode)                                                   \
     xxx(NoRenameNodeInDestinationRequest)                                      \
+    xxx(BadChildRefUponCommitRenameNodeInSource)                               \
 // FILESTORE_IMPOSSIBLE_EVENTS
 
 ////////////////////////////////////////////////////////////////////////////////

diff --git a/cloud/filestore/libs/storage/tablet/tablet_actor.cpp b/cloud/filestore/libs/storage/tablet/tablet_actor.cpp
@@ -943,6 +943,9 @@ STFUNC(TIndexTabletActor::StateInit)
         HFunc(
             TEvIndexTabletPrivate::TEvNodeUnlinkedInShard,
             HandleNodeUnlinkedInShard);
+        HFunc(
+            TEvIndexTabletPrivate::TEvNodeRenamedInDestination,
+            HandleNodeRenamedInDestination);
         HFunc(
             TEvIndexTabletPrivate::TEvGetShardStatsCompleted,
             HandleGetShardStatsCompleted);
@@ -993,6 +996,9 @@ STFUNC(TIndexTabletActor::StateWork)
         HFunc(
             TEvIndexTabletPrivate::TEvNodeUnlinkedInShard,
             HandleNodeUnlinkedInShard);
+        HFunc(
+            TEvIndexTabletPrivate::TEvNodeRenamedInDestination,
+            HandleNodeRenamedInDestination);
         HFunc(
             TEvIndexTabletPrivate::TEvGetShardStatsCompleted,
             HandleGetShardStatsCompleted);
@@ -1068,6 +1074,9 @@ STFUNC(TIndexTabletActor::StateZombie)
         HFunc(
             TEvIndexTabletPrivate::TEvNodeUnlinkedInShard,
             HandleNodeUnlinkedInShard);
+        HFunc(
+            TEvIndexTabletPrivate::TEvNodeRenamedInDestination,
+            HandleNodeRenamedInDestination);
         HFunc(
             TEvIndexTabletPrivate::TEvGetShardStatsCompleted,
             HandleGetShardStatsCompleted);
@@ -1117,6 +1126,9 @@ STFUNC(TIndexTabletActor::StateBroken)
         HFunc(
             TEvIndexTabletPrivate::TEvNodeUnlinkedInShard,
             HandleNodeUnlinkedInShard);
+        HFunc(
+            TEvIndexTabletPrivate::TEvNodeRenamedInDestination,
+            HandleNodeRenamedInDestination);
         HFunc(
             TEvIndexTabletPrivate::TEvGetShardStatsCompleted,
             HandleGetShardStatsCompleted);

diff --git a/cloud/filestore/libs/storage/tablet/tablet_actor.h b/cloud/filestore/libs/storage/tablet/tablet_actor.h
@@ -741,6 +741,10 @@ class TIndexTabletActor final
         const TEvIndexTabletPrivate::TEvNodeUnlinkedInShard::TPtr& ev,
         const NActors::TActorContext& ctx);
 
+    void HandleNodeRenamedInDestination(
+        const TEvIndexTabletPrivate::TEvNodeRenamedInDestination::TPtr& ev,
+        const NActors::TActorContext& ctx);
+
     void HandleGetShardStatsCompleted(
         const TEvIndexTabletPrivate::TEvGetShardStatsCompleted::TPtr& ev,
         const NActors::TActorContext& ctx);

diff --git a/cloud/filestore/libs/storage/tablet/tablet_actor_createnode.cpp b/cloud/filestore/libs/storage/tablet/tablet_actor_createnode.cpp
@@ -465,7 +465,10 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
             && Config->GetShardIdSelectionInLeaderEnabled()
             && !GetFileSystem().GetShardFileSystemIds().empty()
             && (args.Attrs.GetType() == NProto::E_REGULAR_NODE
-                || Config->GetDirectoryCreationInShardsEnabled()))
+                || Config->GetDirectoryCreationInShardsEnabled()
+                // otherwise there might be some local nodes which breaks
+                // current cross-shard RenameNode implementation
+                && GetLastNodeId() == RootNodeId))
     {
         args.Error = SelectShard(args.Attrs.GetSize(), &args.ShardId);
         if (HasError(args.Error)) {

diff --git a/cloud/filestore/libs/storage/tablet/tablet_actor_renamenode_destination.cpp b/cloud/filestore/libs/storage/tablet/tablet_actor_renamenode_destination.cpp
@@ -50,6 +50,10 @@ void TIndexTabletActor::HandleRenameNodeInDestination(
     //  <NewParentNodeId, NewChildName> now points to the file unlinked in p.4
     //  So both the new and the old file are now deleted plus we have a NodeRef
     //  pointing to a deleted file
+    //
+    //  A more reliable way to prevent such a race is to lock the affected
+    //  NodeRef in destination shard and unlock it after
+    //  CommitRenameNodeInSource
     /*
     const auto requestId = GetRequestId(msg->Record);
     if (const auto* e = session->LookupDupEntry(requestId)) {
@@ -122,7 +126,16 @@ bool TIndexTabletActor::PrepareTx_RenameNodeInDestination(
 
     if (args.NewChildRef) {
         if (HasFlag(args.Flags, NProto::TRenameNodeRequest::F_NOREPLACE)) {
-            args.Error = ErrorAlreadyExists(args.NewName);
+            if (args.NewChildRef->ShardId == args.Request.GetSourceNodeShardId()
+                    && args.NewChildRef->ShardNodeName
+                    == args.Request.GetSourceNodeShardNodeName())
+            {
+                // just an extra precaution for the case of DupCache-related
+                // bugs
+                args.Error = MakeError(S_ALREADY);
+            } else {
+                args.Error = ErrorAlreadyExists(args.NewName);
+            }
             return true;
         }
 
@@ -167,7 +180,7 @@ void TIndexTabletActor::ExecuteTx_RenameNodeInDestination(
     TTransactionContext& tx,
     TTxIndexTablet::TRenameNodeInDestination& args)
 {
-    FILESTORE_VALIDATE_TX_ERROR(RenameNode, args);
+    FILESTORE_VALIDATE_TX_ERROR(RenameNodeInDestination, args);
     if (args.Error.GetCode() == S_ALREADY) {
         return; // nothing to do
     }
@@ -176,7 +189,7 @@ void TIndexTabletActor::ExecuteTx_RenameNodeInDestination(
 
     args.CommitId = GenerateCommitId();
     if (args.CommitId == InvalidCommitId) {
-        return RebootTabletOnCommitOverflow(ctx, "RenameNode");
+        return RebootTabletOnCommitOverflow(ctx, "RenameNodeInDestination");
     }
 
     if (args.NewChildRef) {