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

Allow users to define custom port-mappings for port forwarding [Kubernetes] #6704

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Mar 31, 2023

What type of PR is this:
/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes part of #6479

PR acceptance criteria:

  • Unit test

  • Integration test [WIP]

  • Documentation [TBD]

How to test changes / Special notes to the reviewer:

  1. odo dev --port-forward <localPort>:<containerPort> --port-forward <localPort>:<containerName>:<containerPort>

@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
@netlify
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 406e453
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/642ea7626f618a0008633829
😎 Deploy Preview https://deploy-preview-6704--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 the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 31, 2023
@openshift-ci openshift-ci bot requested review from kadel and rm3l March 31, 2023 11:57
@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

Windows Tests (OCP) on commit 804e410 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@valaparthvi valaparthvi force-pushed the 6479-custom-port-mapping-for-port-forwarding branch from a33e0dc to a3d667e Compare March 31, 2023 12:20
@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

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

@odo-robot
Copy link

odo-robot bot commented Mar 31, 2023

OpenShift Tests on commit 804e410 finished with errors.
View logs: TXT HTML

@valaparthvi valaparthvi changed the title [WIP]Allow users to define custom port-mappings for port forwarding [Kubernetes] Allow users to define custom port-mappings for port forwarding [Kubernetes] Apr 3, 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 3, 2023
@valaparthvi valaparthvi changed the title Allow users to define custom port-mappings for port forwarding [Kubernetes] WIP: Allow users to define custom port-mappings for port forwarding [Kubernetes] Apr 3, 2023
@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 Apr 3, 2023
@odo-robot
Copy link

odo-robot bot commented Apr 3, 2023

Kubernetes Tests on commit 804e410 finished with errors.
View logs: TXT HTML

@valaparthvi valaparthvi changed the title WIP: Allow users to define custom port-mappings for port forwarding [Kubernetes] Allow users to define custom port-mappings for port forwarding [Kubernetes] Apr 3, 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 3, 2023
@odo-robot
Copy link

odo-robot bot commented Apr 3, 2023

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

@valaparthvi valaparthvi changed the title Allow users to define custom port-mappings for port forwarding [Kubernetes] WIP: Allow users to define custom port-mappings for port forwarding [Kubernetes] Apr 3, 2023
@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 Apr 3, 2023
@valaparthvi valaparthvi force-pushed the 6479-custom-port-mapping-for-port-forwarding branch from ffa7662 to 3cf0901 Compare April 3, 2023 17:41
@valaparthvi valaparthvi force-pushed the 6479-custom-port-mapping-for-port-forwarding branch 2 times, most recently from 9212408 to e7868e7 Compare April 4, 2023 08:36
@valaparthvi
Copy link
Contributor Author

windows-integration-test/Windows-test failure is due to same random port returned twice, although I am not sure why. The seed used should've made sure we get a unique port, but it did not here.

  Running odo.exe with args [odo dev --debug --port-forward=35507:3000 --port-forward=35507:5858] and odo env: [ODO_LOG_LEVEL=4 ODO_TRACKING_CONSENT=no]

[odo]  X  There are following issues with values provided by --port-forward flag:
  [odo]  X  values for --port-forward flag are invalid


  [FAILED] Timed out after 360.000s.
  Expected
      <string>: 	- local port 35507 is used more than once, please use unique local ports
      
  to contain substring
      <string>: [Ctrl+c] - Exit

@valaparthvi valaparthvi force-pushed the 6479-custom-port-mapping-for-port-forwarding branch from e7868e7 to 4bfe926 Compare April 4, 2023 10:01
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>
…hen debugging

Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>

Remove the use of seed for randomzing

Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi valaparthvi force-pushed the 6479-custom-port-mapping-for-port-forwarding branch from 7bd97ec to 861a9f4 Compare April 5, 2023 07:42
Comment on lines 60 to 62
if !util.IsPortFree(int(startPort)) {
return GetRandomFreePort()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure IsPortFree is atomic here. You could just expect the port is free.

The random ports are in the range of the Ephemeral ports, in my machine it is:

$ sysctl net.ipv4.ip_local_port_range
net.ipv4.ip_local_port_range = 32768	60999

So we can consider ports below 32768 will be free.

Signed-off-by: Parthvi Vala <pvala@redhat.com>
@valaparthvi valaparthvi force-pushed the 6479-custom-port-mapping-for-port-forwarding branch from 861a9f4 to 69c994a Compare April 5, 2023 08:42
@feloy feloy closed this Apr 5, 2023
@feloy feloy reopened this Apr 5, 2023
@feloy
Copy link
Contributor

feloy commented Apr 5, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 5, 2023
pkg/odo/cli/dev/dev_test.go Outdated Show resolved Hide resolved
pkg/odo/cli/dev/dev.go Show resolved Hide resolved
pkg/odo/cli/dev/dev.go Outdated Show resolved Hide resolved
pkg/odo/cli/dev/dev.go Outdated Show resolved Hide resolved
pkg/portForward/kubeportforward/portForward_test.go Outdated Show resolved Hide resolved
Co-authored-by: Armel Soro <asoro@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 6, 2023
@valaparthvi
Copy link
Contributor Author

  [FAILED] in [AfterEach] - /go/odo_1/tests/helper/helper_dev.go:211 @ 04/06/23 11:22:15.369
  Deleting project: cmd-dev-test1485ipw
  Running oc with args [oc delete project cmd-dev-test1485ipw --wait=false] and odo env: []
  [oc] project.project.openshift.io "cmd-dev-test1485ipw" deleted
  Setting current dir to: /go/odo_1/tests/integration
  Deleting dir: /tmp/2482955252
  Deleting dir: /tmp/1006440071
  << Timeline

  [FAILED] Timed out after 180.001s.

  Expected process to exit.  It did not.
  In [AfterEach] at: /go/odo_1/tests/helper/helper_dev.go:211 @ 04/06/23 11:22:15.369

  [FAIL] odo dev command tests when Starting a PostgreSQL service when creating local files and dir and running odo dev - with metadata.name [AfterEach] should correctly propagate changes to the container

OpenShift Test failure.

@valaparthvi valaparthvi closed this Apr 6, 2023
@valaparthvi valaparthvi reopened this Apr 6, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 6, 2023
@valaparthvi valaparthvi closed this Apr 6, 2023
@valaparthvi valaparthvi reopened this Apr 6, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 6, 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 3 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

@feloy
Copy link
Contributor

feloy commented Apr 6, 2023

/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
/override OpenShift-Integration-tests/OpenShift-Integration-tests

Flaky e2e tests

 [FAILED] [113.660 seconds]
odo dev command tests port-forwarding for the component with custom port mapping when devfile has single endpoint when running odo dev [BeforeEach] should expose the endpoint on localhost
  [BeforeEach] /go/odo_1/tests/integration/cmd_dev_test.go:746
  [It] /go/odo_1/tests/integration/cmd_dev_test.go:761
[FAILED] [211.978 seconds]
E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
/go/odo_1/tests/e2escenarios/e2e_test.go:337

@openshift-ci
Copy link

openshift-ci bot commented Apr 6, 2023

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

In response to this:

/override Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
/override OpenShift-Integration-tests/OpenShift-Integration-tests

Flaky e2e tests

[FAILED] [113.660 seconds]
odo dev command tests port-forwarding for the component with custom port mapping when devfile has single endpoint when running odo dev [BeforeEach] should expose the endpoint on localhost
 [BeforeEach] /go/odo_1/tests/integration/cmd_dev_test.go:746
 [It] /go/odo_1/tests/integration/cmd_dev_test.go:761
[FAILED] [211.978 seconds]
E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
/go/odo_1/tests/e2escenarios/e2e_test.go:337

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 c82583b into redhat-developer:main Apr 6, 2023
@rm3l rm3l added this to the v3.10.0 🚀 milestone Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants