Skip to content

Commit

Permalink
issue-2033: we should ignore DupCache entries with invalid types and …
Browse files Browse the repository at this point in the history 
…proceed with request processing as if the entry didn't exist (for the case of request id collisions) (#2097)

* issue-2033: we should ignore DupCache entries with invalid types and proceed with request processing as if the entry didn't exist (for the case of request id collisions)

* issue-2033: we should ignore DupCache entries with invalid types and proceed with request processing as if the entry didn't exist (for the case of request id collisions) - cleanup

* issue-2033: updated ShouldValidateDupRequests ut
  • Loading branch information
@qkrorlqr @debnatkh
qkrorlqr authored and debnatkh committed Sep 21, 2024
1 parent e6c806a commit 686160b
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 28 deletions.
1 change: 1 addition & 0 deletions cloud/filestore/libs/diagnostics/critical_events.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace NCloud::NFileStore{
xxx(NotEnoughResultsInGetNodeAttrBatchResponses) \
xxx(AsyncDestroyHandleFailed) \
xxx(DuplicateRequestId) \
xxx(InvalidDupCacheEntry) \
// FILESTORE_CRITICAL_EVENTS

#define FILESTORE_IMPOSSIBLE_EVENTS(xxx) \
Expand Down
5 changes: 3 additions & 2 deletions cloud/filestore/libs/storage/tablet/tablet_actor_createhandle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,11 @@ void TIndexTabletActor::HandleCreateHandle(
const auto requestId = GetRequestId(msg->Record);
if (const auto* e = session->LookupDupEntry(requestId)) {
auto response = std::make_unique<TResponse>();
GetDupCacheEntry(e, response->Record);
// sometimes bugged clients send us duplicate request ids
// see #2033
if (msg->Record.GetName().Empty() && msg->Record.GetNodeId()
if (!GetDupCacheEntry(e, response->Record)) {
session->DropDupEntry(requestId);
} else if (msg->Record.GetName().Empty() && msg->Record.GetNodeId()
!= response->Record.GetNodeAttr().GetId())
{
ReportDuplicateRequestId(TStringBuilder() << "CreateHandle response"
Expand Down
23 changes: 14 additions & 9 deletions cloud/filestore/libs/storage/tablet/tablet_actor_createnode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,17 +290,22 @@ void TIndexTabletActor::HandleCreateNode(
}

auto* msg = ev->Get();
if (const auto* e = session->LookupDupEntry(GetRequestId(msg->Record))) {
const auto requestId = GetRequestId(msg->Record);
if (const auto* e = session->LookupDupEntry(requestId)) {
auto response = std::make_unique<TEvService::TEvCreateNodeResponse>();
GetDupCacheEntry(e, response->Record);
if (response->Record.GetNode().GetId() == 0) {
// it's an external node which is not yet created in follower
// this check is needed for the case of leader reboot
*response->Record.MutableError() = MakeError(
E_REJECTED,
"node not yet created in follower");
if (GetDupCacheEntry(e, response->Record)) {
if (response->Record.GetNode().GetId() == 0) {
// it's an external node which is not yet created in follower
// this check is needed for the case of leader reboot
*response->Record.MutableError() = MakeError(
E_REJECTED,
"node not yet created in follower");
}

return NCloud::Reply(ctx, *ev, std::move(response));
}
return NCloud::Reply(ctx, *ev, std::move(response));

session->DropDupEntry(requestId);
}

ui64 parentNodeId = msg->Record.GetNodeId();
Expand Down
10 changes: 7 additions & 3 deletions cloud/filestore/libs/storage/tablet/tablet_actor_renamenode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,14 @@ void TIndexTabletActor::HandleRenameNode(
}

auto* msg = ev->Get();
if (const auto* e = session->LookupDupEntry(GetRequestId(msg->Record))) {
const auto requestId = GetRequestId(msg->Record);
if (const auto* e = session->LookupDupEntry(requestId)) {
auto response = std::make_unique<TEvService::TEvRenameNodeResponse>();
GetDupCacheEntry(e, response->Record);
return NCloud::Reply(ctx, *ev, std::move(response));
if (GetDupCacheEntry(e, response->Record)) {
return NCloud::Reply(ctx, *ev, std::move(response));
}

session->DropDupEntry(requestId);
}

auto requestInfo = CreateRequestInfo(
Expand Down
10 changes: 7 additions & 3 deletions cloud/filestore/libs/storage/tablet/tablet_actor_unlinknode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,14 @@ void TIndexTabletActor::HandleUnlinkNode(
}

auto* msg = ev->Get();
if (const auto* e = session->LookupDupEntry(GetRequestId(msg->Record))) {
const auto requestId = GetRequestId(msg->Record);
if (const auto* e = session->LookupDupEntry(requestId)) {
auto response = std::make_unique<TEvService::TEvUnlinkNodeResponse>();
GetDupCacheEntry(e, response->Record);
return NCloud::Reply(ctx, *ev, std::move(response));
if (GetDupCacheEntry(e, response->Record)) {
return NCloud::Reply(ctx, *ev, std::move(response));
}

session->DropDupEntry(requestId);
}

auto requestInfo = CreateRequestInfo(
Expand Down
2 changes: 1 addition & 1 deletion cloud/filestore/libs/storage/tablet/tablet_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ public:
const NProto::T##name##Response& response, \
ui32 maxEntries); \
\
void GetDupCacheEntry( \
bool GetDupCacheEntry( \
const TDupCacheEntry* entry, \
NProto::T##name##Response& response); \
// FILESTORE_DECLARE_DUPCACHE
Expand Down
12 changes: 7 additions & 5 deletions cloud/filestore/libs/storage/tablet/tablet_state_sessions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ void TIndexTabletState::AddDupCacheEntry( \
} \
} \
\
void TIndexTabletState::GetDupCacheEntry( \
bool TIndexTabletState::GetDupCacheEntry( \
const TDupCacheEntry* entry, \
NProto::T##name##Response& response) \
{ \
Expand All @@ -850,11 +850,13 @@ void TIndexTabletState::GetDupCacheEntry( \
} else if (!entry->Committed) { \
*response.MutableError() = ErrorDuplicate(); \
} else if (!entry->Has##name()) { \
*response.MutableError() = MakeError( \
E_ARGUMENT, TStringBuilder() << "invalid request dup cache type: " \
<< entry->ShortUtf8DebugString().Quote()); \
ReportDuplicateRequestId(response.GetError().GetMessage()); \
ReportInvalidDupCacheEntry(TStringBuilder() \
<< "invalid request dup cache type: " \
<< entry->ShortUtf8DebugString().Quote()); \
return false; \
} \
\
return true; \
} \
// FILESTORE_IMPLEMENT_DUPCACHE

Expand Down
49 changes: 44 additions & 5 deletions cloud/filestore/libs/storage/tablet/tablet_ut_nodes.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,17 +1098,30 @@ Y_UNIT_TEST_SUITE(TIndexTabletTest_Nodes)
UNIT_ASSERT(!HasError(response->Record.GetError()));

nodeId = response->Record.GetNode().GetId();
UNIT_ASSERT(nodeId != InvalidNodeId);
UNIT_ASSERT_VALUES_UNEQUAL(InvalidNodeId, nodeId);
}

{
auto response = tablet.ListNodes(RootNodeId);
const auto& names = response->Record.GetNames();
UNIT_ASSERT_VALUES_EQUAL(1, names.size());
UNIT_ASSERT_VALUES_EQUAL("xxx", names[0]);
}

tablet.SendRequest(createOther(100500));
{
auto response = tablet.RecvUnlinkNodeResponse();
UNIT_ASSERT_VALUES_EQUAL_C(
E_ARGUMENT,
S_OK,
response->Record.GetError().GetCode(),
response->Record.GetError().GetMessage());
}

{
auto response = tablet.ListNodes(RootNodeId);
const auto& names = response->Record.GetNames();
UNIT_ASSERT_VALUES_EQUAL(0, names.size());
}
}

Y_UNIT_TEST(ShouldWaitForDupRequestToBeCommitted)
Expand Down Expand Up @@ -1616,16 +1629,25 @@ Y_UNIT_TEST_SUITE(TIndexTabletTest_Nodes)
TIndexTabletClient tablet(env.GetRuntime(), nodeIdx, tabletId);
tablet.InitSession("client", "session");

auto createRequest = [&] (ui64 reqId, ui64 nodeId) {
auto createCreateHandleRequest = [&] (ui64 reqId, ui64 nodeId) {
auto request = tablet.CreateCreateHandleRequest(
nodeId, TCreateHandleArgs::RDWR);
request->Record.MutableHeaders()->SetRequestId(reqId);

return request;
};

auto createCreateNodeRequest = [&] (ui64 reqId, TString name) {
auto request = tablet.CreateCreateNodeRequest(
TCreateNodeArgs::File(RootNodeId, std::move(name)));
request->Record.MutableHeaders()->SetRequestId(reqId);

return request;
};

const TString name1 = "file1";
const TString name2 = "file2";
const TString name3 = "file3";

const auto nodeId1 = tablet.CreateNode(TCreateNodeArgs::File(
RootNodeId,
Expand All @@ -1640,7 +1662,7 @@ Y_UNIT_TEST_SUITE(TIndexTabletTest_Nodes)

const ui64 requestId = 100500;

tablet.SendRequest(createRequest(requestId, nodeId1));
tablet.SendRequest(createCreateHandleRequest(requestId, nodeId1));
{
auto response = tablet.RecvCreateHandleResponse();
UNIT_ASSERT_VALUES_EQUAL_C(
Expand All @@ -1653,7 +1675,7 @@ Y_UNIT_TEST_SUITE(TIndexTabletTest_Nodes)
response->Record.GetNodeAttr().GetId());
}

tablet.SendRequest(createRequest(requestId, nodeId2));
tablet.SendRequest(createCreateHandleRequest(requestId, nodeId2));
{
auto response = tablet.RecvCreateHandleResponse();
UNIT_ASSERT_VALUES_EQUAL_C(
Expand All @@ -1665,6 +1687,23 @@ Y_UNIT_TEST_SUITE(TIndexTabletTest_Nodes)
nodeId2,
response->Record.GetNodeAttr().GetId());
}

tablet.SendRequest(createCreateNodeRequest(requestId, name3));
{
auto response = tablet.RecvCreateNodeResponse();
UNIT_ASSERT_VALUES_EQUAL_C(
S_OK,
response->Record.GetError().GetCode(),
response->Record.GetError().GetMessage());

UNIT_ASSERT_VALUES_UNEQUAL(
nodeId1,
response->Record.GetNode().GetId());
UNIT_ASSERT_VALUES_UNEQUAL(
nodeId2,
response->Record.GetNode().GetId());
UNIT_ASSERT_VALUES_UNEQUAL(0, response->Record.GetNode().GetId());
}
}
}

Expand Down

0 comments on commit 686160b

Please sign in to comment.