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

Declare missing debug endpoints #122

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Sep 14, 2022

What does this PR do?:

This adds missing debug endpoints in Devfiles where a debug command is declared and expected to be listening on the debug port (injected via the DEBUG_PORT environment variable).
This is required in tools like odo, so the endpoint can be detected and port-forwarded using odo dev --debug. See redhat-developer/odo#5988
I've tested this in other tools like Dev Spaces, where I noticed that some of the Devfiles used there are already declaring such endpoints, e.g.: https://github.com/devspaces-samples/quarkus-quickstarts/blob/devspaces-3-rhel-8/devfile.yaml#L12-L15

Which issue(s) this PR fixes:

Fixes redhat-developer/odo#5988

PR acceptance criteria:

  • Contributing guide
    Have you read the devfile registry contributing guide and followed its instructions?
  • Test automation
    Does this repository's tests pass with your changes?
  • Documentation
    Does any documentation need to be updated with your changes?
  • Check Tools Provider
    Have you tested the changes with existing tools, i.e. Odo, Che, Console? (See devfile registry contributing guide on how to test changes)

How to test changes / Special notes to the reviewer:

I manually ran the tests/test.sh script (using odo 2.5.1) against both Minikube and OpenShift clusters. I had to adapt the logic that calls odo url create to dynamically find the endpoints ports, instead of hardcoding for some stacks, or letting odo determine the port (which it would not be able to do because we may now have multiple endpoints).

@openshift-ci openshift-ci bot requested review from ajm01 and BethGriggs September 14, 2022 08:11
@openshift-ci
Copy link

openshift-ci bot commented Sep 14, 2022

Hi @rm3l. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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
Copy link
Member Author

rm3l commented Sep 14, 2022

cc @kadel

@kadel
Copy link
Member

kadel commented Sep 14, 2022

/ok-to-test

@rm3l rm3l changed the title Declare missing endpoint for debug port Declare missing debug endpoints Sep 14, 2022
@rm3l rm3l force-pushed the add_debug_endpoints branch from 366c6d0 to adca148 Compare September 15, 2022 08:14
@rm3l
Copy link
Member Author

rm3l commented Sep 15, 2022

Any idea why the ci/prow/v4.7-registry-test check is not passing? I keep seeing this in the logs, but can't figure out why:

ERRO[2022-09-15T09:05:03Z] Some steps failed:                           
ERRO[2022-09-15T09:05:03Z] 
  * could not run steps: step registry-test failed: "registry-test" test steps failed: "registry-test" pod "registry-test-registry-test-steps" failed: the pod ci-op-xfv085cw/registry-test-registry-test-steps failed after 9s (failed containers: test): ContainerFailed one or more containers exited

@rm3l rm3l force-pushed the add_debug_endpoints branch 2 times, most recently from 7a02745 to b30c890 Compare September 22, 2022 20:05
@rm3l rm3l force-pushed the add_debug_endpoints branch from b30c890 to e267bfe Compare October 5, 2022 08:06
@openshift-ci
Copy link

openshift-ci bot commented Oct 5, 2022

@rm3l: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v4.7-registry-test 7a02745 link true /test v4.7-registry-test

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@johnmcollier
Copy link
Member

/retest

@johnmcollier
Copy link
Member

@rm3l can you rebase please when you get the chance?

rm3l added 2 commits October 21, 2022 10:08
This adds missing debug endpoints in Devfiles where a debug command is
declared and expected to be listening on the debug port (injected via
the DEBUG_PORT environment variable).
This is required in tools like odo, so the endpoint can be detected and
port-forwarded using "odo dev --debug".
Also, Che is already having such endpoint for debug as well. See
https://github.com/devspaces-samples/quarkus-quickstarts/blob/devspaces-3-rhel-8/devfile.yaml?rgh-link-date=2022-09-14T08%3A11%3A25Z#L12-L15

Signed-off-by: Armel Soro <asoro@redhat.com>
Signed-off-by: Armel Soro <asoro@redhat.com>
…priate endpoints

Signed-off-by: Armel Soro <asoro@redhat.com>
@rm3l rm3l force-pushed the add_debug_endpoints branch from 742559b to eed66a4 Compare October 21, 2022 08:22
@johnmcollier
Copy link
Member

@ajm01 @BethGriggs Can you please review the stacks you own in this PR that were changed. Thanks

@rm3l
Copy link
Member Author

rm3l commented Oct 24, 2022

Noticed the following errors reported by the Validate Devfile stacks / with odo v3 job:

2022-10-21T15:07:56.1819727Z     Using namespace: lgmqutwl
2022-10-21T15:07:56.1820421Z     Executing: odo init --devfile-path /home/runner/work/registry/registry/stacks/java-vertx/devfile.yaml --starter vertx-configmap-example --name vertx-configmap-example
2022-10-21T15:07:56.1821002Z     Executing: odo dev
2022-10-21T15:07:56.1821311Z     Waiting for odo to setup port-forwarding. Try 1 of 20
2022-10-21T15:07:56.1821620Z     Executing: odo describe component -o json
2022-10-21T15:07:56.1821943Z     Waiting for odo to setup port-forwarding. Try 2 of 20
2022-10-21T15:07:56.1822252Z     Executing: odo describe component -o json
2022-10-21T15:07:56.1822562Z     Waiting for odo to setup port-forwarding. Try 3 of 20
2022-10-21T15:07:56.1822863Z     Executing: odo describe component -o json
2022-10-21T15:07:56.1823154Z     Found ports [{runtime 127.0.0.1 40013 8080} {runtime 127.0.0.1 40014 5858}]
2022-10-21T15:07:56.1823450Z     Waiting for http://127.0.0.1:40013 to return 200. Try 1 of 12
2022-10-21T15:07:56.1823678Z     http://127.0.0.1:40013 returned 200
2022-10-21T15:07:56.1823928Z     Waiting for http://127.0.0.1:40014 to return 200. Try 1 of 12
2022-10-21T15:07:56.1824214Z     Unable to get Get "http://127.0.0.1:40014": EOF. Trying again in 10.000000
2022-10-21T15:07:56.1824489Z     Waiting for http://127.0.0.1:40014 to return 200. Try 2 of 12
2022-10-21T15:07:56.1824766Z     Unable to get Get "http://127.0.0.1:40014": EOF. Trying again in 10.000000
--- Truncated, but this keeps failing until the number of retries is exceeded. This is expected since application is not running in Debug mode  ---

odo v3 port-forwards all endpoints regardless of their exposure (cf. redhat-developer/odo#6119). The problem is that odo_test.go is picking all ports forwarded by odo and testing if each of them is reachable, which is not the case for most debug endpoints added here. They should be reachable only when running the debug command (via odo dev --debug).

@kadel Do you think it makes sense to make odo_test.go consider only endpoints that have the exposure not set to internal or none? I did something similar in this PR in tests/check_odov2.sh (eed66a4). This would allow testing public endpoints of the application.

@kadel
Copy link
Member

kadel commented Oct 25, 2022

@kadel Do you think it makes sense to make odo_test.go consider only endpoints that have the exposure not set to internal or none?

yes that makes sense, I was also considering simply excluding all ports named debug as the test is currently not running applications in debug mode.
I'll create PR with updated tests script

@kadel
Copy link
Member

kadel commented Oct 25, 2022

#139 should address failing odo v3 failing tests.

Signed-off-by: Tomas Kral <tkral@redhat.com>
@rm3l
Copy link
Member Author

rm3l commented Oct 26, 2022

As discussed, I've cherry-picked 27df44b in this PR.
@kadel You can close #139 - thanks.

@openshift-ci openshift-ci bot added the lgtm Looks good to me label Nov 1, 2022
@johnmcollier
Copy link
Member

@BethGriggs @scottkurz @ajm01 just following up one last time for reviews.

If there's no reviews by end of week, we'll merge this PR in

Copy link
Contributor

@scottkurz scottkurz left a comment

Choose a reason for hiding this comment

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

Hi, just noticed this. For Liberty, LGTM. I haven't tried it or followed the conversation, but just on face value it certainly seems to line up with the underlying command: -DdebugPort=${DEBUG_PORT} (for Maven), etc.

@openshift-ci
Copy link

openshift-ci bot commented Nov 1, 2022

@scottkurz: changing LGTM is restricted to collaborators

In response to this:

LGTM. I haven't tried it or followed the conversation, but just on face value it certainly seems to line up with the underlying command: -DdebugPort=${DEBUG_PORT} (for Maven), etc.

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-ci
Copy link

openshift-ci bot commented Nov 5, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BethGriggs, kadel, rm3l, scottkurz
Once this PR has been reviewed and has the lgtm label, please assign johnmcollier for approval by writing /assign @johnmcollier in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@johnmcollier johnmcollier merged commit 8047927 into devfile:main Nov 7, 2022
@rm3l rm3l deleted the add_debug_endpoints branch November 8, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Registry devfiles are incompatible with odo dev --debug
6 participants