Add support for service containers #1949

GuessWhoSamFoo · 2023-08-07T02:04:44Z

Closes #173

This PR cherry picks:

https://gitea.com/gitea/act/pulls/50 is not moved over as it does not exist as a Github Action feature.

Gitea specific features such as createSimpleContainerName and the AutoRemove flag was removed during this process. In addition, ContainerMaxLifetime is also removed as since running /bin/sleep 0 by default can be confusing and doesn't seem to have a use case outside of usage as an sdk.

mergify · 2023-08-07T02:07:30Z

@GuessWhoSamFoo this pull request has failed checks 🛠

mergify · 2023-08-07T03:14:22Z

@GuessWhoSamFoo this pull request has failed checks 🛠

pkg/runner/runner.go

mergify · 2023-08-07T04:14:26Z

@GuessWhoSamFoo this pull request has failed checks 🛠

GuessWhoSamFoo · 2023-08-07T04:37:16Z

Not sure what is up with the artifact upload/download test. Maybe there's a side effect somewhere, gonna have to investigate later

[Test that artifact uploads and downloads succeed/test-artifacts]   | Create Artifact Container - Attempt 5 of 5 failed with error: connect ECONNREFUSED 127.0.0.1:12345
[Test that artifact uploads and downloads succeed/test-artifacts]   ❗  ::error::Create Artifact Container failed: connect ECONNREFUSED 127.0.0.1:12345

EDIT: Realized it's because ContainerNetworkMode needs to be host otherwise it can't reach the test server

codecov · 2023-08-07T05:06:43Z

Codecov Report

Merging #1949 (93089ed) into master (4989f44) will increase coverage by 0.23%.
Report is 254 commits behind head on master.
The diff coverage is 59.71%.

@@            Coverage Diff             @@
##           master    #1949      +/-   ##
==========================================
+ Coverage   61.22%   61.45%   +0.23%     
==========================================
  Files          46       53       +7     
  Lines        7141     8774    +1633     
==========================================
+ Hits         4372     5392    +1020     
- Misses       2462     2953     +491     
- Partials      307      429     +122

Files	Coverage Δ
pkg/common/executor.go	`51.69% <100.00%> (+1.69%)`	⬆️
pkg/container/docker_cli.go	`82.23% <ø> (ø)`
pkg/container/docker_logger.go	`52.08% <ø> (ø)`
pkg/runner/step_action_local.go	`93.54% <100.00%> (ø)`
pkg/runner/step_action_remote.go	`91.56% <100.00%> (+0.65%)`	⬆️
pkg/runner/step_docker.go	`93.18% <100.00%> (ø)`
pkg/container/file_collector.go	`37.30% <0.00%> (ø)`
pkg/container/util.go	`0.00% <0.00%> (ø)`
pkg/container/docker_build.go	`60.00% <80.00%> (+1.02%)`	⬆️
...ontainer/linux_container_environment_extensions.go	`23.07% <0.00%> (-1.25%)`	⬇️
... and 31 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

wolfogre

LGTM, and I believe more practical tests are needed.

Some notes for other reviewers:

Services always run in containers, so docker will be required to run services (even run jobs in host mode).
It will create a new docker network for service containers, and let the job container join the network. And delete the network when it has done.
IIRC, there will be some problems to conntect to serivces when runing jobs in host mode without job containers. Since there's no job container, it's impossible to let the job container join the network.
A new docker network make it possible to let services bind any ports without conflict, however, it also make things more complex. (That's why the team of Gitea are cautious to port it upstream)
IIRC, act will keep the job container to reuse (while gitea/act never reuse), I'm not sure if the network could be used and deleted correctly.
Docker can only provide a limited number of networks, so the network must be deleted timely.

ChristopherHX · 2023-08-07T07:28:49Z

Please create stubs for NewDockerNetworkCreateExecutor, NewDockerNetworkRemoveExecutor returning an empty noop executor.

In this file: https://github.com/nektos/act/blob/master/pkg/container/docker_stub.go

I rely on extended platform support, where docker libraries does not compile. (see build tags)

Seems like CI tests should be extended to test this

ChristopherHX

This has been a static code review from my side, I haven't tested anything locally.

IIRC, there will be some problems to conntect to serivces when runing jobs in host mode without job containers. Since there's no job container, it's impossible to let the job container join the network.

Yeah that's why GitHub advise to use a job container, the ports section is used to expose ports in that case and also allows assigning an random port.

pkg/runner/job_executor.go

pkg/runner/runner.go

pkg/container/container_types.go

pkg/container/docker_network.go

pkg/runner/run_context.go

ChristopherHX · 2023-08-07T19:12:54Z

Actually it makes sense to just merge this change, otherwise act may never has this feature.
I defer my appoval to a later date in August, unless these points are addressed

GuessWhoSamFoo · 2023-08-07T22:18:03Z

@ChristopherHX Thanks for the review - I'll take a closer look if something got missed in the backport and likely want to explicitly test https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idservicesservice_idvolumes as well

ChristopherHX

I found a file descriptor leak, this have to be fixed in my opinion.
I opened a gitea/act issue about this: https://gitea.com/gitea/act/issues/76
If you don't use act with very large workflows or in watch mode for a long time you won't notice the consequences of this code.
Yes act had this problem on more places, before I fixed them all. In 2021 the fd leak per job was very large, but due to high limits you didn't see any errors in short term.

pkg/container/docker_network.go

pkg/runner/run_context.go

GuessWhoSamFoo · 2023-08-13T00:20:38Z

@wolfogre @ChristopherHX Still missing some tests, but wanted to run by the feedback so far. The host network will be used by default unless a service container is defined or passed through the flag. This can mitigate some concerns around breaking existing users and orphaned networks eventually reaching limits.

I have also thought about creating a network name by convention and re-using instead. Higher chance of port conflicts but less complexity

ChristopherHX

All good now for me

Does gitea/act provide contextdata for services? If not it's also ok for me

pkg/runner/run_context.go

Removed createSimpleContainerName and AutoRemove flag Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Jason Song <i@wolfogre.com> Reviewed-on: https://gitea.com/gitea/act/pulls/42 Reviewed-by: Jason Song <i@wolfogre.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com>

Reviewed-on: https://gitea.com/gitea/act/pulls/45 Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com>

Inside container environment we're not running a second service process, but use service container for act instead. See: nektos/act#1949 Signed-off-by: You-Sheng Yang <vicamo@gmail.com>

* Support services (nektos#42) Removed createSimpleContainerName and AutoRemove flag Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Jason Song <i@wolfogre.com> Reviewed-on: https://gitea.com/gitea/act/pulls/42 Reviewed-by: Jason Song <i@wolfogre.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com> * Support services options (nektos#45) Reviewed-on: https://gitea.com/gitea/act/pulls/45 Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com> * Support intepolation for `env` of `services` (nektos#47) Reviewed-on: https://gitea.com/gitea/act/pulls/47 Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com> * Support services `credentials` (nektos#51) If a service's image is from a container registry requires authentication, `act_runner` will need `credentials` to pull the image, see [documentation](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idservicesservice_idcredentials). Currently, `act_runner` incorrectly uses the `credentials` of `containers` to pull services' images and the `credentials` of services won't be used, see the related code: https://gitea.com/gitea/act/src/commit/0c1f2edb996a87ee17dcf3cfa7259c04be02abd7/pkg/runner/run_context.go#L228-L269 Co-authored-by: Jason Song <i@wolfogre.com> Reviewed-on: https://gitea.com/gitea/act/pulls/51 Reviewed-by: Jason Song <i@wolfogre.com> Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com> * Add ContainerMaxLifetime and ContainerNetworkMode options from: https://gitea.com/gitea/act/commit/b9c20dcaa43899cb3bb327619d447248303170e0 * Fix container network issue (nektos#56) Follow: https://gitea.com/gitea/act_runner/pulls/184 Close https://gitea.com/gitea/act_runner/issues/177 - `act` create new networks only if the value of `NeedCreateNetwork` is true, and remove these networks at last. `NeedCreateNetwork` is passed by `act_runner`. 'NeedCreateNetwork' is true only if `container.network` in the configuration file of the `act_runner` is empty. - In the `docker create` phase, specify the network to which containers will connect. Because, if not specify , container will connect to `bridge` network which is created automatically by Docker. - If the network is user defined network ( the value of `container.network` is empty or `<custom-network>`. Because, the network created by `act` is also user defined network.), will also specify alias by `--network-alias`. The alias of service is `<service-id>`. So we can be access service container by `<service-id>:<port>` in the steps of job. - Won't try to `docker network connect ` network after `docker start` any more. - Because on the one hand, `docker network connect` applies only to user defined networks, if try to `docker network connect host <container-name>` will return error. - On the other hand, we just specify network in the stage of `docker create`, the same effect can be achieved. - Won't try to remove containers and networks berfore the stage of `docker start`, because the name of these containers and netwoks won't be repeat. Co-authored-by: Jason Song <i@wolfogre.com> Reviewed-on: https://gitea.com/gitea/act/pulls/56 Reviewed-by: Jason Song <i@wolfogre.com> Co-authored-by: sillyguodong <gedong_1994@163.com> Co-committed-by: sillyguodong <gedong_1994@163.com> * Check volumes (nektos#60) This PR adds a `ValidVolumes` config. Users can specify the volumes (including bind mounts) that can be mounted to containers by this config. Options related to volumes: - [jobs.<job_id>.container.volumes](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontainervolumes) - [jobs.<job_id>.services.<service_id>.volumes](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idservicesservice_idvolumes) In addition, volumes specified by `options` will also be checked. Currently, the following default volumes (see https://gitea.com/gitea/act/src/commit/a72822b3f83d3e68ffc697101b713b7badf57e2f/pkg/runner/run_context.go#L116-L166) will be added to `ValidVolumes`: - `act-toolcache` - `<container-name>` and `<container-name>-env` - `/var/run/docker.sock` (We need to add a new configuration to control whether the docker daemon can be mounted) Co-authored-by: Jason Song <i@wolfogre.com> Reviewed-on: https://gitea.com/gitea/act/pulls/60 Reviewed-by: Jason Song <i@wolfogre.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com> * Remove ContainerMaxLifetime; fix lint * Remove unused ValidVolumes * Remove ConnectToNetwork * Add docker stubs * Close docker clients to prevent file descriptor leaks * Fix the error when removing network in self-hosted mode (nektos#69) Fixes https://gitea.com/gitea/act_runner/issues/255 Reviewed-on: https://gitea.com/gitea/act/pulls/69 Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com> * Move service container and network cleanup to rc.cleanUpJobContainer * Add --network flag; default to host if not using service containers or set explicitly * Correctly close executor to prevent fd leak * Revert to tail instead of full path * fix network duplication * backport networkingConfig for aliaes * don't hardcode netMode host * Convert services test to table driven tests * Add failing tests for services * Expose service container ports onto the host * Set container network mode in artifacts server test to host mode * Log container network mode when creating/starting a container * fix: Correctly handle ContainerNetworkMode * fix: missing service container network * Always remove service containers Although we usually keep containers running if the workflow errored (unless `--rm` is given) in order to facilitate debugging and we have a flag (`--reuse`) to always keep containers running in order to speed up repeated `act` invocations, I believe that these should only apply to job containers and not service containers, because changing the network settings on a service container requires re-creating it anyway. * Remove networks only if no active endpoints exist * Ensure job containers are stopped before starting a new job * fix: go build -tags WITHOUT_DOCKER --------- Co-authored-by: Zettat123 <zettat123@gmail.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Jason Song <i@wolfogre.com> Co-authored-by: sillyguodong <gedong_1994@163.com> Co-authored-by: ChristopherHX <christopher.homberger@web.de> Co-authored-by: ZauberNerd <zaubernerd@zaubernerd.de>

Inside container environment we're not running a second service process, but use service container for act instead. See: nektos/act#1949 Signed-off-by: You-Sheng Yang <vicamo@gmail.com>

GuessWhoSamFoo requested a review from a team as a code owner August 7, 2023 02:04

pull-request-size bot added the size/L label Aug 7, 2023

GuessWhoSamFoo changed the title ~~Service container~~ Add support for service containers Aug 7, 2023

mergify bot added the needs-work Extra attention is needed label Aug 7, 2023

GuessWhoSamFoo force-pushed the service-container branch from 00f0139 to b858cd2 Compare August 7, 2023 02:54

GuessWhoSamFoo force-pushed the service-container branch from b858cd2 to a4792b3 Compare August 7, 2023 03:58

wolfogre reviewed Aug 7, 2023

View reviewed changes

pkg/runner/runner.go Outdated Show resolved Hide resolved

GuessWhoSamFoo force-pushed the service-container branch from a4792b3 to b74a929 Compare August 7, 2023 04:34

GuessWhoSamFoo force-pushed the service-container branch from b74a929 to de466a6 Compare August 7, 2023 04:50

mergify bot removed the needs-work Extra attention is needed label Aug 7, 2023

mergify bot requested a review from a team August 7, 2023 05:06

wolfogre previously approved these changes Aug 7, 2023

View reviewed changes

ChristopherHX reviewed Aug 7, 2023

View reviewed changes

ChristopherHX requested changes Aug 9, 2023

View reviewed changes

pkg/container/docker_network.go Show resolved Hide resolved

pkg/container/docker_network.go Show resolved Hide resolved

ChristopherHX reviewed Aug 9, 2023

View reviewed changes

pkg/runner/run_context.go Outdated Show resolved Hide resolved

GuessWhoSamFoo dismissed wolfogre’s stale review via 7a29ed9 August 12, 2023 23:41

GuessWhoSamFoo force-pushed the service-container branch from de466a6 to 7a29ed9 Compare August 12, 2023 23:41

mergify bot requested a review from a team August 12, 2023 23:56

ChristopherHX previously approved these changes Aug 13, 2023

View reviewed changes

pkg/runner/run_context.go Outdated Show resolved Hide resolved

Zettat123 and others added 2 commits August 14, 2023 11:45

Support services options (nektos#45)

5cc4bd0

Reviewed-on: https://gitea.com/gitea/act/pulls/45 Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Zettat123 <zettat123@gmail.com> Co-committed-by: Zettat123 <zettat123@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for service containers #1949

Add support for service containers #1949

GuessWhoSamFoo commented Aug 7, 2023

mergify bot commented Aug 7, 2023

mergify bot commented Aug 7, 2023

mergify bot commented Aug 7, 2023

GuessWhoSamFoo commented Aug 7, 2023 •

edited

codecov bot commented Aug 7, 2023 •

edited

wolfogre left a comment •

edited

ChristopherHX commented Aug 7, 2023

ChristopherHX left a comment •

edited

ChristopherHX commented Aug 7, 2023

GuessWhoSamFoo commented Aug 7, 2023

ChristopherHX left a comment •

edited

GuessWhoSamFoo commented Aug 13, 2023

ChristopherHX left a comment

Add support for service containers #1949

Add support for service containers #1949

Conversation

GuessWhoSamFoo commented Aug 7, 2023

mergify bot commented Aug 7, 2023

mergify bot commented Aug 7, 2023

mergify bot commented Aug 7, 2023

GuessWhoSamFoo commented Aug 7, 2023 • edited

codecov bot commented Aug 7, 2023 • edited

Codecov Report

wolfogre left a comment • edited

Choose a reason for hiding this comment

ChristopherHX commented Aug 7, 2023

ChristopherHX left a comment • edited

Choose a reason for hiding this comment

ChristopherHX commented Aug 7, 2023

GuessWhoSamFoo commented Aug 7, 2023

ChristopherHX left a comment • edited

Choose a reason for hiding this comment

GuessWhoSamFoo commented Aug 13, 2023

ChristopherHX left a comment

Choose a reason for hiding this comment

GuessWhoSamFoo commented Aug 7, 2023 •

edited

codecov bot commented Aug 7, 2023 •

edited

wolfogre left a comment •

edited

ChristopherHX left a comment •

edited

ChristopherHX left a comment •

edited