Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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) #2097

Merged
merged 3 commits into from
Sep 21, 2024
Merged

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) #2097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ba9a2d
issue-2033: we should ignore DupCache entries with invalid types and …
qkrorlqr Sep 21, 2024
7eab7ec
issue-2033: we should ignore DupCache entries with invalid types and …
qkrorlqr Sep 21, 2024
950dee8
issue-2033: updated ShouldValidateDupRequests ut
qkrorlqr Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cloud/filestore/libs/diagnostics/critical_events.h
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,7 @@ void TIndexTabletState::AddDupCacheEntry( \
} \
} \
\
void TIndexTabletState::GetDupCacheEntry( \
bool TIndexTabletState::GetDupCacheEntry( \
const TDupCacheEntry* entry, \
NProto::T##name##Response& response) \
{ \
Expand All @@ -849,11 +849,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
Open in desktop
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
Loading