feat: saves containerd user data to a persistent disk

[sam-berning]

this allows users to retain downloaded images, containers, etc. across new installations of finch

Signed-off-by: Sam Berning [email protected]

Issue #, if available: #77

Description of changes:

• Adds a new package for managing the user data persistent disk
• Checks disk configuration & creates disk if necessary on finch vm init
• Attaches disk whenever the VM starts

Testing done:

• Unit testing on disk and cmd/finch
• Manual testing

Open questions:

• Should we also check the configuration on finch vm start?

• 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.

[ningziwen]

Could you add e2e tests?

[ningziwen]

Could this change resolve this issue #11?

[sam-berning]

Could this change resolve this issue #11?

I don't think so. We'd need to move the whole .../lima/data directory under the user's home directory which I think is outside the scope of this PR.

This current implementation is a little awkward though, because saving the datadisk via a symlink into a specific user's home directory necessarily pins the installation to a single user. Overall I'm not sure if this makes things any worse, since many of the files under .../lima/data are already owned by the installing user, but it's interesting to note.

[pendo324]

LGTM for the most part, just left some questions / nits

[sam-berning]

The e2e tests on M1 hosts keep failing in various ways in this PR, most commonly with timeouts on finch vm stop.

One of the recent test runs on this PR has had some weird behavior that I wanted to track down. I was seeing lots of failures to finch vm start with the message:

[hostagent] could not attach disk \\\"finch\\\", in use by instance \\\"finch\\\"

I'm just gonna write down what I investigated and talk through some of my thought process here.

Investigation

I ssh'd into the runner that was running that test to do some investigation. I saw the following:

ec2-user@ip-172-31-60-114 ~ % ls -lah /Users/ec2-user/ar/_work/finch/finch/_output/lima/data/_disks/finch/
total 0
drwx------  4 ec2-user  staff   128B Jan  4 02:54 .
drwx------  3 ec2-user  staff    96B Jan  4 02:42 ..
lrwxr-xr-x  1 ec2-user  staff    31B Jan  4 02:42 datadisk -> /Users/ec2-user/.finch/datadisk
lrwxr-xr-x  1 ec2-user  staff    60B Jan  4 02:54 in_use_by -> /Users/ec2-user/ar/_work/finch/finch/_output/lima/data/finch

ec2-user@ip-172-31-60-114 ~ % LIMA_HOME=/Users/ec2-user/ar/_work/finch/finch/_output/lima/data /Users/ec2-user/ar/_work/finch/finch/_output/lima/bin/limactl ls
NAME     STATUS     SSH            VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
finch    Stopped    127.0.0.1:0    qemu      aarch64    2       4GiB      100GiB    ~/ar/_work/finch/finch/_output/lima/data/finch

The resource lock on the disk (in_use_by) is still engaged even though the instance is Stopped. This should only be the case when the instance is Running.

Seems like this strange state was caused by a finch vm stop timeout in an earlier test:

Finch CLI e2e Tests virtual machine lifecycle when the virtual machine is in stopped status should be able to remove the virtual machine
/Users/ec2-user/ar/_work/finch/finch/e2e/virtual_machine_test.go:39
  time="2023-01-04T02:53:01Z" level=info msg="Stopping existing Finch virtual machine..."
  [FAILED] in [It] - /Users/ec2-user/go/pkg/mod/github.com/runfinch/[email protected]/command/command.go:115 @ 01/04/23 02:54:01.86
• [FAILED] [60.005 seconds]
Finch CLI e2e Tests virtual machine lifecycle when the virtual machine is in stopped status [It] should be able to remove the virtual machine
/Users/ec2-user/ar/_work/finch/finch/e2e/virtual_machine_test.go:39

  [FAILED] Timed out after 60.001s.
  Expected process to exit.  It did not.
  In [It] at: /Users/ec2-user/go/pkg/mod/github.com/runfinch/[email protected]/command/command.go:115 @ 01/04/23 02:54:01.86

Because this stop command took longer than 60s, it timed out before it had time to release the lock on the disk that the instance was using. This put us in the weird state of having a stopped VM using a disk, which meant that when the instance started up again, it would fail because it saw that the disk it was trying to attach was already in use by itself.

Takeaways

Two interesting issues from this, the first on Lima and the second on Finch:

1. In Lima, should a stopped instance holding a lock for a disk fail to start up when trying to attach the same disk? It feels like no, but this is a very strange edge case.

2. In a normal Lima use case, this can be handled by limactl disk unlock finch. However, since we want this user data disk to be transparent to the users of finch, we need to figure out how to remediate this automatically. Should we be checking for locks on the disk on every startup of the finch VM, and unlock if needed?

Of course, there's the question as to how it can take >60 seconds for the M1 11.7 runner to stop the VM. Are we running
too many tests in parallel? Or is finch vm stop really that slow on M1 macs?

The second takeaway also goes back to the open question on this PR -- should we be ensuring the disk only on init or on both init and start? I'm leaning more towards both, just to make it as painless as possible if a user were to manually make some change to the user data disk setup.

On that topic, is the current location of the datadisk good? Currently, the only other file under ~/.finch is finch.yaml which we teach users to edit. Since it's best for users to leave datadisk alone, should we put it somewhere else? Perhaps under another .directory in ~/.finch?

[pendo324]

Very nice writeup on the investigation, a lot of people would just dismiss the integration test failures as intermittent. I tried to respond, roughly in order of importance:

The second takeaway also goes back to the open question on this PR -- should we be ensuring the disk only on init or on both init and start? I'm leaning more towards both, just to make it as painless as possible if a user were to manually make some change to the user data disk setup.

I think I agree with this. It's more work for the CLI, but much or all of the code can probably be reused. It should in theory reduce the chance of errors for Finch users.

On that topic, is the current location of the datadisk good? Currently, the only other file under ~/.finch is finch.yaml which we teach users to edit. Since it's best for users to leave datadisk alone, should we put it somewhere else? Perhaps under another .directory in ~/.finch?

I think I'm good with moving it under a new hidden folder under ~/.finch, just to avoid accidental edits. Maybe just ~/.finch/.disks or something?

In Lima, should a stopped instance holding a lock for a disk fail to start up when trying to attach the same disk? It feels like no, but this is a very strange edge case.

Maybe we can open an issue on the Lima repo that describes this edge case to see if they want to leave it as is, or add some unlocking mechanism when the same instance is re-using a disk from a "dirty shutdown"?

Of course, there's the question as to how it can take >60 seconds for the M1 11.7 runner to stop the VM. Are we running
too many tests in parallel? Or is finch vm stop really that slow on M1 macs?

This was probably a rare occurence that will likely eventually happen again, given the volume that we run the integration tests at. We can probably add some more cleanup code here, and remove it after you implement the init/start check, or it gets changed in Lima itself?

[davidhsingyuchen]

Of course, there's the question as to how it can take >60 seconds for the M1 11.7 runner to stop the VM. Are we running
too many tests in parallel? Or is finch vm stop really that slow on M1 macs?

FWIW, currently e2e tests are run sequentially: https://github.com/onsi/ginkgo/blob/e7e3db7a3ee2fa40d21931f4f98f0c71c34ed3eb/types/config.go#L46

[pendo324]

LGTM, just one more nit