Docker support for pipeline runs #201

michelvocks · 2019-08-01T11:18:55Z

No description provided.

server/server.go

workers/server/worker.go

codecov-io · 2019-09-25T08:31:40Z

Codecov Report

Merging #201 into master will decrease coverage by 0.9%.
The diff coverage is 48.61%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage      62%   61.09%   -0.91%     
==========================================
  Files          49       49              
  Lines        4066     4259     +193     
==========================================
+ Hits         2521     2602      +81     
- Misses       1132     1214      +82     
- Partials      413      443      +30

Impacted Files	Coverage Δ
store/store.go	`74.28% <ø> (ø)`	⬆️
helper/pipelinehelper/pipelinehelper.go	`100% <ø> (ø)`	⬆️
store/pipeline.go	`62.02% <0%> (-5.1%)`	⬇️
workers/scheduler/scheduler.go	`60.45% <10%> (-4.99%)`	⬇️
workers/pipeline/update_pipeline.go	`52.54% <100%> (ø)`	⬆️
workers/pipeline/build_cpp.go	`95.12% <100%> (ø)`	⬆️
workers/pipeline/build_golang.go	`94.64% <100%> (ø)`	⬆️
workers/pipeline/pipeline.go	`94.49% <100%> (+2.36%)`	⬆️
workers/pipeline/build_nodejs.go	`100% <100%> (ø)`	⬆️
workers/pipeline/build_java.go	`89.13% <100%> (ø)`	⬆️
... and 13 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 13f272f...424cb98. Read the comment docs.

michelvocks · 2019-10-07T06:35:34Z

@Skarlso This is ready for review. I still have to work on the documentation part which might be helpful for the review 😄

helper/pipelinehelper/pipelinehelper.go

Skarlso · 2019-10-07T06:38:02Z

Great job! I'll review it. 😊

server/server.go

Skarlso · 2019-10-07T08:49:50Z

store/memdb/memdb.go

+		item := iter.Next()
+
+		if item == nil {
+			break
+		}


Why not put this check into the for?

for item := iter.Next(); item != nil {

Good catch! I will change that! 😄

That actually doesn't work. item := iter.Next() is only executed once before loop iteration.

true I means to say item := node; item != nil; item = iter.Next.

Skarlso · 2019-10-08T08:41:43Z

Sorry, only picking through this slowly. But I'll get there. :)

Skarlso · 2019-10-08T20:49:50Z

workers/scheduler/scheduler.go

+			if err := s.memDBService.InsertDockerWorker(worker); err != nil {
+				gaia.Cfg.Logger.Error("failed to cache docker worker in memdb", "error", err)
+				if err := worker.KillDockerWorker(); err != nil {
+					gaia.Cfg.Logger.Error("failed to kill docker worker", "error", err)


I feel like simply logging this fact is weird. At the least we should wrap it and pass it up, don't you think? Or provide some context, I don't know. If we just log it, I feel like we simply don't care and might as well not log it. Especially since Gaia is very verbose now and this might be just missed. I might entirely miss this, unless I grep for it. And even then I might just not care since this already errorred out because of a different reason I'm trying to solve / fix which will probably fix the docker worker as well.

At the least we should wrap it and pass it up, don't you think?

Can you provide an example? If you mean with "pass it up" returning the error then this is not possible. We are in the schedule function which is periodically triggered and doesn't allow a return parameter.

The reason why I added this output is only for debugging purposes. What would you expect to happen here?

Yeah the scheduler never returns an error. I guess it's okay, it's already in a failing statement.

michelvocks · 2019-10-09T08:47:01Z

Related Docs PR: gaia-pipeline/docs#14

Skarlso · 2019-10-10T04:32:28Z

workers/docker/docker.go

+		gaia.Cfg.Logger.Info("try to pull docker image and then try it again...")
+
+		// Pull worker image
+		_, err = cli.ImagePull(ctx, workerImage, types.ImagePullOptions{})


It's worth mentioning that right now this only supports public docker registry and no private registries. I guess we can add that functionality later on?

Good catch! Yes, exactly. I wouldn't prioritize this yet since we can easily add this at a later point in time.

Skarlso · 2019-10-11T07:07:40Z

workers/docker/docker.go

+
+// KillDockerWorker disconnects the existing docker worker and
+// kills the docker container.
+func (w *Worker) KillDockerWorker() error {


Why not start the container with --rm? I'm sure the api can do that?

As far as I know --rm does not work with detached containers (e.g. -d). Additionally, I have to stop the container anyway so that the container would be removed by the --rm.

Skarlso · 2019-10-11T07:32:30Z

@michelvocks So I wanted to test this now, and I found two bugs...

First, when I created the pipeline and enabled the docker thing on the pipeline creation ui, it wasn't enabled for the pipeline. The indicator on the pipeline view said disabled and in fact it did not run in a container.

Once I enabled this though... It failed to run unfortunately by containously spewing this:

2019-10-11T09:29:30.006+0200 [INFO ] Gaia: try to pull docker image and then try it again...
2019-10-11T09:29:31.864+0200 [ERROR] Gaia: failed to create worker container after pull: error="Error response from daemon: No such image: gaiapipeline/gaia:latest"
2019-10-11T09:29:31.864+0200 [ERROR] Gaia: failed to setup docker worker for pipeline run: error="Error response from daemon: No such image: gaiapipeline/gaia:latest"
2019-10-11T09:29:33.007+0200 [ERROR] Gaia: failed to create worker container: error="Error response from daemon: No such image: gaiapipeline/gaia:latest"
2019-10-11T09:29:33.007+0200 [INFO ] Gaia: try to pull docker image and then try it again...
2019-10-11T09:29:35.262+0200 [ERROR] Gaia: failed to create worker container after pull: error="Error response from daemon: No such image: gaiapipeline/gaia:latest"
2019-10-11T09:29:35.262+0200 [ERROR] Gaia: failed to setup docker worker for pipeline run: error="Error response from daemon: No such image: gaiapipeline/gaia:latest"

Also, this made me realise that there is no timeout on this thing. :D This can keep failing and retrying indefinitely.

Not sure what's wrong because gaiapipeline/gaia:latest is an existing image. But it failed to pull it. I think it didn't block and wait for pulling the image?

michelvocks · 2019-10-11T08:16:47Z

First, when I created the pipeline and enabled the docker thing on the pipeline creation ui, it wasn't enabled for the pipeline. The indicator on the pipeline view said disabled and in fact it did not run in a container.

Let me try to reproduce this.

Once I enabled this though... It failed to run unfortunately by containously spewing this:
Not sure what's wrong because gaiapipeline/gaia:latest is an existing image. But it failed to pull it. I think it didn't block and wait for pulling the image?

It should wait for the pull to finish but I will test that again. However, please keep in mind that we didn't release a new version (and also not a new docker image). That means the current Gaia version in the latest image has not the worker stuff included and will therefore not work. I manually build a image from master-branch and used that for testing.

Also, this made me realise that there is no timeout on this thing. :D This can keep failing and retrying indefinitely.

Right. I thought about that too and I personally think that we maybe don't want a time out? e.g. try it as long as possible strategy.

Skarlso · 2019-10-11T08:20:12Z

The problem with as long as possible is that this continued on until I killed Gaia. Which means the pipeline is blocked until a Gaia restart occurs. I don't think that's a good idea.

True, the image wouldn't have the new worker stuff, but that's not the error. :) The error is that the image is not found. Which is weird because the image IS there. Did you try pushing your image up somewhere and deleting it locally and trying to run it like that?

michelvocks · 2019-12-02T08:02:46Z

@Skarlso

First, when I created the pipeline and enabled the docker thing on the pipeline creation ui, it wasn't enabled for the pipeline. The indicator on the pipeline view said disabled and in fact it did not run in a container.

I tried to reproduce this but it somehow works for me. Can you just give it another try? Maybe you clicked in the create pipeline UI on the cancel button? 😄

True, the image wouldn't have the new worker stuff, but that's not the error. :) The error is that the image is not found. Which is weird because the image IS there. Did you try pushing your image up somewhere and deleting it locally and trying to run it like that?

There was a bug in the image pull code. Should now work 👍

The problem with as long as possible is that this continued on until I killed Gaia. Which means the pipeline is blocked until a Gaia restart occurs. I don't think that's a good idea.

I'm wondering if this is actually a problem in general and should be addressed in a different PR. I mean this can also happen for normal pipeline runs, right? I think we should add a "context" to a pipeline run with a timeout which makes sure after a certain time a pipeline will be canceled?

Skarlso · 2019-12-03T08:27:18Z

yo. :) I'll re-try as soon as possible! :)

Skarlso · 2019-12-03T08:28:33Z

I'm wondering if this is actually a problem in general and should be addressed in a different PR. I mean this can also happen for normal pipeline runs, right? I think we should add a "context" to a pipeline run with a timeout which makes sure after a certain time a pipeline will be canceled?

Agreed!

workers/server/server_test.go

Skarlso · 2019-12-07T14:10:24Z

going to try this now on a fresh laptop. :)

Skarlso · 2019-12-07T14:19:21Z

eh, couldn't build the image...

E: Version '8u181-b13-2~deb9u1' for 'openjdk-8-jdk' was not found
E: Version '20170531+nmu1' for 'ca-certificates-java' was not found

Skarlso · 2019-12-07T14:27:12Z

These new versions worked:

ENV JAVA_VERSION 8u232
ENV JAVA_DEBIAN_VERSION 8u232-b09-1~deb9u1

# see https://bugs.debian.org/775775
# and https://github.com/docker-library/java/issues/19#issuecomment-70546872
ENV CA_CERTIFICATES_JAVA_VERSION 20170929~deb9u3

How do you build your image?

Skarlso · 2019-12-07T14:28:05Z

Okay, obviously you can't just build this thing. :D

COPY failed: stat /var/lib/docker/tmp/docker-builder354759785/docker/settings-docker.xml: no such file or directory

Ah I see. You expected it to be built from the main folder.

Skarlso · 2019-12-07T15:05:20Z

Oh buuu

Step 46/51 : COPY gaia-linux-amd64 /app
COPY failed: stat /var/lib/docker/tmp/docker-builder648944948/gaia-linux-amd64: no such file or directory

michelvocks · 2019-12-10T09:12:22Z

It feels like every 2-3 month the newest Java version is already deprecated again and it is not possible to build the docker image again. I think we just have to regularly bump the version.

Usually, the docker image build process is started by goreleaser that is why it is expected to be in the root folder.

Skarlso · 2020-01-28T06:04:54Z

@michelvocks I had a play with this last night and I feel like it's doing what it's supposed to do. :D The building is clunky, true. :/ I have yet to find a good solution for that. We can create a ticket for that maybe and track it?

So, LGTM! :)

Skarlso · 2020-02-14T06:08:32Z

@michelvocks How about we ditch the pinning to a specific Java 8 open jdk version? I don't see any problems with trying to use whatever latest stable is in there, do you?

So this, works perfectly fine:

	apt-get install -y --no-install-recommends \
		openjdk-8-jdk \
		ca-certificates-java \

\o/

Successfully built 8f528061c799
Successfully tagged gaiapipeline/gaia:latest

michelvocks · 2020-02-14T09:15:04Z

@Skarlso Sounds good. I will do that in a separate PR.

michelvocks added the Needs Work The PR still requires some work from the submitter. label Aug 1, 2019

michelvocks force-pushed the docker_pipeline_run branch from 0ab2143 to 195b8f8 Compare September 5, 2019 11:27

golangcibot reviewed Sep 5, 2019

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

golangcibot reviewed Sep 9, 2019

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

golangcibot reviewed Sep 9, 2019

View reviewed changes

workers/server/worker.go Outdated Show resolved Hide resolved

michelvocks marked this pull request as ready for review October 7, 2019 06:29

michelvocks requested a review from Skarlso October 7, 2019 06:29

michelvocks added needs review Ready To Merge PR is ready to be merged into master and removed Needs Work The PR still requires some work from the submitter. labels Oct 7, 2019

Skarlso reviewed Oct 7, 2019

View reviewed changes

helper/pipelinehelper/pipelinehelper.go Show resolved Hide resolved

Skarlso reviewed Oct 7, 2019

View reviewed changes

Skarlso reviewed Oct 8, 2019

View reviewed changes

michelvocks mentioned this pull request Oct 9, 2019

Docker support pipeline runs gaia-pipeline/docs#14

Merged

Skarlso reviewed Oct 10, 2019

View reviewed changes

Skarlso reviewed Oct 11, 2019

View reviewed changes

michelvocks added 6 commits December 2, 2019 07:50

First implementation

18c3897

More work done on the worker docker run implementation part

516c1a9

Docker pipeline runs should now also work on remote worker

6fc8006

More work on gaia worker docker pipeline execution

94bd0a4

Almost working

8cce08a

Almost working

74f3bea

michelvocks added 6 commits December 2, 2019 07:55

Added a few tests

f887183

Implemented frontend docker part

e556f02

Fixed tests

95f1e78

Fixed other test

6fe751e

Added more unit tests

fda57e9

Fix image pull bug

2d0b508

michelvocks force-pushed the docker_pipeline_run branch from 1889c8c to 2d0b508 Compare December 2, 2019 07:56

Optimized global loop. Added tests

7f33dab

Fix agent tests

f1aafc5

golangcibot reviewed Dec 3, 2019

View reviewed changes

workers/server/server_test.go Show resolved Hide resolved

Fix sub goroutine test failure

424cb98

michelvocks mentioned this pull request Dec 10, 2019

Release Schedule? #231

Closed

Skarlso approved these changes Jan 28, 2020

View reviewed changes

michelvocks merged commit b2dfb95 into gaia-pipeline:master Feb 14, 2020

michelvocks deleted the docker_pipeline_run branch February 14, 2020 09:15

This was referenced Feb 14, 2020

[WIP] docker support #114

Closed

Support pipeline execution in docker container #92

Closed

Job outputs? #225

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker support for pipeline runs #201

Docker support for pipeline runs #201

michelvocks commented Aug 1, 2019

codecov-io commented Sep 25, 2019 •

edited

Loading

michelvocks commented Oct 7, 2019

Skarlso commented Oct 7, 2019

Skarlso Oct 7, 2019

michelvocks Oct 8, 2019

michelvocks Dec 2, 2019

Skarlso Dec 7, 2019 •

edited

Loading

Skarlso commented Oct 8, 2019

Skarlso Oct 8, 2019 •

edited

Loading

michelvocks Oct 9, 2019

Skarlso Oct 9, 2019

michelvocks commented Oct 9, 2019

Skarlso Oct 10, 2019

michelvocks Oct 10, 2019

Skarlso Oct 11, 2019

michelvocks Oct 11, 2019

Skarlso commented Oct 11, 2019 •

edited

Loading

michelvocks commented Oct 11, 2019 •

edited

Loading

Skarlso commented Oct 11, 2019

michelvocks commented Dec 2, 2019 •

edited

Loading

Skarlso commented Dec 3, 2019

Skarlso commented Dec 3, 2019

Skarlso commented Dec 7, 2019

Skarlso commented Dec 7, 2019

Skarlso commented Dec 7, 2019

Skarlso commented Dec 7, 2019 •

edited

Loading

Skarlso commented Dec 7, 2019

michelvocks commented Dec 10, 2019

Skarlso commented Jan 28, 2020

Skarlso commented Feb 14, 2020 •

edited

Loading

michelvocks commented Feb 14, 2020

Docker support for pipeline runs #201

Docker support for pipeline runs #201

Conversation

michelvocks commented Aug 1, 2019

codecov-io commented Sep 25, 2019 • edited Loading

Codecov Report

michelvocks commented Oct 7, 2019

Skarlso commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skarlso Dec 7, 2019 • edited Loading

Choose a reason for hiding this comment

Skarlso commented Oct 8, 2019

Skarlso Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelvocks commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skarlso commented Oct 11, 2019 • edited Loading

michelvocks commented Oct 11, 2019 • edited Loading

Skarlso commented Oct 11, 2019

michelvocks commented Dec 2, 2019 • edited Loading

Skarlso commented Dec 3, 2019

Skarlso commented Dec 3, 2019

Skarlso commented Dec 7, 2019

Skarlso commented Dec 7, 2019

Skarlso commented Dec 7, 2019

Skarlso commented Dec 7, 2019 • edited Loading

Skarlso commented Dec 7, 2019

michelvocks commented Dec 10, 2019

Skarlso commented Jan 28, 2020

Skarlso commented Feb 14, 2020 • edited Loading

michelvocks commented Feb 14, 2020

codecov-io commented Sep 25, 2019 •

edited

Loading

Skarlso Dec 7, 2019 •

edited

Loading

Skarlso Oct 8, 2019 •

edited

Loading

Skarlso commented Oct 11, 2019 •

edited

Loading

michelvocks commented Oct 11, 2019 •

edited

Loading

michelvocks commented Dec 2, 2019 •

edited

Loading

Skarlso commented Dec 7, 2019 •

edited

Loading

Skarlso commented Feb 14, 2020 •

edited

Loading