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

ProxyStrategies: code cleanup and improve test coverage. #604

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 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
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

pkg/server/backend_manager.go Outdated Show resolved Hide resolved
pkg/server/backend_manager_test.go Outdated Show resolved Hide resolved
pkg/server/backend_manager_test.go Outdated Show resolved Hide resolved
@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()

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.

@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