Move node related methods to framework/node package #78282

jiatongw · 2019-05-24T07:00:26Z

What type of PR is this?

/kind cleanup
/priority backlog

What this PR does / why we need it:

Move util.go node related functions to framework/node package, alias e2enode
Fix references of some methods to use the right package

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

Special notes for your reviewer:

There are a lot of node related functions in util.go and service_util.go are calling Failf() or ExpectNoError(). I leave those unchanged because moving them to framework/node will cause a import cycle error (i.e. import e2enode in util.go and import framework in framework/node package ). I plan to do follow up PRs to completely clean up the rest of node related functions.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-05-24T07:00:33Z

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-24T07:01:48Z

/assign @andrewsykim @timothysc

jiatongw · 2019-05-24T07:12:07Z

test/e2e/framework/util.go

@@ -135,8 +135,6 @@ const (

 	// PollShortTimeout is the short timeout value in polling.
 	PollShortTimeout = 1 * time.Minute
-	// PollLongTimeout is the long timeout value in polling.
-	PollLongTimeout = 5 * time.Minute



This const is not in use so removed

figo · 2019-05-24T16:15:16Z

/ok-to-test

yastij · 2019-05-24T17:02:15Z

test/e2e/apps/network_partition.go

@@ -30,14 +30,15 @@ import (

 	"k8s.io/client-go/tools/cache"

-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"


this change isn't needed (same below)

This was added automatically be goreturns when saving files.. will remove it.

jiatongw · 2019-05-25T06:09:08Z

test/e2e/framework/node/resource.go

+
+// FirstNodeAddresses returns the first address of the given type of each node.
+func FirstNodeAddresses(nodelist *v1.NodeList, addrType v1.NodeAddressType) []string {
+	hosts := []string{}


The original method name was NodeAddresses. Rename this method to FirstNodeAddresses is because e2enode.NodeAddresses will cause a golint error (stutters).

Does this function need to return []string, wondering why not just string? I see it was always like this but might be worth doing a refactor now, or at least leave a TODO for later

Agreed this signature is weird, and the name is weird.

Should be FirstAddress and return string, or add a //TODO here.

jiatongw · 2019-05-25T06:09:52Z

test/e2e/framework/node/resource.go

+
+// ProxyRequest performs a get on a node proxy endpoint given the nodename and rest client.
+func ProxyRequest(c clientset.Interface, node, endpoint string, port int) (restclient.Result, error) {
+	// proxy tends to hang in some cases when Node is not ready. Add an artificial timeout for this call.


The original method name was NodeProxyRequest. Rename this method to ProxyRequest is because e2enode.NodeProxyRequest will cause a golint error (stutters).

andrewsykim

Minor comments, overall PR looks good. I think test/e2e/framework/node can use some more cleaning up, but we can do this in a follow-up PR once things are moved into it's own package.

andrewsykim · 2019-05-28T20:05:42Z

test/e2e/framework/node/resource.go

+
+// FirstNodeAddresses returns the first address of the given type of each node.
+func FirstNodeAddresses(nodelist *v1.NodeList, addrType v1.NodeAddressType) []string {
+	hosts := []string{}


Does this function need to return []string, wondering why not just string? I see it was always like this but might be worth doing a refactor now, or at least leave a TODO for later

andrewsykim · 2019-05-28T20:06:09Z

test/e2e/framework/node/resource.go

+	return hosts
+}
+
+func isNodeConditionSetAsExpected(node *v1.Node, conditionType v1.NodeConditionType, wantTrue, silent bool) bool {


this function 🙈

I will add a TODO for FirstNodeAddresses , and will make the change along with others in the following PR.

Please add a TODO to cleanup this function

andrewsykim · 2019-05-28T20:21:24Z

test/e2e/framework/node/resource.go

+
+const (
+	// Poll is how often to Poll pods, nodes and claims.
+	Poll = 2 * time.Second


can this be unexported?

https://cs.k8s.io/?q=framework.Poll&i=nope&files=&repos=kubernetes/kubernetes

This is used in other packages.

That's a different constant that the one here though?

That's the same. The Poll here is which I copied from util.go, to avoid import cycles.
But we can make this unexported, since other packages import from framework.Poll, we can make this Poll just use within node package?

andrewsykim · 2019-05-28T20:21:28Z

test/e2e/framework/node/resource.go

+	Poll = 2 * time.Second
+
+	// PollShortTimeout is the short timeout value in polling.
+	PollShortTimeout = 1 * time.Minute


can this be unexported?

https://cs.k8s.io/?q=PollShortTimeout&i=nope&files=&repos=kubernetes/kubernetes

This is also referred by other packages

andrewsykim · 2019-05-28T20:21:32Z

test/e2e/framework/node/resource.go

+	// SingleCallTimeout is how long to try single API calls (like 'get' or 'list'). Used to prevent
+	// transient failures from failing tests.
+	// TODO: client should not apply this timeout to Watch calls. Increased from 30s until that is fixed.
+	SingleCallTimeout = 5 * time.Minute


can this be unexported?

https://cs.k8s.io/?q=SingleCallTimeout&i=nope&files=&repos=kubernetes/kubernetes

same above

jiatongw · 2019-06-06T05:34:26Z

Changed as requested @timothysc

Renamed exported functions whose name have redundant strings. I leave unexported functions unchanged because those will not be used as e2enode.xxx, and it's readable since some of them don't have comments.

I have a second commit for reviewers convenience. Once approved, I'll squash to one.

jiatongw · 2019-06-06T08:29:43Z

/test pull-kubernetes-e2e-gce

timothysc

minor comments, then lgtm
/approve

timothysc · 2019-06-10T20:53:57Z

test/e2e/framework/networking_utils.go

@@ -579,7 +580,7 @@ func (config *NetworkingTestConfig) setup(selector map[string]string) {
 	ginkgo.By("Getting node addresses")
 	ExpectNoError(WaitForAllNodesSchedulable(config.f.ClientSet, 10*time.Minute))
 	nodeList := GetReadySchedulableNodesOrDie(config.f.ClientSet)
-	config.ExternalAddrs = NodeAddresses(nodeList, v1.NodeExternalIP)
+	config.ExternalAddrs = e2enode.FirstAddresses(nodeList, v1.NodeExternalIP)


FirstAddresses?

timothysc · 2019-06-10T20:56:17Z

test/e2e/framework/node/resource.go

+
+// FirstNodeAddresses returns the first address of the given type of each node.
+func FirstNodeAddresses(nodelist *v1.NodeList, addrType v1.NodeAddressType) []string {
+	hosts := []string{}


Agreed this signature is weird, and the name is weird.

Should be FirstAddress and return string, or add a //TODO here.

timothysc · 2019-06-10T20:57:23Z

test/e2e/framework/node/resource.go

+	return hosts
+}
+
+func isNodeConditionSetAsExpected(node *v1.Node, conditionType v1.NodeConditionType, wantTrue, silent bool) bool {


Please add a TODO to cleanup this function

timothysc · 2019-06-10T20:59:36Z

test/e2e/framework/node/wait.go

+// there is no not-ready nodes in it. By cluster size we mean number of Nodes
+// excluding Master Node.
+func CheckReady(c clientset.Interface, size int, timeout time.Duration) ([]v1.Node, error) {
+	for start := time.Now(); time.Since(start) < timeout; time.Sleep(20 * time.Second) {


please factor the magic numbers to be consts in this file, or add it as a //TODO.

factored 20 * time.Second to a const sleepTime

k8s-ci-robot · 2019-06-10T21:01:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiatongw, 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/OWNERS~~ [timothysc]

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

alejandrox1 · 2019-06-10T21:28:50Z

test/e2e/framework/node/wait.go

+// WaitForReady waits up to timeout for cluster to has desired size and
+// there is no not-ready nodes in it. By cluster size we mean number of Nodes
+// excluding Master Node.
+func WaitForReady(c clientset.Interface, size int, timeout time.Duration) error {


Why change the signature of this method from e2enode.WaitForReadyNodes to e2enode.WaitForReady?
1WaitForReadycould wait for any general condition whileWaitForReadyNodes` makes it sound more explicit that the condition is a certain number of nodes

As per Tim's suggestion, we have a e2enode package, so the signature should remove the redundant node(s) in naming. e2enode.WaitForReady has a e2enode prefix, so people know that this is a node related function

what do you think?

Makes sense removing the redundant naming but for this one, if i was looking at godos (as an example), e2enode.WaitForReadyNodes sounds like it waits for some number of nodes, while e2enode.WaitForReady sounds like i could pass some "condition" object and check if a certain node meets that condition.

👍 make more sense

alejandrox1 · 2019-06-10T21:32:12Z

test/e2e/framework/node/wait.go

+}
+
+// WaitForNameToBeReady returns whether node name is ready within timeout.
+func WaitForNameToBeReady(c clientset.Interface, name string, timeout time.Duration) bool {


Here you change the signature from WaitForNodeToBeReady to WaitForNameToBeReady, yes?
wouldn't it read better e2enode.WaitForNodeToBeReady?
I think the name makes more sense given what it does.

same as above

For this one, e2enode.WaitForNameToBeReady sounds a bit weird to me, whereas e2enode.WaitForNodeToBeReady tells me exactly what it does. I think keeping the extra "node" in the method signature adds value (is not purely redundant).

will fix, thanks

alejandrox1 · 2019-06-10T21:36:26Z

test/e2e/framework/node/resource.go

+)
+
+const (
+	// Poll is how often to Poll pods, nodes and claims.


s/Poll/poll

thanks for catching this. will fix

alejandrox1 · 2019-06-10T21:36:44Z

test/e2e/framework/node/resource.go

+	// Poll is how often to Poll pods, nodes and claims.
+	poll = 2 * time.Second
+
+	// SingleCallTimeout is how long to try single API calls (like 'get' or 'list'). Used to prevent


s/SingleCallTimeout/singleCallTimeout

timothysc · 2019-06-11T13:53:30Z

Just poke me on the #testing-commons slack channel when you feel it's all ready to go.

timothysc

Going to call good enough. I could totally needle stuff, but it's not your code, it's what was inherited.

/lgtm

alejandrox1 · 2019-06-11T20:15:42Z

/retest

- Add a package "node" under e2e/framework and alias e2enode; - Rename some functions whose name have redundant string. Signed-off-by: Jiatong Wang <wangjiatong@vmware.com>

jiatongw · 2019-06-18T00:04:53Z

@timstclair Hi Tim, could you re-review and lgtm? Merging conflicts so I rebased.

alejandrox1

/lgtm

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 24, 2019

k8s-ci-robot requested review from aleksandra-malinowska and andrewsykim May 24, 2019 07:01

k8s-ci-robot assigned andrewsykim and timothysc May 24, 2019

jiatongw commented May 24, 2019

View reviewed changes

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 24, 2019

yastij reviewed May 24, 2019

View reviewed changes

jiatongw force-pushed the e2e/framework/utilNode branch 2 times, most recently from 8efc18b to a67b879 Compare May 25, 2019 06:06

jiatongw commented May 25, 2019

View reviewed changes

andrewsykim reviewed May 28, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2019

jiatongw force-pushed the e2e/framework/utilNode branch from a67b879 to fbe9115 Compare May 29, 2019 18:28

timothysc added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jun 4, 2019

jiatongw force-pushed the e2e/framework/utilNode branch from 3a50b08 to be14076 Compare June 6, 2019 05:28

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2019

jiatongw force-pushed the e2e/framework/utilNode branch 2 times, most recently from 26b0eb3 to 53805a0 Compare June 6, 2019 06:29

timothysc suggested changes Jun 10, 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 Jun 10, 2019

alejandrox1 reviewed Jun 10, 2019

View reviewed changes

jiatongw force-pushed the e2e/framework/utilNode branch from 53805a0 to 62cc152 Compare June 11, 2019 18:57

timothysc approved these changes Jun 11, 2019

View reviewed changes

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2019

Move node related methods to framework/node package

b1c346c

- Add a package "node" under e2e/framework and alias e2enode; - Rename some functions whose name have redundant string. Signed-off-by: Jiatong Wang <wangjiatong@vmware.com>

jiatongw force-pushed the e2e/framework/utilNode branch from 62cc152 to b1c346c Compare June 18, 2019 00:01

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 Jun 18, 2019

alejandrox1 approved these changes Jun 18, 2019

View reviewed changes

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

k8s-ci-robot merged commit eaf89cf into kubernetes:master Jun 18, 2019

jiatongw deleted the e2e/framework/utilNode branch June 18, 2019 09:04

jiatongw mentioned this pull request Jul 8, 2019

[e2e] Refactor and Move node related methods to framework/node package[1] #79879

Merged

jiatongw mentioned this pull request Aug 5, 2019

[e2e] Track TODOs in merged PRs #80975

Closed

15 tasks

kidlj mentioned this pull request Apr 8, 2022

e2e framework sub-packages should not import the core framework package to avoid circular imports #109391

Closed

Move node related methods to framework/node package #78282

Move node related methods to framework/node package #78282

Conversation

jiatongw commented May 24, 2019

k8s-ci-robot commented May 24, 2019

jiatongw commented May 24, 2019

Choose a reason for hiding this comment

figo commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw May 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw May 25, 2019 • edited

Choose a reason for hiding this comment

andrewsykim 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

jiatongw May 28, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiatongw commented Jun 6, 2019

jiatongw commented Jun 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 10, 2019

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

timothysc commented Jun 11, 2019

timothysc left a comment

Choose a reason for hiding this comment

alejandrox1 commented Jun 11, 2019

jiatongw commented Jun 18, 2019

alejandrox1 left a comment

Choose a reason for hiding this comment

jiatongw May 25, 2019 •

edited

jiatongw May 25, 2019 •

edited

jiatongw May 28, 2019 •

edited