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 until expected ports are opened in the container before starting port-forwarding #6701

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Mar 31, 2023

What type of PR is this:
/kind feature
/area dev

What does this PR do / why we need it:
To fix some flaky behavior due to the app sometimes not being ready when sending out requests to the local forwarded ports, this PR leverages the port detector (introduced in #6620 to detect if container ports are bound on the container loopback interface) to wait until the container ports are actually opened before trying to start the port forwarding logic. See #6667 (comment)
For consistency, this is done on both Kubernetes and Podman platforms.

At the moment, this additional check will not prevent odo dev from starting, but a warning will just be displayed if the timeout (arbitrarily set to 1m) is reached and the port is not opened yet in the container, e.g.:

$ odo dev

↪ Running on the cluster in Dev mode
 •  Waiting for Kubernetes resources  ...
 ⚠  Pod is Pending
 ✓  Pod is Running
 ✓  Syncing files into the container [122ms]
 ✓  Building your application in container (command: install) [3s]
 •  Executing the application (command: run)  ...
 ✗  Waiting for the application to be ready [1m]
 ⚠  Port Forwarding might not work correctly: timeout while checking for ports in container "runtime"; ports not listening: 3000: context deadline exceeded
 -  Forwarding from 127.0.0.1:20002 -> 3000

↪ Dev mode
 Status:
 Watching for changes in the current directory /home/asoro/work/tmp/6667-wait-until-expected-ports-are-opened-before-starting-port-forwarding/nodejs

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

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

Run odo dev against any project on either Kubernetes or Podman. A new Waiting for the application to be ready check will be displayed in the output and should timeout if the container port (declared in the Devfile) cannot be opened after 1 minute.

@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 Mar 31, 2023
@openshift-ci
Copy link

openshift-ci bot commented Mar 31, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@netlify
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit d800b94
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6437f745fb45820008b8979b
😎 Deploy Preview https://deploy-preview-6701--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 added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation area/dev Issues or PRs related to `odo dev` labels Mar 31, 2023
@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

Windows Tests (OCP) on commit adb1e94 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@rm3l rm3l force-pushed the 6667-wait-until-expected-ports-are-opened-before-starting-port-forwarding branch from 66a6719 to c205ece Compare March 31, 2023 16:20
@rm3l rm3l force-pushed the 6667-wait-until-expected-ports-are-opened-before-starting-port-forwarding branch from c205ece to 0d71e0e Compare April 11, 2023 08:30
@rm3l rm3l changed the title [WIP] Wait until expected ports are opened in the container before starting port-forwarding Wait until expected ports are opened in the container before starting port-forwarding Apr 11, 2023
@odo-robot
Copy link

odo-robot bot commented Apr 11, 2023

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

@rm3l rm3l marked this pull request as ready for review April 11, 2023 11:26
@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 11, 2023
@openshift-ci openshift-ci bot requested review from feloy and rnapoles-rh April 11, 2023 11:26
@rm3l rm3l requested review from valaparthvi and removed request for rnapoles-rh April 11, 2023 11:26
pkg/devfile/adapters/kubernetes/component/adapter.go Outdated Show resolved Hide resolved
pkg/dev/podmandev/reconcile.go Outdated Show resolved Hide resolved
pkg/port/port.go Outdated Show resolved Hide resolved
@rm3l rm3l requested a review from valaparthvi April 13, 2023 07:29
@rm3l rm3l closed this Apr 13, 2023
@rm3l rm3l reopened this Apr 13, 2023
rm3l added 3 commits April 13, 2023 14:36
…are actually opened and in LISTEN state in specified containers
…ts to forward to are actually opened (or a timeout expires)

This allows to display port-forwarding messages when the application is ready to accept requests.
Otherwise, if the application takes time to listen on the port,
and we try to reach the local forwarded port, port forwarding will
be restarted.
If we are using --random-ports, new random ports will be generated.

Known issue: with the backo-go exponential backoff implementation,
it seems hitting Ctrl-C does not stop waiting =>
we need to wait until the timeout is reached or the backoff is done.
…o forward to are actually opened (or a timeout expires)

This allows to display port-forwarding messages when the application is ready to accept requests.
Otherwise, if the application takes time to listen on the port,
and we try to reach the local forwarded port, port forwarding will
be restarted.
If we are using --random-ports, new random ports will be generated.

Known issue: with the backo-go exponential backoff implementation,
it seems hitting Ctrl-C does not stop waiting =>
we need to wait until the timeout is reached or the backoff is done.
rm3l and others added 8 commits April 13, 2023 14:36
While iterating on the application, it might feel
weird to stop the Dev Session immediately.
Plus, a lot of integration tests are not passing because
of source code not strictly matching the ports declared in the
(too many) Devfiles.
…hy app is not listening on the expected ports

Co-authored-by: Parthvi Vala <pvala@redhat.com>
…e list to the users

Co-authored-by: Parthvi Vala <pvala@redhat.com>
Co-authored-by: Parthvi Vala <pvala@redhat.com>
…stand why app is not listening on the expected ports
…stand why app is not listening on the expected ports
@rm3l rm3l force-pushed the 6667-wait-until-expected-ports-are-opened-before-starting-port-forwarding branch from 87f7548 to d800b94 Compare April 13, 2023 12:36
@rm3l
Copy link
Member Author

rm3l commented Apr 13, 2023

Rebased to fix the issue with Doc tests (#6735).

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

rm3l commented Apr 13, 2023

  [FAILED] Expected
      <*url.Error | 0xc000674690>: {
          Op: "Post",
          URL: "http://127.0.0.1:59167/api/newuser",
          Err: <*errors.errorString | 0xc00008a130>{s: "EOF"},
      }
  to be nil
  In [It] at: C:/Users/Administrator.ANSIBLE-TEST-VS/3688/tests/e2escenarios/e2e_test.go:319 @ 04/13/23 08:07:55.699

  There were additional failures detected.  To view them in detail run ginkgo -vv
------------------------------

Summarizing 1 Failure:
  [FAIL] E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
  C:/Users/Administrator.ANSIBLE-TEST-VS/3688/tests/e2escenarios/e2e_test.go:319

Flaky E2E test (#6582)

/override windows-integration-test/Windows-test

@openshift-ci
Copy link

openshift-ci bot commented Apr 13, 2023

@rm3l: Overrode contexts on behalf of rm3l: windows-integration-test/Windows-test

In response to this:

 [FAILED] Expected
     <*url.Error | 0xc000674690>: {
         Op: "Post",
         URL: "http://127.0.0.1:59167/api/newuser",
         Err: <*errors.errorString | 0xc00008a130>{s: "EOF"},
     }
 to be nil
 In [It] at: C:/Users/Administrator.ANSIBLE-TEST-VS/3688/tests/e2escenarios/e2e_test.go:319 @ 04/13/23 08:07:55.699

 There were additional failures detected.  To view them in detail run ginkgo -vv
------------------------------

Summarizing 1 Failure:
 [FAIL] E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
 C:/Users/Administrator.ANSIBLE-TEST-VS/3688/tests/e2escenarios/e2e_test.go:319

Flaky E2E test (#6582)

/override windows-integration-test/Windows-test

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 rm3l closed this Apr 13, 2023
@rm3l rm3l reopened this Apr 13, 2023
@sonarqubecloud
Copy link

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 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rm3l
Copy link
Member Author

rm3l commented Apr 13, 2023

Running oc.exe with args [oc get pods --namespace cmd-dev-test1671gjx --selector=component=tvztql -o jsonpath={.items[*].metadata.name}] and odo env: []
  [oc] tvztql-app-7546c66cf7-znfzwRunning oc.exe with args [oc exec tvztql-app-7546c66cf7-znfzw --namespace cmd-dev-test1671gjx -- ls -lai /apps/webapp] and odo env: []
  [oc] error: error sending request: Post "[https://c115-e.eu-de.containers.cloud.ibm.com:31700/api/v1/namespaces/cmd-dev-test1671gjx/pods/tvztql-app-7546c66cf7-znfzw/exec?command=ls&amp;command=-lai&amp;command=%2Fapps%2Fwebapp&amp;container=runtime&amp;stderr=true&amp;stdout=true&quot;:](https://c115-e.eu-de.containers.cloud.ibm.com:31700/api/v1/namespaces/cmd-dev-test1671gjx/pods/tvztql-app-7546c66cf7-znfzw/exec?command=ls&command=-lai&command=%2Fapps%2Fwebapp&container=runtime&stderr=true&stdout=true%22:) dial tcp 161.156.12.82:31700: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
  [FAILED] in [It]

...

Summarizing 1 Failure:
  [FAIL] odo dev command tests when project and clonePath is present in devfile and running odo dev - with metadata.name [It] should sync to the correct dir in container
  C:/Users/Administrator.ANSIBLE-TEST-VS/3692/tests/helper/helper_cmd_wrapper.go:101

Ran 437 of 805 Specs in 1512.399 seconds
FAIL! -- 436 Passed | 1 Failed | 0 Pending | 368 Skipped

Previous run was passing

/override windows-integration-test/Windows-test

@openshift-ci
Copy link

openshift-ci bot commented Apr 13, 2023

@rm3l: Overrode contexts on behalf of rm3l: windows-integration-test/Windows-test

In response to this:

Running oc.exe with args [oc get pods --namespace cmd-dev-test1671gjx --selector=component=tvztql -o jsonpath={.items[*].metadata.name}] and odo env: []
 [oc] tvztql-app-7546c66cf7-znfzwRunning oc.exe with args [oc exec tvztql-app-7546c66cf7-znfzw --namespace cmd-dev-test1671gjx -- ls -lai /apps/webapp] and odo env: []
 [oc] error: error sending request: Post "[https://c115-e.eu-de.containers.cloud.ibm.com:31700/api/v1/namespaces/cmd-dev-test1671gjx/pods/tvztql-app-7546c66cf7-znfzw/exec?command=ls&amp;command=-lai&amp;command=%2Fapps%2Fwebapp&amp;container=runtime&amp;stderr=true&amp;stdout=true&quot;:](https://c115-e.eu-de.containers.cloud.ibm.com:31700/api/v1/namespaces/cmd-dev-test1671gjx/pods/tvztql-app-7546c66cf7-znfzw/exec?command=ls&command=-lai&command=%2Fapps%2Fwebapp&container=runtime&stderr=true&stdout=true%22:) dial tcp 161.156.12.82:31700: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
 [FAILED] in [It]

...

Summarizing 1 Failure:
 [FAIL] odo dev command tests when project and clonePath is present in devfile and running odo dev - with metadata.name [It] should sync to the correct dir in container
 C:/Users/Administrator.ANSIBLE-TEST-VS/3692/tests/helper/helper_cmd_wrapper.go:101

Ran 437 of 805 Specs in 1512.399 seconds
FAIL! -- 436 Passed | 1 Failed | 0 Pending | 368 Skipped

Previous run was passing

/override windows-integration-test/Windows-test

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 72ea3cd into redhat-developer:main Apr 13, 2023
@rm3l rm3l deleted the 6667-wait-until-expected-ports-are-opened-before-starting-port-forwarding branch April 13, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev Issues or PRs related to `odo dev` kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation 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.

Wait until expected ports are opened before starting port-forwarding
3 participants