Skip to content

Move containerd fs package in tree#101

Merged
stevvooe merged 3 commits intocontainerd:masterfrom
cpuguy83:move_c8d_fs_pkg
Feb 5, 2018
Merged

Move containerd fs package in tree#101
stevvooe merged 3 commits intocontainerd:masterfrom
cpuguy83:move_c8d_fs_pkg

Conversation

@cpuguy83
Copy link
Member

This allows the fs package to be versioned separately from containerd
and makes for a nice interface for filesystem interactions in
continuity.

@cpuguy83
Copy link
Member Author

I have not yet setup the Makefile for root tests ala containerd.

@stevvooe
Copy link
Member

LGTM on first pass.

@justincormack
Copy link
Contributor

Ah good, I need to find some time to work on this now...

This allows the fs package to be versioned separately from containerd
and makes for a nice interface for filesystem interactions in
continuity.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Copy link
Member Author

For some reason we need sudo to run the fs tests.
copy_file_range is getting EPERM otherwise.... could be related to a seccomp policy or something in the travis container config?

@cpuguy83
Copy link
Member Author

Ok, this is green.

@AkihiroSuda
Copy link
Member

LGTM, I think we should reorganize README to cover non-proto packages like this

@justincormack
Copy link
Contributor

justincormack commented Jan 25, 2018 via email

@cpuguy83
Copy link
Member Author

@justincormack Well, when sudo is used on travis you get a full VM rather than a container.

@justincormack
Copy link
Contributor

Ah. Well almost certainly then they are running a seccomp policy/docker version that is older than their kernel, so doesnt know about copy_file_range and blocks it, while the kernel does support it..

@dmcgowan
Copy link
Member

dmcgowan commented Feb 1, 2018

Can you drop the github.com/containerd/containerd files now that they are no longer needed. I see you already removed the errdefs requirement and removed from the vendor file.

This seems to be required for the fs tests.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Copy link
Member Author

cpuguy83 commented Feb 2, 2018

Good catch, done.


// CopyDir copies the directory from src to dst.
// Most efficient copy of files is attempted.
func CopyDir(dst, src string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can this just be Copy?

Copy link
Member

Choose a reason for hiding this comment

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

Let's save this discussion. We need to do a 3 way merge of this function, with one Tonis wrote, and one in Moby. Yes we should rename it, but we can do that after we move it.

Copy link
Member

@dmcgowan dmcgowan Feb 2, 2018

Choose a reason for hiding this comment

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

Let me elaborate a bit more...

  • This function only handles doing a direct copy of directories, attempts to use copy file range to speed things up.
  • Moby function is also doing a directory copy but other optimizations have been added to make sure the copy is fast and there is a an open PR to speed it up more with parallelizing the slower parts.
  • Tonis has a similar copy function he uses for build kit but can also apply filters (and maybe works on more than just directories?).

Copy link
Member

Choose a reason for hiding this comment

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

@dmcgowan File a bug for that. @ijc has opinions there, as well.

Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@stevvooe stevvooe merged commit 1a794c0 into containerd:master Feb 5, 2018
@cpuguy83 cpuguy83 deleted the move_c8d_fs_pkg branch February 7, 2018 17:32
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.

5 participants