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

buildPortsToEndpointsMap should use flattened value type #81378

Merged
merged 1 commit into from Aug 16, 2019

Conversation

@tedyu
Copy link
Contributor

commented Aug 13, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
Currently buildPortsToEndpointsMap uses []hostPortPair as value type.
Looking at the calls to this func:

		newEndpoints := flattenValidEndpoints(portsToEndpoints[portname])

We always flatten the value before using it.

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

cc @danwinship
I will use this PR to address your comments from #76737

@k8s-ci-robot k8s-ci-robot requested review from dnardo and freehan Aug 13, 2019

@k8s-ci-robot k8s-ci-robot added sig/network and removed needs-sig labels Aug 13, 2019

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from 0292d83 to 9dbb2b3 Aug 13, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@danwinship
I added TestBuildPortsToEndpointsMap.
After your review, I will port it over to the other test file.

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from 9dbb2b3 to f9b4b87 Aug 14, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@danwinship
I was thinking about reducing duplicate code (buildPortsToEndpointsMap)

One idea is to create a utility class under pkg/proxy/ for this func (and isValidEndpoint).
The two LoadBalancerRR's would call this func from utility class.

Let me know what you think.

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

/test pull-kubernetes-integration

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from f9b4b87 to 5a52b08 Aug 14, 2019

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

One idea is to create a utility class under pkg/proxy/ for this func (and isValidEndpoint).

Yes. There's even pkg/proxy/util/ already

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from 5a52b08 to 04cb590 Aug 14, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@danwinship
I have moved BuildPortsToEndpointsMap to util/utils.go

Please take a look.

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch 2 times, most recently from 002e28e to 1cbf5ac Aug 14, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

/test pull-kubernetes-bazel-test

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@danwinship
All tests passed - including the new test.

pkg/proxy/userspace/roundrobin_test.go Outdated Show resolved Hide resolved
pkg/proxy/userspace/roundrobin_test.go Outdated Show resolved Hide resolved
pkg/proxy/userspace/roundrobin_test.go Outdated Show resolved Hide resolved
pkg/proxy/userspace/roundrobin_test.go Outdated Show resolved Hide resolved
pkg/proxy/util/utils.go Outdated Show resolved Hide resolved
pkg/proxy/util/utils.go Outdated Show resolved Hide resolved
pkg/proxy/util/utils.go Outdated Show resolved Hide resolved
@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

After moving TestValidateWorks and TestBuildPortsToEndpointsMap to pkg/proxy/util/utils_test.go , I got the following:

FAIL	k8s.io/kubernetes/pkg/proxy/util [setup failed]
# k8s.io/kubernetes/pkg/proxy/util
import cycle not allowed in test
package k8s.io/kubernetes/pkg/proxy/util (test)
	imports k8s.io/kubernetes/pkg/proxy
	imports k8s.io/kubernetes/pkg/proxy/util
@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I think the cycle comes from :

func TestBuildPortsToEndpointsMap(t *testing.T) {
	service := proxy.ServicePortName{NamespacedName: types.NamespacedName{Namespace: "testnamespace", Name: "foo"}, Port: "p"}

where ServicePortName is in proxy

I will keep TestBuildPortsToEndpointsMap in roundrobin_test

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from 1cbf5ac to 84de44a Aug 15, 2019

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I think the cycle comes from :
...
where ServicePortName is in proxy

ah, but you don't actually need to use ServicePortName in the test anyway. Just put the name and namespace into endpoints.ObjectMeta directly

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch 2 times, most recently from 35f0cf4 to b2ecea7 Aug 15, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@danwinship
I have moved TestBuildPortsToEndpointsMap to utils_test.go

Please take another look.

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

/test pull-kubernetes-e2e-gce-100-performance

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from b2ecea7 to db784b2 Aug 15, 2019

@tedyu tedyu force-pushed the tedyu:ports-2-endpt branch from db784b2 to 2f67134 Aug 15, 2019

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, tedyu

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 47e78f3 into kubernetes:master Aug 16, 2019

23 checks passed

cla/linuxfoundation tedyu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.