Extract validateNodeIP test to node status test file. #46903

gyliu513 · 2017-06-03T04:45:16Z

The function of validateNodeIP is belong to kubelet_node_status,
so the unit test of this function should be in node status test file.

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

none

gyliu513 · 2017-06-03T04:45:55Z

/release-note-none

gyliu513 · 2017-06-05T07:02:39Z

Related to #40780

timstclair · 2017-06-28T20:52:23Z

pkg/kubelet/kubelet_node_status.go

@@ -976,7 +976,6 @@ func (kl *Kubelet) validateNodeIP() error {
 		var ip net.IP
 		switch v := addr.(type) {
 		case *net.IPNet:
-			ip = v.IP


Golang doesn't fallthrough. By removing this line, you're ignoring all net.IPNet addressses, which I don't think is the intent?

timstclair · 2017-06-28T20:53:39Z

pkg/kubelet/kubelet_node_status_test.go

+			nodeIP:   "1.2.3.4",
+			success:  false,
+			testName: "IPv4 address that doesn't belong to host",
+		},


It would be good to have 1 test case that actually checks a valid IP. UNfortunately theres no way to mock golang net.InterfaceAddrs, so choices are to either pick an address to use in the unit test, or change the validateNodeIP() method to take a list of host interface addrs.

Seems mock golang net.InterfaceAddrs maybe better as this can make the caller simple. But I found that many places calling net.InterfaceAddrs() does not have such a unit test, how about file another issue to handle this?

Sure. It worries me that all tests (even the e2e's ??) passed with the fallthrough issue though.

Yes, I filed an issue #48315 here to trace

gyliu513 · 2017-06-29T05:01:15Z

/retest

gyliu513 · 2017-06-29T08:21:14Z

/retest

timstclair · 2017-06-29T21:05:27Z

/lgtm

gyliu513 · 2017-07-09T23:09:46Z

/retest

gyliu513 · 2017-07-10T02:08:00Z

/retest

gyliu513 · 2017-07-10T05:58:13Z

/retest

gyliu513 · 2017-07-10T08:14:06Z

/retest

gyliu513 · 2017-07-10T23:57:58Z

/retest

fejta · 2017-07-11T20:18:32Z

/test pull-kubernetes-e2e-kops-aws

gyliu513 · 2017-07-12T02:50:07Z

ping @sjpotter @timstclair for lgtm and approve

tallclair · 2017-07-12T17:11:47Z

/lgtm

k8s-github-robot · 2017-07-18T23:04:14Z

@k8s-bot test this

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2017-12-09T08:10:23Z

This PR hasn't been active in 30 days. It will be closed in 59 days (Feb 6, 2018).

cc @gyliu513 @sjpotter @tallclair

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

fejta-bot · 2018-03-09T09:09:34Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-04-08T09:57:52Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

cblecker · 2018-04-08T18:23:55Z

@gyliu513 is this still valid?

gyliu513 · 2018-04-09T02:53:42Z

@cblecker Yes, still valid, I will rebase the PR soon.

The function of `validateNodeIP` is belong to kubelet_node_status, so the unit test of this function should be in node status test file.

gyliu513 · 2018-04-09T04:03:50Z

@cblecker @tallclair the PR is updated, can you help review again? Thanks.

tallclair · 2018-04-09T18:06:03Z

I know I lgtm'd this before, but this test seems fine in kubelet_network_test. Why move it?

gyliu513 · 2018-04-10T10:38:13Z

@tallclair my thinking is the function of validateNodeIP is belong to kubelet_node_status, so the unit test of this function should be in node status test file as well.

tallclair · 2018-04-11T00:39:20Z

Right, that makes sense.

/lgtm

k8s-ci-robot · 2018-04-11T00:39:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyliu513, tallclair, timstclair

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:

~~pkg/kubelet/OWNERS~~ [tallclair]

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

gyliu513 · 2018-04-11T02:15:17Z

/test pull-kubernetes-e2e-kops-aws

fejta-bot · 2018-04-11T05:55:58Z

/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 comment for consistent failures.

gyliu513 · 2018-04-11T06:01:23Z

/retest

k8s-github-robot · 2018-04-11T06:59:48Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-04-11T07:45:43Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2017

k8s-github-robot assigned timstclair and sjpotter Jun 3, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 3, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 3, 2017

timstclair reviewed Jun 28, 2017

View reviewed changes

gyliu513 force-pushed the validateip branch from 6eb05e5 to 74b3c7f Compare June 29, 2017 03:40

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

gyliu513 mentioned this pull request Jun 30, 2017

Add one valid test case for net.InterfaceAddrs #48315

Closed

gyliu513 force-pushed the validateip branch from 74b3c7f to 0f467eb Compare July 9, 2017 11:10

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2017

tallclair assigned tallclair and unassigned timstclair Jul 12, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2017

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 9, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 8, 2018

gyliu513 force-pushed the validateip branch from a1bd6a6 to c6be121 Compare April 9, 2018 03:48

kubernetes deleted a comment from k8s-github-robot Apr 9, 2018

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

Extract validateNodeIP test to node status test file.

8d92814

The function of `validateNodeIP` is belong to kubelet_node_status, so the unit test of this function should be in node status test file.

gyliu513 force-pushed the validateip branch from c6be121 to 8d92814 Compare April 9, 2018 04:02

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

k8s-github-robot merged commit cc9f1a5 into kubernetes:master Apr 11, 2018

gyliu513 deleted the validateip branch April 12, 2018 01:14

Extract validateNodeIP test to node status test file. #46903

Extract validateNodeIP test to node status test file. #46903

Conversation

gyliu513 commented Jun 3, 2017 • edited Loading

gyliu513 commented Jun 3, 2017

gyliu513 commented Jun 5, 2017

timstclair Jun 28, 2017

Choose a reason for hiding this comment

gyliu513 Jun 29, 2017

Choose a reason for hiding this comment

timstclair Jun 28, 2017

Choose a reason for hiding this comment

gyliu513 Jun 29, 2017

Choose a reason for hiding this comment

timstclair Jun 29, 2017

Choose a reason for hiding this comment

gyliu513 Jun 30, 2017

Choose a reason for hiding this comment

gyliu513 commented Jun 29, 2017

gyliu513 commented Jun 29, 2017

timstclair commented Jun 29, 2017

gyliu513 commented Jul 9, 2017

gyliu513 commented Jul 10, 2017

gyliu513 commented Jul 10, 2017

gyliu513 commented Jul 10, 2017

gyliu513 commented Jul 10, 2017

fejta commented Jul 11, 2017

gyliu513 commented Jul 12, 2017

tallclair commented Jul 12, 2017

k8s-github-robot commented Jul 18, 2017

k8s-github-robot commented Dec 9, 2017

fejta-bot commented Mar 9, 2018

fejta-bot commented Apr 8, 2018

cblecker commented Apr 8, 2018

gyliu513 commented Apr 9, 2018

gyliu513 commented Apr 9, 2018

tallclair commented Apr 9, 2018

gyliu513 commented Apr 10, 2018

tallclair commented Apr 11, 2018

k8s-ci-robot commented Apr 11, 2018

gyliu513 commented Apr 11, 2018

fejta-bot commented Apr 11, 2018

gyliu513 commented Apr 11, 2018

k8s-github-robot commented Apr 11, 2018

k8s-github-robot commented Apr 11, 2018

gyliu513 commented Jun 3, 2017 •

edited

Loading