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

Lifecycle events proposal #3413

128 changes: 128 additions & 0 deletions docs/proposals/lifecycle_events.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
# Support execution of the commands based on lifecycle events

## Abstract
Devfile 2.0.0 allows binding of commands to specific lifecycle events: https://github.com/devfile/kubernetes-api/issues/32. These commands are meant to be executed when those specific lifecycle events are triggered.

The lifecycle events that have been implemented in Devfile 2.0.0 are generic. Their effect/meaning might be slightly different in the context a workspace (Che) or a single project (odo). However, odo should aim to provide a similar user experience within the scope of a single project.

## Motivation
Devfiles support commands that can be triggered based on development lifecycle events. With the implementation of lifecycle events, stack creators will be able to leverage all the capabilities of devfiles when building the correct experience/features for their stacks. `odo` should support/execute these commands at appropriate times within `odo` development flow.

## 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.

- Add support for `postStart` commands: [container initialization support](https://github.com/openshift/odo/issues/2936)
- Add support for `preStop` commands:

## Design overview
Some of the events might not be useful for `odo` to adopt. We should only support the ones that have a clear use-case, and then add more as other use-cases emerge.

For odo, the most important events are the `postStart`, and the `preStop` because they have clear, reasonable use case within odo’s flow. There is no clear use case within odo for `preStart` and `postStop` today.

- `preStart` - Do not support in odo
Copy link
Member

Choose a reason for hiding this comment

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

We should support this odo. This can be easily implemented as an initContainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kadel Do you have a clear use case in mind for preStart, that can't be achieved by postStart?

We did consider init containers - convert devfile command into a command string and pass it as the command for the init container. But, weren't sure it would make sense if the command was going to use the same build/run container. So left it out as a future piece of work.

Copy link
Member

Choose a reason for hiding this comment

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

@kadel Do you have a clear use case in mind for preStart, that can't be achieved by postStart?

Currently, I don't.
But this is how it was defined in devfile. We should implement it. Supporting only half the events in odo is going to just confuse everyone who will try to create devfiles.
If there are no technical reasons not to implement it we should implement it.

Copy link

Choose a reason for hiding this comment

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

It's just not clear to me how we can handle cleanly in odo. If we try to support this and someone specify a command that uses the build/run container for the preStart, how do you even handle that? This command is meant to be called before those container get started. In order to execute those command, we need to start the container.

Copy link
Member

@kadel kadel Jul 9, 2020

Choose a reason for hiding this comment

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

preStart needs to be executed before the workspace pod (in odo case the component) is started. This is exactly what initContainer does. Che is going to implement this in the same way. Check discussion in devfile/api#32

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tried to re-read : devfile/api#32 and found something that I had missed earlier:

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.

There is a clear requirement that container used for preStart and postStart commands will not overlap with containers of the main workspace pod. This makes is a bit clearer and perhaps we can implement preStart as init container or take an approach similar to postStop and run them as normal container commands before we run any postStart commands.

If there is no overlap in containers, there should not really be any difference in the outcome and it might be simpler to implement without init containers as we have all that we need for that already.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just not clear to me how we can handle cleanly in odo. If we try to support this and someone specify a command that uses the build/run container for the preStart, how do you even handle that? This command is meant to be called before those container get started. In order to execute those command, we need to start the container.

@elsony Please see my comment above. This is something that I had missed too, but the spec clearly states that there should not be any overlap of containers between main workspace containers (build, run, test etc.) and preStart, postStop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should go with preStart => initContainers and only run them when the component is bieng created.

Copy link
Member

Choose a reason for hiding this comment

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

when the component is bieng created.

This part might be tricky. I'm not sure if it will be possible. The initContainer will be executed everytime Deployment is triggered. Which might be right, or not :-( depending on what the command is doing.

Copy link
Contributor

@neeraj-laad neeraj-laad Jul 13, 2020

Choose a reason for hiding this comment

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

I was thinking that we could add init containers to the deployment.yaml only for the first push. and consecutive push, we can generate the deployment without init containers. But these can be looked at in more details during implementation phase as each will be implemented under separate issue.

For now we can update the proposal to include:

preStart - execute commands via init containers
postStop - execute commands via standalone pod or job and delete it once complete

@elsony @kadel would this make sense?

- `postStart` - support executing these commands as part of component initialization
- `preStop` - support executing these commands as part of the component delete
- `postStop` - Do not support in odo
Copy link
Member

Choose a reason for hiding this comment

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

I think that this can be supported in odo. It can simply execute after preStop commands.

Copy link
Contributor

@neeraj-laad neeraj-laad Jul 9, 2020

Choose a reason for hiding this comment

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

We can support postStop, but it has no real value as it will be same as preStop. We have listed this as a futures as it might be better to support the ones where there is a clear use case and then build up.

@kadel are you recommending that we support both and do the same thing for them effectively? It might be ok for completeness but I feel that might be more confusing for devfile creators.

Copy link
Member

Choose a reason for hiding this comment

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

We have listed this as a futures as it might be better to support the ones where there is a clear use case and then build up.

It already got into devfile spec, so someone probably has a use case for it.

@kadel are you recommending that we support both and do the same thing for them effectively? It might be ok for completeness but I feel that might be more confusing for devfile creators.

I don't see any problems with this.
But I see a problem when there is devfile that uses this event and someone tries to use it with odo. Would we error out and say that this devfile is unsupported? Or just show a warning and never execute the commands? Both are pretty bad options :-(

Copy link

Choose a reason for hiding this comment

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

It's not clear on how we want to implement this. If someone specify a postStop command using the build/run container, we'll be starting up the exact containers that we just stopped.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point on partial implementation. However, if we consider stop is equivalent to deleting the component and containers, the flow would be:

- preStop
- stop (delete component and all containers)
- postStop

The issue now is that by the time we have done the stop/delete operation, we don't have any existing deployment where we can execute the the postStop commands, and I don't think we want to create another one and then delete it for this.

If I change this to what you are proposing:

- preStop
- postStop
- stop (delete component and all containers)

Then we really are changing the meaning of postStop and I would be concerned about that.

I feel the real use case for odo could potentially come from actions that need to be take on "local" target. As in you want to clean up some temporary files from the developer's workspace on the host. That could be a good fit, but the way to execute them will be different. than running them on the cluster, and I'm not sure if odo supports that today.

Implementing this will be trivial as we just need to parse few more commands, but I'm trying to get clarity on use case before we go down this path.

Copy link
Member

Choose a reason for hiding this comment

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

I see your point on partial implementation. However, if we consider stop is equivalent to deleting the component and containers, the flow would be:

- preStop
- stop (delete component and all containers)
- postStop

That is true. This can be fairly easily implemented. We can just create standalone Pod or Job after the component is deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tried to re-read : devfile/api#32 and found something that I had missed earlier:

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.

There is a clear requirement that container used for preStart and postStart commands will not overlap with containers of the main workspace pod. This makes is a bit clearer and perhaps we can implement postStop as:

  • create a new deployment for running just these postStop command
  • run the postStop command
  • delete the deployment

or change the meaning slightly and:

  • run the commands in the same component after preStop but before deleting the component and all containers

Copy link
Contributor

@EnriqueL8 EnriqueL8 Jul 9, 2020

Choose a reason for hiding this comment

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

What would we be deleting with PostStop when starting a new Job or Pod in the cluster? a PVC? a database? In these scenarios, we don't need to create a resource instead we would be deleting a resource. Example: a logging sidecar receiving information from the runtime container. Then it makes sense to delete the runtime container, before deleteing the logging sidecar.
Even though, I still think this step makes more sense in a Che workspace, and maybe in the local docker support were we have created volumes locally or configuration files on the host.

Copy link
Contributor

@neeraj-laad neeraj-laad Jul 9, 2020

Choose a reason for hiding this comment

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

I think preStop commands will more be like stop the database and back up data to a volume. vs simply deleting a container.

and postStop ones will be more cleaning up other things not in the main deployment.

Copy link
Contributor

@neeraj-laad neeraj-laad Jul 9, 2020

Choose a reason for hiding this comment

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

We can update the proposal to use a standalone pod for postStop commands and then delete the pod. All of this will be done as part of odo delete.

I'll reach out to the devfiles team to get more clarity on the use case they had in mind too.


As per the Devfile 2.0.0 design specification:
- Commands associated with a lifecycle binding 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 with the same lifecycle binding do not guarantee to be executed sequentially or in any order. To run commands in a given order a user should use a composite.

The flow for initializing and deleting components should be updated to add execution of commands bound to relevant lifecycle event while initializing and destroying any containers.

### Initialize containers
- Identify which containers have been created as part of creating/updating the component deployment
- Initial implementation can assume containers are only initialised during the component creation (first push). We can improve this in future to allow execution of postStart commands at a more granular level.
- Iterate over the list of `postStart` commands and identify the ones that are associated with newly started containers
upLukeWeston marked this conversation as resolved.
Show resolved Hide resolved
- execute the identified commands one by one
- execute the command in the container
- on failure, display a meaningful error message and stop
- on success, execute next command in the list (if any)
- consider the component has been successfully initialised and continue as usual

Note: This functionality would act as a replacement for `devInit` in devfile v1.0.

### Destroy containers
- Identify which containers will be deleted as of a result of deleting the component deployment
- prepare a list of `preStop` commands that are associated with the containers being destroyed
- execute the preStop commands one by one
- execute the command in the container
- on failure, display a meaningful error message and stop
- on success, execute next command in the list (if any)
- destroy/delete the component and continue as usual

## Example devfile with lifecycle binding:
```
schemaVersion: "2.0.0"
metadata:
name: test-devfile
projects:
- name: nodejs-web-app
git:
location: "https://github.com/che-samples/web-nodejs-sample.git"
components:
- container:
id: tools
image: quay.io/eclipse/che-nodejs10-ubi:nightly
name: "tools"
- container:
id: runtime
image: quay.io/eclipse/che-nodejs10-ubi:nightly
name: "runtime"
commands:
- exec:
id: download dependencies
commandLine: "npm install"
component: tools
workingDir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
group:
kind: build
- exec:
id: run the app
commandLine: "nodemon app.js"
component: runtime
workingDir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app
group:
kind: run
- exec:
id: firstPostStartCmd
commandLine: echo I am the first PostStart
component: tools
workingDir: ${CHE_PROJECTS_ROOT}/
- exec:
id: secondPostStartCmd
commandLine: echo I am the second PostStart
component: tools
workingDir: ${CHE_PROJECTS_ROOT}/
- exec:
id: disconnectDatabase
commandLine: echo disconnecting from the database
component: tools
workingDir: ${CHE_PROJECTS_ROOT}/
events:
postStart:
- "firstPostStartCmd"
- "secondPostStartCmd"
preStop:
- "disconnectDatabase"
```

The example flow for **odo push** in this case would be:
- create the containers
- execute firstPostStartCmd within the tools container
- execute secondPostStartCmd within the tools container
- execute build command
- execute run command

The example flow for **odo delete**, in this case, would be:
- execute disconnectDatabase
- clean up the pod and deployment etc. (as done today)

## Future Evolution

The initial implementation of postStart events assumes all containers are initilised during component startup (first push). In future we could make this granular and allow executing these postStart commands if containers are restarted by other commands/events too.

Example: A potential flow could be to check for postStart commands associated with a specific container when initializing it, storing those commands in an Object(?) and then executing only those postStart commands when the containers have all finished initializing (in the order that they have been defined within the devfile). This would benefit cases where an individual container has been re-initialized, but not the whole component. For example, if a build container has been restarted as a part of **odo push --force** and there is a postStart command associated with the build container in the devfile that is required to re-run, that single postStart command would be executed before continuing to the build/run phase of the push command.

Today, *preStop* and *postStop* events are supported in Devfile 2.0.0, but there is no clear use case for them in odo. As and when we have a clear use case, we should consider implementing those for odo too.