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

Adds the debug info command #2518

Merged
merged 1 commit into from
Feb 15, 2020

Conversation

mik-dass
Copy link
Contributor

@mik-dass mik-dass commented Jan 20, 2020

What type of PR is this?
Adds the debug info command

/kind feature

What does does this PR do / why we need it:
This PR adds the debug info command. It detects if a debug session is running on the component or not.

Which issue(s) this PR fixes:

Fixes #2015

How to test changes / Special notes to the reviewer:

  • Start a debug port-forward session on a component
  • run debug info on the component from a different terminal window

@openshift-ci-robot 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 kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Jan 20, 2020
@mik-dass mik-dass force-pushed the debug_info branch 3 times, most recently from 6bfc059 to 8ccf903 Compare January 20, 2020 11:26

stopChannel := make(chan int)
go func() {
helper.CmdShouldRunAndTerminate(60*time.Second, stopChannel, "odo", "debug", "port-forward", "--local-port", "9000", "--context", context)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mik-dass +100
Thanks for removing sleep()

@@ -42,6 +42,28 @@ func CmdShouldRunWithTimeout(timeout time.Duration, program string, args ...stri
return string(session.Out.Contents())
}

// CmdShouldRunAndTerminate waits and returns stdout after a closed signal is passed on the closed channel
func CmdShouldRunAndTerminate(timeoutAfter time.Duration, stopChan chan int, program string, args ...string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@amitkrout
Copy link
Contributor

amitkrout commented Jan 22, 2020

Hitting test failure on windows

PS C:\Users\Admin\go\src\github.com\openshift\odo> make test-cmd-debug
The system cannot find the path specified.
'grep' is not recognized as an internal or external command,
operable program or batch file.
ginkgo -v  -randomizeAllSpecs -slowSpecThreshold=120 -timeout 7200s -nodes=1 -focus="odo debug command tests" tests/integration/
Running Suite: Integration Suite
================================
Random Seed: 1579690215 - Will randomize all specs
Will run 1 of 143 specs

SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
------------------------------
[...]
Running odo.exe with args [odo debug port-forward --local-port 9000 --context C:\Users\Admin\AppData\Local\Temp\662339075]
[odo] Started port forwarding at ports - 9000:5858
Running odo.exe with args [odo debug info --context C:\Users\Admin\AppData\Local\Temp\662339075 -v 4]




[odo] I0122 10:51:26.152451    6592 info.go:115] error sending signal 0 to pid 6024, 
cause: not supported by windows




[...]
+ Failure [71.306 seconds]
odo debug command tests
C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:14
  odo debug info should work on a odo component
  C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:69
    should start a debug session and run debug info on a running debug session [It]
    C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:70
    Expected
        <string>: Debug is not running for the component nodejs-cmp-zzikejgtqg
    to contain substring
        <string>: 9000
    C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:84
------------------------------
SSSSSSSSSSSSSSSSSS
Summarizing 1 Failure:
[Fail] odo debug command tests odo debug info should work on a odo component [It] should start a debug session and run debug info on a running debug session
C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:84
[...]

runningString := helper.CmdShouldPass("odo", "debug", "info", "--context", context)
Expect(runningString).To(ContainSubstring("9000"))
Expect(helper.ListFilesInDir(os.TempDir())).To(ContainElement(project + "-app" + "-nodejs-cmp-" + project + "-odo-debug.json"))
stopChannel <- 0
Copy link
Contributor

@amitkrout amitkrout Jan 22, 2020

Choose a reason for hiding this comment

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

[odo] I0122 10:51:26.152451    6592 info.go:115] error sending signal 0 to pid 6024, 
cause: not supported by windows

Details - #2518 (comment)

@mik-dass mik-dass force-pushed the debug_info branch 3 times, most recently from 1776ffb to 60cc770 Compare January 23, 2020 12:05
Expect(runningString).To(ContainSubstring(freePort))
stopChannel <- 0
failString := helper.CmdShouldPass("odo", "debug", "info", "--context", context)
Expect(failString).To(ContainSubstring("not running"))
Copy link
Contributor

Choose a reason for hiding this comment

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

@mik-dass Fails here

Created dir: C:\Users\Admin\AppData\Local\Temp\208489730
Creating a new project: xfvetizwbz
Running odo.exe with args [odo project create xfvetizwbz -w -v4]
[odo]  -  Waiting for project to come up  ...
[odo] I0123 12:54:57.497491    4288 occlient.go:537] Project xfvetizwbz now exists
 V  Waiting for project to come up [2s]
[odo]  V  Project 'xfvetizwbz' is ready for use
[odo]  V  New project created and now using project: xfvetizwbz
Running odo.exe with args [odo component create nodejs:8 nodejs-cmp-xfvetizwbz --project xfvetizwbz --context C:\Users\Admin\AppData\Local\Temp\208489730]
 V  Validating component [158ms]
[odo]
[odo] Please use `odo push` command to create the component with source deployed
Running odo.exe with args [odo push --context C:\Users\Admin\AppData\Local\Temp\208489730]
[odo] Validation
 V  Checking component [258ms]
[odo]
[odo] Configuration changes
[odo]  V  Initializing component
 V  Creating component [741ms]
[odo]
[odo] Pushing to component nodejs-cmp-xfvetizwbz of type local
 V  Checking files for pushing [107ms]
 V  Waiting for component to start [35s]
 V  Syncing files to the component [17s]
 V  Building component [9s]
[odo]  V  Changes successfully pushed to component
Checking http://localhost:52540, for WebSockets request was expected
try 0 of 12
Running odo.exe with args [odo debug port-forward --local-port 52540 --context C:\Users\Admin\AppData\Local\Temp\208489730]
[odo] Started port forwarding at ports - 52540:5858
Running odo.exe with args [odo debug info --context C:\Users\Admin\AppData\Local\Temp\208489730]
[odo] Debug is running for the component on the local port : 52540
[odo]
Running odo.exe with args [odo debug info --context C:\Users\Admin\AppData\Local\Temp\208489730]
[odo] E0123 12:56:03.690735    9500 portforward.go:316] error copying from local connection to remote stream: read tcp6 [::1]:52540->[::1]:52548: wsarecv: An existing connection was forcibly closed by the remote host.
[odo] Debug is running for the component on the local port : 52540
[odo]
Deleting project: xfvetizwbz
Running odo.exe with args [odo project delete xfvetizwbz -f]
[odo] E0123 12:56:04.345777    9500 portforward.go:316] error copying from local connection to remote stream: read tcp6 [::1]:52540->[::1]:52551: wsarecv: An existing connection was forcibly closed by the remote host.
[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-cmp-xfvetizwbz
[odo] No services / could not get services
 V  Deleting project xfvetizwbz [6s]
[odo]  V  Deleted project : xfvetizwbz
Deleting dir: C:\Users\Admin\AppData\Local\Temp\208489730

------------------------------
+ Failure [77.365 seconds]
odo debug command tests
C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:15
  odo debug info should work on a odo component
  C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:70
    should start a debug session and run debug info on a closed debug session [It]
    C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:92

    Expected
        <string>: Debug is running for the component on the local port : 52540


    to contain substring
        <string>: not running

    C:/Users/Admin/go/src/github.com/openshift/odo/tests/integration/cmd_debug_test.go:111

@mik-dass mik-dass force-pushed the debug_info branch 6 times, most recently from 6de4cf4 to bac1c74 Compare January 24, 2020 10:17
@mik-dass mik-dass changed the title [WIP] Adds the debug info command Adds the debug info command Jan 24, 2020
@openshift-ci-robot 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 Jan 24, 2020
@mik-dass
Copy link
Contributor Author

@amitkrout I have added some comments describing the problems with windows. Please have a look.

@amitkrout
Copy link
Contributor

amitkrout commented Feb 6, 2020

@mik-dass Unit test is failing on travis CI - macOS. Seems your UTs changes are failing. I can see the same failure on my local macOS too
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Feb 6, 2020
@amitkrout
Copy link
Contributor

@mik-dass Unit test is failing on travis CI - macOS. Seems your UTs changes are failing. I can see the same failure on my local macOS too
/hold

@mik-dass Can you please handle #2570 in along with your pr.

@mik-dass
Copy link
Contributor Author

mik-dass commented Feb 6, 2020

@mik-dass Can you please handle #2570 in along with your pr.

@amitkrout I will handle that in a different PR

@amitkrout
Copy link
Contributor

@mik-dass Can you please handle #2570 in along with your pr.

@amitkrout I will handle that in a different PR

@mik-dass It is not possible this to handle in a separate pr because the unit test itself is failing due to your current pr changes. we can not merge pr with Travis CI failure. If it is a necessary requirement to separate those then you can remove the failure UTs from your pr only if it does not hurt the core functionality of your pr and create separate issue for it

@mik-dass
Copy link
Contributor Author

mik-dass commented Feb 6, 2020

@mik-dass It is not possible this to handle in a separate pr because the unit test itself is failing due to your current pr changes. we can not merge pr with Travis CI failure. If it is a necessary requirement to separate those then you can remove the failure UTs from your pr only if it does not hurt the core functionality of your pr and create separate issue for it

My PR doesn't introduce #2570 . It was introduced in #2549 (comment). The watch failure is the same I had reported there. Please have a look. So I won't fix #2570 in this PR.

@amitkrout
Copy link
Contributor

amitkrout commented Feb 6, 2020

@mik-dass It is not possible this to handle in a separate pr because the unit test itself is failing due to your current pr changes. we can not merge pr with Travis CI failure. If it is a necessary requirement to separate those then you can remove the failure UTs from your pr only if it does not hurt the core functionality of your pr and create separate issue for it

My PR doesn't introduce #2570 . It was introduced in #2549 (comment). The watch failure is the same reported there. Please have a look. So I won't fix #2570 in this PR.

@mik-dass Thanks for the clarification. However the current issue you are facing is nothing to deal with the error that we were getting here in #2549 (comment). BTW the watch failure you are talking about has been fixed in the same pr just by using the latest stable golang version. Not sure why its again back, lets track that in separate issue - #2570

Current UTs failure -

ok  	github.com/openshift/odo/pkg/config	1.094s
--- FAIL: Test_getDebugInfo (0.02s)
    --- FAIL: Test_getDebugInfo/case_1:_the_debug_file_exists (0.01s)
        info_test.go:317: getDebugInfo() got = {{ } 0    0 0}, want {{OdoDebugInfo v1} 8078 testing-1 app nodejs-ex 5858 9001}
        info_test.go:320: getDebugInfo() got1 = false, want true
FAIL
FAIL	github.com/openshift/odo/pkg/debug	0.181s

@mik-dass
Copy link
Contributor Author

mik-dass commented Feb 6, 2020

@mik-dass Thanks for the clarification. However the current issue you are facing is nothing to deal with the error that we were getting here in #2549 (comment). BTW the watch failure you are talking about has been fixed in the same pr just by using the latest stable golang version

I know. I reported this to you and asked you to block my PR :D

@mik-dass Can you please handle #2570 in along with your pr.

I am just clarifying I won't fix this flake in this PR as this has nothing to do with the current PR.

@mik-dass
Copy link
Contributor Author

[odo]  ✗  Failed To Update Config To Component Deployed

/retest

@mik-dass mik-dass force-pushed the debug_info branch 2 times, most recently from ec539fb to 1b83b7b Compare February 11, 2020 10:57
@mik-dass
Copy link
Contributor Author

ok  	github.com/openshift/odo/pkg/config	1.094s
--- FAIL: Test_getDebugInfo (0.02s)
    --- FAIL: Test_getDebugInfo/case_1:_the_debug_file_exists (0.01s)
        info_test.go:317: getDebugInfo() got = {{ } 0    0 0}, want {{OdoDebugInfo v1} 8078 testing-1 app nodejs-ex 5858 9001}
        info_test.go:320: getDebugInfo() got1 = false, want true
FAIL
FAIL	github.com/openshift/odo/pkg/debug	0.181s

@amitkrout fixed now.
/unhold

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Feb 11, 2020
@kadel
Copy link
Member

kadel commented Feb 14, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 14, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit df2a46a into redhat-developer:master Feb 15, 2020
@mik-dass mik-dass deleted the debug_info branch February 18, 2020 05:56
@rm3l rm3l added the estimated-size/XL (40-60) Rough sizing for Epics. About 3 sprints of work for a person label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. estimated-size/XL (40-60) Rough sizing for Epics. About 3 sprints of work for a person 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.

add odo debug info command
8 participants