Run as a container (#8) including Docker-in-Docker. #84

Merged
wolfogre merged 2 commits from :telackey/dind into main 2023-04-11 02:58:13 +00:00
Contributor

This adds a very simple Dockerfile and run script for running act_runner as a container.

It also allows setting Privileged and ContainerOptions flags via the new config file when spawning task containers. The combination makes it possible to use Docker-in-Docker (if installed in the select task executor image), which requires privileged mode, as well as pass any other options child Docker containers may require.

For example, if Gitea is running in Docker on the same machine, for the checkout action to behave as expected from a task container launched by act_runner, it might be necessary to map the hostname via something like:

container:
  network_mode: bridge
  privileged: true
  options: --add-host=my.gitea.hostname:host-gateway

NOTE: Description updated to reflect latest code.
NOTE: Description updated to reflect latest code (again).

This adds a very simple Dockerfile and run script for running `act_runner` as a container. It also allows setting `Privileged` and `ContainerOptions` flags via the new config file when spawning task containers. The combination makes it possible to use Docker-in-Docker (if installed in the select task executor image), which requires `privileged` mode, as well as pass any other options child Docker containers may require. For example, if Gitea is running in Docker on the same machine, for the `checkout` action to behave as expected from a task container launched by `act_runner`, it might be necessary to map the hostname via something like: ``` container: network_mode: bridge privileged: true options: --add-host=my.gitea.hostname:host-gateway ``` > NOTE: Description updated to reflect latest code. > NOTE: Description updated to reflect latest code (again).
telackey added 5 commits 2023-03-27 18:42:52 +00:00
telackey added 1 commit 2023-03-27 19:08:29 +00:00
Some checks failed
check and test
7ddec9699a
Fix spacing.
lunny added the
kind
feature
label 2023-03-28 00:15:16 +00:00
lunny reviewed 2023-03-28 00:16:21 +00:00
Dockerfile Outdated
@ -0,0 +5,4 @@
RUN make build
FROM ubuntu:22.04 as runner
Owner

The base image looks too big? Maybe we could use a smaller image.

The base image looks too big? Maybe we could use a smaller image.
telackey marked this conversation as resolved
lunny reviewed 2023-03-28 16:28:23 +00:00
runtime/task.go Outdated
@ -79,6 +79,7 @@ func NewTask(forgeInstance string, buildID int64, client client.Client, runnerEn
Input: &TaskInput{
envs: runnerEnvs,
containerNetworkMode: "bridge", // TODO should be configurable
privileged: true, // TODO should be configurable
Owner

Is this safe enough for forked repository pull request?

Is this safe enough for forked repository pull request?
telackey marked this conversation as resolved
telackey added 1 commit 2023-03-28 20:22:39 +00:00
All checks were successful
check and test
fdd29d2eeb
Use more compact image.
telackey added 1 commit 2023-03-28 20:43:28 +00:00
telackey added 1 commit 2023-03-28 20:48:59 +00:00
telackey added 1 commit 2023-03-28 20:51:13 +00:00
All checks were successful
check and test
0ba4f94676
Merge
telackey added 1 commit 2023-03-28 20:53:03 +00:00
All checks were successful
check and test
de6abab9a8
network mode
Author
Contributor

In response to comments, I updated the the code to add the following configuration options:

dockerContainerOptions / GITEA_RUNNER_DOCKER_CONTAINER_OPTIONS
dockerNetworkMode / GITEA_RUNNER_DOCKER_NETWORK_MODE default:"bridge"
dockerPrivileged / GITEA_RUNNER_DOCKER_PRIVILEGED default:"false"

That way privileged mode can remain disabled by default, for those not needing dind support, and additional container options can be specified more easily (eg, --add-host=my.gitea.host:host-gateway).

NOTE: Outdated by config file.

In response to comments, I updated the the code to add the following configuration options: ``` dockerContainerOptions / GITEA_RUNNER_DOCKER_CONTAINER_OPTIONS dockerNetworkMode / GITEA_RUNNER_DOCKER_NETWORK_MODE default:"bridge" dockerPrivileged / GITEA_RUNNER_DOCKER_PRIVILEGED default:"false" ``` That way `privileged` mode can remain disabled by default, for those not needing dind support, and additional container options can be specified more easily (eg, `--add-host=my.gitea.host:host-gateway`). > NOTE: Outdated by config file.
Author
Contributor

I also switched to alpine for the act_runner base image, to slim it down. The final size dropped from 159MB to 38MB.

I also switched to alpine for the `act_runner` base image, to slim it down. The final size dropped from 159MB to 38MB.
telackey added 1 commit 2023-03-28 21:07:54 +00:00
All checks were successful
check and test
f5206b44d4
Typo
Author
Contributor

Incidentally, I'm open to leaving Dockerfile.task-executor out of this PR, as that exact image is not required, it is simply a working example.

Incidentally, I'm open to leaving `Dockerfile.task-executor` out of this PR, as that exact image is not required, it is simply a working example.
telackey added 1 commit 2023-03-28 21:26:12 +00:00
Owner

Incidentally, I'm open to leaving Dockerfile.task-executor out of this PR, as that exact image is not required, it is simply a working example.

Yes, at least not this PR.

> Incidentally, I'm open to leaving `Dockerfile.task-executor` out of this PR, as that exact image is not required, it is simply a working example. Yes, at least not this PR.
telackey added 1 commit 2023-03-29 01:07:03 +00:00
All checks were successful
check and test
0cf83294c7
Remove task executor Dockefile.
telackey added 1 commit 2023-03-29 01:07:59 +00:00
All checks were successful
check and test
3350b21606
Remove env.example
Author
Contributor

Incidentally, I'm open to leaving Dockerfile.task-executor out of this PR, as that exact image is not required, it is simply a working example.

Yes, at least not this PR.

Done.

> > Incidentally, I'm open to leaving `Dockerfile.task-executor` out of this PR, as that exact image is not required, it is simply a working example. > > Yes, at least not this PR. Done.
lunny reviewed 2023-03-29 01:45:01 +00:00
@ -33,3 +36,2 @@
}
if s.CacheHandler != nil {
env["ACTIONS_CACHE_URL"] = s.CacheHandler.ExternalURL() + "/"
env["ACTIONS_CACHE_URL"] = s.CacheHandler.ExternalURL() + "/"
Owner

Why this changed? s.CacheHandler maybe nil?

Why this changed? s.CacheHandler maybe nil?
telackey marked this conversation as resolved
telackey added 1 commit 2023-03-29 15:27:37 +00:00
All checks were successful
check and test
1bfe39fd2e
Fix merge error.
telackey added 1 commit 2023-03-29 15:29:32 +00:00
All checks were successful
check and test
fc45ca8f78
Merge branch 'main' into telackey/dind
Author
Contributor

For some reason my replies to the comment about ACTIONS_CACHE_URL are not being saved. The short answer is that was a good catch, as it was an inadvertent change, the result of a merge error when I rebased.

It is fixed now.

For some reason my replies to the comment about `ACTIONS_CACHE_URL` are not being saved. The short answer is that was a good catch, as it was an inadvertent change, the result of a merge error when I rebased. It is fixed now.
telackey added 1 commit 2023-03-29 15:37:38 +00:00
All checks were successful
check and test
b2d02a4a4d
Add comment
Author
Contributor

Are there any other changes or improvements you would like me to make to this PR?

Are there any other changes or improvements you would like me to make to this PR?
telackey requested review from lunny 2023-04-06 19:03:36 +00:00
telackey requested review from wolfogre 2023-04-06 19:04:15 +00:00
Owner

Please resolve the conflicts. I will have a test again after that.

Please resolve the conflicts. I will have a test again after that.
Owner

Sorry for causing conflicts in this PR. They mainly come from Refactor environment variables to configuration and registration, but it has been in my mind for a long time, see #21.

Sorry for causing conflicts in this PR. They mainly come from [Refactor environment variables to configuration and registration](https://gitea.com/gitea/act_runner/pulls/90), but it has been in my mind for a long time, see #21.
telackey force-pushed telackey/dind from b2d02a4a4d to c94db953fe 2023-04-11 02:29:55 +00:00 Compare
Author
Contributor

It was more like a port than a merge, but it seems to be working for me. The new code also simplified things a good deal.

It was more like a port than a merge, but it seems to be working for me. The new code also simplified things a good deal.
telackey added 1 commit 2023-04-11 02:47:57 +00:00
All checks were successful
checks / check and test (pull_request) Successful in 1m34s
8d9bc970d5
Show the default as false (which it is)
lunny approved these changes 2023-04-11 02:55:37 +00:00
wolfogre approved these changes 2023-04-11 02:57:51 +00:00
wolfogre left a comment
Owner

LGTM

LGTM
wolfogre merged commit 5a8134410d into main 2023-04-11 02:58:13 +00:00
telackey deleted branch telackey/dind 2023-04-11 17:27:21 +00:00
Author
Contributor

Great, thanks!

Great, thanks!
Contributor

Are there plans to publish this to a public registry, so it can be used in docker-compose setups?

Are there plans to publish this to a public registry, so it can be used in docker-compose setups?
Owner

I found there are some problems, I think we can release it to dockerhub or other places once it fixed.

I found there are some problems, I think we can release it to dockerhub or other places once it fixed.
Author
Contributor

Let me know if there is anything I need to do/fix.

Let me know if there is anything I need to do/fix.
Owner

Let me know if there is anything I need to do/fix.

The running container cannot be shutdown with ctrl + C

> Let me know if there is anything I need to do/fix. The running container cannot be shutdown with ctrl + C
Author
Contributor

Let me know if there is anything I need to do/fix.

The running container cannot be shutdown with ctrl + C

I'm not able to reproduce this, at least not at first glance:

❯ docker run -it cerc/act-runner:local
level=info msg="Registering runner, arch=amd64, os=linux, version=0.1.1."
level=error msg="Invalid input, please re-run act command." error="instance address is empty"
Waiting to retry ...
^C
❯ echo $?
130

One thing that might be worth trying is to explicitly trap Ctrl-C:

diff --git a/run.sh b/run.sh
index aa8f456..b2f8208 100755
--- a/run.sh
+++ b/run.sh
@@ -1,5 +1,12 @@
 #!/usr/bin/env bash

+trap ctrl_c INT
+
+function ctrl_c() {
+  echo "aborted" 1>&2
+  exit 129
+}
+
 if [[ ! -d /data ]]; then
   mkdir -p /data
 fi

After which I get:

❯ docker run -it cerc/act-runner:local
level=info msg="Registering runner, arch=amd64, os=linux, version=0.1.1."
level=error msg="Invalid input, please re-run act command." error="instance address is empty"
Waiting to retry ...
^Caborted
❯ echo $?
129
> > Let me know if there is anything I need to do/fix. > > The running container cannot be shutdown with ctrl + C I'm not able to reproduce this, at least not at first glance: ``` ❯ docker run -it cerc/act-runner:local level=info msg="Registering runner, arch=amd64, os=linux, version=0.1.1." level=error msg="Invalid input, please re-run act command." error="instance address is empty" Waiting to retry ... ^C ❯ echo $? 130 ``` One thing that might be worth trying is to explicitly trap Ctrl-C: ``` diff --git a/run.sh b/run.sh index aa8f456..b2f8208 100755 --- a/run.sh +++ b/run.sh @@ -1,5 +1,12 @@ #!/usr/bin/env bash +trap ctrl_c INT + +function ctrl_c() { + echo "aborted" 1>&2 + exit 129 +} + if [[ ! -d /data ]]; then mkdir -p /data fi ``` After which I get: ``` ❯ docker run -it cerc/act-runner:local level=info msg="Registering runner, arch=amd64, os=linux, version=0.1.1." level=error msg="Invalid input, please re-run act command." error="instance address is empty" Waiting to retry ... ^Caborted ❯ echo $? 129 ```
Contributor

Using podman with the docker compat layer, I can reproduce the described behavior when using docker run local/act_runner without the -it flag.

Trapping INT in run.sh did fix the problem for me.

Using podman with the docker compat layer, I can reproduce the described behavior when using `docker run local/act_runner` without the `-it` flag. Trapping `INT` in `run.sh` did fix the problem for me.
Sign in to join this conversation.
No description provided.