fix: switch from rootless containers to rootful containers

[vsiravar]

Issue #, if available:
Fixes #196
Description of changes:
Change finch vm configuration to support rootful containers. Opening a draft PR to get early feedback while investigating lima-vm/lima#1376.
Testing done:

2 e2e tests are failing due to lima-vm/lima#1376.

Additional tests for persistent disk feature to persist volumes and networks after vm is reinitialized

Test for additonal_disk

Retains volumes after vm is restarted

$ finch vm stop && finch vm remove && finch vm init
$ finch run --name test-container -v ~/workplace:/workplace alpine ls /workplace
testfiles
$ finch vm stop && finch vm remove && finch vm init
$ finch start --attach test-container              
testfiles

Retains created network after vm is reinitialized

$ finch vm stop && finch vm remove && finch vm init
$ finch network create my-bridge-network-1
756c515526ed29831c59d9f7d1b1142ec3e440b19096dc224ee25e9a883d00c1
$ finch network ls
NETWORK ID      NAME                   FILE
17f29b073143    bridge                 /etc/cni/net.d/nerdctl-bridge.conflist
d2f8cc31a256    my-bridge-network-1    /etc/cni/net.d/nerdctl-my-bridge-network-1.conflist
ce498ce9f1f0    my-bridge-network      /etc/cni/net.d/nerdctl-my-bridge-network.conflist

$ finch vm stop && finch vm remove && finch vm init
$ finch network ls                                 
NETWORK ID      NAME                   FILE
17f29b073143    bridge                 /etc/cni/net.d/nerdctl-bridge.conflist
d2f8cc31a256    my-bridge-network-1    /etc/cni/net.d/nerdctl-my-bridge-network-1.conflist
ce498ce9f1f0    my-bridge-network      /etc/cni/net.d/nerdctl-my-bridge-network.conflist

# Connect container to the network
$ finch run --network my-bridge-network-1 alpine

Tests for persisting images, containers, networks and volumes between rootless and rootful.

Test setup

1. Install finch 0.4.0 from Installer(rootless).
2. Create images, containers, networks and volumes.

$ finch pull alpine 
$ finch run alpine
$ finch network create rootless-network
$ finch run --name test-container -v ~/workplace:/workplace alpine ls /workplace
$ finch images
REPOSITORY    TAG       IMAGE ID        CREATED          PLATFORM          SIZE       BLOB SIZE
alpine        latest    69665d02cb32    2 minutes ago    linux/arm64/v8    7.8 MiB    3.1 MiB
$ finch ps -a
CONTAINER ID    IMAGE                              COMMAND            CREATED               STATUS                           PORTS    NAMES
1a1684bba87e    docker.io/library/alpine:latest    "/bin/sh"          2 minutes ago         Exited (0) 2 minutes ago                  alpine-1a168
924bd7b54549    docker.io/library/alpine:latest    "ls /workplace"    About a minute ago    Exited (0) About a minute ago 

3. Stop and remove vm.
4. Build finch from this branch(rootful) and replace the finch binary under /Applications/Finch/bin/.
5. Edit /Applications/Finch/os/finch.yaml with finch.yaml file from this branch.
6. Start the vm and check its running in rootful mode.

$ LIMA_HOME=/Applications/Finch/lima/data /Applications/Finch/lima/bin/limactl shell finch 
$ ps aux | grep containerd
root        1595  0.0  0.4 757868 33160 ?        Ssl  17:41   0:00 /usr/local/bin/containerd-stargz-grpc --log-level=debug --config=/etc/containerd-stargz-grpc/config.toml
root        1720  0.3  0.5 1419508 42832 ?       Ssl  17:41   0:00 /usr/local/bin/containerd
root        1911  0.0  0.1 720460  9640 ?        Sl   17:41   0:00 /usr/local/bin/containerd-shim-runc-v2 -namespace finch -id 1a1684bba87ea1d5a73285ae29ad890c91c27f74a9c7c43f236d34245fa4cc5c -address /run/containerd/containerd.sock
root        2142  0.0  0.1 720716 10284 ?        Sl   17:42   0:00 /usr/local/bin/containerd-shim-runc-v2 -namespace finch -id 

7. Run tests

$ finch images  
REPOSITORY    TAG       IMAGE ID        CREATED           PLATFORM          SIZE       BLOB SIZE
alpine        latest    69665d02cb32    10 minutes ago    linux/arm64/v8    7.8 MiB    3.1 MiB
$ finch ps -a   
CONTAINER ID    IMAGE                              COMMAND            CREATED           STATUS     PORTS    NAMES
1a1684bba87e    docker.io/library/alpine:latest    "/bin/sh"          10 minutes ago    Created             alpine-1a168
924bd7b54549    docker.io/library/alpine:latest    "ls /workplace"    10 minutes ago    Created             test-container
$ finch start 1a1684bba87e
1a1684bba87e

$ finch network ls
NETWORK ID      NAME                FILE
17f29b073143    bridge              /etc/cni/net.d/nerdctl-bridge.conflist
4b2aad24cab9    rootless-network    /etc/cni/net.d/nerdctl-rootless-network.conflist
                host                
                none                

$ finch start --attach test-container
finch

- [X] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[davidhsingyuchen]

LGTM re. the current code, thanks!

[ningziwen]

Should this be feat! (bumps minor version) or fix (bumps patch version)?

Following semantic versioning, my thought is the switching implementation details but not impacting external features should be fix, even if rootful is a big switch.

However, this broke two existing e2e tests, but these 2 e2e tests are specifically to how we implement the tests. It only happens in using netcat to run a server in container and test port forwarding. The reason is when container starts in rootful mode, it will ping the netcat port for one time, and netcat will exit after being pinged for one time. We are changing from netcat to nginx and they would all passed because nginx can be consistently pinged.

[estesp]

I'm more inclined to fix over feat! by nature of the fact that end user interfaces and usage does not change at all for the user. Some user's experiences with finch will improve due to the removal of the underlying limitations of rootless, which definitely would be seen as a "fix" by users, not a new feature.

[davidhsingyuchen]

Maybe we can change the PR title, which will be commit msg and appear in changelog later, to be something like switch from rootless containers to rootful containers. The current title may not be clear that all the containers are going to be rootful, not just supported.

[pendo324]

If the change is transparent to users, meaning that users before the change will not have their VM stop working, and users after the change will still have their vm init/vm start working without any errors or changes needed, then I don't think it's a breaking change, and we can use fix!.

Yes, we had to refactor the test, but most users aren't running containers that only publish a message on a port once ("one-shot" containers), so I actually think the new test is more representative of the actual usage of the -p option, and we should treat the followup into why the behavior is different between rootless and rootfull as a separate issue.

However, in my testing, this does (or at least can) break users with existing VMs when they update and run vm start again. At least I hit an error case like this:

$ ./_output/bin/finch vm start
INFO[0000] Starting existing Finch virtual machine...
INFO[0028] Finch virtual machine started successfully
FATA[0028] failed to setup ssh client: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain
$ ./_output/bin/finch images
FATA[0000] cannot access containerd socket "/run/containerd/containerd.sock": no such file or directory
FATA[0000] exit status 1

Repro is:

1. init a new VM with the latest main
2. stop it
3. pull this PR
4. make finch
5. try to run vm start

[davidhsingyuchen]

this does (or at least can) break users with existing VMs when they update and run vm start again

A Finch user will use an installer to update their Finch, and the existing VM will be erased by the installer, so this problem may not happen for normal users? In other words, it should only impact Finch devs.

Wondering this:

1. A user has some containers and images.
2. Run Finch installer to upgrade their Finch to a version that contains this change.
3. finch vm init
4. Will the previous containers and images be usable?

Since we're mounting the rootful counterparts into the same location in the persistent disk, theoretically it should work, but maybe we should verify that because if it's not working, then it'll be a breaking change for our users.

[vsiravar]

Wondering this:

1. A user has some containers and images.
2. Run Finch installer to upgrade their Finch to a version that contains this change.
3. finch vm init
4. Will the previous containers and images be usable?

Since we're mounting the rootful counterparts into the same location in the persistent disk, theoretically it should work, but maybe we should verify that because if it's not working, then it'll be a breaking change for our users.

I tried this test. The test was setup using

1. Install finch 0.4.0 from Installer(rootless).
2. Create images, containers, networks and volumes.
3. Stop and remove vm.
4. Build finch from this branch(rootful) and replace the finch binary under /Applications/Finch/bin/.
5. Edit /Applications/Finch/os/finch.yaml with finch.yaml file from this branch.
6. Start the vm.

Observations.

1. The images and containers persist between rootless and rootful.
2. Images can be run without errors. However while starting the containers using finch start command, there is an error complaining about mounts. Error log below

% finch start  77a9e1787dff
FATA[0000] failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error mounting "/home/siravara.linux/.local/share/nerdctl/1935db59/containers/finch/77a9e1787dffb48651a2a72204636d604af437aa63fd0d53fa69d65cd0291839/hostname" to rootfs at "/etc/hostname": stat /home/siravara.linux/.local/share/nerdctl/1935db59/containers/finch/77a9e1787dffb48651a2a72204636d604af437aa63fd0d53fa69d65cd0291839/hostname: no such file or directory: unknown 

This would be a breaking change for users who want to start their saved containers after upgrading to rootful.

[pendo324]

this does (or at least can) break users with existing VMs when they update and run vm start again

A Finch user will use an installer to update their Finch, and the existing VM will be erased by the installer, so this problem may not happen for normal users? In other words, it should only impact Finch devs.

That's a good point, the scenario I posted can only really happen to devs. We can include a mention in the release notes to say that it's expected and also list how to fix it.

Wondering this:

1. A user has some containers and images.
2. Run Finch installer to upgrade their Finch to a version that contains this change.
3. finch vm init
4. Will the previous containers and images be usable?

Since we're mounting the rootful counterparts into the same location in the persistent disk, theoretically it should work, but maybe we should verify that because if it's not working, then it'll be a breaking change for our users.

I just tested this (basically just removing my dev vm before re-init), and it worked fine fwiw. I think we already have e2e tests for persistent volume, but not across versions so this sounds like something we will have to manually verify.

[pendo324]

Observations.

1. The images and containers persist between rootless and rootful.
2. Images can be run without errors. However while starting the containers using finch start command, there is an error complaining about mounts. Error log below

% finch start  77a9e1787dff
FATA[0000] failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error mounting "/home/siravara.linux/.local/share/nerdctl/1935db59/containers/finch/77a9e1787dffb48651a2a72204636d604af437aa63fd0d53fa69d65cd0291839/hostname" to rootfs at "/etc/hostname": stat /home/siravara.linux/.local/share/nerdctl/1935db59/containers/finch/77a9e1787dffb48651a2a72204636d604af437aa63fd0d53fa69d65cd0291839/hostname: no such file or directory: unknown 

This would be a breaking change for users who want to start their saved containers after upgrading to rootful.

Interesting, I guess my tests weren't as in depth as yours. I think we can actually fix that by mounting the persistent data volume to both the old rootless and new rootfull locations. Can you try that out?

[vsiravar]

Interesting, I guess my tests weren't as in depth as yours. I think we can actually fix that by mounting the persistent data volume to both the old rootless and new rootfull locations. Can you try that out?

This actually works. However should we make this change though just to support rootless containers which were created prior to this change?

[davidhsingyuchen]

Interesting, I guess my tests weren't as in depth as yours. I think we can actually fix that by mounting the persistent data volume to both the old rootless and new rootfull locations. Can you try that out?

This actually works. However should we make this change though just to support rootless containers which were created prior to this change?

Thanks for looking into it. I think its ok to mount both locations, it shouldn't hurt anything else 👍. Why we do it might be slightly confusing to more advanced users though, so just document it as a comment in the finch.yaml provisioning script.

I'm also good with this. However, my assumption is that eventually we still want to remove those rootless mounts to avoid confusion and reduce complexity of our code, and we're delaying that, so the blast radius of it can be minimized. To be specific, I suppose we'd like to add a TODO: in a future release, we will remove those mounts in a BREAKING CHANGES PR, and in the release notes, we will say something around this: "Containers created by Finch <= v0.4.0 will be malfunctional after upgrading to this version", and hopefully it will be already long enough since the next version of v0.4.0 is out so that the possibility of our users still having such containers running is low.

Seems mounting both locations persistently doesn't add any performance burden or very minimum maintainence burden? Instead of a TODO to remove it as a breaking change, how about adding a comment to say mounting both locations gives the ability of switching between rootful and rootless without breaking change?

Yeah that's fair. I don't have a strong opinion on this. By "confusion & complexity", I was mainly referring to the fact that yet another set of paths that needs to be understood by Finch devs.

[vsiravar]

Added tests in PR description testing persistence of images, containers, volumes and networks between rootless and rootful.

[vsiravar]

Adding additional observations with forward compatibility.

I was doing some more testing between rootful and rootless mode. If there is a situation where users who produce rootful containers will for some reason downgrade the version of finch to a rootless version(say 0.4.0) the persisted data will not run in the rootelss context and users might lose persisted data. I found that rootful containers can't run in rootless mode from my test.
Which makes it not forward compatible.
Maybe in the release notes we can mention that "Containers produced using this version and later versions may misbehave in versions <= 0.4.0"?

[pendo324]

Adding additional observations with forward compatibility.

I was doing some more testing between rootful and rootless mode. If there is a situation where users who produce rootful containers will for some reason downgrade the version of finch to a rootless version(say 0.4.0) the persisted data will not run in the rootelss context and users might lose persisted data. I found that rootful containers can't run in rootless mode from my test. Which makes it not forward compatible. Maybe in the release notes we can mention that "Containers produced using this version and later versions may misbehave in versions <= 0.4.0"?

Yeah, we should definitely mention that in the patch notes. Eventually, we may support both modes and need to come up with a more permanent fix for this (maybe just two different sets of mount points, or two different hosts), but for now, I think it makes sense as a note. We might even want to add it to a doc page/README too.

[vsiravar]

Adding additional observations with forward compatibility.
I was doing some more testing between rootful and rootless mode. If there is a situation where users who produce rootful containers will for some reason downgrade the version of finch to a rootless version(say 0.4.0) the persisted data will not run in the rootelss context and users might lose persisted data. I found that rootful containers can't run in rootless mode from my test. Which makes it not forward compatible. Maybe in the release notes we can mention that "Containers produced using this version and later versions may misbehave in versions <= 0.4.0"?

Yeah, we should definitely mention that in the patch notes. Eventually, we may support both modes and need to come up with a more permanent fix for this (maybe just two different sets of mount points, or two different hosts), but for now, I think it makes sense as a note. We might even want to add it to a doc page/README too.

Cool will add the below note in the release notes.

Note on rootful containers

Containers created by Finch >= v0.4.1 will malfunction in versions <=0.4.0.

[pendo324]

Note on rootful containers

Containers created by Finch >= v0.4.1 will malfunction in versions <=0.4.0.

Looks good, but is it possible to add some more context into why this would occur?

[vsiravar]

Note on rootful containers

Containers created by Finch >= v0.4.1 will malfunction in versions <=0.4.0.

Looks good, but is it possible to add some more context into why this would occur?

Changed to

"Containers created using Finch version 0.4.1 or later may not function properly in versions 0.4.0 or earlier due to how the data of rootful containers is stored with root user permissions. This can cause issues when accessed by older versions of Finch that do not support rootful containers."