Move service_util endpoints related functions to framework/endpoints/ports.go #77720

jiatongw · 2019-05-10T05:28:34Z

What type of PR is this?

/kind cleanup
/priority backlog

What this PR does / why we need it:

Move service_util.go endpoints related functions to framework/endpoints/ports.go.
Fix references of some methods to use the right package

Reference issue #77197, #76206 (parent issue #75601 )

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-05-10T05:28:41Z

Hi @jiatongw. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jiatongw · 2019-05-10T05:30:18Z

test/e2e/kubectl/kubectl.go

@@ -1072,7 +1072,7 @@ metadata:
 			})
 			validateService := func(name string, servicePort int, timeout time.Duration) {
 				err := wait.Poll(framework.Poll, timeout, func() (bool, error) {
-					endpoints, err := c.CoreV1().Endpoints(ns).Get(name, metav1.GetOptions{})
+					ep, err := c.CoreV1().Endpoints(ns).Get(name, metav1.GetOptions{})
 					if err != nil {


Rename variable endpoints to ep because we need to import endpoints package in this file

jiatongw · 2019-05-10T05:35:35Z

/assign @andrewsykim

neolit123 · 2019-05-10T11:52:46Z

thanks for the PR @jiatongw
/approve
/ok-to-test

jiatongw · 2019-05-10T20:25:41Z

/assign @timothysc

timothysc

minor nits then lgtm

timothysc · 2019-05-13T20:26:54Z

test/e2e/network/service.go

@@ -37,6 +37,7 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/kubernetes/pkg/controller/endpoint"
 	"k8s.io/kubernetes/test/e2e/framework"
+	"k8s.io/kubernetes/test/e2e/framework/endpoints"


given the other naming patterns we should prefix with e2e to avoid collisions with existing libraries.

e2eendpoints is a bit weird.. do you have some suggestions?

i think it's fine. the double ee is not a problem.

Thanks Lubomir and Tim

changes coming

timothysc · 2019-05-14T14:17:23Z

/approve

to unblock post renaming in case others want to lgtm earlier.

k8s-ci-robot · 2019-05-14T14:18:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiatongw, neolit123, timothysc

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:

~~test/e2e/framework/OWNERS~~ [neolit123,timothysc]
~~test/e2e/kubectl/OWNERS~~ [timothysc]
~~test/e2e/network/OWNERS~~ [timothysc]

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

figo · 2019-05-16T02:05:32Z

/lgtm

jiatongw · 2019-05-16T18:02:06Z

There are conflicts because we both modified service_util.go. I integrated both yours and mine comments on PortsByPodName, PortsByPodUID, GetContainerPortsByPodUID and ValidateEndpointsOrFail, which in ports.go. Some of them I chose to keep my original comments, please kindly review @atoato88

jiatongw · 2019-05-16T18:10:27Z

/assign @oomichi

Assign you as well since you are the approver of PR #77643. Please kindly review :)

oomichi

/test pull-kubernetes-e2e-gce-device-plugin-gpu

oomichi · 2019-05-16T18:27:43Z

test/e2e/framework/endpoints/ports.go

+		}
+		portsByUID[pod.ObjectMeta.UID] = portList
+	}
+	// Logf("successfully translated pod names to UIDs: %v -> %v on namespace %s", expectedEndpoints, portsByUID, ns)


Should we remove this commented-out line?

Those code are already commented out before I opened this PR so I don't touch them.
I think they are good to remove since we are more interested in error logs. I'll delete those lines. Thanks

Looks like these comments were here before, agreed that they're probably safe to remove

oomichi · 2019-05-16T18:29:36Z

test/e2e/framework/endpoints/ports.go

+			e2elog.Logf("Get endpoints failed (%v elapsed, ignoring for 5s): %v", time.Since(start), err)
+			continue
+		}
+		// Logf("Found endpoints %v", endpoints)


oomichi · 2019-05-16T18:29:45Z

test/e2e/framework/endpoints/ports.go

+		// Logf("Found endpoints %v", endpoints)
+
+		portsByPodUID := GetContainerPortsByPodUID(ep)
+		// Logf("Found port by pod UID %v", portsByPodUID)


andrewsykim · 2019-05-16T18:42:03Z

test/e2e/framework/endpoints/ports.go

+	for name, portList := range expectedEndpoints {
+		pod, err := c.CoreV1().Pods(ns).Get(name, metav1.GetOptions{})
+		if err != nil {
+			framework.Failf("failed to get pod %s, that's pretty weird. validation failed: %s", name, err)


Maybe for a future PR, we should try to avoid importing framework here. Maybe a better pattern is to return an err here and have the calling test run framework.Failf.

Actually, if you don't mind I think it would be ideal to do this refactor now. It'll keep the dependency graph cleaner for later

updated, ready to review

andrewsykim · 2019-05-16T18:43:49Z

test/e2e/framework/endpoints/ports.go

+		}
+		portsByUID[pod.ObjectMeta.UID] = portList
+	}
+	// Logf("successfully translated pod names to UIDs: %v -> %v on namespace %s", expectedEndpoints, portsByUID, ns)


Looks like these comments were here before, agreed that they're probably safe to remove

andrewsykim · 2019-05-16T18:44:32Z

test/e2e/framework/endpoints/ports.go

+}
+
+// ValidateEndpointsOrFail validates that the given service exists and is served by the given expectedEndpoints.
+func ValidateEndpointsOrFail(c clientset.Interface, namespace, serviceName string, expectedEndpoints PortsByPodName) {


Can we rename this to ValidateEndpointsPortsOrFail?

jiatongw · 2019-05-16T23:00:00Z

Keep 2 commits for now, if good to merge, I'll squash them to 1 commit

jiatongw · 2019-05-16T23:11:39Z

/test pull-kubernetes-bazel-test

figo · 2019-05-16T23:15:07Z

Keep 2 commits for now, if good to merge, I'll squash them to 1 commit

it is better to squash if just two commits and very small.

andrewsykim · 2019-05-16T23:32:24Z

test/e2e/framework/endpoints/ports.go

+	} else {
+		e2elog.Logf("Can't list pod debug info: %v", err)
+	}
+	return fmt.Errorf("Timed out waiting for service %s in namespace %s to expose endpoints %v (%v elapsed)", serviceName, namespace, expectedEndpoints, framework.ServiceStartTimeout)


If framework.ServiceStartTimeout is the only reason we're importing framework here I think it would be okay to duplicate this constant into this package or import it somewhere else. What do you think?

Yes, I think it's ok to do that. Thanks! Updated.

atoato88 · 2019-05-17T09:44:25Z

@jiatongw Thank you for mention. I've confirmed that your comments on next items are OK for me. 😃

PortsByPodName
PortsByPodUID
GetContainerPortsByPodUID
ValidateEndpointsPortsOrFail (formerly ValidateEndpointsOrFail)

Signed-off-by: Jiatong Wang <wangjiatong@vmware.com>

oomichi · 2019-05-21T09:58:14Z

/lgtm

fejta-bot · 2019-05-21T13:20:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-05-21T16:31:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 10, 2019

k8s-ci-robot requested review from dcbw and pohly May 10, 2019 05:29

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 10, 2019

jiatongw commented May 10, 2019

View reviewed changes

k8s-ci-robot assigned andrewsykim May 10, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 10, 2019

k8s-ci-robot assigned timothysc May 10, 2019

timothysc previously requested changes May 13, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2019

jiatongw force-pushed the e2e/framework/service_util branch 2 times, most recently from d88e859 to bf00831 Compare May 14, 2019 20:36

k8s-ci-robot assigned figo May 16, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2019

jiatongw force-pushed the e2e/framework/service_util branch from bf00831 to 61c1791 Compare May 16, 2019 17:58

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2019

k8s-ci-robot assigned oomichi May 16, 2019

jiatongw force-pushed the e2e/framework/service_util branch 2 times, most recently from 6a0df2d to e5fc1d0 Compare May 16, 2019 18:18

oomichi reviewed May 16, 2019

View reviewed changes

andrewsykim reviewed May 16, 2019

View reviewed changes

jiatongw force-pushed the e2e/framework/service_util branch from e5fc1d0 to 7175ed2 Compare May 16, 2019 22:58

andrewsykim reviewed May 16, 2019

View reviewed changes

Move service_util endpoints related to framework/endpoints

76f7645

Signed-off-by: Jiatong Wang <wangjiatong@vmware.com>

jiatongw force-pushed the e2e/framework/service_util branch from 7175ed2 to 76f7645 Compare May 20, 2019 18:49

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2019

k8s-ci-robot merged commit 2fd1556 into kubernetes:master May 21, 2019

jiatongw deleted the e2e/framework/service_util branch May 21, 2019 18:41

jiatongw mentioned this pull request May 29, 2019

REQUEST: New membership for jiatongw kubernetes/org#854

Closed

6 tasks

Move service_util endpoints related functions to framework/endpoints/ports.go #77720

Move service_util endpoints related functions to framework/endpoints/ports.go #77720

Conversation

jiatongw commented May 10, 2019 • edited

k8s-ci-robot commented May 10, 2019

Choose a reason for hiding this comment

jiatongw commented May 10, 2019

neolit123 commented May 10, 2019

jiatongw commented May 10, 2019

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw May 14, 2019 • edited

Choose a reason for hiding this comment

timothysc commented May 14, 2019

k8s-ci-robot commented May 14, 2019

figo commented May 16, 2019

jiatongw commented May 16, 2019 • edited

jiatongw commented May 16, 2019

oomichi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw commented May 16, 2019

jiatongw commented May 16, 2019

figo commented May 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atoato88 commented May 17, 2019

oomichi commented May 21, 2019

fejta-bot commented May 21, 2019

fejta-bot commented May 21, 2019

jiatongw commented May 10, 2019 •

edited

jiatongw May 14, 2019 •

edited

jiatongw commented May 16, 2019 •

edited