kic: Use bind IP for minikube IP on linux #7267

priyawadhwa · 2020-03-26T21:00:50Z

We want to return the external ip when running $(minikube ip). This was causing a bug in TestAddons/parallel/Registry, which expects $(minikube ip):5000 to connect to an nginx service when the registry addon is enabled.

make integration -e TEST_ARGS="-test.run TestAddons/parallel/Registry --profile=minikube --cleanup=false --minikube-start-args="--driver=docker""

fixes #7191

The registry addon expects $(minikube ip):5000 to be exposed. Since `minikube ip` is localhost for the docker driver, we need to expose port 5000 on creation in case the registry addon is running so that this works as expected.

k8s-ci-robot · 2020-03-26T21:01:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: priyawadhwa

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

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

priyawadhwa · 2020-03-26T21:03:08Z

/ok-to-test

minikube-pr-bot · 2020-03-26T21:12:36Z

Error: running mkcmp: exit status 1

We want to return the external ip when running $(minikube ip). This was causing a bug in `TestAddons/parallel/Registry`, which expects $(minikube ip):5000 to connect to an nginx service when the registry addon is enabled.

…testaddons-registry

cmd/minikube/cmd/docker-env.go

tstromberg · 2020-03-26T22:37:27Z

/ok-to-test

codecov-io · 2020-03-26T22:38:35Z

Codecov Report

Merging #7267 into master will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #7267      +/-   ##
==========================================
- Coverage   37.86%   37.84%   -0.03%     
==========================================
  Files         147      147              
  Lines        9089     9094       +5     
==========================================
  Hits         3442     3442              
- Misses       5222     5227       +5     
  Partials      425      425

Impacted Files	Coverage Δ
cmd/minikube/cmd/docker-env.go	`46.07% <0.00%> (-2.38%)`	⬇️

minikube-pr-bot · 2020-03-26T22:49:38Z

All Times minikube: [ 62.954377 64.257964 64.012811]
All Times Minikube (PR 7267): [ 61.605680 63.319693 64.318429]

Average minikube: 63.741717
Average Minikube (PR 7267): 63.081267

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7267) |
+----------------------+-----------+--------------------+
| minikube v           |  0.217919 |           0.206064 |
| Creating kvm2        | 40.587631 |          40.061374 |
| Preparing Kubernetes | 21.187745 |          21.307882 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

medyagh

need to verify this on mac os. (our CI machines for mac os dont work) so need to verify the tests manaully

medyagh · 2020-03-26T23:28:55Z

pkg/minikube/mustload/mustload.go

@@ -111,10 +110,6 @@ func Running(name string) ClusterController {
 		exit.WithError("Unable to get driver IP", err)
 	}

-	if driver.IsKIC(host.DriverName) {


If I am not wrong, I believe we still need this for macOS.
we can not hit the IP of the container but the DefaultBind 127.0.0.1

priyawadhwa · 2020-03-26T23:35:53Z

So on Mac I'm getting an i/o timeout:

=== RUN   TestAddons
=== RUN   TestAddons/parallel
=== RUN   TestAddons/parallel/Registry
=== PAUSE TestAddons/parallel/Registry
=== CONT  TestAddons/parallel/Registry
2020/03/26 16:32:07 [DEBUG] GET http://172.17.0.2:5000
2020/03/26 16:32:37 [ERR] GET http://172.17.0.2:5000 request failed: Get http://172.17.0.2:5000: dial tcp 172.17.0.2:5000: i/o timeout
2020/03/26 16:32:37 [DEBUG] GET http://172.17.0.2:5000: retrying in 1s (4 left)
2020/03/26 16:33:08 [ERR] GET http://172.17.0.2:5000 request failed: Get http://172.17.0.2:5000: dial tcp 172.17.0.2:5000: i/o timeout

tstromberg · 2020-03-28T18:06:51Z

If I understand, this test can't really work on macOS, because the port isn't exposed.

@medyagh - What's the mechanism that regular users should be using to expose this port properly? The test should be using that, at least in the IsKIC() case.

afbjorklund · 2020-04-01T16:08:26Z

Not sure there is anything proper about 5000, probably should use a tunnel to expose it ?

Where docker0 is broken

On linux, we want to keep the IP set to the internal IP. This is because this IP is exposed and so we can use it on Linux. This doesn't fix the IP issue on mac or windows, it's just a temporary fix for the linux case. We will need to find a way to expose internal ports on Mac/Windows so that the registry addon works as expected on those platforms.

priyawadhwa · 2020-04-01T17:51:09Z

cc @medyagh PTAL as a temporary fix on linux

minikube-pr-bot · 2020-04-01T18:01:29Z

All Times minikube: [ 64.116723 58.581442 63.641151]
All Times Minikube (PR 7267): [ 63.333213 63.467703 61.655178]

Average minikube: 62.113105
Average Minikube (PR 7267): 62.818698

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7267) |
+----------------------+-----------+--------------------+
| minikube v           |  0.214848 |           0.208009 |
| Creating kvm2        | 39.665113 |          40.325322 |
| Preparing Kubernetes | 20.703763 |          20.505551 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

Expose port 5000 to host machine for docker/podman drivers

7067916

The registry addon expects $(minikube ip):5000 to be exposed. Since `minikube ip` is localhost for the docker driver, we need to expose port 5000 on creation in case the registry addon is running so that this works as expected.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 26, 2020

k8s-ci-robot requested review from afbjorklund and medyagh March 26, 2020 21:01

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 added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 26, 2020

Priya Wadhwa added 2 commits March 26, 2020 15:15

master

6336323

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

priyawadhwa changed the title ~~Expose port 5000 to host machine for docker/podman drivers~~ kic: Use bind IP for docker-env command and external IP otherwise Mar 26, 2020

Merge branch 'master' of https://github.com/kubernetes/minikube into …

8ae97bc

…testaddons-registry

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

tstromberg reviewed Mar 26, 2020

View reviewed changes

cmd/minikube/cmd/docker-env.go Outdated Show resolved Hide resolved

code review

bd482df

medyagh requested changes Mar 26, 2020

View reviewed changes

tstromberg mentioned this pull request Mar 29, 2020

mustload: Replace DriverIP with Forwarded(IP|Port) #7310

Merged

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

Rebased on master

c047e52

Priya Wadhwa added 2 commits April 1, 2020 10:43

Reverted back to original state

8defa94

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 1, 2020

priyawadhwa changed the title ~~kic: Use bind IP for docker-env command and external IP otherwise~~ kic: Use bind IP for minikube IP on linux Apr 1, 2020

priyawadhwa closed this Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kic: Use bind IP for minikube IP on linux #7267

kic: Use bind IP for minikube IP on linux #7267

priyawadhwa commented Mar 26, 2020 •

edited

Loading

k8s-ci-robot commented Mar 26, 2020

priyawadhwa commented Mar 26, 2020

minikube-pr-bot commented Mar 26, 2020

tstromberg commented Mar 26, 2020

codecov-io commented Mar 26, 2020 •

edited

Loading

minikube-pr-bot commented Mar 26, 2020

medyagh left a comment

medyagh Mar 26, 2020

priyawadhwa commented Mar 26, 2020

tstromberg commented Mar 28, 2020

afbjorklund commented Apr 1, 2020

priyawadhwa commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

kic: Use bind IP for minikube IP on linux #7267

kic: Use bind IP for minikube IP on linux #7267

Conversation

priyawadhwa commented Mar 26, 2020 • edited Loading

k8s-ci-robot commented Mar 26, 2020

priyawadhwa commented Mar 26, 2020

minikube-pr-bot commented Mar 26, 2020

tstromberg commented Mar 26, 2020

codecov-io commented Mar 26, 2020 • edited Loading

Codecov Report

minikube-pr-bot commented Mar 26, 2020

medyagh left a comment

Choose a reason for hiding this comment

medyagh Mar 26, 2020

Choose a reason for hiding this comment

priyawadhwa commented Mar 26, 2020

tstromberg commented Mar 28, 2020

afbjorklund commented Apr 1, 2020

priyawadhwa commented Apr 1, 2020

minikube-pr-bot commented Apr 1, 2020

priyawadhwa commented Mar 26, 2020 •

edited

Loading

codecov-io commented Mar 26, 2020 •

edited

Loading