test/e2e/framework/node/:remove TODO and and make some functions private #88374

tanjunchen · 2020-02-20T16:36:18Z

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-02-20T16:38:22Z

test/e2e/framework/node/resource.go

@@ -67,51 +67,40 @@ func FirstAddress(nodelist *v1.NodeList, addrType v1.NodeAddressType) string {
 	return ""
 }

-// TODO: better to change to a easy read name


i think this name is ok,so i want to remove this TODO

tanjunchen · 2020-02-20T16:39:52Z

test/e2e/framework/node/resource.go

 // TODO: need to discuss wether to return bool and error type
-func IsNodeUntainted(node *v1.Node) bool {


this function is used only test/e2e/framework/node , so make it private.

tanjunchen · 2020-02-20T16:40:30Z

test/e2e/framework/node/resource.go

@@ -138,8 +126,8 @@ func IsConditionSetAsExpectedSilent(node *v1.Node, conditionType v1.NodeConditio
 	return isNodeConditionSetAsExpected(node, conditionType, wantTrue, true)
 }

-// IsConditionUnset returns true if conditions of the given node do not have a match to the given conditionType, otherwise false.
-func IsConditionUnset(node *v1.Node, conditionType v1.NodeConditionType) bool {


this function is used only test/e2e/framework/node , so make it private.

tanjunchen · 2020-02-20T16:45:25Z

test/e2e/framework/node/resource.go

 	for _, cond := range node.Status.Conditions {
-		// Ensure that the condition type and the status matches as desired.
-		if cond.Type == conditionType {


For line 75 and 77, This looks a little weird.
function IsConditionSetAsExpected and IsConditionSetAsExpectedSilent use isNodeConditionSetAsExpected,
and all values passed to conditionType are "NodeReady"

the two different function seem to wrap this one while passing silent=true/false.

but the diff here removes the check against conditionType, which makes the name of the function invalid.
i suggest this function is kept like that for now and only try to remove this TODO:

// TODO: better to change to a easy read name

thanks your review. The code implementation here always feels a bit weird i feel.
how about this change now?

why modify the contents of the function?

see https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/node/resource.go#L75 ,
if cond.Type == conditionType is true and cond.Type == v1.NodeReady is true, https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/node/resource.go#L114 never run。
I think it is possible to judge this condition in advance, so move this code. If you don't think need to change it, I can revert it.I can just try to remove this TODO.

so in this PR i think we should only:

removed the TODO // TODO: better to change to a easy read name.

make the functions is....() private.

leave this TODO:

// TODO: check if the Node is tainted once we enable NC notReady/unreachable taints by default

to SIG Node and not touch it.

done,updated

neolit123 · 2020-02-20T16:57:04Z

test/e2e/framework/node/resource.go

 				}
 				return false
 			}
-			if (wantTrue && (cond.Status == v1.ConditionTrue)) || (!wantTrue && (cond.Status != v1.ConditionTrue)) {
+			// TODO: check if the Node is tainted once we enable NC notReady/unreachable taints by default


this TODO is for sig-node to resolve if they want to.
taint based eviction is enabled by default in 1.13:
https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/#taint-based-evictions

yes,i want to resolve this if i can in later.

tanjunchen · 2020-02-21T11:43:42Z

/retest

neolit123

thanks
/lgtm
/approve

neolit123 · 2020-02-21T16:04:41Z

/retest

k8s-ci-robot · 2020-02-21T16:05:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

fejta-bot · 2020-02-21T19:44:42Z

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

tanjunchen requested review from SataQiu and neolit123 February 20, 2020 16:36

tanjunchen changed the title ~~test/e2e/framework/node/:refactor isNodeConditionSetAsExpected~~ test/e2e/framework/node/:refactor function isNodeConditionSetAsExpected and and make some functions private Feb 20, 2020

k8s-ci-robot requested a review from BenTheElder February 20, 2020 16:38

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 Feb 20, 2020

tanjunchen commented Feb 20, 2020

View reviewed changes

neolit123 reviewed Feb 20, 2020

View reviewed changes

test/e2e/framework/node/:remove TODO and make some functions private

7c3f6fa

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 21, 2020

tanjunchen changed the title ~~test/e2e/framework/node/:refactor function isNodeConditionSetAsExpected and and make some functions private~~ test/e2e/framework/node/:remove TODO and and make some functions private Feb 21, 2020

neolit123 reviewed Feb 21, 2020

View reviewed changes

k8s-ci-robot assigned neolit123 Feb 21, 2020

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

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

k8s-ci-robot merged commit 8ac7a5b into kubernetes:master Feb 22, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 22, 2020

tanjunchen deleted the remove-TODO-simplify-code branch February 22, 2020 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test/e2e/framework/node/:remove TODO and and make some functions private #88374

test/e2e/framework/node/:remove TODO and and make some functions private #88374

tanjunchen commented Feb 20, 2020 •

edited

tanjunchen Feb 20, 2020

tanjunchen Feb 20, 2020

tanjunchen Feb 20, 2020

tanjunchen Feb 20, 2020

neolit123 Feb 20, 2020

tanjunchen Feb 21, 2020

neolit123 Feb 21, 2020

tanjunchen Feb 21, 2020

neolit123 Feb 21, 2020

tanjunchen Feb 21, 2020

tanjunchen Feb 21, 2020

neolit123 Feb 20, 2020

tanjunchen Feb 21, 2020

tanjunchen commented Feb 21, 2020

neolit123 left a comment

neolit123 commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

fejta-bot commented Feb 21, 2020

		// TODO: need to discuss wether to return bool and error type
		func IsNodeUntainted(node *v1.Node) bool {

test/e2e/framework/node/:remove TODO and and make some functions private #88374

test/e2e/framework/node/:remove TODO and and make some functions private #88374

Conversation

tanjunchen commented Feb 20, 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

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

tanjunchen commented Feb 21, 2020

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

fejta-bot commented Feb 21, 2020

tanjunchen commented Feb 20, 2020 •

edited