test/e2e/framework: remove direct imports to k8s.io/kubernetes/pkg #89504

tanjunchen · 2020-03-26T07:53:16Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

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

None

tanjunchen · 2020-03-26T07:56:19Z

/priority important-longterm
/kind cleanup

/cc @neolit123 @oomichi
Thank you very much.

tanjunchen · 2020-03-26T07:59:02Z

test/e2e/framework/pod/resource.go

@@ -379,7 +378,8 @@ func FilterNonRestartablePods(pods []*v1.Pod) []*v1.Pod {
 }

 func isNotRestartAlwaysMirrorPod(p *v1.Pod) bool {
-	if !kubetypes.IsMirrorPod(p) {


// IsMirrorPod returns true if the passed Pod is a Mirror Pod. func IsMirrorPod(pod *v1.Pod) bool { _, ok := pod.Annotations[ConfigMirrorAnnotationKey] return ok } const ( ConfigSourceAnnotationKey = "kubernetes.io/config.source" ConfigMirrorAnnotationKey = v1.MirrorPodAnnotationKey ConfigFirstSeenAnnotationKey = "kubernetes.io/config.seen" ConfigHashAnnotationKey = "kubernetes.io/config.hash" )

tanjunchen · 2020-03-26T08:00:21Z

test/e2e/framework/pod/wait.go

@@ -302,7 +301,7 @@ func WaitForMatchPodsCondition(c clientset.Interface, opts metav1.ListOptions, d
 				return fmt.Errorf("Unexpected error: %v", err)
 			}
 			if !done {
-				conditionNotMatch = append(conditionNotMatch, format.Pod(&pod))


format.Pod() // Pod returns a string representing a pod in a consistent human readable format, // with pod UID as part of the string. func Pod(pod *v1.Pod) string { return PodDesc(pod.Name, pod.Namespace, pod.UID) } // PodDesc returns a string representing a pod in a consistent human readable format, // with pod UID as part of the string. func PodDesc(podName, podNamespace string, podUID types.UID) string { // Use underscore as the delimiter because it is not allowed in pod name // (DNS subdomain format), while allowed in the container name format. return fmt.Sprintf("%s_%s(%s)", podName, podNamespace, podUID) }

neolit123 · 2020-03-26T16:32:28Z

test/e2e/framework/pod/wait.go

@@ -302,7 +301,7 @@ func WaitForMatchPodsCondition(c clientset.Interface, opts metav1.ListOptions, d
 				return fmt.Errorf("Unexpected error: %v", err)
 			}
 			if !done {
-				conditionNotMatch = append(conditionNotMatch, format.Pod(&pod))
+				conditionNotMatch = append(conditionNotMatch, fmt.Sprintf("%s_%s(%s)", pod.Name, pod.Namespace, pod.UID))


just noting that this can be problematic, if for some reason the definition in pkg/kubelet/util/format changes!

at the minimum we should include the following comment over the line:

// This should match the format defined in kubernetes/pkg/kubelet/util/format.

sure , we can trace this function with comment.
Thanks

k8s-ci-robot · 2020-03-26T16:52:30Z

@tanjunchen: Reopened this PR.

In response to this:

/reopen

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.

alejandrox1 · 2020-03-27T02:39:18Z

test/e2e/framework/pod/resource.go

@@ -379,7 +378,8 @@ func FilterNonRestartablePods(pods []*v1.Pod) []*v1.Pod {
 }

 func isNotRestartAlwaysMirrorPod(p *v1.Pod) bool {
-	if !kubetypes.IsMirrorPod(p) {
+	_, ok := p.Annotations[v1.MirrorPodAnnotationKey]


Could you add a comment here, please?
Something simple like "checking that pod is mirror pod" or something.

if IsMirrorPod(p) is kind of self-descriptive.
if _, ok := p.Annotations[v1.MirrorPodAnnotationKey]; !ok would do well with some additional context,I think.

sure , i will update it. it looks good with comment.

alejandrox1

minor comment but lgtm otherwise.
Thank you @tanjunchen

/retest

tanjunchen · 2020-03-27T03:29:10Z

@alejandrox1 done

tanjunchen · 2020-03-27T06:12:43Z

/test pull-kubernetes-verify

tanjunchen · 2020-03-27T07:29:24Z

/test pull-kubernetes-verify

tanjunchen · 2020-03-27T09:36:23Z

Errors from golint:
test/e2e/framework/pod/wait.go:319:6: func name will be used as pod.PodDesc by other packages, and that stutters; consider calling this Desc？

tanjunchen · 2020-03-27T10:15:23Z

/cc @neolit123

neolit123 · 2020-03-27T14:11:25Z

test/e2e/framework/pod/resource.go

@@ -379,7 +378,8 @@ func FilterNonRestartablePods(pods []*v1.Pod) []*v1.Pod {
 }

 func isNotRestartAlwaysMirrorPod(p *v1.Pod) bool {
-	if !kubetypes.IsMirrorPod(p) {
+	// Checking that pod is mirror pod


// Check if the pod is a mirror pod

neolit123 · 2020-03-27T14:40:49Z

see the golint error:

test/e2e/framework/pod/wait.go:319:6: func name will be used as pod.PodDesc by other packages, and that stutters; consider calling this Desc

Please review the above warnings. You can test via "golint" and commit the result.
If the above warnings do not make sense, you can exempt this package from golint
checking by adding it to hack/.golint_failures (if your reviewer is okay with it).

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/89504/pull-kubernetes-verify/1243440072976502784

the function should be called something else - e.g. Desc

tanjunchen · 2020-03-28T02:37:42Z

test/e2e/framework/pod/wait.go

@@ -301,7 +302,7 @@ func WaitForMatchPodsCondition(c clientset.Interface, opts metav1.ListOptions, d
 				return fmt.Errorf("Unexpected error: %v", err)
 			}
 			if !done {
-				conditionNotMatch = append(conditionNotMatch, fmt.Sprintf("%s_%s(%s)", pod.Name, pod.Namespace, pod.UID))
+				conditionNotMatch = append(conditionNotMatch, GetPodDesc(pod.Name, pod.Namespace, pod.UID))


see #89504 (comment)
It is has been changed to fmt.Sprintf("%s_%s(%s)" , but i want to update them according to your comment.Thanks.
/cc @neolit123

alejandrox1 · 2020-03-28T17:38:57Z

test/e2e/framework/pod/wait.go

+// GetPodDesc returns a string representing a pod in a consistent human readable format,
+// with pod UID as part of the string.
+// This should match the format defined in kubernetes/pkg/kubelet/util/format.
+func GetPodDesc(podName, podNamespace string, podUID types.UID) string {


could we do something more verbose, i.e., GetPodDescription, or PodIdentifier, etc?
GetPodDesc sounds crptic 🤔

/cc @alejandrox1
updated

tanjunchen · 2020-03-29T07:19:06Z

/test pull-kubernetes-conformance-kind-ipv6-parallel

SataQiu · 2020-03-30T15:17:28Z

test/e2e/framework/pod/wait.go

+// GetPodDescription returns a string representing a pod in a consistent human readable format,
+// with pod UID as part of the string.
+// This should match the format defined in kubernetes/pkg/kubelet/util/format.
+func GetPodDescription(podName, podNamespace string, podUID types.UID) string {


If there are no other references, I think this function can be set to private.
SGTM otherwise.

oomichi · 2020-03-30T15:19:23Z

test/e2e/framework/pod/wait.go

+// GetPodDescription returns a string representing a pod in a consistent human readable format,
+// with pod UID as part of the string.
+// This should match the format defined in kubernetes/pkg/kubelet/util/format.
+func GetPodDescription(podName, podNamespace string, podUID types.UID) string {


Why we need to add this function?
This addition doesn't remove import of k/k code from this package.

see #89504 (comment)

@tanjunchen Since b748c70 the direct import(format) has been removed already by @SataQiu . So we don't need to change here today.

OK , I prefer to this change, revert this . Thanks

/cc @SataQiu
/cc @oomichi
updated . could you approve this?Thanks

tanjunchen · 2020-03-31T03:31:29Z

/retest

oomichi

Thanks for updating.

/lgtm

tanjunchen · 2020-04-01T05:39:04Z

/cc @SataQiu

oomichi · 2020-04-01T15:36:46Z

/approve

k8s-ci-robot · 2020-04-01T15:37:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi, tanjunchen

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~~ [oomichi]

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

oomichi · 2020-04-01T17:17:19Z

/test pull-kubernetes-e2e-kind

k8s-ci-robot requested review from neolit123 and pohly March 26, 2020 07:54

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. labels Mar 26, 2020

k8s-ci-robot requested a review from oomichi March 26, 2020 07:56

tanjunchen commented Mar 26, 2020

View reviewed changes

neolit123 reviewed Mar 26, 2020

View reviewed changes

tanjunchen closed this Mar 26, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2020

k8s-ci-robot reopened this Mar 26, 2020

alejandrox1 reviewed Mar 27, 2020

View reviewed changes

k8s-ci-robot requested a review from neolit123 March 27, 2020 10:15

neolit123 reviewed Mar 27, 2020

View reviewed changes

tanjunchen commented Mar 28, 2020

View reviewed changes

alejandrox1 reviewed Mar 28, 2020

View reviewed changes

k8s-ci-robot requested a review from alejandrox1 March 30, 2020 01:18

SataQiu reviewed Mar 30, 2020

View reviewed changes

oomichi reviewed Mar 30, 2020

View reviewed changes

test/e2e/framework: remove direct imports to k8s.io/kubernetes/pkg

fb07834

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 31, 2020

k8s-ci-robot requested review from oomichi and SataQiu March 31, 2020 01:29

oomichi reviewed Mar 31, 2020

View reviewed changes

k8s-ci-robot assigned oomichi Mar 31, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2020

k8s-ci-robot merged commit 6e9bec8 into kubernetes:master Apr 1, 2020

k8s-ci-robot added this to the v1.19 milestone Apr 1, 2020

tanjunchen deleted the remove-dependency-001 branch April 2, 2020 01:22

test/e2e/framework: remove direct imports to k8s.io/kubernetes/pkg #89504

test/e2e/framework: remove direct imports to k8s.io/kubernetes/pkg #89504

Conversation

tanjunchen commented Mar 26, 2020

tanjunchen commented Mar 26, 2020

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

k8s-ci-robot commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandrox1 left a comment

Choose a reason for hiding this comment

tanjunchen commented Mar 27, 2020

tanjunchen commented Mar 27, 2020

tanjunchen commented Mar 27, 2020

tanjunchen commented Mar 27, 2020

tanjunchen commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Mar 27, 2020

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

tanjunchen commented Mar 29, 2020

SataQiu Mar 30, 2020 • edited

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

tanjunchen Mar 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanjunchen commented Mar 31, 2020

oomichi left a comment

Choose a reason for hiding this comment

tanjunchen commented Apr 1, 2020

oomichi commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

oomichi commented Apr 1, 2020

SataQiu Mar 30, 2020 •

edited

tanjunchen Mar 31, 2020 •

edited