Add support for service containers by GuessWhoSamFoo · Pull Request #1949 · nektos/act

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>

devnoname120 · 2024-11-01T11:44:48Z

PR to update the documentation accordingly: nektos/act-docs#30

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>

devnoname120 mentioned this pull request Nov 1, 2024

Remove mention of services not being supported nektos/act-docs#30

Merged

Uh oh!

Conversation

GuessWhoSamFoo commented Aug 7, 2023

Uh oh!

mergify bot commented Aug 7, 2023

Uh oh!

mergify bot commented Aug 7, 2023

Uh oh!

Uh oh!

mergify bot commented Aug 7, 2023

Uh oh!

GuessWhoSamFoo commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wolfogre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherHX commented Aug 7, 2023

Uh oh!

ChristopherHX left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristopherHX commented Aug 7, 2023

Uh oh!

GuessWhoSamFoo commented Aug 7, 2023

Uh oh!

ChristopherHX left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuessWhoSamFoo commented Aug 13, 2023

Uh oh!

ChristopherHX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devnoname120 commented Nov 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

GuessWhoSamFoo commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 7, 2023 •

edited

Loading

wolfogre left a comment •

edited

Loading

ChristopherHX left a comment •

edited

Loading

ChristopherHX left a comment •

edited

Loading