Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dockerd internal port changing on restart #7021

Merged
merged 20 commits into from
Mar 13, 2020

Conversation

medyagh
Copy link
Member

@medyagh medyagh commented Mar 12, 2020

closes #7017

Gotcha 0 : driver.GetURL() is a big Lie !

GetURL in our driver is a very dirty hack, and has nothing to do with our Machine's URL it is just something to satify libmachine that wants to find out the insalled docker inside the node.
so I had to make kic driver follow hyperkit driver and make GetURL to return a BIG fat lie !
and later replace it in setupKubeConfig function in start ...very hacky and dirty
but till we come up with a solution for machine driver. that is the solution.

related: https://github.com/docker/machine/blob/master/libmachine/provision/utils.go#L159_L175

Gotcha 1 : Daemon reload

That was the ps aux params for docker in kic driver:

root@minikube:/home/docker# ps aux | grep dockerd
root     42776  2.3  1.2 2118696 98856 ?       Ssl  20:48   0:21 /usr/bin/dockerd -H fd:// --containerd=/run/containerd/containerd.sock

and this was for VM

root      2097  1.1  1.6 632732 65972 ?        Ssl  20:35   0:20 /usr/bin/dockerd -H tcp://0.0.0.0:2376 -H unix:///var/run/docker.sock --default-ulimit=nofile=1048576:1048576 --tlsverify --tlscacert /etc/docker/ca.pem --tlscert /etc/docker/server.pem --tlskey /etc/docker/server-key.pem --label provider=hyperkit --insecure-registry 10.96.0.0/12

after doing daemon-reload the KIC one also picked up the right settings.

Gotcha 2: Cert pining was missing 127.0.01

when dockerd for kic has the right parameters (same as vm) it compains about cert not having the same IP
but the cert is generated by libmachine. so I had to turn of the tls verify for docker inside minikube

Error during connect: Get https://127.0.0.1:32799/v1.40/containers/json: x509: certificate is valid for 172.17.0.2, not 127.0.0.1

for that I had to Fork libmachine and add 127.0.0.1 in addition to "localhost" to the certs
I created a PR in libmachine, till that PR gets merged, we are gonna use my fork because we need to release https://github.com/machine-drivers/machine/pull/28/files

@afbjorklund might be able to help

Gotcha 3: Libmachuine expects Docker Port to be 2376

it is poiintless to have dockerport in a variable, because of libmachine it always has to be "2376"
so I hard coded it for less confusing debugging for next person. to save some of their hair on their head.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: medyagh

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2020
@codecov-io
Copy link

codecov-io commented Mar 12, 2020

Codecov Report

Merging #7021 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7021   +/-   ##
=======================================
  Coverage   37.24%   37.24%           
=======================================
  Files         145      145           
  Lines        9115     9115           
=======================================
  Hits         3395     3395           
  Misses       5296     5296           
  Partials      424      424           
Impacted Files Coverage Δ
pkg/drivers/kvm/kvm.go 0.00% <ø> (ø)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 12, 2020
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 12, 2020
@medyagh medyagh changed the title daemon-reload daemon-reload after configuring docker service Mar 12, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 12, 2020
@k8s-ci-robot 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 12, 2020
pkg/provision/ubuntu.go Show resolved Hide resolved
pkg/provision/buildroot.go Show resolved Hide resolved
@medyagh medyagh changed the title daemon-reload after configuring docker service wip: daemon-reload after configuring docker service Mar 13, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 13, 2020
@medyagh medyagh changed the title wip: daemon-reload after configuring docker service daemon-reload after configuring docker service Mar 13, 2020
@medyagh
Copy link
Member Author

medyagh commented Mar 13, 2020

/ok-to-test

@k8s-ci-robot 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 13, 2020
@minikube-pr-bot
Copy link

Error: running mkcmp: exit status 1

pkg/drivers/kic/kic.go Outdated Show resolved Hide resolved
pkg/drivers/kic/kic.go Outdated Show resolved Hide resolved
pkg/minikube/node/config.go Outdated Show resolved Hide resolved
pkg/minikube/node/config.go Outdated Show resolved Hide resolved
pkg/provision/buildroot.go Show resolved Hide resolved
@minikube-pr-bot
Copy link

All Times minikube: [ 61.252995 62.238452 59.258249]
All Times Minikube (PR 7021): [ 60.884828 60.242879 60.475568]

Average minikube: 60.916565
Average Minikube (PR 7021): 60.534425

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7021) |
+----------------------+-----------+--------------------+
| minikube v           |  0.212867 |           0.202196 |
| Creating kvm2        | 40.520697 |          39.103108 |
| Preparing Kubernetes |  0.748157 |           0.712156 |
| Pulling images       |           |                    |
| Launching Kubernetes | 18.204043 |          19.114234 |
| Waiting for cluster  |  0.057529 |           0.058571 |
+----------------------+-----------+--------------------+

@medyagh medyagh changed the title daemon-reload after configuring docker service Fix docker-env on restart Mar 13, 2020
@medyagh medyagh changed the title Fix docker-env on restart Fix docker-env internal port changing on restart Mar 13, 2020
@medyagh medyagh changed the title Fix docker-env internal port changing on restart Fix dockerd internal port changing on restart Mar 13, 2020
pkg/provision/buildroot.go Outdated Show resolved Hide resolved
pkg/provision/ubuntu.go Outdated Show resolved Hide resolved
@minikube-pr-bot
Copy link

All Times minikube: [ 63.674208 63.763663 66.397892]
All Times Minikube (PR 7021): [ 63.934506 60.394724 63.405047]

Average Minikube (PR 7021): 62.578092
Average minikube: 64.611921

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7021) |
+----------------------+-----------+--------------------+
| minikube v           |  0.222080 |           0.209696 |
| Creating kvm2        | 41.779353 |          41.419336 |
| Preparing Kubernetes |  0.739956 |           0.753535 |
| Pulling images       |           |                    |
| Launching Kubernetes | 20.522860 |          18.946415 |
| Waiting for cluster  |  0.066132 |           0.054353 |
+----------------------+-----------+--------------------+

@minikube-pr-bot
Copy link

All Times minikube: [ 63.282492 62.515837 60.603508]
All Times Minikube (PR 7021): [ 59.616934 62.866924 64.462648]

Average Minikube (PR 7021): 62.315502
Average minikube: 62.133946

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7021) |
+----------------------+-----------+--------------------+
| minikube v           |  0.201958 |           0.204406 |
| Creating kvm2        | 40.488033 |          40.964072 |
| Preparing Kubernetes |  0.727542 |           0.736675 |
| Pulling images       |           |                    |
| Launching Kubernetes | 19.096846 |          19.051440 |
| Waiting for cluster  |  0.234358 |           0.054556 |
+----------------------+-----------+--------------------+

@minikube-pr-bot
Copy link

All Times minikube: [ 62.347032 59.214839 61.901233]
All Times Minikube (PR 7021): [ 64.140082 59.502647 65.611169]

Average minikube: 61.154368
Average Minikube (PR 7021): 63.084632

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7021) |
+----------------------+-----------+--------------------+
| minikube v           |  0.199567 |           0.225846 |
| Creating kvm2        | 39.264983 |          40.628676 |
| Preparing Kubernetes |  0.735695 |           0.732618 |
| Pulling images       |           |                    |
| Launching Kubernetes | 19.567304 |          19.894899 |
| Waiting for cluster  |  0.063252 |           0.064344 |
+----------------------+-----------+--------------------+

@medyagh medyagh merged commit 6d16146 into kubernetes:master Mar 13, 2020
@medyagh medyagh deleted the reload_dameojn branch May 2, 2020 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issue with docker-env on restart
7 participants