Correct assumptions for forwarded hostname & IP handling #7360

tstromberg · 2020-04-01T19:01:32Z

Follow-up to #7310 - addressing the hairier code review feedback.

Docker on Linux should use a true IP/port when possible rather than forwarded IP/port
update-context command should update the Docker port

Fixes an inconsistency:

start saves --apiserver-name into kubeconfig, but update-context does not

Consistently fixing the last issue required replacing IP's with strings throughout.

Work to be done

Integration tests to ensure that things are done correctly.

… Linux IP

k8s-ci-robot · 2020-04-01T19:01:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tstromberg

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:

~~OWNERS~~ [tstromberg]

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

tstromberg · 2020-04-01T20:02:21Z

/ok-to-test

minikube-pr-bot · 2020-04-01T20:09:53Z

Error: running mkcmp: exit status 1

minikube-pr-bot · 2020-04-01T21:07:48Z

All Times minikube: [ 63.786531 63.350283 61.864727]
All Times Minikube (PR 7360): [ 61.911601 65.280329 64.165405]

Average minikube: 63.000514
Average Minikube (PR 7360): 63.785778

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7360) |
+----------------------+-----------+--------------------+
| minikube v           |  0.222270 |           0.210476 |
| Creating kvm2        | 40.373566 |          40.155298 |
| Preparing Kubernetes | 20.803860 |          21.386878 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

test/integration/main.go

test/integration/cert_options_test.go

medyagh · 2020-04-01T21:15:17Z

this PR almost made my day ! thank you for doing this.

the concept looks good

codecov-io · 2020-04-01T21:46:20Z

Codecov Report

Merging #7360 into master will decrease coverage by 0.02%.
The diff coverage is 28.98%.

@@            Coverage Diff             @@
##           master    #7360      +/-   ##
==========================================
- Coverage   37.18%   37.16%   -0.03%     
==========================================
  Files         145      146       +1     
  Lines        8836     8839       +3     
==========================================
- Hits         3286     3285       -1     
- Misses       5154     5166      +12     
+ Partials      396      388       -8

Impacted Files	Coverage Δ
cmd/minikube/cmd/docker-env.go	`48.45% <0.00%> (ø)`
cmd/minikube/cmd/ip.go	`0.00% <0.00%> (ø)`
cmd/minikube/cmd/status.go	`20.49% <0.00%> (ø)`
cmd/minikube/cmd/update-context.go	`0.00% <0.00%> (ø)`
pkg/minikube/bootstrapper/bootstrapper.go	`0.00% <ø> (ø)`
pkg/minikube/driver/driver.go	`62.74% <0.00%> (-1.70%)`	⬇️
pkg/minikube/driver/endpoint.go	`0.00% <0.00%> (ø)`
pkg/minikube/kubeconfig/kubeconfig.go	`68.68% <70.37%> (+8.20%)`	⬆️
cmd/minikube/cmd/start.go	`34.34% <100.00%> (-0.27%)`	⬇️
... and 4 more

minikube-pr-bot · 2020-04-01T21:51:29Z

All Times minikube: [ 65.404799 68.977979 63.318920]
All Times Minikube (PR 7360): [ 64.317981 68.144020 61.789103]

Average Minikube (PR 7360): 64.750368
Average minikube: 65.900566

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7360) |
+----------------------+-----------+--------------------+
| minikube v           |  0.208236 |           0.203511 |
| Creating kvm2        | 41.295619 |          41.133820 |
| Preparing Kubernetes | 21.966067 |          21.504292 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-04-01T22:56:12Z

All Times minikube: [ 64.073471 63.420222 62.701076]
All Times Minikube (PR 7360): [ 62.508193 65.187109 66.180598]

Average minikube: 63.398257
Average Minikube (PR 7360): 64.625300

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7360) |
+----------------------+-----------+--------------------+
| minikube v           |  0.211658 |           0.208355 |
| Creating kvm2        | 40.113345 |          40.923396 |
| Preparing Kubernetes | 21.075861 |          21.528906 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

medyagh

too big to study carefully. but it does look good. if you feel good about the tests . I approve. lets merge

tstromberg · 2020-04-02T00:19:52Z

/ok-to-test

medyagh · 2020-04-02T01:19:47Z

Docker Driver Failures:

Known Flakes with issue:

TestStartStop/group/containerd - discuss: minikube wait for default service accont ? #7011
TestStartStop/group/embed-certs - is also related to discuss: minikube wait for default service accont ? #7011
TestFunctional/parallel/ProfileCmd/profile_not_create - TestFunctional/parallel/ProfileCmd/profile_not_create #7370

Other failures need to confirm not related to this PR:

TestStartStop/group/crio - 782.71s

Warning  FailedCreatePodSandBox  15s (x11 over 4m11s)  kubelet, crio-20200401t175453.296999333-32003  (combined from similar events): Failed create pod sandbox: rpc error: code = Unknown desc = failed to create pod network sandbox k8s_busybox_default_1aa75488-2899-4d7d-98cb-50dc9ff62636_0(f0e756dfc24a327ae0ead9c042c4db89328d0dea75c47b92cf415845ef6fa48f): failed to set bridge addr: could not add IP address to "cni0": permission denied

https://storage.googleapis.com/minikube-builds/logs/7360/ba84f94/Docker_Linux.html#fail_TestStartStop%2fgroup%2fcrio

TestFunctional/parallel/ServiceCmd - 69.9s

functional_test.go:665: get failed: GET http://172.17.0.2:32093 giving up after 5 attempts

medyagh

plz see above comment

tstromberg · 2020-04-02T17:44:11Z

Thanks! I did some additional confirmation here locally on the Docker failures:

TestStartStop/group/containerd - 215.51s

TestStartStop/group/embed-certs - 173.21s

Error from server (Forbidden): error when creating "testdata/busybox.yaml": pods "busybox" is forbidden: error looking up service account default/default: serviceaccount "default" not found

Known flake: discuss: minikube wait for default service accont ? #7011

TestStartStop/group/crio - 782.71s

Warning FailedCreatePodSandBox 7m18s kubelet, crio-20200401t175453.296999333-32003 Failed create pod sandbox: rpc error: code = Unknown desc = failed to create pod network sandbox k8s_busybox_default_1aa75488-2899-4d7d-98cb-50dc9ff62636_0(b2c34f2a8500f6f48f8d267786cbf3a04834cc756f94052f391ef074f5aac7a4): failed to set bridge addr: could not add IP address to "cni0": permission denied

Recent unrelated regression: docker driver + crio: failed to set bridge addr: could not add IP address to "cni0": permission denied #7380
Reproducible on macOS: Yes
Reproducible on Linux: Yes

TestFunctional/parallel/ServiceCmd - 69.9s

functional_test.go:648: (dbg) Run: out/minikube-linux-amd64 -p functional-20200401T175116.759279844-32003 service hello-node --url functional_test.go:662: url: http://172.17.0.2:32093 functional_test.go:665: get failed: GET http://172.17.0.2:32093 giving up after 5 attempts

Reproducible on macOS*: No - but did open docker: TestFunctional/parallel/ServiceCmd broken due to tunnel requirement #7383
Reproducible on Linux: No
NOTE: This seems to be a very common flake looking at the Jenkins log

TestFunctional/parallel/ProfileCmd/profile_not_create - 10.81s

functional_test.go:537: out/minikube-linux-amd64 profile list --output json failed: invalid character '*' looking for beginning of value

Opened issue: Slow docker message corrupts JSON output #7381
PR: https://github.com/kubernetes/minikube/pull/7382/files

medyagh · 2020-04-02T19:42:15Z

Thank you for creating the issues.i recommend putting this comment of the PR as a required model of response for all future PRs.

Maybe add it to the contributor guide?( In a separate pr)

How to handle integration tests failure .

looks good we can merge

minikube-pr-bot · 2020-04-02T19:45:45Z

All Times minikube: [ 60.202194 65.028108 64.896733]
All Times Minikube (PR 7360): [ 61.203428 63.167608 62.571514]

Average minikube: 63.375678
Average Minikube (PR 7360): 62.314183

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7360) |
+----------------------+-----------+--------------------+
| minikube v           |  0.203006 |           0.220335 |
| Creating kvm2        | 39.545826 |          40.375650 |
| Preparing Kubernetes | 21.588146 |          20.211429 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

test/integration/functional_test.go

Standardize port-forwarding and kubeconfig hostname logic, fix Docker…

c76fac7

… Linux IP

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2020

k8s-ci-robot requested review from priyawadhwa and sharifelgamal April 1, 2020 19:01

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

Update integration tests

b7f3f04

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2020

tstromberg changed the title ~~WIP: Use raw Docker linux IP, standardize port-forwarding & hostname logic~~ Make forwarded hostname & IP handling consistent and correct Apr 1, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2020

tstromberg changed the title ~~Make forwarded hostname & IP handling consistent and correct~~ Correct assumptions for forwarded hostname & IP handling Apr 1, 2020

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 1, 2020

Add missing endpoint file

a7106b4

tstromberg added 2 commits April 1, 2020 13:53

Remove IP check, because it's wrong on Docker

96a87e8

Update IP check message

0432095

medyagh reviewed Apr 1, 2020

View reviewed changes

test/integration/main.go Outdated Show resolved Hide resolved

medyagh reviewed Apr 1, 2020

View reviewed changes

test/integration/cert_options_test.go Outdated Show resolved Hide resolved

Pick a more universal hostname, add comment about docker

1dcd5cd

Apply a memory limit

ba84f94

medyagh approved these changes Apr 2, 2020

View reviewed changes

medyagh requested changes Apr 2, 2020

View reviewed changes

Merge branch 'master' into wrong-url

a71edc3

tstromberg added 2 commits April 2, 2020 10:51

Add response to get error, disable service check on macOS/Windows

5266b69

Run goimports

2e9ad82

medyagh requested changes Apr 2, 2020

View reviewed changes

test/integration/functional_test.go Show resolved Hide resolved

tstromberg merged commit 78ecf53 into kubernetes:master Apr 2, 2020

fmotrifork mentioned this pull request Apr 3, 2020

minikube 1.9.1 fishworks/fish-food#667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct assumptions for forwarded hostname & IP handling #7360

Correct assumptions for forwarded hostname & IP handling #7360

tstromberg commented Apr 1, 2020 •

edited

Loading

k8s-ci-robot commented Apr 1, 2020

tstromberg commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

medyagh commented Apr 1, 2020

codecov-io commented Apr 1, 2020 •

edited

Loading

minikube-pr-bot commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

medyagh left a comment

tstromberg commented Apr 2, 2020

medyagh commented Apr 2, 2020 •

edited

Loading

medyagh left a comment •

edited

Loading

tstromberg commented Apr 2, 2020

medyagh commented Apr 2, 2020

minikube-pr-bot commented Apr 2, 2020

Correct assumptions for forwarded hostname & IP handling #7360

Correct assumptions for forwarded hostname & IP handling #7360

Conversation

tstromberg commented Apr 1, 2020 • edited Loading

Work to be done

k8s-ci-robot commented Apr 1, 2020

tstromberg commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

medyagh commented Apr 1, 2020

codecov-io commented Apr 1, 2020 • edited Loading

Codecov Report

minikube-pr-bot commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

medyagh left a comment

Choose a reason for hiding this comment

tstromberg commented Apr 2, 2020

medyagh commented Apr 2, 2020 • edited Loading

Docker Driver Failures:

Known Flakes with issue:

Other failures need to confirm not related to this PR:

medyagh left a comment • edited Loading

Choose a reason for hiding this comment

tstromberg commented Apr 2, 2020

TestStartStop/group/containerd - 215.51s

TestStartStop/group/embed-certs - 173.21s

TestStartStop/group/crio - 782.71s

TestFunctional/parallel/ServiceCmd - 69.9s

TestFunctional/parallel/ProfileCmd/profile_not_create - 10.81s

medyagh commented Apr 2, 2020

minikube-pr-bot commented Apr 2, 2020

tstromberg commented Apr 1, 2020 •

edited

Loading

codecov-io commented Apr 1, 2020 •

edited

Loading

medyagh commented Apr 2, 2020 •

edited

Loading

medyagh left a comment •

edited

Loading