Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lifecycle bindings to bind commands to specific events #32

Closed
davidfestal opened this issue Mar 24, 2020 · 14 comments · Fixed by #35
Closed

Add lifecycle bindings to bind commands to specific events #32

davidfestal opened this issue Mar 24, 2020 · 14 comments · Fixed by #35
Milestone

Comments

@davidfestal
Copy link
Collaborator

davidfestal commented Mar 24, 2020

We might want to introduce a syntax that would allow binding commands to a given workspace event.

A few use cases

  • Open a file in the IDE at workspace startup
  • Run build after clone of the project
  • Copy files before workspace startup
  • Backup files at workspace shutdown

Lifecycle Bindings

The lifecycle binding could be provided by a new top-level construct that could look like the following:

events:
  <lifecycleBinding>:
    - <commandName>

where can be one of the following:

  • preStart
  • postStart
  • preStop
  • postStop

Spec Details

⚠️ lifecycleBindings are processed sequentially. For example preStart commands will be invoked after every postCreate command has completed.

⚠️ In the case of Che-Theia, the postStart bindings should happen after all plugins and extensions have started, including project cloning. This means that those commands are not triggered until the user opens the IDE in his browser.

⚠️ Implementation of preStart commands may need execution of a workspace pod init container. That means that the component associated to the preStart command should not be part of the workspace pod. The same applies to postStop bindings.

⚠️ Commands associated to a lifecycleBinding should provide a mechanism to figure out when they have completed. That means that the command should be terminating or having a readiness probe.

⚠️ Commands associated to the same lifecycleBinding are not guarantee to be executed sequentially or in any order. To run commands in a given order a user should use a composite.

Example

components:
  - container:        # <== This component is associated with a **preStart** event only.
      name: "copier"  #     That means it won't be included in the workspace pod but
      image: ''       #     will be run before it (as an initContainer for example).
      env:
        - name: ""
          value: ""
      volume: ....
  - container:
      name: "maven"
      image: ''
      env:
        - name: ""
          value: ""
      volume: ....
  - plugin:
      id: theia
commands:
  - exec:
      name: "copyNeededFiles"
      component: "copier"
      commandLine: "cp somefile"
  - exec:
      name: "buildAll"
      component: "maven"
      commandLine: "mvn ..."
  - vsCodeTask:
      name: "openFile"
      component: "theia"
      inline: |
        { 
          xxxx
        }
events:
  preStart:
    - "copyNeededFiles" # <== Start of the workspace happens
                        #     after this command is completed
  postStart:
    - "buildAll"  # <== The Order of execution of the commands
    - "openFile"  #     under a same binding is not guarantee 

Not included in this spec

  • Custom lifecycleBindings of components
@davidfestal davidfestal changed the title Add lifecycle bindings to binfd commands to specific events Add lifecycle bindings to bind commands to specific events Mar 24, 2020
@davidfestal
Copy link
Collaborator Author

This might be related to issue #30 if we consider that an init action is mainly just a specific case of a wider concept of command bindings.

@metlos
Copy link

metlos commented Mar 26, 2020

In addition to the events and scopes, I think we may generalize the pre-, post- and first- as being applicable to any event. So maybe we could define attributes like before: true|false and once: true|false that could capture those in a generic manner:

events:
  editor:
    open:
      command: "open Readme"
      once: true
    clone:
      command: "build all"
  workspace:
    init: 
      command: "initialize something"
    stop:
      command: "do some cleanup"
      before: true
  componentStart:
      component: maven
      before: "copy files"
      after: "cleanup files"
  custom:
    - scope: "my custom scope"
      event: "my custom event"
      command: "some specific command"

@l0rd
Copy link
Contributor

l0rd commented Apr 7, 2020

Reviewed and approved

davidfestal added a commit that referenced this issue Apr 10, 2020
Signed-off-by: David Festal <dfestal@redhat.com>
@amisevsk
Copy link
Contributor

Sorry I missed this until now, but I'm unclear on the semantics of

 - container:        # <== This component is associated with a **preStart** event only.
     name: "copier"  #     That means it won't be included in the workspace pod but
     image: ''       #     will be run before it (as an initContainer for example).

Does this mean

  • Container in devfile with no events associated -> run in main deployment
  • Container in devfile with only preStart events associated -> run as e.g. init container
  • Container with a preStart event, but with another command also associated with it -> ? (run once as init, and add to main deployment?)

This seems to leave a little too much grey area, and it introduces a bit of a "change one line and everything is different" issue: visually, a user has to match

  • command names in events array to commands in commands array
  • components in commands to component names in components array
  • which phase each command is in
  • (potentially) any other commands targeting the component

to determine if a container will be in the main deployment or not.

I think it would be clearer to explicitly have to mark containers as 'external' to the workspace deployment. With the current set-up:

  • You can't specify an init container that doesn't have a command and event associated with it (e.g. if the command is built-in to the container itself and it only needs to be run)
  • You can't (?) use the same container for a prestart binding while having it in the main deployment
  • The spec implies a fairly complicated validation/matching task to determine where containers should end up in the prestart/main deployment/poststop sequence.

@sleshchenko
Copy link
Member

1.

...
commands:
  - exec:
      name: "copyNeededFiles"
      component: "copier"             # it references preStart component. Does it mean that it's not available after workspace start?
      commandLine: "cp somefile"

If would vote for initContainers instead of components and commands bound for preStart phase.

2.

⚠️ In the case of Che-Theia, the postStart bindings should happen after all plugins and extensions have started, including project cloning. This means that those commands are not triggered until the user opens the IDE in his browser.

It may be confusing if we take into account postStart phase of the k8s model https://kubernetes.io/docs/tasks/configure-pod-container/attach-handler-lifecycle-event/

3 What is actually postStop? If it a dedicated deployment run to execute some command after the main deployment is totally removed?

@l0rd
Copy link
Contributor

l0rd commented Apr 15, 2020

@amisevsk agree. On one hand I am concerned as you are about that. On the other end I am afraid that we are over-engineering it. Supporting the case of a container used as both init container and main container is really required? If not I would prefer if we start with this spec and work on an external/init optional fields in next iteration.

@sleshchenko 1) initContainer would work for preStart phase events but what we do with other events? We are trying to find a consistent way to express actions linked to those 4 phases of a workspace. The other reason against initContainer is because the actual implementation may be different than a Kubernetes initContainer. 2) yes I agree, it doesn't match Kubernetes postStart but the workspace postStart. I couldn't find a better idea though. 3) stop is when the workspace is stopped, not when it is deleted. You may have some actions before the stop command is sent to the components of the workspace (preStop) or after the workspace components have been stopped (postStop).

@amisevsk
Copy link
Contributor

@l0rd

Supporting the case of a container used as both init container and main container is really required?

I moreso referring to the grey area that's not explictly documented -- where do my components end up based on a set of commands and event bindings? It can be hard to reason about, which I imagine will make implementation similarly tricky/subtle.

lifecycle-devfile

To determine where containers end up, we need to consider, for each command:

  • Where it shows up in the lifecycle bindings
  • Which components it refers to
  • What other commands also refer to those components
  • Where those commands show up in lifecycle bindings
  • etc.

The example above would be further complicated by considering postStart, preStop, and postStop.

@l0rd
Copy link
Contributor

l0rd commented Apr 15, 2020

@amisevsk one thing is how readable the devfile is. Another how hard is it to validate/reconcile.

We are making implementation harder but it should be easy to read. I am worried if you think that's hard to read too.

On making implementation easier: even if we add a component init field to explicitly specify that the component should be executed before the main workspace pod creation:

components:
  - container:
      name: "copier"
      init: true

we still need to validate that preStart commands are run in containers that have init: true. Basically the flow that you have above. So that won't help. How could we make it easy to read and easy to implement then?

@amisevsk
Copy link
Contributor

amisevsk commented Apr 17, 2020

On the reading side, I think being explicit about where each component is to be used improves things -- if I can set

components:
  - container:
      name: "copier"
      external: true # or init/similar

to signify that I intend this container to be a 'setup/teardown' container, it's easier to reason about.

On the parsing side, this also enables simpler validation:

  • For each command in the preStart/postStop phase, target must be external
  • For each other command, target must not be external.

This would also encourage using specific components for your prestart/poststop actions, but would potentially block reusing a component for prestart and during the deployment.

Also, I'm not 100% sure on using regular commands for the preStart/postStop phases, since it's a bit strange -- these commands are mixed in with commands that the user will use regularly, but should not necessarily be listed in whatever UI shows them.

Edit because I forgot to address this:

we still need to validate that preStart commands are run in containers that have init: true. Basically the flow that you have above

The difference here would be that the check goes in one direction, otherwise we have to implicitly populate this as internal metadata and check it that way.

@amisevsk
Copy link
Contributor

Alternatively, the implementation I would produce for the current spec would mean that whatever containers are used for prestart/poststop actions are created before/after the main deployment. In a pathlogical case, this could mean running every container in the workspace three times.

What would a best-practice workspace configuration look like? IMO it would be a separate component that's used for setup/teardown, so I think the spec should encourange the user in that direction.

davidfestal added a commit that referenced this issue Apr 22, 2020
* Implement agreement on issue #18
* Implement agreement on issue #17
* Implement agreement on issue #21
* Implement agreement on issue #22
* Implement agreement on issue #27
* Implement agreement on issue #9
* Implement agreement on issue #10
* Fix PR comments about issue #9 PR comment #35 (comment)
* Fix PR comments about issue #17 PR comment #35 (comment)
* Implement agreement on issue #14 and  #19
* Add the spring boot example
* Implement agreement on issue #32
* Fix last PR comments

Signed-off-by: David Festal <dfestal@redhat.com>

Co-Authored-By: Sergii Leshchenko <sleshche@redhat.com>
@l0rd
Copy link
Contributor

l0rd commented May 18, 2020

I have added the proposal to add a dedicatedPod attribute to components #48

@l0rd
Copy link
Contributor

l0rd commented May 19, 2020

Reviewed and agreed that with:

  • the addition of proposal Out of main pod containers #48
  • a new apply command (@davidfestal will create an issue)
  • the use of label to specify if the command will appear in an interactive menu (e.g. theia tasks)
    we are ok with the spec in this issue.

@davidfestal
Copy link
Collaborator Author

Issue opened here: #56

@davidfestal
Copy link
Collaborator Author

Implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants