Skip to content

[WIP] Port forwarding using client-go#1538

Closed
slinkydeveloper wants to merge 7 commits into
knative:masterfrom
slinkydeveloper:serious_port_forwarding
Closed

[WIP] Port forwarding using client-go#1538
slinkydeveloper wants to merge 7 commits into
knative:masterfrom
slinkydeveloper:serious_port_forwarding

Conversation

@slinkydeveloper
Copy link
Copy Markdown
Contributor

@slinkydeveloper slinkydeveloper commented Jul 22, 2020

Removed code to perform port forwarding using an external process and replaced with code to perform port forwarding directly with client-go, included with proper shutdown and logging

Fixes #1536

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 22, 2020
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release labels Jul 22, 2020
@slinkydeveloper slinkydeveloper mentioned this pull request Jul 22, 2020
Closed
evankanderson
evankanderson reviewed Jul 22, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

A few notes; I'm assuming that overall we expect this to be somewhat faster with better cleanup because we aren't managing processes?

Comment thread test/monitoring/monitoring.go
func PortForward(logf logging.FormatLogger, config *rest.Config, clientSet *kubernetes.Clientset, pod *v1.Pod, localPort, remotePort int) (chan struct{}, error) {
req := clientSet.RESTClient().Post().Resource("pods").Namespace(pod.Namespace).Name(pod.Name).SubResource("portforward")
portForwardUrl := req.URL()
if !strings.HasPrefix(portForwardUrl.Path, "/api/v1") {
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jul 22, 2020

Choose a reason for hiding this comment

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

??? This seems like it deserves a comment at least.

Comment thread test/monitoring/monitoring.go
if err != nil {
return 0, fmt.Errorf("failed to port forward: %w", err)
// To close the port forwarding, just close the channel
func PortForward(logf logging.FormatLogger, config *rest.Config, clientSet *kubernetes.Clientset, pod *v1.Pod, localPort, remotePort int) (chan struct{}, error) {
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jul 22, 2020

Choose a reason for hiding this comment

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

The previous version took a *v1.PodList". (I also wonder if passing the logger, clientSet, etc would be more cleanly done via injection/context.Context.)

Copy link
Copy Markdown
Contributor Author

@slinkydeveloper slinkydeveloper Jul 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That really didn't made sense, because you port forward one pod at the time. If you want to port forward more pods at the time, i think it's better the callee figures it out managing the various close ch

Comment thread test/logging/logger_writer.go
"sync"
)

type loggerWriter struct {
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jul 22, 2020

Choose a reason for hiding this comment

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

Should these be "prefixedWriter" or "prefixWriter"?

Copy link
Copy Markdown
Contributor Author

@slinkydeveloper slinkydeveloper Jul 24, 2020

Choose a reason for hiding this comment

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

My idea to name it loggerWriter is to clarify that you build this writer from a logger function

Comment thread test/monitoring/monitoring.go
c := exec.Command(parts[0], parts[1:]...) // #nosec
if err := c.Start(); err != nil {
return nil, fmt.Errorf("%s command failed: %w", cmd, err)
transport, upgrader, err := spdy.RoundTripperFor(config)
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jul 22, 2020

Choose a reason for hiding this comment

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

Wow, this really is SPDY: kubernetes/kubernetes#89163

Copy link
Copy Markdown
Contributor Author

@slinkydeveloper slinkydeveloper Jul 24, 2020

Choose a reason for hiding this comment

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

yeah :) i took that code straight from kubectl

@slinkydeveloper slinkydeveloper force-pushed the serious_port_forwarding branch from 6b75fe2 to 9ec82b9 Compare July 24, 2020 06:55
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jul 24, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: slinkydeveloper
To complete the pull request process, please assign grantr
You can assign the PR to them by writing /assign @grantr in a comment when ready.

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

Details 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

@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

slinkydeveloper commented Jul 24, 2020 •
edited
Loading

I'm assuming that overall we expect this to be somewhat faster with better cleanup because we aren't managing processes?

That's the point of this change plus we have the logging

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Jul 24, 2020

The following is the coverage report on the affected files.
Say /test pull-knative-pkg-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/logging/logger_writer.go Do not exist 100.0%
test/prometheus/prometheus.go 31.6% 32.4% 0.9

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2020
@slinkydeveloper
Fix knative#1536
2c84fd9 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Removed usage of cleanup
9f94583 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
More changes
549e36e 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
panic updated
6a59a99 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
More fixing
5e76590 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
change
14b6ded 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper
Another rebase
9472b73 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper force-pushed the serious_port_forwarding branch from fc14798 to 9472b73 Compare August 13, 2020 13:48
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Aug 25, 2020

@slinkydeveloper: PR needs rebase.

Details

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.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2020
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 24, 2020

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 24, 2020
@github-actions github-actions Bot closed this Dec 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evankanderson evankanderson evankanderson left review comments

@chaodaiG chaodaiG Awaiting requested review from chaodaiG

@coryrc coryrc Awaiting requested review from coryrc

Assignees

No one assigned

Labels

area/test-and-release cla: yes Indicates the PR's author has signed the CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Port forward in monitoring should log errors

5 participants

@slinkydeveloper @knative-prow-robot @knative-metrics-robot @evankanderson @googlebot