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 support for command field in container components #5648

Closed
kadel opened this issue Apr 8, 2022 · 5 comments · Fixed by #5768
Closed

add support for command field in container components #5648

kadel opened this issue Apr 8, 2022 · 5 comments · Fixed by #5768
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).

Comments

@kadel
Copy link
Member

kadel commented Apr 8, 2022

devfile/registry#102 (comment)

Odo is not able to use devfiles that have container component with a custom command. Problem is that in the current odo implementation it overrides the default process in deployment which means that supervisord is not started. When supervisord is not running, odo is not able to execute any commands in the container.

A quick fix is for odo to ignore command and args fields in container components.
This has been done in #5620

The proper fix is going to be to keep using supervisord as pid1, and execute the commmand with args "on the side" as a separate process.

/kind bug
/priority high

@openshift-ci openshift-ci bot added kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels Apr 8, 2022
@rm3l rm3l self-assigned this May 10, 2022
@rm3l rm3l moved this to In Progress in odo v3-beta1 May 10, 2022
@rm3l
Copy link
Member

rm3l commented May 13, 2022

I'm summarizing my notes below after looking into this issue:

The approach suggested here to run the user-defined command and args on the side as a separate process would be ok if they were both mandatory in the Devfile spec, I think.

As command and args are optional in the Devfile spec, there might be cases where an args could be defined without a command and vice-versa, in which case (as mentioned in devfile/api#679 and in the spec) we should use whatever is defined in the container image. I guess this is because containers might generally need to do some work on startup and overriding the entrypoint would probably break that.
So it might be complicated for odo to determine in advance what should be the command-line of that main process without starting the container.

As discussed, I investigated and did some preliminary research/testing to see if we could instead run our supervisord process in the container and attach it as a child of the container main one, and unfortunately, it looks like there is no way to change the ppid of a given process outside of the kernel. Even if it was possible to do so, I am not sure about signal handling from the parent to its children here. Also, in case the container restarts (for any reason due to the ephemeral nature of a pod), I think we would lose the supervisord process.


I was thinking about an alternative approach that builds upon the fact that, to me, Kubernetes Pods were precisely designed to solve this issue of having multiple cooperating processes (as containers) that form a cohesive unit of service. We could run multiple containers in the same pod, sharing a same network namespace and volumes; they could even share the process namespace.

So with that in mind, the Deployment and Pod specification generated by odo could be made up of multiple containers :

  1. the main container that would just meet the Devfile spec, by using either the command and args defined in the Devfile or whatever is defined in the image. It does not matter if the main container runs a terminating process, because the sidecar container(s) below would still be running and non-terminating. So port-forwarding would still work, I think.
  2. in the same pod, odo could inject additional sidecar container(s) that would share the projects volume, plus other environment variables defined in the main container:

I am not sure yet to what extent this would solve the issue here (because I may probably be missing some pieces).

@kadel @feloy Any thoughts?

@feloy
Copy link
Contributor

feloy commented May 13, 2022

Note that it could also be possible to share the Process (Pid) namespace between containers of a same pod:
https://kubernetes.io/docs/tasks/configure-pod-container/share-process-namespace/

@feloy
Copy link
Contributor

feloy commented May 13, 2022

As discussed with @rm3l , I suggest to get rid of supervisord, and take advantage of the fact that odo dev is now a long running command (as opposed to odo push), and we could start the run command from odo without releasing it (with the help of some go routine).
We could pass the cmd and args as is to the Deployment's container

@kadel
Copy link
Member Author

kadel commented May 13, 2022

As discussed with @rm3l , I suggest to get rid of supervisord, and take advantage of the fact that odo dev is now a long running command (as opposed to odo push), and we could start the run command from odo without releasing it (with the help of some go routine).
We could pass the cmd and args as is to the Deployment's container

This is a good idea 👍 It looks like a nice and clean solution.

The only downside that I can see here is that the odo dev command will always have to stay as "long running" command once we do this. But that might be actually ok as we are already relaying on this with port-forwarding.

Let's try to implement this approach. This could drastically simplify odo code.

@rm3l
Copy link
Member

rm3l commented May 13, 2022

+1 for this approach. Thank you all for your feedback.

@rm3l rm3l moved this from In Progress to For Review in odo v3-beta1 May 31, 2022
Repository owner moved this from For Review to Done in odo v3-beta1 Jun 17, 2022
@rm3l rm3l added the v3 label Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants