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 an ability to specify entrypoint for kube and openshift tool in Devfile #12693

Closed
l0rd opened this issue Feb 19, 2019 · 13 comments
Closed

Add an ability to specify entrypoint for kube and openshift tool in Devfile #12693

l0rd opened this issue Feb 19, 2019 · 13 comments
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.

Comments

@l0rd
Copy link
Contributor

l0rd commented Feb 19, 2019

Description

@sleshchenko proposal for the format:

tools:
  - name: app
    type: kubernetes
    local: app.yaml
    entrypoints:
      - pod: container #optional
        container: app #optional
        command: ['/bin/sh', '-c']
        args: ['tail -f /dev/null']

Applying that to a devfile that uses yamls from Kubernetes Node.js and MongoDB sample:

specVersion: 0.0.1
name: fix-the-kube
projects:
  - name: nodejs-mongo-app
    source:
      type: git
      location: 'https://github.com/ijason/NodeJS-Sample-App.git'
tools:
  - name: theia-editor
    type: cheEditor
    id: org.eclipse.che.editor.theia:1.0.0
  - name: exec-plugin
    type: chePlugin
    id: che-machine-exec-plugin:0.0.1
  - name: mongodb
    type: kubernetes
    local: mongo-controller.yaml
  - name: nodejs
    type: kubernetes
    local: node-controller.yaml
    entrypoints:
      - command: ['/bin/sh', '-c']
        args: ['tail -f /dev/null']
@slemeur slemeur mentioned this issue Feb 19, 2019
69 tasks
@sleshchenko sleshchenko added kind/enhancement A feature request - must adhere to the feature request template. team/platform labels Feb 19, 2019
@skabashnyuk skabashnyuk added the status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. label Feb 21, 2019
@skabashnyuk
Copy link
Contributor

Do you have an idea of how you want to define it in devfile?

@sleshchenko
Copy link
Member

@l0rd Also it would be nice to see a use-case description here, with an example, motivation, etc.

@sleshchenko
Copy link
Member

sleshchenko commented Feb 21, 2019

As about format, it may be like the following

---
apiVersion: v1
kind: List
items:
- apiVersion: v1
  kind: Pod
  metadata:
    name: petclinic
    labels:
      app.kubernetes.io/name: petclinic
      app.kubernetes.io/component: webapp
      app.kubernetes.io/part-of: petclinic
  spec:
    containers:
    - name: server
      image: mariolet/petclinic
    - name: mysql
      image: centos/mysql-57-centos7
specVersion: 0.0.1
name: che-in-che
tools:
  - name: che-dev
    type: kubernetes
    local: .che-dev.yaml
    entrypoints:
      - pod: petclinic \\ pod or deployment name. Is optional if local file contains only one pod/deployment
        container: server \\ cointainer name. Is optional if pod/deployment contains one container
        command:
        arg:

WDYT?

@garagatyi
Copy link

Both pod and container could be optional if local YAML contains just a single pod with a single container which is common practice.

@sleshchenko
Copy link
Member

@garagatyi Makes sense. Fixed the proposal.

@l0rd
Copy link
Contributor Author

l0rd commented Feb 21, 2019

@sleshchenko I have updated the description using your proposal

@sleshchenko
Copy link
Member

sleshchenko commented Feb 22, 2019

@l0rd You specified

      - pod: container #optional 
        container: app #optional

Do you think it should be always optional? What should we do if tool has multiple pods/containers?
Or description should be changed to

      - pod: petclinic \\ pod or deployment name. Is optional if local file contains only one pod/deployment
        container: server \\ cointainer name. Is optional if pod/deployment contains one container

?
Note: This conditional optional is a thing that kubectl/oc clients have for container operation like exec.

@l0rd
Copy link
Contributor Author

l0rd commented Feb 22, 2019

@sleshchenko yeah. I mean, from the point of view of the devfile syntax it should be optional. From a semantics point of view I am perfectly +1 with your proposal and there may be other cases like this one (i.e. if a user specify args but not command in the devfile what should we do? Using common sense I would say that we should consider the devfile valid if there is a command in the linked application yaml, invalid otherwise. But the solution should be simple as well. So it's up to you or whoever is going to implement it to decide on these subtle semantics decisions).

@skabashnyuk skabashnyuk added the severity/P1 Has a major impact to usage or development of the system. label Mar 6, 2019
@metlos
Copy link
Contributor

metlos commented Mar 11, 2019

@l0rd In #12856 I've added a couple more options to filter the containers by namely:

  • add the ability to also filter by deployment in addition to pod.
  • add the ability to use deploymentSelector and podSelector to match the depls/pods by labels.

The thinking behind this is that it is feasible to think that 2 different deployments may have pods with the same generateName and we'd like to add entrypoints to the containers of only one of them. I've added the selectors as well to be in line with the capability we already have to filter out what to apply from a kubernetes list.

As for the missing command or args. I think neither of them is mandatory and can be applied independently. Their usage depends on how the docker image they're used with was built and is summarized in the kubernetes docs: https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes

So all in all, the entrypoint can look like this:

entrypoints:
  - deployment: che
    pod: che-host
    container: che
    command: ['sleep', 'infinity']
  - deploymentSelector:
      app: che
    pod: postgres
    args: ['-h', '0.0.0.0']
  - podSelector:
       ....

Still, it is possible to use the simplied version of:

endpoints:
- command: ['sleep', 'infinity']

in which case the command would be applied to all containers in the kubernetes list. I specifically chose not to throw any kind of error if the "filter" matches more than 1 container because that could actually be what user wanted.

Do you think that is a reasonable thing to do?

@l0rd
Copy link
Contributor Author

l0rd commented Mar 11, 2019

That makes sense and is very exhaustive! Great job @metlos. When you are done with the PR please remember to add those bits in the devfile documentation or all your great work will be difficult to use.

@metlos metlos removed the status/info-needed More information is needed before the issue can move into the “analyzing” state for engineering. label Mar 12, 2019
@metlos
Copy link
Contributor

metlos commented Mar 14, 2019

So after a lengthy discussion with @sleshchenko that cleared up some of my misconceptions about the kubernetes model and the complexity that can or cannot arise, we arrived at a simpler model that I would like to agree on.

It is very similar to the originally agreed upon proposal #12693 (comment) but has clarified semantics and in our opinion clearer naming.

The most important finding is that it doesn't make sense to apply the selectors and name matching on any nested objects, just on the top level ones in the list (lists cannot nest and we don't handle lists of templates yet). If we'd like to for example match on labels of a pod template of a deployment (i.e. on a nested object), it is equivalent to matching on the name of the parent deployment (which needs to be unique). This is actually the only "drawback" of the proposal - a single declaration that would match on the pod template labels will now have to be repeated for each deployment (unless the same could be achieved by matching on the parent deployment's labels, which seems like a reasonable scenario).

So the simplified matching model looks like this:

entrypoints:
- parentName: <the name of the top level object to consider when looking for containers>
  parentSelector: <selector for the labels of the top level object>
  containerName: <the name of the container to apply the entrypoint to>
  command: <the command>
  args: <the args>
- ...

Of course, the simpliest version still is:

entrypoints:
- command: ...

@l0rd wdyt?

@l0rd
Copy link
Contributor Author

l0rd commented Mar 15, 2019

👍

@metlos
Copy link
Contributor

metlos commented Mar 20, 2019

PR #12856 has been merged.

@metlos metlos closed this as completed Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

5 participants