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.

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

support send large file to workload #532

Merged
merged 32 commits into from
Sep 7, 2023

Conversation

hongjijun233
Copy link
Contributor

关于cli的send命令,之前的send是直接整个文件发到core里面,但是core的grpc配置为单条message最大只能为20M,所以有些文件是无法传输到core端的。

这里是增加了一个叫做SendLargeFile的方法,用stream的方式接受cli端分chunk发送过来的数据,并拼接起来再一并发送到容器里面。

rpc/rpc.go Outdated Show resolved Hide resolved
@jschwinger233 jschwinger233 changed the title Add send large file from cli [WIP] Add send large file from cli Jan 7, 2022
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
@@ -73,6 +73,7 @@ type Cluster interface {
// file methods
Copy(ctx context.Context, opts *types.CopyOptions) (chan *types.CopyMessage, error)
Send(ctx context.Context, opts *types.SendOptions) (chan *types.SendMessage, error)
SendLargeFile(ctx context.Context, opts chan *types.SendLargeFileOptions, resp chan *types.SendMessage) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

其实不用加新的 interface, 直接改旧的 Send 就可以了.

rpc/gen/core.proto Outdated Show resolved Hide resolved
rpc/gen/core.proto Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
@jschwinger233
Copy link
Member

要是有问题可以开个 zoom 来讨论

@hongjijun233
Copy link
Contributor Author

我改了下代码,现在看起来是能够跑了。但是会有些比较诡异的情况,时而成功时而失败,让我再debug一下。然后关于上面的review意见我也再改一下。

Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自己多测试一下, 包括老的 Send 接口是否还能用, 还有发送一些 >3G 的文件, 都是我们真实存在的业务场景.

rpc/rpc.go Outdated Show resolved Hide resolved
rpc/rpc.go Outdated Show resolved Hide resolved
types/options.go Outdated Show resolved Hide resolved
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
utils.SentryGo(func(ID, name string, size int64, content io.Reader, uid, gid int, mode int64) func() {
return func() {
defer wg.Done()
if err := c.withWorkloadLocked(ctx, ID, func(ctx context.Context, workload *types.Workload) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

每个 chunk 都上一次锁, 虽然临界区是小了, 但是代价可能有点大. 考虑下从头到尾只上一把大锁, 传个大文件(>1G)对比一下耗时.

cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
@@ -39,6 +39,7 @@ type API interface {
VirtualizationCreate(ctx context.Context, opts *enginetypes.VirtualizationCreateOptions) (*enginetypes.VirtualizationCreated, error)
VirtualizationResourceRemap(context.Context, *enginetypes.VirtualizationRemapOptions) (<-chan enginetypes.VirtualizationRemapMessage, error)
VirtualizationCopyTo(ctx context.Context, ID, target string, content []byte, uid, gid int, mode int64) error
VirtualizationCopyChunkTo(ctx context.Context, ID, target string, size int64, content io.Reader, uid, gid int, mode int64) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CopyTo 和 CopyChunkTo 可以合并吧, 让 cluster.Copy 和 cluster.CopyLarge 都调用一个 engine 接口

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

刚才简单讨论了一下, rpc 层还是两个 API, 但是 cluster 层和 engine 都只有一个 send 接口.

engine/docker/container.go Show resolved Hide resolved
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
cluster/calcium/sendlarge.go Outdated Show resolved Hide resolved
@CMGS CMGS force-pushed the master branch 3 times, most recently from 06977aa to 837bec8 Compare March 9, 2022 04:21
@DuodenumL DuodenumL force-pushed the master branch 2 times, most recently from 0c06f3a to 34dacc4 Compare April 4, 2022 09:15
@CMGS
Copy link
Contributor

CMGS commented Jul 11, 2023

unbelievable

@hongjijun233 hongjijun233 force-pushed the add-send-large-file branch from a982876 to 924a9a8 Compare July 19, 2023 08:39
rpc/rpc.go Outdated Show resolved Hide resolved
rpc/rpc.go Outdated Show resolved Hide resolved
@CMGS
Copy link
Contributor

CMGS commented Jul 31, 2023

resolve conflicts.

engine/docker/container.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
engine/docker/container.go Outdated Show resolved Hide resolved
rpc/rpc.go Outdated Show resolved Hide resolved
@CMGS
Copy link
Contributor

CMGS commented Aug 18, 2023

另外注意一下单元测试覆盖率和实际测试下多路文件并发 send

@CMGS CMGS changed the title [WIP] Add send large file from cli support send large file to workload Sep 7, 2023
@CMGS CMGS requested a review from jschwinger233 September 7, 2023 06:20
@CMGS CMGS merged commit 825eaf0 into projecteru2:master Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants