Lifecycle events proposal #3413
Conversation
|
Hi @upLukeWeston. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
needs-ok-to-test
Codecov Report
@@ Coverage Diff @@
## master #3413 +/- ##
==========================================
- Coverage 46.26% 46.07% -0.20%
==========================================
Files 112 114 +2
Lines 11214 11471 +257
==========================================
+ Hits 5188 5285 +97
- Misses 5524 5673 +149
- Partials 502 513 +11
Continue to review full report at Codecov.
|
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
docs/proposals/lifecycle_events.md
Outdated
| # 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. |
There was a problem hiding this comment.
Change to devfile 2.0 instead of 2.0.0 devfiles
docs/proposals/lifecycle_events.md
Outdated
| **Component Initialization** | ||
| - After the preStart event has completed, the containers specified in the Devfile are initialised and created if the component doesn’t already exist. | ||
|
|
||
| **postStart** |
There was a problem hiding this comment.
- Consider to add some implementation details on this design, e.g. before starting a given container in odo, we scan for all the events
postStartevents defined in the devfile and find out all the commands that uses the same container. This is the logic to identify the list ofpostStartevent that is needed to run for a given container. - Add error handling. What if the
postStartcommand failed to execute, how do we handle it and how do we surface that error to the user. - The type of container (i.e. the command group that links to a given container) may affect the behaviour, e.g. the
postStartonly gets executed in the firstodo pushin most cases for build containers (containers used by the build command) since build containers will stay up and running after the first push. However, forruncontainers, the container may or may not get restarted depending on therestartflag. So thepostStartmay gets executed again forruncontainers. What about containers used bydebugandtestgroup of commands? We may want to clarify the behaviour in this proposal.
There was a problem hiding this comment.
I think point#1 and point#3 of #3413 (comment) have not been addressed yet.
There was a problem hiding this comment.
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.
docs/proposals/lifecycle_events.md
Outdated
| **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. |
There was a problem hiding this comment.
If we ignore the preStart and just execute the postStart, we won't have this problem.
docs/proposals/lifecycle_events.md
Outdated
| **Clean up resources** | ||
| - Clean up the pod and deployment etc. (as done today) | ||
|
|
||
| **postStop** |
There was a problem hiding this comment.
- Similar to the
preStart, I don't think it makes too much sense for odo to executepostStop. ThepostStopcommand 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 aboutpostStopof workspace vs a given component in odo. - Add error handling as mentioned in
postStartcomment
docs/proposals/lifecycle_events.md
Outdated
|
|
||
|
|
||
| ### Conclusions: | ||
| - We think that the most important event is the postStart because it has a clear, reasonable use case within odo’s flow. |
There was a problem hiding this comment.
I think both postStart and preStop are userful.
docs/proposals/lifecycle_events.md
Outdated
|
|
||
| ### Conclusions: | ||
| - We think that the most important event is the postStart because it has a clear, reasonable use case within odo’s flow. | ||
| - 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. |
There was a problem hiding this comment.
I agree preStart will cause confusion but I don't think preStop will cause confusion at all.
docs/proposals/lifecycle_events.md
Outdated
| ## Abstract | ||
| Add support for the lifecycle events which can be defined in 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. |
docs/proposals/lifecycle_events.md
Outdated
| **Component Initialization** | ||
| - After the preStart event has completed, the containers specified in the Devfile are initialised and created if the component doesn’t already exist. | ||
|
|
||
| **postStart** |
There was a problem hiding this comment.
could we add yaml definitions of events here, as how it would refer components and commands.
docs/proposals/lifecycle_events.md
Outdated
| **Component Initialization** | ||
| - After the preStart event has completed, the containers specified in the Devfile are initialised and created if the component doesn’t already exist. | ||
|
|
||
| **postStart** |
There was a problem hiding this comment.
I think point#1 and point#3 of #3413 (comment) have not been addressed yet.
|
@elsony Does the latest update address your review comments? |
@neeraj-laad @adisky See comment #3413 (comment). I don't think those has been addressed yet. |
|
Looking at it again, the requested info have been added the info on line 135 |
|
/lgtm |
openshift-ci-robot
added
the
lgtm
Signed-off-by: Neeraj Laad <neeraj.laad@gmail.com>
Lifecycle events proposal
openshift-ci-robot
removed
the
lgtm
docs/proposals/lifecycle_events.md
Outdated
| - `preStart` - Do not support in odo | ||
| - `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 |
There was a problem hiding this comment.
I think that this can be supported in odo. It can simply execute after preStop commands.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docs/proposals/lifecycle_events.md
Outdated
|
|
||
| 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 |
There was a problem hiding this comment.
We should support this odo. This can be easily implemented as an initContainer.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we should go with preStart => initContainers and only run them when the component is bieng created.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Added brief mention of support for preStart and postStop
|
@kadel Is there any other changes needed on this proposal? I believe we have addressed all the comments discussed above. |
|
/override ci/prow/v4.2-integration-e2e /lgtm |
|
@kadel: Overrode contexts on behalf of kadel: ci/prow/v4.2-integration-e2e, ci/prow/v4.3-integration-e2e, ci/prow/v4.4-integration-e2e, ci/prow/v4.5-integration-e2e In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kadel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/override ci/prow/unit |
|
@kadel: Overrode contexts on behalf of kadel: ci/prow/unit In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind design
[skip ci]
What does does this PR do / why we need it:
This is a design proposal for adding support for devfile lifecycle events.
Which issue(s) this PR fixes:
Relates to: #2936
How to test changes / Special notes to the reviewer:
This is a design proposal, once the design is agreed upon we can submit a PR for the implementation.