Add container copy/cp command for host-container file transfer

[simone-panico]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

Adds the container copy (aliased as cp) command to copy files between a running container and the local filesystem.

I saw #1023 and the feedback from @dcantah — the previous attempt relied on tar being installed inside the container.
This implementation takes the recommended approach:
file transfers go through the guest agent via the existing copyIn/copyOut methods on the core Containerization, with no dependency on container tooling.

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

[JaewonHur]

Related to #232

[JaewonHur]

⏺ CopyOut/CopyIn Scenarios: Current vs Docker Behavior

Scenario 1: copy(in/out) src dst (src must exist)

• File, Directory → Non-existing:

  • Docker (desired): save as dst
  • Current: File is saved as dst, Directory is not supported.

• File → File

  • Docker (desired): replace dst
  • Current: replace dst

• Directory → File

  • Docker (desired): error
  • Current: Directory is not supported

• File, Directory → Directory

  • Docker (desired): save under dst
  • Current: File returns error, Directory is not supported.

Scenario 2: copy(in/out) src dst/ (dst must be a directory if existing)

• File → Non-existing:

  • Docker (desired): error no such directory
  • Current: dst is created and src file is saved under dst.

• Directory → Non-existing:

  • Docker (desired): save as dst
  • Current: Directory is not supported.

• File, Directory → Directory

  • Docker (desired): save under dst
  • Current: File is saved under dst. Directory is not supported.

Scenario 3: copy(in/out) src/ dst (src must be an existing directory)

• Directory → Non-existing:

  • Docker (desired): save as dst
  • Current: Directory is not supported.

• Directory → File

  • Docker (desired): error
  • Current: Directory is not supported.

• Directory → Directory

  • Docker (desired): save under dst
  • Current: Directory is not supported.

Scenario 4: copy(in/out) src/ dst/ (src must be an existing directory & dst must be a directory if existing)

• Directory → Non-existing:

  • Docker (desired): save as dst
  • Current: Directory is not supported.

• Directory → Directory

  • Docker (desired): save under dst
  • Current: Directory is not supported.

[JaewonHur]

Hi @simone-panico
Above I summarized the desired behavior and current behavior.

The bold italic phrases are what is different from desired behavior (i.e., docker) now, and can be addressed.

Below is general comments.

1. Copying directory is not supported yet, we need to handle it later.
2. If dst/, it would be better to check in command line if dst is directory if it exists.

[JaewonHur]

Hi @simone-panico
Could you try this PR with apple/containerization#474, which supports copying directories.

You may need to build this project with local containerization (BUILDING.md), and wire up SandboxService.

[dcantah]

@JaewonHur the change above does not allow copying directories. It adds a tar reader/writer that will be used to support this, but it's not in the change itself.

[simone-panico]

If you’re interested, I could try to create a Directory Copier within containerization. It would also be a great challenge for me

[simone-panico]

@JaewonHur @dcantah, I created apple/containerization#543 for clarity.

As soon as apple/containerization#474 is merged I think I could start with implementing the directory copying. What do you think?

[JaewonHur]

Hi @simone-panico, sorry for the delay!

We have an internal discussion how to better support container cp, and want to pause this for now. We will definitely revisit this PR once every thing becomes clear! Thanks!

[simone-panico]

Hey Guys, I experimented a bit with the ContainerizationTar and got a running version for Copying Directories #563. It would be great if you could give me a short Feedback if that is the right way to go

[JaewonHur]

Hi @simone-panico, sorry for the late reply!

We finally merged support copying directory in containerization (apple/containerization#571).

Previously, we did custom implementation of tar reader/writer (apple/containerization#474) for vminitd (which runs in linux VM) as the swift linux sdk didn't include libarchive.
However, they are finally included, and we are back using it so that we can copy directories using tar.

You would be able to check it under apple/containerization#571, and we can continue this PR to add full support for container copy :)

[JaewonHur]

Here is test script.
You can run either ./run_copy_test.sh docker or ./run_copy_test.sh container.

run_copy_tests.sh

[JaewonHur]

Current test result

=========================================
Test Results Summary
=========================================
Total : 70
Passed: 54
Failed: 16

Failed tests:
  ✗ CopyOut S2: Directory → Non-existing/: succeeds
  ✗ CopyOut S2: Directory → Non-existing/: dst created
  ✗ CopyOut S2: Directory → Non-existing/: dst/file1.txt
  ✗ CopyOut S4: Directory/ → Non-existing/: succeeds
  ✗ CopyOut S4: Directory/ → Non-existing/: dst created
  ✗ CopyOut S4: Directory/ → Non-existing/: dst/file1.txt
  ✗ CopyIn S1: File → Existing directory: succeeds
  ✗ CopyIn S1: File → Existing directory: dst/file.txt exists
  ✗ CopyIn S1: Directory → Existing directory: dst/dir exists
  ✗ CopyIn S1: Directory → Existing directory: dst/dir/file1.txt
  ✗ CopyIn S2: File → Non-existing/: errors
  ✗ CopyIn S2: Directory → Non-existing/: dst/file1.txt
  ✗ CopyIn S3: Directory/ → Existing directory: dst/dir exists
  ✗ CopyIn S3: Directory/ → Existing directory: dst/dir/file1.txt
  ✗ CopyIn S3: Directory/ → Existing directory: dst/dir/sub exists
  ✗ CopyIn S4: Directory/ → Non-existing/: dst/file1.txt

[JaewonHur]

./run_copy_test.sh container has some failing cases.

[simone-panico]

./run_copy_test.sh container has some failing cases.

Yes, I already saw it, but Thank you very much 😄
I haven't had a chance to dig into it yet, but I believe this is a deeper issue. I think there is also a bug in the containerization implementation, but not 100% sure.
I will get back to you :)

[JaewonHur]

Thank you! :)

[simone-panico]

Hey @JaewonHur, all the tests are passing now :)
As mentioned, I had to make some changes in 586#containerization to get everything working. All the changes I made to Containerization were done before to the grpc-swift-2 upgrade, since they wouldn't have been compatible with container otherwise. Once 1308#Container is done and merged I'll adapt the changes and run the tests again.

[JaewonHur]

Hi @simone-panico thank you so much for all your hard work!

I looked into the code, but my small concern is whether it is the best approach to modify vminitd API. Current vminitd's copy(src, dst) API, looks to me, has clear semantics: create dst if non-existing, overwrite if both src and dst are files, extract to dst if both are directories, otherwise error. Adding additional parameters here might make it unclear for later implementation of new vminitd.

On the other hand, I see the problem is we cannot stat whether a given path in guest container is file, directory or non-existing.

We might need more discussion. One possible other direction would be to rely on exception handling. For example, given container cp foo guest:/bar/, first try copy(foo, guest:/bar/foo), and if it fails, then try copy(foo, guest:/bar) if foo is directory.

@dcantah @jglogan Do you have any thoughts?

[simone-panico]

Hi @simone-panico thank you so much for all your hard work!

I looked into the code, but my small concern is whether it is the best approach to modify vminitd API. Current vminitd's copy(src, dst) API, looks to me, has clear semantics: create dst if non-existing, overwrite if both src and dst are files, extract to dst if both are directories, otherwise error. Adding additional parameters here might make it unclear for later implementation of new vminitd.

On the other hand, I see the problem is we cannot stat whether a given path in guest container is file, directory or non-existing.

We might need more discussion. One possible other direction would be to rely on exception handling. For example, given container cp foo guest:/bar/, first try copy(foo, guest:/bar/foo), and if it fails, then try copy(foo, guest:/bar) if foo is directory.

@dcantah @jglogan Do you have any thoughts?

Yeah, I completely see your point, it's not the most elegant solution. I think handling it with the retry approach is also not the best solution, because if it starts creating and writing files and then fails we would need to first delete everything and then recreate.

Another option could be by making a Stat RPC that gives back two Bools (exists and is_directory). Like that we could handle the logic in Container but get the relevant Information we need. Something like the Unix stat syscall.

[JaewonHur]

if it starts creating and writing files and then fails we would need to first delete everything and then recreate.

Could you describe more how it fails in the middle of writing files? I thought vminitd's copy would fail before starting any persistent change if initial conditions don't meet.

Another option could be by making a Stat RPC that gives back two Bools (exists and is_directory). Like that we could handle the logic in Container but get the relevant Information we need. Something like the Unix stat syscall.

Yes, we might modify current copy RPC or add an RPC for stat.

[simone-panico]

container cp foo guest:/bar/, first try copy(foo, guest:/bar/foo), and if it fails, then try copy(foo, guest:/bar) if foo is directory.

I initially thought the first copy(foo, guest:/bar/foo) would create parent directories as a side effect, making rollback necessary. But actually we can do copy(foo, guest:/bar/foo, createParents: false) on the first call, so it fails.
My Bad ;)

Yes, we might modify current copy RPC or add an RPC for stat.

Sure, just let me know what the team decides and I'll implement it :)

[JaewonHur]

Thanks a lot! :)
Will be in touch soon!

[JaewonHur]

Hi @simone-panico we'd like to keep the API change in vminitd as small as possible.

Would it be able to achieve it without changing vminitd?

[simone-panico]

Hi @simone-panico we'd like to keep the API change in vminitd as small as possible.

Would it be able to achieve it without changing vminitd?

Hey, yeah sure, I'll try it :)