Skip to content

ProxyStrategies: code cleanup and improve test coverage. #604

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

Merged
merged 1 commit into from
May 3, 2024

Conversation

jkh52
Copy link
Contributor

@jkh52 jkh52 commented Apr 2, 2024

  • Validate the "no strategies" case (such a server would be non-functional).
  • Improves pkg/server/backend_manager.go code coverage from 72.6% to 86.3%.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 2, 2024
@k8s-ci-robot k8s-ci-robot requested review from ipochi and tallclair April 2, 2024 20:24
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2024
@jkh52
Copy link
Contributor Author

jkh52 commented Apr 2, 2024

Test flake:

--- FAIL: TestProxyDial_RequestCancelled_Concurrent_GRPC (61.92s)
    proxy_test.go:399: Timed out waiting for tunnel to close
    proxy_test.go:426: Agent connections leaked: 
        
        Diff:
        --- metric output does not match expectation; want
        +++ got:
        @@ -2,3 +2,3 @@
         # TYPE konnectivity_network_proxy_agent_open_endpoint_connections gauge
        -konnectivity_network_proxy_agent_open_endpoint_connections 0
        +konnectivity_network_proxy_agent_open_endpoint_connections 1

@jkh52
Copy link
Contributor Author

jkh52 commented Apr 2, 2024

/retest

1 similar comment
@jkh52
Copy link
Contributor Author

jkh52 commented Apr 2, 2024

/retest

@jkh52 jkh52 force-pushed the proxy-strategies branch 2 times, most recently from 3ad8ed5 to 64edc83 Compare April 2, 2024 21:26
@jkh52
Copy link
Contributor Author

jkh52 commented Apr 2, 2024

The linter is failing despite my attempt to fix it (see second commit here).

Note that this PR uses go1.21 feature 'slices.Contains', I think that's the trigger.

@tallclair or @liangyuanpeng - make lint passes on one of my dev machines, but fails checks here. Could the problem be that we need a newer kubekins image? Or, should we just pass it an explicit GO_VERSION >= 1.21?

EDIT: I side-stepped this and moved the lint update to #605

@jkh52 jkh52 force-pushed the proxy-strategies branch 2 times, most recently from ad0e7b9 to c328988 Compare April 5, 2024 18:20
@jkh52
Copy link
Contributor Author

jkh52 commented Apr 5, 2024

Latest snapshot removes the "duplicate" validation per offline discussion with @cheftako.

/assign @cheftako

@jkh52 jkh52 force-pushed the proxy-strategies branch from c328988 to 4f43f97 Compare April 5, 2024 19:03
@jkh52
Copy link
Contributor Author

jkh52 commented Apr 5, 2024

Test flake for "e2e / e2e (dual, v1.28.7)":

Run # output_dir
ERROR: failed to create cluster: failed to list nodes: command "docker ps -a --filter label=io.x-k8s.kind.cluster=kind --format '{{.Names}}'" failed with error: exit status 1

Command Output: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

Stack Trace: 
sigs.k8s.io/kind/pkg/errors.WithStack
	sigs.k8s.io/kind/pkg/errors/errors.go:59
sigs.k8s.io/kind/pkg/exec.(*LocalCmd).Run
...
github.com/spf13/cobra.(*Command).execute
	github.com/spf13/cobra@v1.4.0/command.go:8[56](https://github.com/kubernetes-sigs/apiserver-network-proxy/actions/runs/8574515244/job/23501575878?pr=604#step:5:57)
github.com/spf13/cobra.(*Command).ExecuteC
	github.com/spf13/cobra@v1.4.0/command.go:974
github.com/spf13/cobra.(*Command).Execute
	github.com/spf13/cobra@v1.4.0/command.go:902
sigs.k8s.io/kind/cmd/kind/app.Run
	sigs.k8s.io/kind/cmd/kind/app/main.go:53
sigs.k8s.io/kind/cmd/kind/app.Main
	sigs.k8s.io/kind/cmd/kind/app/main.go:35
main.main
	sigs.k8s.io/kind/main.go:25
runtime.main
	runtime/proc.go:250
runtime.goexit
	runtime/asm_amd64.s:1[59](https://github.com/kubernetes-sigs/apiserver-network-proxy/actions/runs/8574515244/job/23501575878?pr=604#step:5:60)8
Error: Process completed with exit code 1.

/retest

@jkh52 jkh52 force-pushed the proxy-strategies branch from 4f43f97 to 23b64cc Compare April 5, 2024 19:59
@jkh52 jkh52 force-pushed the proxy-strategies branch from 23b64cc to 08e6210 Compare April 9, 2024 00:10
@jkh52
Copy link
Contributor Author

jkh52 commented Apr 9, 2024

Lint error:

pkg/server/backend_manager_test.go:458:21: unusedresult: result of (sigs.k8s.io/apiserver-network-proxy/pkg/server.ProxyStrategy).String call not used (govet)
					tc.input.String()

@jkh52 jkh52 force-pushed the proxy-strategies branch from 08e6210 to 4b7a81e Compare April 9, 2024 00:20
@jkh52 jkh52 force-pushed the proxy-strategies branch from 4b7a81e to f707f94 Compare April 22, 2024 05:10
case ProxyStrategyDefaultRoute:
return "defaultRoute"
}
panic("unhandled ProxyStrategy String()")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to panic we need to have a very clear error/log message first. Some clue about what ps is set to is pretty essential.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jkh52 jkh52 force-pushed the proxy-strategies branch from f707f94 to 64f8c7c Compare May 3, 2024 19:31
@cheftako
Copy link
Contributor

cheftako commented May 3, 2024

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, jkh52

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 21bd11b into kubernetes-sigs:master May 3, 2024
16 checks passed
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants