Introducing a new Task specification #859

gerred · 2019-09-25T15:09:57Z

We introduce a new Task specification, which will allow us to better encapsulate different kinds of tasks (e.g. Helm, Pipe-tasks, etc) in the future. Here is how the API changed:
Before: tasks were a list of resources, which were applied in step with an optional delete: true signaling, that resources have to be deleted.

# operator.yaml
  ...
  tasks:
    deploy:
      resources:
        - deploy.yaml
    remove:
      reources:
        - deploy.yaml

  plans:
    deploy:
      strategy: serial
      phases:
        - name: phase
          strategy: serial
          steps:
            - name: step
              tasks:
                - deploy
    remove:
      strategy: serial
      phases:
        - name: phase
          strategy: serial
          steps:
            - name: step
              tasks:
                - remove
              delete: true

The new task definition requires specifying tasks name, kind and spec fields. Here is the same example using new Apply and Delete tasks kinds:

  tasks:
    - Name: deploy
      Kind: Apply
      Spec:
        resources:
          - deploy.yaml
    - Name: uninstall
      Kind: Delete
      Spec:
        resources:
          - deploy.yaml

Its usage from the step is almost the same, merely delete: true has been removed.

Additionally, plan execution logic was reworked, It now respects phases strategy and propagates fatal and transient execution errors differently (see this comment for more information).

Fixes: #839

by embeding all task types into a separate `struct TaskSpec`. Additionally, renamed `CreateTask` into `ApplyTask` since that better reflects how we handle its resources.

kensipe

I'm wonder if this is mispackaged... what do you think of a package named task

then you would have task.Apply, task.Kubernetes, task.Multi, task.Output, task.Builder... you get it. using the package as part of communication is really growing on me...

also... nice start!

# Conflicts: # cmd/manager/main.go # pkg/apis/kudo/v1alpha1/instance_types.go # pkg/controller/instance/instance_controller.go # pkg/controller/instance/plan_execution_engine.go # pkg/controller/instance/plan_execution_engine_test.go # pkg/kudoctl/cmd/plan/plan_history.go # pkg/util/health/ready.go # test/integration/upgrade-command/01-assert.yaml

# Conflicts: # go.mod # go.sum # pkg/controller/instance/plan_execution_engine.go # pkg/controller/instance/plan_execution_engine_test.go

for task.go, task_apply.go and task_delete.go

gerred

super nice! just a few clarifications really, I love how this was made. I can't approve since it's my own PR 🙃

pkg/engine/task/task_apply.go

gerred · 2019-10-15T11:44:15Z

pkg/engine/task/task_delete.go

+	}
+
+	// 2. - Kustomize them with metadata -
+	kustomized, err := kustomize(rendered, ctx.Meta, ctx.Enhancer)


why do we need to kustomize them? is it because of the name decoration we do? sounds about right, just worth documenting since it looks a little odd on first look.

I'm not sure we have to. This logic got adopted from master and I didn't question it. I'd also rather tackle this in a separate PR.

yeah I actually think we don't have to. To be honest, I think that the DeleteTask should not even accept resources, specifying the identifier of that resource should be enough but it would probably need some API design... having resources there is easier now :)

@alenkacz how would we know the templating of those names? I know at least for one operator where the name is (or is becoming) a Go template, potentially.

pkg/engine/task/task_delete.go

pkg/engine/task/task_apply.go

zen-dog · 2019-10-15T19:51:11Z

I can't approve since it's my own PR

Lol, I can but I'm obviously biased 😆

alenkacz

Okay, first pass done 👍 I like how the logic is getting broken into smaller and smaller pieces, we really came a long way from that super ugly controller we had in the beginning, love it. Especially how delete and apply are separated now. It will be a great ground work for future tasks.

I left few comments, some of them are just to start a conversation and does not have to be addressed in this PR (I tried to mark them in an obvious way) :)

pkg/controller/instance/plan_execution_engine.go

pkg/engine/task/task.go

pkg/engine/task/task_apply.go

alenkacz · 2019-10-16T07:49:02Z

pkg/engine/task/task_delete.go

+	}
+
+	// 2. - Kustomize them with metadata -
+	kustomized, err := kustomize(rendered, ctx.Meta, ctx.Enhancer)


yeah I actually think we don't have to. To be honest, I think that the DeleteTask should not even accept resources, specifying the identifier of that resource should be enough but it would probably need some API design... having resources there is easier now :)

kensipe

this general looks good and I'm ok with it landed the way it is... the biggest issue for me is consistency around errors and fatal

config/crds/kudo_v1alpha1_operatorversion.yaml

pkg/apis/kudo/v1alpha1/zz_generated.deepcopy.go

pkg/controller/instance/instance_controller.go

pkg/engine/task/task.go

kensipe · 2019-10-16T15:03:06Z

pkg/engine/task/task_apply.go

+	return nil
+}
+
+func prettyPrint(key client.ObjectKey) string {


I'm not a fan of this being here... the naming is off. task.prettyPrint seems like it would pretty print a task... but this pretty prints a objectkey. It is private and isn't a big deal, but it seems like it would have utility in other areas of code.

I am actually fine with this tiny code duplication, but no strong feelings here :)

A common purpose prettyPrint method would need a new file, more error handling etc. I decided to keep it small and narrow-purposed for now.

…ed/tasker

# Conflicts: # pkg/controller/instance/instance_controller.go # pkg/controller/instance/plan_execution_engine.go # pkg/controller/instance/plan_execution_engine_test.go

alenkacz and others added 18 commits September 17, 2019 13:49

Cleanup controller registration to the runtime 0.2.0

7e4605c

Formatting

7a9e240

More cleanup

639e55e

Skeleton of the new instance controller

eb920c9

Progress

f894bfd

Merge

46d754d

Implement plan execution in instance controller

ea1c146

Merge

a31c100

Typo

673bc9b

Remove dead code, better registration

4dcfc66

Fix CLI commands and make linter happy

a927c61

Fix integration test, implement deploy plan trigger

57b37b5

Upgrade

123f496

Update

d6d0a40

OV reconciliation and update fix

b2e23c0

Go back to maps from slice of statuses

0213ef6

Fix when the plan snapshot has to be stored

c3fc262

spike out tasker and update generated code

b8ca7f6

gerred requested review from alenkacz, jbarrick-mesosphere, kensipe and zen-dog as code owners September 25, 2019 15:09

alenkacz changed the base branch from master to av/instance-controller-v2 September 25, 2019 15:10

Removed custom json unmarshaller for the Task type

2ccde6d

by embeding all task types into a separate `struct TaskSpec`. Additionally, renamed `CreateTask` into `ApplyTask` since that better reflects how we handle its resources.

zen-dog force-pushed the gerred/tasker branch from c37d2c7 to 2ccde6d Compare September 26, 2019 13:37

gerred added 2 commits September 26, 2019 13:42

a whole bunch of tasks, not necessarily in a working state

d74255f

fixes

b55d879

kensipe reviewed Sep 27, 2019

View reviewed changes

gerred added 2 commits September 27, 2019 11:01

increment

91000b8

zen-dog added 9 commits October 12, 2019 01:00

Merge branch 'master' into gerred/tasker

cd13c9f

# Conflicts: # go.mod # go.sum # pkg/controller/instance/plan_execution_engine.go # pkg/controller/instance/plan_execution_engine_test.go

Added more tests

8f3cdfd

for task.go, task_apply.go and task_delete.go

Reverted update_codegen.sh changes

09ec865

Fixed and simplified task tests

0307055

Fixed unit tests

760e88b

Update integration test and made linter happy

a568cfa

Fixed CRD generation

2cc0519

An even better fix for CRD generation

50432bd

Fixed crd test yet again

3e85dd4

zen-dog changed the title ~~[WIP] Initial spike of Tasker~~ Introducing a new Task specification Oct 14, 2019

zen-dog and others added 3 commits October 14, 2019 12:56

Making linter happy again

18af216

Manually update the CRD

2b09312

Fixing the rest of the integration tests

ef1ba7d

gerred commented Oct 15, 2019

View reviewed changes

pkg/engine/task/task_apply.go Show resolved Hide resolved

zen-dog added 3 commits October 15, 2019 20:18

Removed Apply/DeleteTaskSpec in favour of ResourceTaskSpec

ef926fb

Added missing genclient annotation that got lost in refactoring

507b9e9

...and once more, but this time properly!

4f10196

alenkacz reviewed Oct 16, 2019

View reviewed changes

fixing mispelling of parameter

e47e190

kensipe reviewed Oct 16, 2019

View reviewed changes

kensipe approved these changes Oct 16, 2019

View reviewed changes

zen-dog added 4 commits October 16, 2019 18:23

First round of review feedback

2b33171

Merge branch 'gerred/tasker' of github.com:kudobuilder/kudo into gerr…

2228480

…ed/tasker

Fail on missing phase/step status fields instead of creating them

1e9f36a

Merge branch 'master' into gerred/tasker

89ea599

# Conflicts: # pkg/controller/instance/instance_controller.go # pkg/controller/instance/plan_execution_engine.go # pkg/controller/instance/plan_execution_engine_test.go

zen-dog merged commit 8827667 into master Oct 19, 2019

zen-dog deleted the gerred/tasker branch October 19, 2019 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing a new Task specification #859

Introducing a new Task specification #859

gerred commented Sep 25, 2019 •

edited by zen-dog

Loading

kensipe left a comment

gerred left a comment

gerred Oct 15, 2019

zen-dog Oct 15, 2019 •

edited

Loading

alenkacz Oct 16, 2019

gerred Oct 16, 2019

zen-dog commented Oct 15, 2019

alenkacz left a comment

alenkacz Oct 16, 2019

kensipe left a comment

kensipe Oct 16, 2019

alenkacz Oct 17, 2019

zen-dog Oct 19, 2019

Introducing a new Task specification #859

Introducing a new Task specification #859

Conversation

gerred commented Sep 25, 2019 • edited by zen-dog Loading

kensipe left a comment

Choose a reason for hiding this comment

gerred left a comment

Choose a reason for hiding this comment

gerred Oct 15, 2019

Choose a reason for hiding this comment

zen-dog Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

alenkacz Oct 16, 2019

Choose a reason for hiding this comment

gerred Oct 16, 2019

Choose a reason for hiding this comment

zen-dog commented Oct 15, 2019

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz Oct 16, 2019

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe Oct 16, 2019

Choose a reason for hiding this comment

alenkacz Oct 17, 2019

Choose a reason for hiding this comment

zen-dog Oct 19, 2019

Choose a reason for hiding this comment

gerred commented Sep 25, 2019 •

edited by zen-dog

Loading

zen-dog Oct 15, 2019 •

edited

Loading