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.

Sign up for GitHub

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

Lifecycle events proposal #3413

Merged
merged 12 commits into from
Jul 20, 2020
Merged

Lifecycle events proposal #3413

Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9d94c00
Initial proposal
upLukeWeston Jun 23, 2020
408cfce
Updated abstract
upLukeWeston Jun 23, 2020
6337c26
Added Neeraj's suggestions
upLukeWeston Jun 24, 2020
e34c98f
Revisions from Elson and Aditi
upLukeWeston Jun 26, 2020
cd1ac35
Added further evolution with postStart implementation
upLukeWeston Jun 30, 2020
9e7e394
Updated design proposal
neeraj-laad Jul 8, 2020
c746a9e
Update lifecycle_events.md
neeraj-laad Jul 8, 2020
51f5643
Merge pull request #1 from neeraj-laad/lifecycle_events_proposal
upLukeWeston Jul 9, 2020
ba95914
More clarity for when the postStart events would execute
upLukeWeston Jul 9, 2020
f44bba0
Added brief mention of support for preStart and postStop
upLukeWeston Jul 14, 2020
94a539a
Added links for new event issues
upLukeWeston Jul 15, 2020
2669c6d
Merge pull request #2 from upLukeWeston/lifecycle_updated
upLukeWeston Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions docs/proposals/lifecycle_events.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# Support execution of the commands based on lifecycle events.

## Abstract
Add support for the lifecycle events which can be defined in 2.0.0 devfiles.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to devfile 2.0 instead of 2.0.0 devfiles


Our proposed solution is to perform the preStart and postStart events as a part of the **odo push** command, and the preStop and Poststop events as a part of the **odo delete** command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/Poststop/postStop


Lifecycle bindings for specific events - https://github.com/devfile/kubernetes-api/issues/32


## Motivation
Devfile support commands that can be triggered based on dev lifecycle events. Odo will need to support/execute these commands at appropriate times within the flow.

With the implementation of livecycle events, stack creators will be able to leverage all the capabilities of devfiles when building the correct experience/features for their stacks.

## User Stories
Each lifecycle event would be suited to individual user stories. We propose that at least one issue is made for each of them for tracking and implementation purposes.

An issue has already been created for postStart: [container initiliazation support](https://github.com/openshift/odo/issues/2936)

## Design overview

The lifecycle events that have been implemented in Devfile 2.0.0 are generic. So their effect/meaning might be slightly different when applying them to a workspace (Che) or a single project (odo). However, this proposal will aim to provide a similar user experience within the scope of a single project.

Our proposal is that the preStart and postStart events are designed to run as a part of **odo push**, and that the preStop and postStop events run as a part of **odo delete**.



### The flow for **odo push** including the *preStart* and *postStart* lifecycle events will be as follows:
- preStart - Devfile command gets translated to entrypoint for specified init container - one time only
- Container initialisation (as done today)
- postStart - Exec the specified command(s) in the container - one time only
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a note as this replaces the existing devInit in the devfile 1.0 support.

- Rest of the command execution (as we do today - exec commands for build/run/test/debug)

**preStart**:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the comment earlier. Consider to remove this to avoid confusion and ignore the preStart event.

- The preStart command(s) that are specified in the Devfile will be translated into entry points for their specified containers, and added to the pod spec as init containers.
- These will only run on the first odo push, or when doing a force push (odo push -f).
- These are commands that you’d want to run before the main containers are created.

**Note: We need some good use cases to understand when the user would run commands as preStart opposed to postStart.**

**Component Initialization**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same command as before. Rephrase this to become component/container start.

- After the preStart event has completed, the containers specified in the Devfile are initialised and created if the component doesn’t already exist.

**postStart**
Copy link

@elsony elsony Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Consider to add some implementation details on this design, e.g. before starting a given container in odo, we scan for all the events postStart events defined in the devfile and find out all the commands that uses the same container. This is the logic to identify the list of postStart event that is needed to run for a given container.
  2. Add error handling. What if the postStart command failed to execute, how do we handle it and how do we surface that error to the user.
  3. The type of container (i.e. the command group that links to a given container) may affect the behaviour, e.g. the postStart only gets executed in the first odo push in most cases for build containers (containers used by the build command) since build containers will stay up and running after the first push. However, for run containers, the container may or may not get restarted depending on the restart flag. So the postStart may gets executed again for run containers. What about containers used by debug and test group of commands? We may want to clarify the behaviour in this proposal.

Copy link

@elsony elsony Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think point#1 and point#3 of #3413 (comment) have not been addressed yet.

Copy link
Contributor Author

@upLukeWeston upLukeWeston Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the premise of your point, but I have been struggling to think of any specific use cases where this would be a driving factor.
We (@EnriqueL8, @neeraj-laad, and I) have discussed the potential to be more granular in the solution by identifying which container specifically we are initialising, and whether or not there is a postStart command associated with it. This way we could better utilise the postStart commands and use it in cases where only some of the containers are being initialised.

A potential flow could be to check for postStart commands associated with a specific container when inititialising it, storing those commands in an Object(?) and then executing only those postStart commands when the containers have all finished initialising, in the order that they are defined in within the devfile. However, this isn't necessary for the basic implementation of the postStart event.
There could be room for such a feature though, and a case could be presented to argue the validity of changing the flow of odo push in order to accommodate such cases in a future implementation.

I will update the proposal to reflect this though, as it was definitely something we hadn't considered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add yaml definitions of events here, as how it would refer components and commands.

- If the component is newly created, the postStart events are executed sequentially within the containers given in the Devfile.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by sequentially? Do you mean if there are mutliple postStart events defined for a given container? If it is, do we executed sequentially based on the order of postStart event defined in a devfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. For example with the following devfile content:

...
commands:
  - exec:
      id: firstPostStartCmd
      commandLine: echo I am the first PostStart
      component: tools
      workingDir: ${CHE_PROJECTS_ROOT}/
      group:
        kind: init
commands:
  - exec:
      id: secondPostStartCmd
      commandLine: echo I am the second PostStart
      component: tools
      workingDir: ${CHE_PROJECTS_ROOT}/
      group:
        kind: init
...
events:
  postStart:
   - "firstPostStartCmd"
   - "secondPostStartCmd"

We would execute them in that order - first, then second. Presumably waiting for the previous one to complete before executing the next.

- These commands could all be in the same container, or all in different ones.
Copy link

@elsony elsony Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I understand this. Each command defines the container that it is supposed to be run on. So which container that the command is run on is determined by the command itself, not depending on the event type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right. This wasn't meant to come across as a statement about the postStarts behaviour, rather it was just acknowledging how the commands would be executed.


**Command Execution**
- After postStart, we’d run the usual build/run/test/debug commands (depending on what sort of odo push parameters the user has provided) as usual.

The reason we have chosen to use init containers for preStart rather than to be consistent, is because we don’t think there would be any difference in the preStart and postStart stages if they both ran in the same containers. If we were instead initialising the containers, running preStart in those containers, and then postStart in those containers, what would be different to running them all in preStart, or all in postStart? At least with init containers for preStart, there is a definitive difference between the two, and therefore reason to include them both as separate events.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we ignore the preStart and just execute the postStart, we won't have this problem.




### The flow for **odo delete** including the *preStop* and *postStop* lifecycle events will be as follows:
**preStop**
- Exec the specified command(s) in their respective containers before deleting the deployment and any clean up begins.

**Clean up resources**
- Clean up the pod and deployment etc. (as done today)

**postStop**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Similar to the preStart, I don't think it makes too much sense for odo to execute postStop. The postStop command has the container defined in the command itself. If we try to execute this, we'll ended up bringing up the same container as the one that we just stopped. I think this event makes more sense in Che case since it is talking about postStop of workspace vs a given component in odo.
  2. Add error handling as mentioned in postStart comment

- Execute the command specified by postStop
- Would we be spinning up a new container exclusively for the postStop commands?
- Would the command need to run on the host instead for local clean up?



### Conclusions:
- We think that the most important event is the postStart because it has a clear, reasonable use case within odo’s flow.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both postStart and preStop are userful.

- preStart and preStop could potentially be useful, but in a much more niche range. It would be important to clearly document the execution order/process for these events, because it would likely cause confusion.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree preStart will cause confusion but I don't think preStop will cause confusion at all.

- We aren’t fully conclusive on the necessity of postStop, and what it would be useful for.

## Future Evolution