Asset server implementation #677

KnisterPeter · 2021-05-07T14:47:05Z

Description

This PR contains a naive github-actions (partially-)compatible asset server.

We only reverse engineered the success case.
The implementation is based on the awesome work of @anthonykawa: https://github.com/anthonykawa/artifact-server

This implementation should be seen as a draft and requires an architecture/design review.

Missing things

CLI flags for configuration (port, asset path)
Server start need to be integrated properly into the runner (we do start it for all commands currently)
Tests
Add docs

Prerequisites

--env ACTIONS_RUNTIME_URL=http://localhost:8080/ must be set because it's internally used by the @action/toolkit
--env ACTIONS_RUNTIME_TOKEN=foo must be set (we do not have authentication implemented)

~~This should ideally be implemented (either as port of this PR or #329), so users don't need to set it on their own.~~

These variables are set if act is started with --artifact-server-path and configured for the internal server.
It is also possible to define them as environment variables using --env for an external artifact-server.

Relates to #329

--

Closes #169

codecov · 2021-05-07T14:50:25Z

Codecov Report

Merging #677 (1a6f245) into master (0f04942) will increase coverage by 6.95%.
The diff coverage is 60.80%.

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
+ Coverage   49.27%   56.22%   +6.95%     
==========================================
  Files          23       25       +2     
  Lines        2401     3929    +1528     
==========================================
+ Hits         1183     2209    +1026     
- Misses       1090     1522     +432     
- Partials      128      198      +70

Impacted Files	Coverage Δ
pkg/common/outbound_ip.go	`0.00% <0.00%> (ø)`
pkg/container/docker_volume.go	`0.00% <0.00%> (ø)`
pkg/container/docker_run.go	`5.56% <15.09%> (+3.63%)`	⬆️
pkg/common/git.go	`49.82% <31.81%> (-9.97%)`	⬇️
pkg/model/planner.go	`49.78% <42.50%> (+16.70%)`	⬆️
pkg/container/docker_build.go	`60.81% <57.14%> (+60.81%)`	⬆️
pkg/container/docker_pull.go	`35.29% <61.53%> (+17.11%)`	⬆️
pkg/model/workflow.go	`52.14% <63.63%> (+26.42%)`	⬆️
pkg/artifacts/server.go	`67.32% <67.32%> (ø)`
pkg/runner/runner.go	`74.46% <70.37%> (-2.01%)`	⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a3a47c...1a6f245. Read the comment docs.

catthehacker · 2021-05-07T14:51:27Z

I wouldn't bother with cli flags for this. This is specifically for act usage only. Even better if it's internal network for containers only.

pkg/assets/server.go

cmd/root.go

KnisterPeter · 2021-05-07T15:02:02Z

Let me explain a bit on why we thing that configuration might help here (for our usecase):

This assets could be used for caching things across runs as well. In fact the github @actions/cache is also doing this (but unfortunatelly not availabel for GHE). That wouldn't work out of the box and is probably not even target with act.

We are using act to run github actions in a non-local environment. And we would like to use this feature (and extend on that outside of act) to build a whole caching layer.
To do that we need to provide different configuration (at least for the URL part).

An implicit overridable env-var would be fine for us.

catthehacker · 2021-05-07T15:02:09Z

Ok, I see server is running locally on host. That won't work if the Docker host is a remote server, for example ssh://

catthehacker · 2021-05-07T15:08:50Z

We are using act to run github actions in a non-local environment. And we would like to use this feature (and extend on that outside of act) to build a whole caching layer.
To do that we need to provide different configuration (at least for the URL part).

An implicit overridable env-var would be fine for us.

We should at least follow some standard of non-conflicting envvars, like ACT_*

KnisterPeter · 2021-05-07T15:09:56Z

Even better if it's internal network for containers only.

What do you mean with that? Do you suggest to make the server only available in the docker internal network for the run - for sharing across jobs?

One use case which is in my head would be to expose test result data or coverage information after an act execution.
If that's internal to the container users cannot do that.

catthehacker · 2021-05-07T16:18:08Z

IMO, server should be available only in containers created by act, artefacts can be accessible via specified path.

KnisterPeter · 2021-05-07T16:26:56Z

Hmm, I see your point. But I'm not sure how to implement that. Don't we need the server outside of the containers to write into the host file system?

catthehacker · 2021-05-07T16:41:09Z

Don't worry, proceed with your PR as you planned 😺 I have some idea but that requires much more changes in act

catthehacker · 2021-05-07T16:47:57Z

Missing things

CLI flags for configuration (port, asset path)

since server will be on host for now, please add port flag

--env ACTIONS_RUNTIME_URL=http://localhost:8080/ must be set because it's internally used by the @action/toolkit

--env ACTIONS_RUNTIME_TOKEN=foo must be set (we do not have authentication implemented)

Please add them to withGithubEnv()

act/pkg/runner/run_context.go

Line 652 in cc4e23d

func (rc *RunContext) withGithubEnv(env map[string]string) map[string]string {

KnisterPeter · 2021-05-10T19:22:44Z

@catthehacker @cplee Can you guide us where the proper place for these lines are?

https://github.com/nektos/act/pull/677/files#diff-ab967ab1a2f3a1b769106eeb7bfe892ef0e81d1d27811fa15be08e6749feee1fR69-R72

I think it might not be right in the middle of the CLI parsing.

catthehacker · 2021-05-10T20:22:45Z

Before WithDryRun perhaps?

act/cmd/root.go

Lines 261 to 271 in ef0da2a

    
           r, err := runner.New(config) 
        
           if err != nil { 
        
           	return err 
        
           } 
        
           ctx = common.WithDryrun(ctx, input.dryrun) 
        
           if watch, err := cmd.Flags().GetBool("watch"); err != nil { 
        
           	return err 
        
           } else if watch { 
        
           	return watchAndRun(ctx, r.NewPlanExecutor(plan)) 
        
           }

ChristopherHX · 2021-05-17T16:08:58Z

pkg/artifacts/server.go

+	router.PUT("/upload/:runId", func(w http.ResponseWriter, req *http.Request, params httprouter.Params) {
+		itemPath := req.URL.Query().Get("itemPath")
+		runID := params.ByName("runId")
+
+		filePath := fmt.Sprintf("%s/%s/%s", artifactPath, runID, itemPath)
+
+		err := os.MkdirAll(path.Dir(filePath), os.ModePerm)
+		if err != nil {
+			panic(err)
+		}
+
+		file, err := os.Create(filePath)
+		if err != nil {
+			panic(err)
+		}
+		defer file.Close()
+
+		_, err = io.Copy(file, req.Body)
+		if err != nil {
+			panic(err)
+		}
+
+		json, err := json.Marshal(ResponseMessage{
+			Message: "success",
+		})
+		if err != nil {
+			panic(err)
+		}
+
+		_, err = w.Write(json)
+		if err != nil {
+			panic(err)
+		}
+	})


Does this work for asset files > 8 MB?
I ask this because the actions/toolkit uploads big files in 8MB chunks with range upload.
Currently 2 chunks are uploaded parallel to the server by the toolkit.
I haven't tested this implementation, but the nodejs version also missed chunk upload

I guess it doesn't work. I think think the default go web server does this.
But I also think the support for chunked upload could be defered to a later PR. The API wouldn't change for that.
Splitting this into multiple PRs would add support for smaller chunks earlier/faster.

catthehacker · 2021-05-17T20:51:05Z

Could you also update README with added flags?
go run main.go --help and copy paste flags

KnisterPeter · 2021-05-17T21:58:51Z

@catthehacker Yes, good point. I'm currently writing tests. So this is still WIP.

KnisterPeter · 2021-05-17T22:40:02Z

@catthehacker I'm very inexperienced with go. I think the tests are fine now, can you do a review. Specially with the MemFS parts, I'm unsure if this is 'go-like'.

P.S.: This is still WIP. I need to re-test if the implementation still works.

crawler · 2021-06-01T10:19:46Z

Hi, @KnisterPeter
Thank you for doing the really useful feature. Once I tried this awesome project I immediately noticed that it missing cache and every run makes the dependencies downloaded every time. So despite I not common with GO I installed your's fork and tried different options with no luck. With the lsof command I see that the server is started, and I can connect to it using 'curl' but the getCacheEntry can't connect to it:

act -s GITHUB_TOKEN=$GITHUB_TOKEN -j test --artifact-server-path=cache

results to

::debug::getCacheEntry - Attempt 2 of 2 failed with error: connect ECONNREFUSED 127.0.0.1:34567

same thing with the ACTIONS_RUNTIME_URL

act -s GITHUB_TOKEN=$GITHUB_TOKEN -j test --artifact-server-path=cache --artifact-server-port=7676 --env ACTIONS_RUNTIME_URL=http://spitfire.lan:7676

results to

::debug::getCacheEntry - Attempt 2 of 2 failed with error: connect ECONNREFUSED 127.0.0.1:7676

spitfire.lan - is local domain name of the my workstation

Will appreciate any information)

KnisterPeter · 2021-06-01T10:23:53Z

@crawler

missing cache

First of all I have to say that this is not about the cache, but about artifacts created during the build. The github actions caching is only using parts of this and is not compatible.

Second to note, this is not a fully tested solution and it should be integrated when I'm ready with testing. I haven't had so much time lately.

crawler · 2021-06-01T10:34:36Z

@KnisterPeter Thank you for having time to clear this out for me.

catthehacker · 2021-06-01T11:06:50Z

@crawler Use https://github.com/nektos/act#skipping-steps with -r, --reuse | reuse action containers to maintain state

mergify · 2021-11-01T09:24:39Z

@KnisterPeter this pull request has failed checks 🛠

mergify · 2021-11-01T09:30:51Z

@KnisterPeter this pull request has failed checks 🛠

This change should allow to use the host ip in macos and windows. Since docker is running in an intermediate vm, localhost is not sufficient to have the artifacts in the host system.

To shutdown artifact server, we should not use the already canceled context but the parent context instead. Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

mergify · 2021-11-01T10:10:59Z

@KnisterPeter this pull request has failed checks 🛠

When the pipeline is done the asset server should be shut down gracefully. Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

KnisterPeter · 2021-11-01T10:48:57Z

@catthehacker @cplee We consider this ready for review and merge now.

We implemented the check for the outbound ip (for macos support)
We shutdown the server gracefully if act is done
We rebased and resolved all conflicts

catthehacker

Looks sound 😸

KnisterPeter · 2021-11-10T16:52:45Z

@catthehacker @cplee What need to be done to enable the merge? I wanted to tick the allow maintainers to edit box, but it isn't there.

catthehacker · 2021-11-10T17:02:40Z

I assume @cplee can merge it manually (since I don't have permissions to do so) or you could rebase on master and force push branch, but it would require approval again.

cplee · 2021-11-10T17:57:16Z

@KnisterPeter - the issue is the base branch xing:asset-server isnt allowing updates. I'll force merge though

jonodrew · 2021-12-23T13:46:07Z

I can't see this documented in the README for this project - is it ready to use?

KnisterPeter · 2021-12-23T13:50:22Z

It is documented as 'artifact server' in the readme.
And yes, it's ready to use.

debuggerpk · 2022-04-07T16:42:28Z

Thankyou for your great work team. Having said that, I am unable to run this. The command I am using is

act --secret-file .env.act --artifact-server-path=artifacts

The error I am getting is

 💬  ::debug::Artifact Url: http://10.5.0.2:34567/_apis/pipelines/workflows/1/artifacts?api-version=6.0-preview
| Create Artifact Container - Attempt 1 of 5 failed with error: connect ECONNREFUSED 10.5.0.2:34567
| Create Artifact Container - Attempt 2 of 5 failed with error: connect ECONNREFUSED 10.5.0.2:34567
| Create Artifact Container - Attempt 3 of 5 failed with error: connect ECONNREFUSED 10.5.0.2:34567

Am I missing something?

catthehacker · 2022-04-07T17:25:57Z

Please create new issue or ask on Gitter.

KnisterPeter requested a review from a team as a code owner May 7, 2021 14:47

pull-request-size bot added the size/L label May 7, 2021

catthehacker requested changes May 7, 2021

View reviewed changes

pkg/assets/server.go Outdated Show resolved Hide resolved

pkg/assets/server.go Outdated Show resolved Hide resolved

cmd/root.go Outdated Show resolved Hide resolved

mergify bot requested a review from a team May 7, 2021 15:01

This comment has been minimized.

Sign in to view

mergify bot added the needs-work Extra attention is needed label May 16, 2021

ChristopherHX reviewed May 17, 2021

View reviewed changes

mergify bot removed the needs-work Extra attention is needed label May 17, 2021

KnisterPeter force-pushed the asset-server branch from 31665e5 to 0ee3a45 Compare June 2, 2021 21:45

pull-request-size bot added size/XL and removed size/L labels Jun 2, 2021

KnisterPeter added 3 commits November 1, 2021 10:19

test: add test workflow to testdata

a56fa9b

feat: add log output

87f2a5e

refactor: log shutdown error instead of panic

53eb9f8

KnisterPeter dismissed a stale review via 53eb9f8 November 1, 2021 09:20

KnisterPeter force-pushed the asset-server branch from ce928d7 to 53eb9f8 Compare November 1, 2021 09:20

mergify bot removed the conflict PR has conflicts label Nov 1, 2021

KnisterPeter and others added 2 commits November 1, 2021 11:06

feat: use outbound ip for the asset server

d5e64c8

This change should allow to use the host ip in macos and windows. Since docker is running in an intermediate vm, localhost is not sufficient to have the artifacts in the host system.

fix: do not use canceled context

861695e

To shutdown artifact server, we should not use the already canceled context but the parent context instead. Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

KnisterPeter force-pushed the asset-server branch from 77ae60e to 861695e Compare November 1, 2021 10:06

KnisterPeter and others added 3 commits November 1, 2021 11:23

feat: shutdown artifact server at end of pipeline

a1547cc

When the pipeline is done the asset server should be shut down gracefully. Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

fix: close server if graceful shutdown failed

a39cff3

Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

fix: ignore server closed error from listen call

1a6f245

Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>

mergify bot removed the needs-work Extra attention is needed label Nov 1, 2021

catthehacker approved these changes Nov 9, 2021

View reviewed changes

mergify bot requested a review from a team November 9, 2021 08:48

cplee approved these changes Nov 9, 2021

View reviewed changes

cplee merged commit 11f6ee3 into nektos:master Nov 10, 2021

ZauberNerd deleted the asset-server branch November 22, 2021 14:33

nektos locked as resolved and limited conversation to collaborators Apr 7, 2022

Asset server implementation #677

Asset server implementation #677

Conversation

KnisterPeter commented May 7, 2021 • edited Loading

Description

Missing things

Prerequisites

codecov bot commented May 7, 2021 • edited Loading

Codecov Report

catthehacker commented May 7, 2021

KnisterPeter commented May 7, 2021

catthehacker commented May 7, 2021

catthehacker commented May 7, 2021

KnisterPeter commented May 7, 2021

catthehacker commented May 7, 2021

KnisterPeter commented May 7, 2021

catthehacker commented May 7, 2021

catthehacker commented May 7, 2021 • edited Loading

Missing things

KnisterPeter commented May 10, 2021

catthehacker commented May 10, 2021

This comment has been minimized.

ChristopherHX May 17, 2021

Choose a reason for hiding this comment

KnisterPeter May 17, 2021

Choose a reason for hiding this comment

catthehacker commented May 17, 2021

KnisterPeter commented May 17, 2021

KnisterPeter commented May 17, 2021

crawler commented Jun 1, 2021

KnisterPeter commented Jun 1, 2021

crawler commented Jun 1, 2021

catthehacker commented Jun 1, 2021

mergify bot commented Nov 1, 2021

mergify bot commented Nov 1, 2021

mergify bot commented Nov 1, 2021

KnisterPeter commented Nov 1, 2021

catthehacker left a comment

Choose a reason for hiding this comment

KnisterPeter commented Nov 10, 2021

catthehacker commented Nov 10, 2021

cplee commented Nov 10, 2021

jonodrew commented Dec 23, 2021

KnisterPeter commented Dec 23, 2021

debuggerpk commented Apr 7, 2022

catthehacker commented Apr 7, 2022

KnisterPeter commented May 7, 2021 •

edited

Loading

codecov bot commented May 7, 2021 •

edited

Loading

catthehacker commented May 7, 2021 •

edited

Loading