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

Support exec command #6579

Merged
merged 30 commits into from
Mar 3, 2023

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Feb 8, 2023

What type of PR is this:

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #5701

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

  1. git clone https://github.com/kadel/demo-odo-springboot.git
  2. git checkout d5956f5
  3. Copy the following content in the devfile.yaml
devfile.yaml
schemaVersion: 2.2.0
metadata:
  name: demo
variables:
  APP_IMAGE: quay.io/tkral/devbox-demo-app
  DEV_IMAGE: quay.io/tkral/devbox-demo-devbox

components:
  - container:
      endpoints:
        - name: 8080-tcp
          targetPort: 8080
      env:
        - name: DEBUG_PORT
          value: "5858"
      image: "{{DEV_IMAGE}}"
      mountSources: true
    name: tools
  - container:
      image: golang
    name: pi
  - name: dev-image
    image:
      imageName: "{{DEV_IMAGE}}"
      dockerfile:
        uri: ./devcontainer/Dockerfile
        buildContext: ${PROJECT_SOURCE}

  - name: app-image
    image:
      imageName: "{{APP_IMAGE}}"
      dockerfile:
        uri: ./src/main/docker/Dockerfile
        buildContext: ${PROJECT_SOURCE}

  - name: resource-app
    kubernetes:
      uri: config/default/app.yaml

  - name: resource-pvc
    kubernetes:
      uri: config/default/pvc.yaml

  - name: resource-service
    kubernetes:
      uri: config/default/service.yaml

  - name: resource-route
    kubernetes:
      uri: config/local/route.yaml

commands:
  - exec:
      commandLine: mvn package -Dmaven.test.skip=true
      component: tools
      group:
        isDefault: true
        kind: build
      workingDir: ${PROJECT_SOURCE}
    id: build

  - exec:
      commandLine: mvn spring-boot:run -Dspring-boot.run.profiles=postgres
      component: tools
      hotReloadCapable: true
      group:
        isDefault: true
        kind: run
      workingDir: ${PROJECT_SOURCE}
    id: run

  - exec:
      commandLine:
        java -Xdebug -Xrunjdwp:server=y,transport=dt_socket,address=${DEBUG_PORT},suspend=n
        -jar target/*.jar
      component: tools
      group:
        isDefault: true
        kind: debug
      workingDir: ${PROJECT_SOURCE}
    id: debug

  - exec:
      commandLine: helm repo add bitnami https://charts.bitnami.com/bitnami && helm install my-db bitnami/postgresql
      component: tools
    id: deploy-db
  - exec:
      commandLine: go version
      component: pi
    id: deploy-pi-db
  - apply:
      component: resource-app
    id: deploy-app

  - apply:
      component: resource-pvc
    id: deploy-pvc

  - apply:
      component: resource-service
    id: deploy-service

  - apply:
      component: resource-route
    id: deploy-route

  - apply:
      component: app-image
    id: app-image

  - composite:
      commands:
        # - app-image
        # - deploy-pvc
        # - deploy-service
        # - deploy-route
        # - deploy-app
        - deploy-pi-db
        - deploy-db
      group:
        isDefault: true
        kind: deploy
    id: deploy
  1. odo deploy

The first task(deploy-pi-db) should execute successfully, the second(deploy-db) should fail because of incorrect ServiceAccount setup. In both cases a log should be printed and the job should be deleted in 60s after its completion.
There is a third case where if the job continues to run after 60s minute, log/error will be printed after every 60s which will continue until the user interrupts. This can be tested by changing the image for container comp pi to geolandg or some non-existent image.

@netlify
Copy link

netlify bot commented Feb 8, 2023

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit c54a522
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/640094118b0f8a00083db4cd
😎 Deploy Preview https://deploy-preview-6579--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot requested review from anandrkskd and rm3l February 8, 2023 18:56
@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

OpenShift Unauthenticated Tests on commit fe15d56 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

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

@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

NoCluster Tests on commit fe15d56 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

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

@valaparthvi valaparthvi changed the title Deploy exec Support exec command Feb 8, 2023
@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

Kubernetes Tests on commit fe15d56 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

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

@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

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

@odo-robot
Copy link

odo-robot bot commented Feb 8, 2023

Kubernetes Docs Tests on commit 3adf8e5 finished successfully.
View logs: TXT HTML

pkg/deploy/deploy.go Outdated Show resolved Hide resolved
pkg/deploy/deploy.go Outdated Show resolved Hide resolved
pkg/kclient/interface.go Outdated Show resolved Hide resolved
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

A few comments after testing it.

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Feb 16, 2023

  • after 1 minute, we advise the user to look at odo logs --deploy --follow
  • display the last 100 lines of the output if the execution fails to be consistent with odo deploy.

@valaparthvi valaparthvi changed the title Support exec command WIP: Support exec command Feb 16, 2023
@openshift-ci 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 Feb 16, 2023
@valaparthvi valaparthvi reopened this Feb 17, 2023
@valaparthvi valaparthvi changed the title WIP: Support exec command Support exec command Feb 17, 2023
@openshift-ci 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 Feb 17, 2023
@valaparthvi valaparthvi changed the title Support exec command WIP: Support exec command Feb 17, 2023
@openshift-ci 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 Feb 17, 2023
@valaparthvi valaparthvi changed the title WIP: Support exec command Support exec command Feb 17, 2023
@openshift-ci 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 Feb 17, 2023
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
…re that

Signed-off-by: Parthvi Vala <pvala@redhat.com>
…nto account

Signed-off-by: Parthvi Vala <pvala@redhat.com>
…le pkg

Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
…unc doc and gofmt the files

Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi valaparthvi changed the title WIP: Support exec command Support exec command Mar 2, 2023
@openshift-ci 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 Mar 2, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@feloy
Copy link
Contributor

feloy commented Mar 2, 2023

When I execute a long-running command, and try to get the logs, as documented in the output of odo deploy, I don't get anything:

$ odo logs --deploy
no containers running in the specified mode for the component "my-nodejs-2"

I'm not sure if it is a problem on odo logs side, or if some labels are not set on the Job or on the Pod Template of the Job?

@valaparthvi
Copy link
Contributor Author

When I execute a long-running command, and try to get the logs, as documented in the output of odo deploy, I don't get anything:

$ odo logs --deploy
no containers running in the specified mode for the component "my-nodejs-2"

I'm not sure if it is a problem on odo logs side, or if some labels are not set on the Job or on the Pod Template of the Job?

Are you running odo logs after odo deploy finishes or while it is running? If you run it before or after odo deploy is run, odo logs is bound to throw the no containers error. If that is not the case, can you share the devfile you're running?

@feloy
Copy link
Contributor

feloy commented Mar 2, 2023

When I execute a long-running command, and try to get the logs, as documented in the output of odo deploy, I don't get anything:

$ odo logs --deploy
no containers running in the specified mode for the component "my-nodejs-2"

I'm not sure if it is a problem on odo logs side, or if some labels are not set on the Job or on the Pod Template of the Job?

Are you running odo logs after odo deploy finishes or while it is running? If you run it before or after odo deploy is run, odo logs is bound to throw the no containers error. If that is not the case, can you share the devfile you're running?

I'm running odo logs while the job is running. I've used the devfile used for the integration test (devfile-deploy-exec.yaml), adding sleep 60 after the echo Hello world command:

commands:
  - exec:
      commandLine: npm install
      component: runtime
      group:
        isDefault: true
        kind: build
      workingDir: /project
    id: install
  - exec:
      commandLine: npm start
      component: runtime
      group:
        isDefault: true
        kind: run
      workingDir: /project
    id: run
  - exec:
      commandLine: npm run debug
      component: runtime
      group:
        isDefault: true
        kind: debug
      workingDir: /project
    id: debug
  - exec:
      commandLine: npm test
      component: runtime
      group:
        isDefault: true
        kind: test
      workingDir: /project
    id: test
  - exec:
      commandLine: echo Hello world; sleep 65; echo Bye
      component: runtime
    id: deploy-exec
  - id: deploy
    composite:
      commands:
        - deploy-exec
      group:
        kind: deploy
        isDefault: true
components:
  - container:
      endpoints:
        - name: http-3000
          targetPort: 3000
      image: registry.access.redhat.com/ubi8/nodejs-14:latest
      memoryLimit: 1024Mi
      mountSources: true
      sourceMapping: /project
    name: runtime
metadata:
  description: Stack with Node.js 14
  displayName: Node.js Runtime
  icon: https://nodejs.org/static/images/logos/nodejs-new-pantone-black.svg
  language: javascript
  name: nodejs-prj1-api-abhz
  projectType: nodejs
  tags:
    - NodeJS
    - Express
    - ubi8
  version: 1.0.1
schemaVersion: 2.2.0
starterProjects:
  - git:
      remotes:
        origin: https://github.com/odo-devfiles/nodejs-ex.git
    name: nodejs-starter
variables:
  CONTAINER_IMAGE: quay.io/unknown-account/myimage

@valaparthvi
Copy link
Contributor Author

I cannot seem to reproduce this.
Screenshot from 2023-03-03 14-29-41

@feloy
Copy link
Contributor

feloy commented Mar 3, 2023

I cannot seem to reproduce this.

My bad, I cannot reproduce it either today.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 3, 2023
@feloy
Copy link
Contributor

feloy commented Mar 3, 2023

Thanks @valaparthvi for this work

@feloy
Copy link
Contributor

feloy commented Mar 3, 2023

[FAILED] [33.509 seconds]
odo dev command tests
/go/odo_1/tests/integration/cmd_dev_test.go:34
  when a component is bootstrapped and pushed
  /go/odo_1/tests/integration/cmd_dev_test.go:64
    when a delay is necessary for the component to start and running odo dev
    /go/odo_1/tests/integration/cmd_dev_test.go:494
      [It] should first fail then succeed querying endpoint
      /go/odo_1/tests/integration/cmd_dev_test.go:512

Unrelated failing test
/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

@openshift-ci
Copy link

openshift-ci bot commented Mar 3, 2023

@feloy: Overrode contexts on behalf of feloy: Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

In response to this:

[FAILED] [33.509 seconds]
odo dev command tests
/go/odo_1/tests/integration/cmd_dev_test.go:34
 when a component is bootstrapped and pushed
 /go/odo_1/tests/integration/cmd_dev_test.go:64
   when a delay is necessary for the component to start and running odo dev
   /go/odo_1/tests/integration/cmd_dev_test.go:494
     [It] should first fail then succeed querying endpoint
     /go/odo_1/tests/integration/cmd_dev_test.go:512

Unrelated failing test
/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests

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-merge-robot openshift-merge-robot merged commit 775adfd into redhat-developer:main Mar 3, 2023
anandrkskd pushed a commit to anandrkskd/odo that referenced this pull request Mar 7, 2023
* Support exec command for deploy

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Print log after timeout

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Add helper function to form proper commandLine

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Mockgen kclient

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Enhance error message

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Attempt at fixing unit test failures

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Rename import v1 to batchv1

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Remove TODOs

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Add integration tests and cleanup on user interrupt

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Temp changes

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Log tip to run odo logs after a minute

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* List components to delete even if there are no devfile resources

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Fix integration tests

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Fix deploy exec delete integration test

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Temp Change

* Fix delete command tests

* Fix mockgen client

* Fix validation errors

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Fix unit test failure

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Attemp at writing less flaky integration test

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Remove TODOs

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Add tip after 1 minute and return the go routine if job finishes before that

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Use the container as it is so that container-overrides can be taken into account

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Move job spec code to a different helper function inside the libdevfile pkg

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Modify the Execute method to use the new helper function and refactoring

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Attempt at fixing integration and unit tests

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Move defer to print remaining resources to a separate function, fix func doc and gofmt the files

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Fix test failures

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Cleanup

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Cleanup unused functions

Signed-off-by: Parthvi Vala <pvala@redhat.com>

---------

Signed-off-by: Parthvi Vala <pvala@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support exec command for deploy
4 participants