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

feat: add finch-daemon #1181

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

pendo324
Copy link
Member

Issue #, if available:

Description of changes

Adds finch-daemon, so that it starts inside the VM and is exposed to processes inside the VM (via /run/finch.sock and /var/run/docker.sock), and processes on the native macOS host (via /path/to/lima/data/finch/sock/finch.sock).

To make this possible, these changes had to be made:

  1. Added finch-daemon specific logic to ./finch.yaml.d/mac.yaml
    1. This logic makes it so that the finch-daemon binary and service file are copied into the VM, and then starts the daemon using systemd
  2. In Makefile.darwin, changed the order so that the finch-daemon specific provisioning script would be the final script, after being merged
    1. Also added a add-daemon-mount target, which makes it so that duplicate mounts are not added to Lima. This is done to avoid the default lima provisioning scripts failing due to trying to mount/unmount a directory twice (the end result is users may see cloud-final.service fail after shelling into the VM. although this should only ever really impact developers, it was easy enough to fix)
  3. Added finch@.service

This change also currently relies on a fork of finch-core, which actually has the code necessary for building finch-daemon. Once runfinch/finch-core#424 is merged, I'll update this PR to remove the use of a different finch-core upstream.

Testing done

  • finch-daemon works as expected on macOS:
DOCKER_HOST=unix:///path/to/finch/_output/lima/data/finch/sock/finch.sock docker images
REPOSITORY                                                                   TAG                IMAGE ID       CREATED        SIZE
public.ecr.aws/amazonlinux/amazonlinux                                       2023               75763d26a280   3 weeks ago    51.4MB
public.ecr.aws/docker/library/registry                                       latest             12120425f07d   2 months ago   9.47MB
public.ecr.aws/lambda/nodejs                                                 18-x86_64          97f94536a3af   3 weeks ago    180MB
  • 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.

@pendo324 pendo324 self-assigned this Nov 13, 2024
@pendo324
Copy link
Member Author

Turns out git.exe isn't in the Windows runner's PATH. Added it here: runfinch/infrastructure#769

After that gets deployed to the runners, it should resolve the Windows build failures we're seeing.

.gitmodules Outdated Show resolved Hide resolved
coderbirju
coderbirju previously approved these changes Nov 14, 2024
finch@.service Outdated Show resolved Hide resolved
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Shubhranshu153
Shubhranshu153 previously approved these changes Nov 15, 2024
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
[Unit]
Description=Finch daemon %I
Documentation=https://runfinch.com https://github.com/runfinch/finch-daemon
After=network.target local-fs.target containerd.service buildkit.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make use of finch-daemon socket activation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking. We can do it as a follow up.

sudo systemctl restart containerd.service
# Set a default ulimit for number of files in containerd
sudo mkdir -p /usr/local/lib/systemd/system/containerd.service.d/
printf '[Service]\nLimitNOFILE=1048576\n' | sudo tee /usr/local/lib/systemd/system/containerd.service.d/finch.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

@@ -75,7 +78,7 @@ endif

FINCH_CORE_DIR := $(CURDIR)/deps/finch-core

remote-all: arch-test finch install.finch-core-dependencies finch.yaml networks.yaml config.yaml
remote-all: arch-test finch install.finch-core-dependencies finch.yaml networks.yaml config.yaml $(OUTDIR)/finch-daemon/finch@.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need separate targets for networks.yaml, config.yaml, $(OUTDIR)/finch-daemon/finch@.service?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think anything that writes a file should probably have its own rule so that its not re-created unless the file is deleted

Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
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.

4 participants