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-1350: replacing TABLET_VERIFY with impossible events + errors in CreateNode and CreateHandle #1558

Merged
merged 1 commit into from
Jul 7, 2024
Merged

issue-1350: replacing TABLET_VERIFY with impossible events + errors in CreateNode and CreateHandle #1558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 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 @@ -22,6 +22,13 @@ namespace NCloud::NFileStore{

#define FILESTORE_IMPOSSIBLE_EVENTS(xxx) \
xxx(CancelRoutineIsNotSet) \
xxx(ChildNodeWithoutRef) \
xxx(SessionNotFoundInTx) \
xxx(InvalidNodeIdForLocalNode) \
xxx(ChildNodeIsNull) \
xxx(TargetNodeWithoutRef) \
xxx(ParentNodeIsNull) \
xxx(FailedToCreateHandle) \
// FILESTORE_IMPOSSIBLE_EVENTS

////////////////////////////////////////////////////////////////////////////////
Expand Down
43 changes: 35 additions & 8 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 @@ -2,6 +2,7 @@

#include "helpers.h"

#include <cloud/filestore/libs/diagnostics/critical_events.h>
#include <cloud/filestore/libs/storage/api/tablet_proxy.h>

#include <util/generic/guid.h>
Expand Down Expand Up @@ -271,7 +272,7 @@ void TIndexTabletActor::HandleCreateHandle(
ExecuteTx<TCreateHandle>(
ctx,
std::move(requestInfo),
msg->Record);
std::move(msg->Record));
}

////////////////////////////////////////////////////////////////////////////////
Expand All @@ -289,7 +290,12 @@ bool TIndexTabletActor::PrepareTx_CreateHandle(
args.ClientId,
args.SessionId,
args.SessionSeqNo);
TABLET_VERIFY(session);
if (!session) {
auto message = ReportSessionNotFoundInTx(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return true;
}

args.ReadCommitId = GetReadCommitId(session->GetCheckpointId());
if (args.ReadCommitId == InvalidCommitId) {
Expand Down Expand Up @@ -377,7 +383,6 @@ bool TIndexTabletActor::PrepareTx_CreateHandle(
}

// TODO: AccessCheck
TABLET_VERIFY(args.TargetNode);
}

return true;
Expand All @@ -391,7 +396,12 @@ void TIndexTabletActor::ExecuteTx_CreateHandle(
FILESTORE_VALIDATE_TX_ERROR(CreateHandle, args);

auto* session = FindSession(args.SessionId);
TABLET_VERIFY(session);
if (!session) {
auto message = ReportSessionNotFoundInTx(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

// TODO: check if session is read only
args.WriteCommitId = GenerateCommitId();
Expand All @@ -404,8 +414,19 @@ void TIndexTabletActor::ExecuteTx_CreateHandle(
if (args.TargetNodeId == InvalidNodeId
&& (args.FollowerId.Empty() || args.IsNewFollowerNode))
{
TABLET_VERIFY(!args.TargetNode);
TABLET_VERIFY(args.ParentNode);
if (args.TargetNode) {
auto message = ReportTargetNodeWithoutRef(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

if (!args.ParentNode) {
auto message = ReportParentNodeIsNull(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

if (args.FollowerId.Empty()) {
NProto::TNode attrs =
Expand Down Expand Up @@ -473,7 +494,13 @@ void TIndexTabletActor::ExecuteTx_CreateHandle(
session->GetCheckpointId() ? args.ReadCommitId : InvalidCommitId,
args.Flags);

TABLET_VERIFY(handle);
if (!handle) {
auto message = ReportFailedToCreateHandle(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

args.Response.SetHandle(handle->GetHandle());
auto* node = args.Response.MutableNodeAttr();
ConvertNodeFromAttrs(*node, args.TargetNodeId, args.TargetNode->Attrs);
Expand Down Expand Up @@ -526,7 +553,7 @@ void TIndexTabletActor::CompleteTx_CreateHandle(
args.FollowerName,
ctx.SelfID,
CreateRegularAttrs(args.Mode, args.Uid, args.Gid),
std::move(args.Headers),
std::move(*args.Request.MutableHeaders()),
std::move(args.Response));

auto actorId = NCloud::Register(ctx, std::move(actor));
Expand Down
40 changes: 31 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 @@ -2,6 +2,7 @@

#include "helpers.h"

#include <cloud/filestore/libs/diagnostics/critical_events.h>
#include <cloud/filestore/libs/storage/api/tablet_proxy.h>

namespace NCloud::NFileStore::NStorage {
Expand Down Expand Up @@ -269,10 +270,10 @@ void TIndexTabletActor::HandleCreateNode(
ExecuteTx<TCreateNode>(
ctx,
std::move(requestInfo),
msg->Record,
std::move(msg->Record),
parentNodeId,
targetNodeId,
attrs);
std::move(attrs));
}

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -312,7 +313,6 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
}

// TODO: AccessCheck
qkrorlqr marked this conversation as resolved.
Show resolved Hide resolved
TABLET_VERIFY(args.ParentNode);

// validate target node doesn't exist
TMaybe<IIndexTabletDatabase::TNodeRef> childRef;
Expand All @@ -326,7 +326,13 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
return true;
}

TABLET_VERIFY(!args.ChildNode);
if (args.ChildNode) {
auto message = ReportChildNodeWithoutRef(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return true;
}

if (args.TargetNodeId != InvalidNodeId) {
// hard link: validate link target
args.ChildNodeId = args.TargetNodeId;
Expand All @@ -353,7 +359,6 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
}

// TODO: AccessCheck
TABLET_VERIFY(args.ChildNode);
}

return true;
Expand All @@ -367,7 +372,12 @@ void TIndexTabletActor::ExecuteTx_CreateNode(
FILESTORE_VALIDATE_TX_ERROR(CreateNode, args);

auto* session = FindSession(args.SessionId);
TABLET_VERIFY(session);
if (!session) {
auto message = ReportSessionNotFoundInTx(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

TIndexTabletDatabase db(tx.DB);

Expand Down Expand Up @@ -424,7 +434,12 @@ void TIndexTabletActor::ExecuteTx_CreateNode(
args.FollowerName);

if (args.FollowerId.Empty()) {
TABLET_VERIFY(args.ChildNodeId != InvalidNodeId);
if (args.ChildNodeId == InvalidNodeId) {
auto message = ReportInvalidNodeIdForLocalNode(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

ConvertNodeFromAttrs(
*args.Response.MutableNode(),
Expand All @@ -445,7 +460,9 @@ void TIndexTabletActor::ExecuteTx_CreateNode(
}
}

// TODO(#1350): support DupCache for external nodes
// TODO(#1350): support DupCache for external nodes - modify DupCache entry
// upon CreateNode completion in follower - in the same tx that would
// delete the corresponding CreateNode op from the op log table
}

void TIndexTabletActor::CompleteTx_CreateNode(
Expand Down Expand Up @@ -487,7 +504,12 @@ void TIndexTabletActor::CompleteTx_CreateNode(
CommitDupCacheEntry(args.SessionId, args.RequestId);
}

TABLET_VERIFY(args.ChildNode);
if (!args.ChildNode) {
auto message = ReportChildNodeIsNull(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
*args.Response.MutableError() =
MakeError(E_INVALID_STATE, std::move(message));
}
response->Record = std::move(args.Response);

NProto::TSessionEvent sessionEvent;
Expand Down
18 changes: 7 additions & 11 deletions cloud/filestore/libs/storage/tablet/tablet_tx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,22 +616,20 @@ struct TTxIndexTablet

TCreateNode(
TRequestInfoPtr requestInfo,
const NProto::TCreateNodeRequest request,
NProto::TCreateNodeRequest request,
ui64 parentNodeId,
ui64 targetNodeId,
const NProto::TNode& attrs)
NProto::TNode attrs)
: TSessionAware(request)
, RequestInfo(std::move(requestInfo))
, ParentNodeId(parentNodeId)
, TargetNodeId(targetNodeId)
, Name(request.GetName())
, Attrs(attrs)
, Attrs(std::move(attrs))
, FollowerId(request.GetFollowerFileSystemId())
, FollowerName(CreateGuidAsString())
, Request(std::move(request))
{
if (FollowerId) {
Request = request;
}
}

void Clear()
Expand Down Expand Up @@ -1061,7 +1059,7 @@ struct TTxIndexTablet
const ui32 Uid;
const ui32 Gid;
const TString RequestFollowerId;
NProto::THeaders Headers;
NProto::TCreateHandleRequest Request;

ui64 ReadCommitId = InvalidCommitId;
ui64 WriteCommitId = InvalidCommitId;
Expand All @@ -1076,7 +1074,7 @@ struct TTxIndexTablet

TCreateHandle(
TRequestInfoPtr requestInfo,
const NProto::TCreateHandleRequest& request)
NProto::TCreateHandleRequest request)
: TSessionAware(request)
, RequestInfo(std::move(requestInfo))
, NodeId(request.GetNodeId())
Expand All @@ -1086,10 +1084,8 @@ struct TTxIndexTablet
, Uid(request.GetUid())
, Gid(request.GetGid())
, RequestFollowerId(request.GetFollowerFileSystemId())
, Request(std::move(request))
{
if (RequestFollowerId) {
Headers = request.GetHeaders();
}
}

void Clear()
Expand Down
Loading