Break out task step into core and runtime sides by ddadlani · Pull Request #4109 · concourse/concourse

ddadlani · 2019-07-08T14:37:30Z

This is a draft PR to attempt splitting out the TaskStep into core-specific and runtime-specific concerns.

The idea is to put all runtime-specific concerns for all steps into the worker.Client so that we have a single place to optimize container placement and worker selection for stories such as #3695, #2928 and #3301. This PR attempts it only for the task step.

The tests do not currently pass, however this is a draft PR so that others can give their feedback on this approach. Fixing the tests is a work in progress.

atc/exec/task_step.go

kcmannem · 2019-07-08T14:48:31Z

I like this change

atc/worker/client.go

nader-ziada · 2019-07-09T15:15:32Z

atc/worker/client.go

+		ImageFetcherSpec,
+		TaskProcessSpec,
+		chan string,
+	) (int, []VolumeMount, error)


Why is Run returning VolumeMount?

Because we need to registerOutputs in the TaskStep. Decoupling volumes/storage from Running a step is something we would need to look into, but in the absence of a separate Storage implementation, we would have to return volumes this way.

For example, we would likely register "outputs" beforehand with a specific storage implementation, and then de-register them if the task failed. That would allow us to not depend on VolumeMounts in the future.

atc/exec/task_step.go

atc/worker/pool.go

xtreme-sameer-vohra · 2019-07-12T22:42:33Z

Overall, the Task Step looks much simpler than before and once this style of refactoring is applied to the other steps, they might similarly become much more cleaner, to the point, where there may arise 1 or 2 generic steps.

One thing to watch out for is how Client would evolve as a result of this path ( once we add the other steps) . Might it become quite large with lots of state to initialize and private functions to DRY things up. This might in turn making testing much more complicated as it becomes a thick client. In this regard, the current step design (eg. K8sTaskStep, GardenTaskStep ) would be smaller interfaces with dependencies can easily be injected for testing.

ddadlani · 2019-07-15T18:08:38Z

@xtreme-sameer-vohra thanks for reviewing the PR!

Overall, the Task Step looks much simpler than before and once this style of refactoring is applied to the other steps, they might similarly become much more cleaner, to the point, where there may arise 1 or 2 generic steps.

@nader-ziada and I were looking at the code, and we realized that the Get and Put steps and Checks also do a very similar thing, in that they select a worker, create a container and then run a defined script which is very similar to the setup for the task script. In the future, ideally we would be able to remove the concept of a "type" of step from runtime, and instead have a defined set of things (select worker, create container, run script, return result) that would allow us to run any kind of step.

One thing to watch out for is how Client would evolve as a result of this path ( once we add the other steps) . Might it become quite large with lots of state to initialize and private functions to DRY things up. This might in turn making testing much more complicated as it becomes a thick client. In this regard, the current step design (eg. K8sTaskStep, GardenTaskStep ) would be smaller interfaces with dependencies can easily be injected for testing.

I agree. Currently worker.Client is making a lot of decisions on what to do. In the ideal future I would see it become runtime.Client which simply decides on what runtime backend to use and then invoke the required method on that. The code currently in worker.Client is more suited to being in a runtime implementation rather than in a client. Hopefully future refactor efforts enable this view.

move worker-specific logic into worker.Client Signed-off-by: Divya Dadlani <[email protected]>

use TaskResult as return value Signed-off-by: Nader Ziada <[email protected]>

Signed-off-by: Divya Dadlani <[email protected]>

atc/exec/task_step.go

Fix conflicts caused by PR #4118 - addition of `limit-task-step` container placement strategy

#4052 Signed-off-by: Divya Dadlani <[email protected]>

xtreme-sameer-vohra · 2019-07-26T19:58:56Z

atc/exec/task_step.go

+	go func(logger lager.Logger, config atc.TaskConfig, events chan runtime.Event, delegate TaskDelegate) {
+		for ev := range events {
+			switch ev.EventType {
+			case runtime.InitializingEvent:


Doesn't look like this event is called ?

xtreme-sameer-vohra · 2019-07-26T19:59:11Z

atc/exec/task_step.go

+			case runtime.StartingEvent:
+				step.delegate.Starting(logger, config)
+
+			case runtime.FinishedEvent:


Doesn't look like this event is called ?

we'll address these changes in a follow up PR because it's been a while since this PR has been open and keeping it up to date with master is problematic.

xtreme-sameer-vohra · 2019-07-26T20:04:31Z

atc/exec/task_step.go

-		}

-		step.succeeded = (status == 0)
+	close(events)


Should it be RunTaskStep's responsibility to close the channel since it is the producer?

- task_step - move event processing block closer to consumer - worker/client.go - avoid hoisting to improve readability

Minor refactor Signed-off-by: Divya Dadlani <[email protected]>

Signed-off-by: Divya Dadlani <[email protected]>

ddadlani · 2019-07-29T17:41:49Z

Another thing to consider is that for heavy tasks like Strabo, we will be creating a lot more goroutines since a new event goroutine is created for every task container

jamieklassen · 2019-08-26T20:22:31Z

@concourse/runtime can you folks decide whether this needs a release note? On the surface it's a refactor, but I see allusions to operability effects. If it doesn't need a release note, just add the release/undocumented tag, and if it does need one, can you open a PR making the appropriate modifications to https://github.com/concourse/concourse/blob/master/release-notes/latest.md.

xtreme-sameer-vohra · 2019-08-27T14:16:36Z

@pivotal-jamie-klassen
The change should be transparent to users so I don't believe we want to include anything in the release notes.

@ddadlani
Only a single go routine would be created to manage the events per task as opposed to new go routine per event ?

ddadlani · 2019-08-27T14:43:27Z

I agree with @xtreme-sameer-vohra , I don't think this needs to be documented because behavior shouldn't change for our users. I'll add the label now.

@xtreme-sameer-vohra not sure what you mean? We did not have goroutines for task events before this change

ddadlani requested review from deniseyu, kcmannem and xtreme-sameer-vohra July 8, 2019 14:37

kcmannem requested a review from nader-ziada July 8, 2019 14:38

kcmannem reviewed Jul 8, 2019

View reviewed changes

atc/exec/task_step.go Outdated Show resolved Hide resolved

ddadlani mentioned this pull request Jul 8, 2019

Refactor interface to k8s and garden steps #4052

Closed

ddadlani force-pushed the separate-runtime branch from 5a744c2 to 392c00e Compare July 8, 2019 19:53

aledegano mentioned this pull request Jul 9, 2019

Add locking around steps. #4108

Closed

nader-ziada reviewed Jul 9, 2019

View reviewed changes

jwntrs reviewed Jul 11, 2019

View reviewed changes

atc/exec/task_step.go Outdated Show resolved Hide resolved

xtreme-sameer-vohra reviewed Jul 12, 2019

View reviewed changes

atc/exec/task_step.go Outdated Show resolved Hide resolved

xtreme-sameer-vohra reviewed Jul 12, 2019

View reviewed changes

atc/worker/pool.go Outdated Show resolved Hide resolved

ddadlani force-pushed the separate-runtime branch from 546b51f to 7445678 Compare July 15, 2019 17:58

Divya Dadlani and others added 3 commits July 15, 2019 16:41

split out RunTaskStep from TaskStep

b4c9e0c

move worker-specific logic into worker.Client Signed-off-by: Divya Dadlani <[email protected]>

split out TaskStep's registerOutputs and registerCaches

28a0f88

use TaskResult as return value Signed-off-by: Nader Ziada <[email protected]>

refactor TaskStep.Run for better readability

472ea7f

Signed-off-by: Divya Dadlani <[email protected]>

ddadlani force-pushed the separate-runtime branch from 7445678 to 472ea7f Compare July 15, 2019 20:43

jwntrs reviewed Jul 16, 2019

View reviewed changes

atc/exec/task_step.go Outdated Show resolved Hide resolved

Divya Dadlani added 2 commits July 23, 2019 16:49

Merge branch 'master' into separate-runtime

e38b147

Fix conflicts caused by PR #4118 - addition of `limit-task-step` container placement strategy

handle task events cleanly

c3a5984

#4052 Signed-off-by: Divya Dadlani <[email protected]>

ddadlani marked this pull request as ready for review July 23, 2019 21:41

ddadlani marked this pull request as ready for review July 26, 2019 15:24

Merge branch 'master' into separate-runtime

e0853a9

xtreme-sameer-vohra reviewed Jul 26, 2019

View reviewed changes

Minor refactor

b5617f5

- task_step - move event processing block closer to consumer - worker/client.go - avoid hoisting to improve readability

Merge pull request #4175 from concourse/separate-runtime-sv

bbc3299

Minor refactor Signed-off-by: Divya Dadlani <[email protected]>

ddadlani force-pushed the separate-runtime branch from 7a9db91 to bbc3299 Compare July 29, 2019 16:22

ddadlani mentioned this pull request Jul 29, 2019

Minor refactor #4175

Merged

Merge branch 'master' into separate-runtime

0a8f8d4

Signed-off-by: Divya Dadlani <[email protected]>

ddadlani merged commit d89eca9 into master Jul 29, 2019

aledegano mentioned this pull request Jul 30, 2019

[bugfix] Do not trigger Garden error when asking for container property. #4167

Merged

ddadlani deleted the separate-runtime branch August 1, 2019 15:32

jamieklassen added this to the v5.5.0 milestone Aug 26, 2019

ddadlani added the release/undocumented This didn't warrant being documented or put in release notes. label Aug 27, 2019

Uh oh!

Comments

Conversation

ddadlani commented Jul 8, 2019

Uh oh!

Uh oh!

kcmannem commented Jul 8, 2019

Uh oh!

Uh oh!

nader-ziada Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

ddadlani Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xtreme-sameer-vohra commented Jul 12, 2019

Uh oh!

ddadlani commented Jul 15, 2019

Uh oh!

Uh oh!

xtreme-sameer-vohra Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

xtreme-sameer-vohra Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

ddadlani Jul 29, 2019

Choose a reason for hiding this comment

Uh oh!

xtreme-sameer-vohra Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

ddadlani commented Jul 29, 2019

Uh oh!

jamieklassen commented Aug 26, 2019

Uh oh!

xtreme-sameer-vohra commented Aug 27, 2019

Uh oh!

ddadlani commented Aug 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants