odo experimental debug port-forward command #2043

girishramnani · 2019-08-23T20:20:50Z

What is the purpose of this change? What does it change?

see $SUBJECT

Need to consider having a debug branch in the openshift/odo repo for merging all debug command PRs
Note: very early PR so that any issue can be caught early.

Was the change discussed in an issue?

fixes #2012

How to test changes?

git clone git@github.com:sclorg/nodejs-ex.git nodetest
cd nodetest
odo create nodejs
odo push
odo experimental debug port-forward

then follow https://code.visualstudio.com/docs/nodejs/nodejs-debugging#_setting-up-an-attach-configuration in VS code
or

node inspect 127.0.0.1:5858

pkg/odo/cli/experimental/debug/portforward.go

girishramnani · 2019-09-12T12:29:18Z

@kadel forgot to add the debug port config in the env variable, will add that ASAP. rest can be tested

pkg/odo/cli/experimental/debug/portforward.go

kadel · 2019-09-12T14:31:20Z

I'm confused. How this can be used?

▶ odo experimental debug port-forward 5858:5858
 ✗  TYPE/NAME and list of ports are required for port-forward
See 'odo experimental debug port-forward -h' for help and examples.

@girishramnani how did you test it? Even running it as oc port-forward doesn't work for me odo experimental debug port-forward pod/java-spring-boot-wzco-app-1-8w44x 49200:49200

kadel · 2019-09-13T09:09:22Z

When a component is created on the cluster (during odo push), the DEBUG_PORT env variable needs to be populated based on DebugPort config

girishramnani · 2019-09-16T07:41:06Z

@kadel added debug_port in the env variable list

amitkrout · 2019-09-16T12:42:06Z

@girishramnani Once the pr is ready i will review and verify it with respect to test.

girishramnani · 2019-09-17T10:09:42Z

@amitkrout @kadel added odo debug tests

pkg/debug/portforward.go

kadel · 2019-09-17T12:09:55Z

pkg/debug/portforward.go

+	k8sgenclioptions "k8s.io/kubernetes/pkg/kubectl/genericclioptions"
+)
+
+type PortForwarder interface {


There should be a comment explaining what is a purpose of this interface

That comment doesn't provide much more clarification than the title itself :-(

will remove the interface

done removed

girishramnani · 2019-09-17T14:24:18Z

@kadel seeing this error on travis CI.

try 0 of 10
Running odo with args [odo experimental debug port-forward --context /tmp/741760523]
error while requesting: Get http://localhost:5858: dial tcp 127.0.0.1:5858: connect: connection refused
[odo] Started port forwarding at ports - 5858:5858
try 1 of 10
[odo] E0917 13:24:54.058928   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:24:54 socat[23961] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 2 of 10
[odo] E0917 13:24:56.071476   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:24:56 socat[23968] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 3 of 10
[odo] E0917 13:24:58.079784   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:24:58 socat[23978] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 4 of 10
[odo] E0917 13:25:00.088447   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:25:00 socat[23988] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 5 of 10
[odo] E0917 13:25:02.100187   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:25:02 socat[24018] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 6 of 10
[odo] E0917 13:25:04.108339   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:25:04 socat[24019] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 7 of 10
error while requesting: Get http://localhost:5858: EOF
[odo] E0917 13:25:06.116314   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:25:06 socat[24023] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
try 8 of 10
[odo] E0917 13:25:08.282519   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:25:08 socat[24030] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
try 9 of 10
[odo] E0917 13:25:10.291034   23919 portforward.go:331] an error occurred forwarding 5858 -> 5858: error forwarding port 5858 to pod 9934d5c300d9ddf11af0f6cc1f3a65d73271c1f789b6c39522719600d44a72c9, uid : exit status 1: 2019/09/17 13:25:10 socat[24031] E connect(5, AF=2 127.0.0.1:5858, 16): Connection refused
error while requesting: Get http://localhost:5858: EOF
Last output from http://localhost:5858: 
Deleting project: dcnhegrckp
Running odo with args [odo project delete dcnhegrckp -f]
[odo] This project contains the following applications, which will be deleted
[odo] Application app
[odo] This application has following components that will be deleted
[odo] component named nodejs-741760523-lzek
[odo] No services / could not get services
[odo]  •  Deleting project dcnhegrckp  ...
[odo]  ✓  Deleting project dcnhegrckp [6s]
[odo]  ✓  Deleted project : dcnhegrckp
Deleting dir: /tmp/741760523

and this is not showing up locally

amitkrout · 2019-09-18T06:43:53Z

/test v4.2-integration

amitkrout · 2019-09-18T06:44:55Z

.travis.yml

      script:
        - ./scripts/oc-cluster.sh
        - make bin
        - sudo cp odo /usr/bin
        - odo login -u developer
        - travis_wait make test-cmd-pref-config
        - travis_wait make test-cmd-url
+        - travis_wait make test-cmd-debug


+1 for name test-cmd-debug

amitkrout · 2019-09-18T06:51:05Z

tests/integration/cmd_debug_test.go

+			helper.CmdShouldPass("odo", "push", "--context", context)
+
+			go func() {
+				helper.CmdShouldRunWithTimeout(20*time.Second, "odo", "debug", "port-forward", "--local-port", "5050", "--context", context)


@girishramnani Can you please explain the background of this command, i mean what it does while executing the debug command with a timeout.

the comment on the function explains it

amitkrout

$ odo debug port-forward -h
Forward a local port to a remote port on the pod where the application is listening for a debugger. 

By default the local port and the remote port will be same but that can be changed using --local-port.

Usage:
  odo debug port-forward [flags]

Examples:
  # Listen on default port on all addresses, forwarding to the default port in the pod
  odo debug port-forward
  
  # Listen on the 5000 port locally, forwarding to default port in the pod
  odo debug port-forward --local-port 5000

Flags:
      [...]
Global Flags:
  [...]

@girishramnani you have only implemented one scenario. Can you implement the missing scenario in integration test.

amitkrout · 2019-09-18T07:11:07Z

tests/integration/cmd_debug_test.go

+	BeforeEach(func() {
+		SetDefaultEventuallyTimeout(10 * time.Minute)
+		SetDefaultConsistentlyDuration(30 * time.Second)
+		context = helper.CreateNewContext()


@girishramnani Update the global config path, like we have done in other test file.

amitkrout · 2019-09-18T07:16:04Z

tests/integration/cmd_debug_test.go

+	// This is before every spec (It)
+	BeforeEach(func() {
+		SetDefaultEventuallyTimeout(10 * time.Minute)
+		SetDefaultConsistentlyDuration(30 * time.Second)


+1, IMO keeping failure timeout won't hurt us though there is no failure command test.

amitkrout

@girishramnani No doc reference found for this feature. Can you please add a doc reference of how to uses it, what are the parameters need to be taken care, how to disable/enable it etc..

amitkrout · 2019-09-18T11:35:52Z

@girishramnani No doc reference found for this feature. Can you please add a doc reference of how to uses it, what are the parameters need to be taken care, how to disable/enable it etc..

#1995 (review)

mik-dass · 2019-09-20T11:47:50Z

pkg/odo/cli/debug/portforward.go

+		}
+	}()
+
+	req := o.Client.BuildPortForwardReq(pod.Name)


same as above

Reasoning same as above

mik-dass · 2019-09-20T11:55:01Z

tests/integration/cmd_debug_test.go

+			}()
+
+			// debug port
+			helper.HttpWaitForWithStatus("http://localhost:5858", "WebSockets request was expected", 12, 5, 400)


some comments here to describe why are we expecting 400 in this test scenario

mik-dass · 2019-09-20T12:00:39Z

pkg/debug/portforward.go

+	if err != nil {
+		return err
+	}
+	transport, upgrader, err := spdy.RoundTripperFor(conf)


maybe some comments explaining the code

The RoundTripperFor has comments to explain it

kadel · 2019-09-20T12:06:13Z

@girishramnani Please refer [1] and [2] for scenario coverage. Use java component in some scenario, i would say use both component in alternate scenario.

We discussed this with @girishramnani. We couldn't come up with easy way how to test this for Java component, we settled down on just NodeJS for now.

The issue was that we don't know how to verify that there actually is a Java debugger on the other side without introducing new dependencies into the tests.
The only thing that we could do now, is to just verify that the port is open.

odo create component java
odo debug port-forward
Try to open tcp connection to the port, to check if it is open.

@amitkrout Do you think that it would make sense to have such test? Not sure if will actually test something :-( Compared to NodeJS one where at least we can verify that there is something using WebSockets

kadel · 2019-09-20T12:10:03Z

/approve

but @mik-dass and @amitkrout still have some good points, that need to be addressed.

Leaving the rest of the discussion up to you @mik-dass and @amitkrout.

openshift-ci-robot · 2019-09-20T12:10:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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~~ [kadel]

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

girishramnani · 2019-09-20T12:16:15Z

I can think about adding java tests in a follow up PR.
Will add the reason of 400 response @mik-dass

amitkrout · 2019-09-20T12:23:41Z

The only thing that we could do now, is to just verify that the port is open.
odo create component java
odo debug port-forward
Try to open tcp connection to the port, to check if it is open.

@girishramnani TBH i was not aware of java test complexity, however as per @kadel suggestion atleast we can add the basic java scenario.

girishramnani · 2019-09-20T12:24:27Z

The only thing that we could do now, is to just verify that the port is open.
odo create component java
odo debug port-forward
Try to open tcp connection to the port, to check if it is open.

@girishramnani TBH i was not aware of java test complexity, however as per @kadel suggestion atleast we can add the basic java scenario.

I would prefer to add that in a follow up PR

amitkrout · 2019-09-20T12:31:43Z

The only thing that we could do now, is to just verify that the port is open.
odo create component java
odo debug port-forward
Try to open tcp connection to the port, to check if it is open.

@girishramnani TBH i was not aware of java test complexity, however as per @kadel suggestion atleast we can add the basic java scenario.

I would prefer to add that in a follow up PR

Anyway way you have some more review comment to address, so in the mean time you can try basic java scenario ;) or else i won't mind if you take this in a very next pr.

girishramnani · 2019-09-20T12:49:33Z

resolved all comments other then adding tests @amitkrout, I will add java tests in a follow up PR

girishramnani · 2019-09-20T12:55:08Z

opened a follow up issue #2153 @amitkrout

girishramnani · 2019-09-20T12:55:21Z

resolved all your comments @mik-dass

mik-dass · 2019-09-20T12:58:52Z

pkg/debug/portforward.go

+	}
+
+	if pod.Status.Phase != corev1.PodRunning {
+		return fmt.Errorf("unable to forward port because pod is not running. Current status=%v", pod.Status.Phase)


if the pod is not running then

[mrinaldas@localhost nodejs-ex]$ odo debug port-forward Started port forwarding at ports - 5858:5858 ✗ unable to forward port because pod is not running. Current status=Pending

the output looks weird as first it says started port forwarding started and then crashes, maybe a helper function for checking and then starting port forward?

nice catch, resolving

mik-dass

/lgtm
works for me, cool stuff 👍

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. size/L labels Aug 23, 2019

openshift-ci-robot requested review from dharmit and mohammedzee1000 August 23, 2019 20:21

girishramnani force-pushed the feature/debug-port-forward branch from a16b0e2 to 405b44b Compare September 10, 2019 20:31

kadel reviewed Sep 11, 2019

View reviewed changes

pkg/odo/cli/experimental/debug/portforward.go Outdated Show resolved Hide resolved

girishramnani marked this pull request as ready for review September 11, 2019 14:05

openshift-ci-robot 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 Sep 11, 2019

girishramnani force-pushed the feature/debug-port-forward branch from b74692c to 8b5fb92 Compare September 12, 2019 08:55

kadel reviewed Sep 12, 2019

View reviewed changes

pkg/odo/cli/experimental/debug/portforward.go Outdated Show resolved Hide resolved

kadel 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 Sep 12, 2019

girishramnani force-pushed the feature/debug-port-forward branch from b64a4de to 2daba90 Compare September 16, 2019 07:21

girishramnani 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 Sep 17, 2019

kadel reviewed Sep 17, 2019

View reviewed changes

pkg/debug/portforward.go Outdated Show resolved Hide resolved

kadel reviewed Sep 17, 2019

View reviewed changes

amitkrout reviewed Sep 18, 2019

View reviewed changes

mik-dass reviewed Sep 20, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 20, 2019

made port forwarder more independent and added comment

2fe51a1

major refactor to resolve mrinal's comments

f68ae8c

openshift-ci-robot added size/XL and removed size/L labels Sep 20, 2019

girishramnani mentioned this pull request Sep 20, 2019

Add java integration tests for debug #2153

Closed

mik-dass reviewed Sep 20, 2019

View reviewed changes

girishramnani added 3 commits September 20, 2019 18:42

moved logging

74d2766

doc typo

e778065

doc typo

56e2e2c

mik-dass approved these changes Sep 20, 2019

View reviewed changes

openshift-ci-robot assigned mik-dass Sep 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 20, 2019

openshift-merge-robot merged commit 8b253d0 into redhat-developer:master Sep 20, 2019

girishramnani mentioned this pull request Sep 23, 2019

Provide debug/port-forward capabilities for the deployment #822

Closed

dharmit mentioned this pull request Sep 26, 2019

Bump odo version to 1.0.0-beta6 #2182

Merged

rm3l added the estimated-size/XL (40-60) Rough sizing for Epics. About 3 sprints of work for a person label Jun 19, 2023

odo experimental debug port-forward command #2043

odo experimental debug port-forward command #2043

Conversation

girishramnani commented Aug 23, 2019 • edited Loading

What is the purpose of this change? What does it change?

Was the change discussed in an issue?

How to test changes?

girishramnani commented Sep 12, 2019

kadel commented Sep 12, 2019 • edited Loading

kadel commented Sep 13, 2019

girishramnani commented Sep 16, 2019

amitkrout commented Sep 16, 2019

girishramnani commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girishramnani commented Sep 17, 2019

amitkrout commented Sep 18, 2019

amitkrout Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitkrout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitkrout left a comment

Choose a reason for hiding this comment

amitkrout commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Sep 20, 2019

kadel commented Sep 20, 2019

openshift-ci-robot commented Sep 20, 2019

girishramnani commented Sep 20, 2019

amitkrout commented Sep 20, 2019

girishramnani commented Sep 20, 2019

amitkrout commented Sep 20, 2019

girishramnani commented Sep 20, 2019

girishramnani commented Sep 20, 2019

girishramnani commented Sep 20, 2019

mik-dass Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

girishramnani commented Aug 23, 2019 •

edited

Loading

kadel commented Sep 12, 2019 •

edited

Loading

amitkrout Sep 18, 2019 •

edited

Loading

mik-dass Sep 20, 2019 •

edited

Loading