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

Wait when Build command fails #6771

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Apr 25, 2023

What type of PR is this:

/kind code-refactoring

What does this PR do / why we need it:

this PR refactors the pkg/dev package to make more code generic between podman and kubernetes platforms. It changes the behaviour when the Build command fails.

Which issue(s) this PR fixes:

Fixes #6671

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/code-refactoring labels Apr 25, 2023
@netlify
Copy link

netlify bot commented Apr 25, 2023

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 50cee02
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6450b79c4c712f0008d3135b
😎 Deploy Preview https://deploy-preview-6771--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.

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

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

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

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

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

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

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

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

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

Kubernetes Tests on commit 491337b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

OpenShift Tests on commit 491337b finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 25, 2023

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

@odo-robot
Copy link

odo-robot bot commented Apr 26, 2023

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

@feloy feloy force-pushed the refacto/simplify-devfile-adapter-step-2-main branch from abbc747 to dac019c Compare April 26, 2023 14:35
@feloy feloy closed this Apr 27, 2023
@feloy feloy reopened this Apr 27, 2023
@feloy feloy closed this Apr 27, 2023
@feloy feloy reopened this Apr 27, 2023
@feloy feloy force-pushed the refacto/simplify-devfile-adapter-step-2-main branch from dac019c to 20b069a Compare April 28, 2023 07:27
@feloy feloy changed the title [WIP] Simplify devfile adapter [step 2] Wait when Build command fails Apr 28, 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 Apr 28, 2023
@feloy feloy removed the request for review from anandrkskd April 28, 2023 07:58
@feloy feloy requested review from rm3l and valaparthvi and removed request for ritudes April 28, 2023 07:58
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.

It works great overall - thanks for this refactoring!

One thing I just noticed: when the build command fails on Kubernetes, odo dev displays the logs and waits (which is expected); but when I tried pressing p, but it did not retrigger the sync..

$ odo dev --debug --no-watch
  __
 /  \__     Developing using the "my-nodejs" Devfile
 \__/  \    Namespace: default
 /  \__/    odo version: v3.9.0
 \__/

 ⚠  You are using "default" namespace, odo may not work as expected in the default namespace.
 ⚠  You may set a new namespace by running `odo create namespace <name>`, or set an existing one by running `odo set namespace <name>`

↪ Running on the cluster in Dev mode
 •  Waiting for Kubernetes resources  ...
 ⚠  Pod is Pending
 ✓  Pod is Running
 ✓  Syncing files into the container [132ms]
 ✗  Building your application in container (command: install) [57ms]
Exiting...
Error occurred on Push - watch command was unable to push component: unable to exec command [/bin/sh -c cd ${PROJECT_SOURCE} && (echo Exiting... && exit 1 && npm install) 1>>/proc/1/fd/1 2>>/proc/1/fd/2]: error while streaming command: command terminated with exit code 1


↪ Dev mode
 Keyboard Commands:
[Ctrl+c] - Exit and delete resources from the cluster
     [p] - Manually apply local changes to the application on the cluster
p

Nothing happened after pressing p (I tried multiple times at different intervals to check). It works as expected on Podman however.
Is it just me?

@valaparthvi
Copy link
Member

valaparthvi commented Apr 28, 2023

Nothing happened after pressing p (I tried multiple times at different intervals to check). It works as expected on Podman however.
Is it just me?

It works for me on K8s cluster. But were you using a different kind of project? I used the normal starter project to test this. I need to test it a little extensively, I think.

Details

$ odo dev --debug --no-watch
  __
 /  \__     Developing using the "my-nodejs-app" Devfile
 \__/  \    Namespace: default
 /  \__/    odo version: v3.9.0
 \__/

 ⚠  You are using "default" namespace, odo may not work as expected in the default namespace.
 ⚠  You may set a new namespace by running `odo create namespace <name>`, or set an existing one by running `odo set namespace <name>`

↪ Running on the cluster in Dev mode
 •  Waiting for Kubernetes resources  ...
 ⚠  Pod is Pending
 ✓  Pod is Running
 ✓  Syncing files into the container [138ms]
 ✓  Building your application in container (command: install) [1s]
 •  Executing the application (command: debug)  ...
 ✓  Waiting for the application to be ready [1s]
 -  Forwarding from 127.0.0.1:20001 -> 3000

 -  Forwarding from 127.0.0.1:20002 -> 5858


↪ Dev mode
 Keyboard Commands:
[Ctrl+c] - Exit and delete resources from the cluster
     [p] - Manually apply local changes to the application on the cluster
pPushing files...

 •  Waiting for Kubernetes resources  ...
 ✓  Syncing files into the container [65ms]
 ✓  Building your application in container (command: install) [789ms]
 ✗  Finished executing the application (command: debug) [35s]
 •  Executing the application (command: debug)  ...
 ✓  Waiting for the application to be ready [1s]

↪ Dev mode
 Keyboard Commands:
[Ctrl+c] - Exit and delete resources from the cluster
     [p] - Manually apply local changes to the application on the cluster

But, it does not work if I modify the Devfile. I started with an invalid npm install-non command in my devfile, odo stopped when it encountered the error. Then I modified the devfile, and pressed p, but nothing happened.

Details

$ odo dev --debug --no-watch
  __
 /  \__     Developing using the "my-nodejs-app" Devfile
 \__/  \    Namespace: default
 /  \__/    odo version: v3.9.0
 \__/

 ⚠  You are using "default" namespace, odo may not work as expected in the default namespace.
 ⚠  You may set a new namespace by running `odo create namespace <name>`, or set an existing one by running `odo set namespace <name>`

↪ Running on the cluster in Dev mode
 •  Waiting for Kubernetes resources  ...
 ⚠  Pod is Pending
 ✓  Pod is Running
 ✓  Syncing files into the container [131ms]
 ✗  Building your application in container (command: install) [232ms]
Unknown command: "install-non-existent"

To see a list of supported npm commands, run:
  npm help
Error occurred on Push - watch command was unable to push component: unable to exec command [/bin/sh -c cd ${PROJECT_SOURCE} && (npm install-non-existent) 1>>/proc/1/fd/1 2>>/proc/1/fd/2]: error while streaming command: command terminated with exit code 1


↪ Dev mode
 Keyboard Commands:
[Ctrl+c] - Exit and delete resources from the cluster
     [p] - Manually apply local changes to the application on the cluster
pp
p

@@ -0,0 +1,20 @@
commands:
Copy link
Member

Choose a reason for hiding this comment

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

This file and the others seem to have been pushed accidentally.

@feloy
Copy link
Contributor Author

feloy commented May 2, 2023

It works great overall - thanks for this refactoring!

One thing I just noticed: when the build command fails on Kubernetes, odo dev displays the logs and waits (which is expected); but when I tried pressing p, but it did not retrigger the sync..

$ odo dev --debug --no-watch
  __
 /  \__     Developing using the "my-nodejs" Devfile
 \__/  \    Namespace: default
 /  \__/    odo version: v3.9.0
 \__/

 ⚠  You are using "default" namespace, odo may not work as expected in the default namespace.
 ⚠  You may set a new namespace by running `odo create namespace <name>`, or set an existing one by running `odo set namespace <name>`

↪ Running on the cluster in Dev mode
 •  Waiting for Kubernetes resources  ...
 ⚠  Pod is Pending
 ✓  Pod is Running
 ✓  Syncing files into the container [132ms]
 ✗  Building your application in container (command: install) [57ms]
Exiting...
Error occurred on Push - watch command was unable to push component: unable to exec command [/bin/sh -c cd ${PROJECT_SOURCE} && (echo Exiting... && exit 1 && npm install) 1>>/proc/1/fd/1 2>>/proc/1/fd/2]: error while streaming command: command terminated with exit code 1


↪ Dev mode
 Keyboard Commands:
[Ctrl+c] - Exit and delete resources from the cluster
     [p] - Manually apply local changes to the application on the cluster
p

Nothing happened after pressing p (I tried multiple times at different intervals to check). It works as expected on Podman however. Is it just me?

I have made a fix. thanks for the catch

@sonarcloud
Copy link

sonarcloud bot commented May 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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

Thanks for the refactoring and fixes! It works fine now.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 2, 2023
@rm3l
Copy link
Member

rm3l commented May 2, 2023

Nothing happened after pressing p (I tried multiple times at different intervals to check). It works as expected on Podman however.
Is it just me?

It works for me on K8s cluster. But were you using a different kind of project? I used the normal starter project to test this. I need to test it a little extensively, I think.

Yes, the normal case worked fine for me as well. I tested the second case you mentioned below (which was the scope of the related issue): a Devfile with a failing build command..
But it is working now with the latest commits.

Details
But, it does not work if I modify the Devfile. I started with an invalid npm install-non command in my devfile, odo stopped when it encountered the error. Then I modified the devfile, and pressed p, but nothing happened.

Details

@feloy
Copy link
Contributor Author

feloy commented May 2, 2023

/override OpenShift-Integration-tests/OpenShift-Integration-tests

[FAILED] [169.775 seconds]
odo remove binding command tests when the component with binding is bootstrapped (bindingName=my-nodejs-app-cluster-sample-ocp) when odo dev is running when binding is removed [It] should have led odo dev to delete ServiceBinding from the cluster
/go/odo_1/tests/integration/cmd_remove_binding_test.go:71

[...]

[odo] I0502 07:47:27.510696    6779 exec.go:49] ExecuteCommand returned an an err: error while streaming command: error sending request: Post "[https://c100-e.eu-de.containers.cloud.ibm.com:31049/api/v1/namespaces/cmd-remove-binding-test71ook/pods/mynode-app-6f4678f6db-bx42v/exec?command=%2Fbin%2Fsh&amp;command=-c&amp;command=cat+%2Fopt%2Fodo%2F.odo_cmd_run.pid+%7C%7C+true&amp;container=runtime&amp;stderr=true&amp;stdout=true&quot;:](https://c100-e.eu-de.containers.cloud.ibm.com:31049/api/v1/namespaces/cmd-remove-binding-test71ook/pods/mynode-app-6f4678f6db-bx42v/exec?command=%2Fbin%2Fsh&command=-c&command=cat+%2Fopt%2Fodo%2F.odo_cmd_run.pid+%7C%7C+true&container=runtime&stderr=true&stdout=true%22:) dial tcp: lookup c100-e.eu-de.containers.cloud.ibm.com: operation was canceled. for command '[/bin/sh -c cat /opt/odo/.odo_cmd_run.pid || true]'

@openshift-ci
Copy link

openshift-ci bot commented May 2, 2023

@feloy: Overrode contexts on behalf of feloy: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

/override OpenShift-Integration-tests/OpenShift-Integration-tests

[FAILED] [169.775 seconds]
odo remove binding command tests when the component with binding is bootstrapped (bindingName=my-nodejs-app-cluster-sample-ocp) when odo dev is running when binding is removed [It] should have led odo dev to delete ServiceBinding from the cluster
/go/odo_1/tests/integration/cmd_remove_binding_test.go:71

[...]

[odo] I0502 07:47:27.510696    6779 exec.go:49] ExecuteCommand returned an an err: error while streaming command: error sending request: Post "[https://c100-e.eu-de.containers.cloud.ibm.com:31049/api/v1/namespaces/cmd-remove-binding-test71ook/pods/mynode-app-6f4678f6db-bx42v/exec?command=%2Fbin%2Fsh&amp;command=-c&amp;command=cat+%2Fopt%2Fodo%2F.odo_cmd_run.pid+%7C%7C+true&amp;container=runtime&amp;stderr=true&amp;stdout=true&quot;:](https://c100-e.eu-de.containers.cloud.ibm.com:31049/api/v1/namespaces/cmd-remove-binding-test71ook/pods/mynode-app-6f4678f6db-bx42v/exec?command=%2Fbin%2Fsh&command=-c&command=cat+%2Fopt%2Fodo%2F.odo_cmd_run.pid+%7C%7C+true&container=runtime&stderr=true&stdout=true%22:) dial tcp: lookup c100-e.eu-de.containers.cloud.ibm.com: operation was canceled. for command '[/bin/sh -c cat /opt/odo/.odo_cmd_run.pid || true]'

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 191ee6f into redhat-developer:main May 2, 2023
18 checks passed
@feloy feloy mentioned this pull request May 2, 2023
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/refactoring Issues or PRs related to code refactoring 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.

odo dev should exit if the build command fails
4 participants