curvefs: recovey recyclebin file #259

SeanHai · 2021-02-08T06:01:55Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Side effects(Breaking backward compatibility? Performance regression?):

Check List

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

bai-charisu · 2021-02-19T06:52:37Z

proto/nameserver2.proto

+    required string     owner = 2;
+    optional string     signature = 3;
+    required uint64     date = 4;
+    optional uint64     fileId = 5;


fileId没看到在命令行里指定，什么时候会用到fileId？

fileId没看到在命令行里指定，什么时候会用到fileId？

原本是如果recyclebin中有多个同名删除的文件，则恢复最近删除的，这里fileId是留了个接口。这次review修改实现了--id选项（可选）来恢复指定文件。

bai-charisu · 2021-02-19T07:29:57Z

src/mds/nameserver2/namespace_service.cpp

+    StatusCode retCode;
+    retCode = GetRecoverFileInfo(request->filename(), &recoverFileInfo);
+    if (retCode != StatusCode::kOK ||
+        request->filename() != recoverFileInfo.originalfullpathname()) {


originfullpathname在里面不是已经判断过了吗？怎么又判断一遍？

originfullpathname在里面不是已经判断过了吗？怎么又判断一遍？

done

bai-charisu · 2021-02-19T07:34:02Z

src/mds/nameserver2/namespace_service.cpp

+        signature = request->signature();
+    }
+
+    retCode = kCurveFS.CheckFileOwner(recoverFileName,


除了检查recoverfilename的owner，恢复后的filename的权限也需要检查一下

除了检查recoverfilename的owner，恢复后的filename的权限也需要检查一下

done

bai-charisu · 2021-02-19T07:40:26Z

src/mds/nameserver2/curvefs.cpp

+        recycleFileInfo.set_filestatus(FileStatus::kFileCreated);
+    }
+
+    auto ret1 = storage_->RecoverFile(recycleFileInfo, recoverFileInfo);


这里不能直接使用rename吗？

这里不能直接使用rename吗？

done

cw123 · 2021-02-19T07:59:04Z

curvefs_python/curvefs_wrap.cxx

@@ -6979,6 +7053,7 @@ static PyMethodDef SwigMethods[] = {
 	 { (char *)"Extend", _wrap_Extend, METH_VARARGS, NULL},
 	 { (char *)"Unlink", _wrap_Unlink, METH_VARARGS, NULL},
 	 { (char *)"DeleteForce", _wrap_DeleteForce, METH_VARARGS, NULL},
+   { (char *)"Recover", _wrap_Recover, METH_VARARGS, NULL},


保持对齐

保持对齐

done

cw123 · 2021-02-19T07:59:13Z

curvefs_python/curvefs_wrap.cxx

@@ -6996,6 +7071,7 @@ static PyMethodDef SwigMethods[] = {
 	 { (char *)"CBDClient_Create2", _wrap_CBDClient_Create2, METH_VARARGS, NULL},
 	 { (char *)"CBDClient_Unlink", _wrap_CBDClient_Unlink, METH_VARARGS, NULL},
 	 { (char *)"CBDClient_DeleteForce", _wrap_CBDClient_DeleteForce, METH_VARARGS, NULL},
+   { (char *)"CBDClient_Recover", _wrap_CBDClient_Recover, METH_VARARGS, NULL},


保持对齐

保持对齐

done

cw123 · 2021-02-19T08:00:56Z

proto/nameserver2.proto

@@ -137,6 +137,10 @@ enum  StatusCode {
    kSnapshotCloneConnectFail = 136;
    // 快照克隆服务未初始化
    kSnapshotCloneServerNotInit = 137;
+    // recover file status is BeingCloned


注释不对

注释不对

done

cw123 · 2021-02-19T08:09:04Z

src/client/mds_client.h

+     * recover file
+     * @param: userinfo
+     * @param: filename
+     * @param: id is inodeid，default 0，will not send to mds if not be set


id没有看到从哪里传进来

id没有看到从哪里传进来

原本是如果recyclebin中有多个同名删除的文件，则恢复最近删除的，这里fileId是留了个接口。这次review修改实现了--id选项（可选）来恢复指定文件。

cw123 · 2021-02-19T08:30:26Z

src/mds/nameserver2/curvefs.cpp

+StatusCode CurveFS::RecoverFile(const std::string & originFileName,
+                                const std::string & recycleFileName,
+                                uint64_t fileId) {
+    // check the same file exists


参考rename做一些前置判断。根目录，相同的name

参考rename做一些前置判断。根目录，相同的name

前置判断在NameSpaceService::RecoverFile中做了，主要是检查originFileName的合法性（IsPathValid），如果是根目录也是合法的，但是在recycleBin中找对应文件信息时会失败

cw123 · 2021-02-19T08:40:05Z

src/mds/nameserver2/namespace_service.cpp

@@ -196,6 +196,144 @@ void NameSpaceService::DeleteFile(::google::protobuf::RpcController* controller,
    return;
 }

+StatusCode GetRecoverFileInfo(const std::string& originFileName,


这个逻辑放curvefs感觉更好一点

这个逻辑放curvefs感觉更好一点

done

cw123 · 2021-02-19T08:46:45Z

src/mds/nameserver2/curvefs.cpp

@@ -1655,7 +1726,9 @@ StatusCode CurveFS::CheckPathOwnerInternal(const std::string &filename,
                return StatusCode::kNotDirectory;
            }

-            if (fileInfo.owner() != owner) {
+            // fileInfo.filename() != RECYCLEBINDIRNAME for recovery file
+            if (fileInfo.owner() != owner &&


这里不应该用&&

这里不应该用&&

done, 新增CheckRecycleFileOwner

cw123 · 2021-02-19T08:49:33Z

src/mds/nameserver2/namespace_service.cpp

+    }
+    recoverFileName = RECYCLEBINDIR + "/" + recoverFileInfo.filename();
+
+    FileWriteLockGuard guard(fileLockManager_, recoverFileName);


两个filename都要加锁，参考rename的加锁

两个filename都要加锁，参考rename的加锁

done

cw123 · 2021-02-19T08:53:46Z

src/mds/nameserver2/namespace_storage.cpp

@@ -130,6 +130,69 @@ StoreStatus NameServerStorageImp::DeleteFile(InodeID id,
    return getErrorCode(resCode);
 }

+StoreStatus NameServerStorageImp::RecoverFile(const FileInfo &recycleFInfo,


复用rename

复用rename

done

cw123 · 2021-02-22T03:09:06Z

src/mds/nameserver2/curvefs.cpp

+                                       FileInfo* recoverFileInfo) {
+    std::vector<FileInfo> fileInfoList;
+    recoverFileInfo->set_ctime(0);
+    const std::string recoverDir = "/RecycleBin";


不要硬编码

不要硬编码

done

cw123 · 2021-02-22T03:13:16Z

src/mds/nameserver2/curvefs.cpp

+    std::string lastEntry;
+    auto ret = WalkPath(originFileName, &parentFileInfo, &lastEntry);
+    if ( ret != StatusCode::kOK ) {
+        LOG(ERROR) << "WalkPath failed, the middle path of "


慎重使用ERROR级别的打印。只有需要电话告警的错误才需要ERROR打印，检查下这次commit中的所有ERROR打印是否必要。

慎重使用ERROR级别的打印。只有需要电话告警的错误才需要ERROR打印，检查下这次commit中的所有ERROR打印是否必要。

done

cw123 · 2021-02-22T03:26:03Z

src/mds/nameserver2/namespace_service.cpp

+        }
+        return;
+    }
+


destination 需要check isPathValid
RecycleFile和destination 需要共同check，参考IsRenamePathValid。

destination 需要check isPathValid
RecycleFile和destination 需要共同check，参考IsRenamePathValid。

destination应该不用检查吧，因为恢复后的路径是从fileInfo中获得的（创建时已经处理过了），并且应该不会出现互相包含路径的情况，因为destiantion路径不会包含/RecycleBin

如果是/a/RecycleBin，也会有问题

如果是/a/RecycleBin，也会有问题

实际测试了 /a/RecycleBin(或/a/RecycleBin/file1) 也没问题，因为rename那应该主要是检查出现目录包含后获取锁的问题，而recover场景下， /a/RecycleBin/file 和 /RecycleBin/file 并不是目录包含关系

cw123 · 2021-02-22T03:26:42Z

src/mds/nameserver2/namespace_service.cpp

+        fileId = request->fileid();
+    }
+
+    retCode = kCurveFS.GetRecoverFileInfo(request->filename(),


这里是否需要先加一小段的读锁，再考虑一下

这里是否需要先加一小段的读锁，再考虑一下

done

bai-charisu · 2021-02-22T05:55:02Z

src/mds/nameserver2/curvefs.cpp

@@ -1841,6 +1953,52 @@ StatusCode CurveFS::CheckFileOwner(const std::string &filename,
    }
 }

+StatusCode CurveFS::CheckRecycleFileOwner(const std::string &filename,


为什么要增加这个函数，以前的FileOwner函数不能直接用吗

为什么要增加这个函数，以前的FileOwner函数不能直接用吗

因为原来的检查fileowner需要保证目标文件和上层目录的owner都是相同的，但是恢复recyclebin文件时，recyclebin目录的owner是root，里面的文件owner可以是其他的

bai-charisu · 2021-02-22T05:55:41Z

src/mds/nameserver2/namespace_service.cpp

+        return;
+    }
+
+    retCode = kCurveFS.CheckDestinationOwner(


同上，为什么不能统一使用CheckFileOwner

同上，为什么不能统一使用CheckFileOwner

CheckFileOwner 在检查文件owner时如果文件不存在则返回失败，CheckDestinationOwner和CheckFileOwner的区别是当文件不存在时返回成功（recover、rename等目的文件应该是不存在的，只检查上层路径owner即可）

那这个不需要，你只需要把路径里的上层目录传给CheckFileOwner就可以了吧。

cw123 · 2021-02-23T11:16:27Z

src/mds/nameserver2/namespace_service.cpp

+    }
+
+    {
+        FileReadLockGuard guard(fileLockManager_, RECYCLEBINDIR);


不是对这个文件加锁RECYCLEBINDIR

request->filename()

request->filename()

这个request->filename 是删除前的文件名，这个文件是不存在的，这个是根据删除前的文件名去recyclebin找现在在recyclebin中对应的删除后的文件信息

[WIP] Refine wording for notice

SeanHai force-pushed the recover_recyclebin_data branch 2 times, most recently from 697e706 to 4aea70f Compare February 8, 2021 07:31

bai-charisu reviewed Feb 19, 2021

View reviewed changes

cw123 requested changes Feb 19, 2021

View reviewed changes

SeanHai force-pushed the recover_recyclebin_data branch 2 times, most recently from dfb9628 to e30a701 Compare February 22, 2021 02:06

cw123 requested changes Feb 22, 2021

View reviewed changes

bai-charisu reviewed Feb 22, 2021

View reviewed changes

SeanHai force-pushed the recover_recyclebin_data branch 2 times, most recently from 53dd97c to 0ab9ffd Compare February 22, 2021 08:50

curvefs: recovey recyclebin file

ddffa79

SeanHai force-pushed the recover_recyclebin_data branch from 0ab9ffd to ddffa79 Compare February 22, 2021 09:24

bai-charisu approved these changes Feb 23, 2021

View reviewed changes

cw123 requested changes Feb 23, 2021

View reviewed changes

cw123 approved these changes Feb 23, 2021

View reviewed changes

ilixiaocui approved these changes Feb 23, 2021

View reviewed changes

ilixiaocui merged commit b5e46ba into opencurve:master Feb 23, 2021

SeanHai deleted the recover_recyclebin_data branch April 29, 2021 01:51

ilixiaocui pushed a commit to ilixiaocui/curve that referenced this pull request Feb 6, 2023

Merge pull request opencurve#259 from zacharysarah/tweak-wording

e6703ba

[WIP] Refine wording for notice

curvefs: recovey recyclebin file #259

curvefs: recovey recyclebin file #259

Conversation

SeanHai commented Feb 8, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment