Add support for composite run/debug commands #5841

rm3l · 2022-06-20T07:52:41Z

What type of PR is this:
/kind feature
/area devfile-spec

What does this PR do / why we need it:
This improves our coverage of the Devfile spec, by supporting composite commands as run or debug commands for odo dev.

Which issue(s) this PR fixes:
Fixes #5054

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:
Example of devfile with composite run commands: https://github.com/rm3l/odo/blob/5054-add-support-for-composite-run-commands/tests/examples/source/devfiles/nodejs/devfileCompositeRunAndDebug.yaml

netlify · 2022-06-20T07:52:46Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`f3bb6cf`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62b5b8388a7521000980ffe5

odo-robot · 2022-06-20T08:00:09Z

Unit Tests on commit 0e1407e finished successfully.
View logs: TXT HTML

odo-robot · 2022-06-20T08:07:48Z

Kubernetes Tests on commit 0e1407e finished successfully.
View logs: TXT HTML

odo-robot · 2022-06-20T08:08:43Z

Windows Tests (OCP) on commit 0e1407e finished successfully.
View logs: TXT HTML

odo-robot · 2022-06-20T08:09:25Z

OpenShift Tests on commit 0e1407e finished successfully.
View logs: TXT HTML

odo-robot · 2022-06-20T08:15:50Z

Validate Tests on commit 0e1407e finished successfully.
View logs: TXT HTML

…'.odo_cmd_<name>.pid' The implementation in kubeexec.go is indeed able to run any kind of commands, not only Devfile-related.

Drop validation rules that prevented from going further with composite run or debug commands.

…point overridden or env vars updated The previous implementation was limited to Exec commands solely. This now makes it easier to support other kind of commands. For example, when handling a composite command, we are now recursively determining the containers (a.k.a components) that would get used by this composite command.

This allows to more accurately determine how the process could be reported.

…ent of lines

…site command

This way, users would be able to display them (and follow them) with `odo logs` or `kubectl logs`

…xited

rm3l · 2022-06-23T15:03:04Z

Or, if you want to test building in a container and running in anorther one, with a shared volume:

Thanks!

It seems that we need to set explicitely the tail -f /dev/null command to some containers

I'll test with this Devfile, but you had to do that only with the containers used by the build command, right?

feloy · 2022-06-23T15:08:22Z

Or, if you want to test building in a container and running in anorther one, with a shared volume:

Thanks!

It seems that we need to set explicitely the tail -f /dev/null command to some containers

I'll test with this Devfile, but you had to do that with the containers used by the build command, right?

Yes, you are right, only for build containers. Here is a more appropriate devfile to illustrate (with 2 sleeperbuild and sleeperrun containers instead of one common one):

commands:
- id: build
  composite:
    parallel: true
    commands:
    - build1
    - build2
    group:
      isDefault: true
      kind: build
- exec:
    commandLine: (echo sleeping for 2 seconds before build; sleep 2; echo done) > /proc/1/fd/1
    component: sleeperbuild
    workingDir: ${PROJECT_SOURCE}
  id: build1
- exec:
    commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go; mv main /artifacts/; echo build done 
    component: builder
    workingDir: ${PROJECT_SOURCE}
  id: build2
- composite:
    parallel: true
    commands:
    - run1
    - run2
    group:
      isDefault: true
      kind: run
  id: run
- exec:
    commandLine: (echo sleeping for 2 seconds before run; sleep 2; echo done) 
    component: sleeperrun
    workingDir: ${PROJECT_SOURCE}
  id: run1
- exec:
    commandLine: ./main 
    component: runtime
    workingDir: /artifacts
  id: run2
components:
- container:
    endpoints:
    - name: http
      targetPort: 8080
    image: quay.io/devfile/golang:latest
    memoryLimit: 1024Mi
    mountSources: false
    volumeMounts:
    - name: artifacts
      path: /artifacts
  name: runtime
- container:
    image: quay.io/devfile/golang:latest
    memoryLimit: 1024Mi
    mountSources: true
    volumeMounts:
    - name: artifacts
      path: /artifacts
    command: ["tail"]
    args: ["-f", "/dev/null"]
  name: builder
- container:
    image: quay.io/devfile/golang:latest
    memoryLimit: 256Mi
    mountSources: false
    command: ["tail"]
    args: ["-f", "/dev/null"]
  name: sleeperbuild
- container:
    image: quay.io/devfile/golang:latest
    memoryLimit: 256Mi
    mountSources: false
  name: sleeperrun
- name: artifacts
  volume:
    ephemeral: true
    size: 1Gi
metadata:
  description: Stack with the latest Go version
  displayName: Go Runtime
  icon: https://raw.githubusercontent.com/devfile-samples/devfile-stack-icons/main/golang.svg
  language: go
  name: my-go-app
  projectType: go
  tags:
  - Go
  version: 1.0.0
schemaVersion: 2.1.0
starterProjects:
- git:
    checkoutFrom:
      revision: main
    remotes:
      origin: https://github.com/devfile-samples/devfile-stack-go.git
  name: go-starter

rm3l · 2022-06-23T15:21:54Z

Or, if you want to test building in a container and running in anorther one, with a shared volume:

Thanks!

It seems that we need to set explicitely the tail -f /dev/null command to some containers

I'll test with this Devfile, but you had to do that with the containers used by the build command, right?

Yes, you are right, only for build containers. Here is a more appropriate devfile to illustrate (with 2 sleeperbuild and sleeperrun containers instead of one common one):

commands:
- id: build
  composite:
    parallel: true
    commands:
    - build1
    - build2
    group:
      isDefault: true
      kind: build
- exec:
    commandLine: (echo sleeping for 2 seconds before build; sleep 2; echo done) > /proc/1/fd/1
    component: sleeperbuild
    workingDir: ${PROJECT_SOURCE}
  id: build1
- exec:
    commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go; mv main /artifacts/; echo build done 
    component: builder
    workingDir: ${PROJECT_SOURCE}
  id: build2
- composite:
    parallel: true
    commands:
    - run1
    - run2
    group:
      isDefault: true
      kind: run
  id: run
- exec:
    commandLine: (echo sleeping for 2 seconds before run; sleep 2; echo done) 
    component: sleeperrun
    workingDir: ${PROJECT_SOURCE}
  id: run1
- exec:
    commandLine: ./main 
    component: runtime
    workingDir: /artifacts
  id: run2
components:
- container:
    endpoints:
    - name: http
      targetPort: 8080
    image: quay.io/devfile/golang:latest
    memoryLimit: 1024Mi
    mountSources: false
    volumeMounts:
    - name: artifacts
      path: /artifacts
  name: runtime
- container:
    image: quay.io/devfile/golang:latest
    memoryLimit: 1024Mi
    mountSources: true
    volumeMounts:
    - name: artifacts
      path: /artifacts
    command: ["tail"]
    args: ["-f", "/dev/null"]
  name: builder
- container:
    image: quay.io/devfile/golang:latest
    memoryLimit: 256Mi
    mountSources: false
    command: ["tail"]
    args: ["-f", "/dev/null"]
  name: sleeperbuild
- container:
    image: quay.io/devfile/golang:latest
    memoryLimit: 256Mi
    mountSources: false
  name: sleeperrun
- name: artifacts
  volume:
    ephemeral: true
    size: 1Gi
metadata:
  description: Stack with the latest Go version
  displayName: Go Runtime
  icon: https://raw.githubusercontent.com/devfile-samples/devfile-stack-icons/main/golang.svg
  language: go
  name: my-go-app
  projectType: go
  tags:
  - Go
  version: 1.0.0
schemaVersion: 2.1.0
starterProjects:
- git:
    checkoutFrom:
      revision: main
    remotes:
      origin: https://github.com/devfile-samples/devfile-stack-go.git
  name: go-starter

Okay, I see. Because currently, we are adding the tail -f /dev/null command only to containers that would be used by run or debug commands, not the Build one:
https://github.com/rm3l/odo/blob/0402a0522719c5843812767334d344ba16a1dc5b/pkg/devfile/adapters/kubernetes/utils/utils.go#L118-L128

It makes sense to me to do the same thing for Build containers, if any. I'll change that as well.

Following the grooming discussion about supporting exec commands for Deploy, I think we should actually do the same thing for all container components, regardless of the kind of the command. What do you think? (But this would be done in the scope of #5701)

feloy · 2022-06-23T16:33:34Z

Following the grooming discussion about supporting exec commands for Deploy, I think we should actually do the same thing for all container components, regardless of the kind of the command. What do you think? (But this would be done in the scope of #5701)

Yes, I think so also

Build commands (which could also be composite ones) might be executed in a container different from the run or debug ones.

…D 1 process streams This way, we can also have them displayed with `odo logs` or `kubectl logs`. This includes Build and Devfile events commands.

…ere not empty This reduces the clutter in case of an error with the command execution.

…ebug commands running in different containers and with a shared volume)

rm3l · 2022-06-24T08:50:06Z

Following discussion during Backlog Grooming call, it could be interesting to have integration tests with several containers, where some steps of run/debug are executed in container1, others in container2

Okay, I'll add that. I was actually manually testing this way. But it makes sense to have this as a test case.

3c14855 adds an integration test case with a more complex Devfile, i.e. with commands running in different containers. Thanks @feloy for the Devfile you sent me, which I took inspiration from.

I also fixed the issue with missing tail -f to build containers, if needed.
Build command logs should also redirected to the PID 1 output, just like we do with the other run/debug commands.
Please take another look when you get a chance.

feloy

/approve

Just a comment about the spinner for the app

pkg/devfile/adapters/kubernetes/component/commandhandler.go

openshift-ci · 2022-06-24T09:01:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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:

~~OWNERS~~ [feloy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…elated command is done

sonarcloud · 2022-06-24T13:13:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

feloy · 2022-06-24T14:24:47Z

/lgtm

feloy · 2022-06-24T14:29:03Z

/override ci/prow/unit

Tests pass on IBM cloud

openshift-ci · 2022-06-24T14:29:08Z

@feloy: Overrode contexts on behalf of feloy: ci/prow/unit

In response to this:

/override ci/prow/unit

Tests pass on IBM cloud

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.

feloy · 2022-06-24T14:52:30Z

/override ci/prow/v4.10-integration-e2e

Tests pass on IBM Cloud

openshift-ci · 2022-06-24T14:52:34Z

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

Tests pass on IBM Cloud

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.

rm3l · 2022-06-24T15:22:47Z

/override ci/prow/unit

openshift-ci · 2022-06-24T15:22:51Z

@rm3l: Overrode contexts on behalf of rm3l: ci/prow/unit

In response to this:

/override ci/prow/unit

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.

feloy · 2022-06-24T16:43:43Z

/override ci/prow/v4.10-integration-e2e

openshift-ci · 2022-06-24T16:43:47Z

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

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.

* Change integration test expectations regarding composite run commands * Add integration test supporting composite debug commands * Rename in-container pid file from '.odo_devfile_cmd_<name>.pid' into '.odo_cmd_<name>.pid' The implementation in kubeexec.go is indeed able to run any kind of commands, not only Devfile-related. * Pave the way to supporting composite run/debug commands Drop validation rules that prevented from going further with composite run or debug commands. * Update logic for determining which containers should have their entrypoint overridden or env vars updated The previous implementation was limited to Exec commands solely. This now makes it easier to support other kind of commands. For example, when handling a composite command, we are now recursively determining the containers (a.k.a components) that would get used by this composite command. * Store the process exit code in the pid file handled by kubeexec.go This allows to more accurately determine how the process could be reported. * Fix issue with util#DisplayLogs potentially not picking the last element of lines * Make sure to execute the Build command only once when running a composite command * Test that the Build command is executed once running a composite command * Make sure not to retrieve the parent default command uselessly * Log the command Id when it is about to be restarted * Redirect build command logs to PID 1 process This way, users would be able to display them (and follow them) with `odo logs` or `kubectl logs` * Handle Errored case when logging information about the process that exited * Make sure to override container entrypoint if needed for Build commands Build commands (which could also be composite ones) might be executed in a container different from the run or debug ones. * Redirect output from commands handled by component.exec_handler to PID 1 process streams This way, we can also have them displayed with `odo logs` or `kubectl logs`. This includes Build and Devfile events commands. * Make remotecmd#ExecuteCommand log stdout or stderr content only if there not empty This reduces the clutter in case of an error with the command execution. * Add integration test with more complex Devfile (composite build/run/debug commands running in different containers and with a shared volume) * Update the 'Executing the application' spinner accordingly when the related command is done

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 20, 2022

openshift-ci bot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. labels Jun 20, 2022

openshift-ci bot requested review from anandrkskd and feloy June 20, 2022 07:52

rm3l removed request for feloy and anandrkskd June 20, 2022 07:53

rm3l marked this pull request as draft June 20, 2022 08:17

rm3l added 7 commits June 21, 2022 10:05

Change integration test expectations regarding composite run commands

38cd1e6

Add integration test supporting composite debug commands

c8b5b24

Rename in-container pid file from '.odo_devfile_cmd_<name>.pid' into …

bd9e2d4

…'.odo_cmd_<name>.pid' The implementation in kubeexec.go is indeed able to run any kind of commands, not only Devfile-related.

Pave the way to supporting composite run/debug commands

5c568b0

Drop validation rules that prevented from going further with composite run or debug commands.

Store the process exit code in the pid file handled by kubeexec.go

b4e83ac

This allows to more accurately determine how the process could be reported.

Fix issue with util#DisplayLogs potentially not picking the last elem…

8595906

…ent of lines

rm3l force-pushed the 5054-add-support-for-composite-run-commands branch from 9b8e50c to 8595906 Compare June 21, 2022 08:32

rm3l changed the title ~~[WIP] Add support for composite run/debug commands~~ Add support for composite run/debug commands Jun 21, 2022

rm3l marked this pull request as ready for review June 21, 2022 09:19

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 21, 2022

openshift-ci bot requested review from dharmit and feloy June 21, 2022 09:19

rm3l requested a review from valaparthvi June 21, 2022 09:19

rm3l added 2 commits June 22, 2022 16:36

Make sure to execute the Build command only once when running a compo…

7f136cd

…site command

Test that the Build command is executed once running a composite command

c63ecf7

rm3l added 2 commits June 23, 2022 16:58

Redirect build command logs to PID 1 process

966b14f

This way, users would be able to display them (and follow them) with `odo logs` or `kubectl logs`

Handle Errored case when logging information about the process that e…

5f63fe3

…xited

rm3l added 4 commits June 23, 2022 21:18

Make sure to override container entrypoint if needed for Build commands

04cb064

Build commands (which could also be composite ones) might be executed in a container different from the run or debug ones.

Redirect output from commands handled by component.exec_handler to PI…

a5b95d9

…D 1 process streams This way, we can also have them displayed with `odo logs` or `kubectl logs`. This includes Build and Devfile events commands.

Make remotecmd#ExecuteCommand log stdout or stderr content only if th…

251e9b4

…ere not empty This reduces the clutter in case of an error with the command execution.

Add integration test with more complex Devfile (composite build/run/d…

3c14855

…ebug commands running in different containers and with a shared volume)

feloy reviewed Jun 24, 2022

View reviewed changes

pkg/devfile/adapters/kubernetes/component/commandhandler.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 24, 2022

Update the 'Executing the application' spinner accordingly when the r…

f3bb6cf

…elated command is done

openshift-ci bot assigned feloy Jun 24, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 24, 2022

openshift-ci bot merged commit 77ff2b8 into redhat-developer:main Jun 24, 2022

rm3l deleted the 5054-add-support-for-composite-run-commands branch December 1, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for composite run/debug commands #5841

Add support for composite run/debug commands #5841

rm3l commented Jun 20, 2022 •

edited

Loading

netlify bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

rm3l commented Jun 23, 2022 •

edited

Loading

feloy commented Jun 23, 2022

rm3l commented Jun 23, 2022

feloy commented Jun 23, 2022

rm3l commented Jun 24, 2022

feloy left a comment

openshift-ci bot commented Jun 24, 2022

sonarcloud bot commented Jun 24, 2022

feloy commented Jun 24, 2022

feloy commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

feloy commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

rm3l commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

feloy commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

Add support for composite run/debug commands #5841

Add support for composite run/debug commands #5841

Conversation

rm3l commented Jun 20, 2022 • edited Loading

netlify bot commented Jun 20, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented Jun 20, 2022 • edited Loading

odo-robot bot commented Jun 20, 2022 • edited Loading

odo-robot bot commented Jun 20, 2022 • edited Loading

odo-robot bot commented Jun 20, 2022 • edited Loading

odo-robot bot commented Jun 20, 2022 • edited Loading

rm3l commented Jun 23, 2022 • edited Loading

feloy commented Jun 23, 2022

rm3l commented Jun 23, 2022

feloy commented Jun 23, 2022

rm3l commented Jun 24, 2022

feloy left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 24, 2022

sonarcloud bot commented Jun 24, 2022

feloy commented Jun 24, 2022

feloy commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

feloy commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

rm3l commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

feloy commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

rm3l commented Jun 20, 2022 •

edited

Loading

netlify bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

odo-robot bot commented Jun 20, 2022 •

edited

Loading

rm3l commented Jun 23, 2022 •

edited

Loading