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

minikube-iso: add cri-o runtime #1998

Merged
merged 1 commit into from
Sep 27, 2017
Merged

minikube-iso: add cri-o runtime #1998

merged 1 commit into from
Sep 27, 2017

Conversation

vbatts
Copy link
Contributor

@vbatts vbatts commented Sep 22, 2017

https://github.com/kubernetes-incubator/cri-o

Updated the runc version to its latest master commit.
Got crio into the automounter to get off the tmpfs

This feature bubbles up to the minikube command by reusing the
--container-runtime= flag, by enabling the value of "crio" (minikube start --container-runtime=crio), while
the flags/config passed to localkube are more like k8s
(--container-runtime=remote --remote-runtime-endpoint=/var/run/crio.sock)

This is mostly ready for review. It is still lacking having
--insecure-registry plumbed through, but for now the policy.json is
open.

Will rebase on crio master once cri-o/cri-o#940 is merged.

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 22, 2017
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@codecov-io
Copy link

codecov-io commented Sep 22, 2017

Codecov Report

Merging #1998 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1998      +/-   ##
==========================================
- Coverage   29.85%   29.81%   -0.04%     
==========================================
  Files          77       77              
  Lines        4763     4769       +6     
==========================================
  Hits         1422     1422              
- Misses       3161     3167       +6     
  Partials      180      180
Impacted Files Coverage Δ
pkg/localkube/localkube.go 51.92% <ø> (ø) ⬆️
pkg/localkube/kubelet.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7bb7c3...2788714. Read the comment docs.

@r2d4
Copy link
Contributor

r2d4 commented Sep 22, 2017

@minikube-bot ok to test

@vbatts
Copy link
Contributor Author

vbatts commented Sep 22, 2017

sorry. I had to update the checksum. Please retest.

@r2d4
Copy link
Contributor

r2d4 commented Sep 22, 2017

@minikube-bot retest this please

1 similar comment
@r2d4
Copy link
Contributor

r2d4 commented Sep 23, 2017

@minikube-bot retest this please

@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

interesting that the cross build fails on a mkdir, i'll look into why that fails

@vbatts vbatts force-pushed the crio-bin branch 2 times, most recently from d0110e6 to 71b5e6d Compare September 25, 2017 14:20
@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

updated and rebased

@vbatts vbatts force-pushed the crio-bin branch 2 times, most recently from 508fd6f to 4d4331f Compare September 25, 2017 16:56
@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

@r2d4 once I get the dust settled for this addition, a next step will be adding logic to the provisioner about not starting services that are unneeded for particular --container-runtime=. Opinions about that?

@r2d4
Copy link
Contributor

r2d4 commented Sep 25, 2017

@vbatts I'm guessing you mean the docker service primarily? I think that should be fine and I'm open to whatever changes we need to make, although you might encounter some difficultly with the leaky abstractions in that a few things might wait on the docker endpoint to be available.

We're also planning to hopefully move away from localkube in the future and use kubeadm, for which we already have the --bootstrapper kubeadm flag for. I plan on sending a PR soon to make sure that all the configuration we set for localkube gets passed through kubeadm similarly. I can make sure this works for that bootstrapper, but just something to keep in mind.

@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

@r2d4 i expect leaky abstractions to be going away. But while testing this, it is both docker and rkt services are started.

@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

ohman go1.7. Let me work on getting go1.8 into the build 🙏

@r2d4
Copy link
Contributor

r2d4 commented Sep 25, 2017

Ah, actually we have a dockerized build, if you add this

https://github.com/kubernetes/minikube/blob/master/hack/jenkins/minikube_cross_build_and_upload.sh#L38
This line should be changed to:
IN_DOCKER=1 make out/minikube.iso

the docker image will build with go 1.8

@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

How does that work for the jenkins job?

@r2d4
Copy link
Contributor

r2d4 commented Sep 25, 2017

Jenkins just calls that script for the cross build job

@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

ah. lemme give it a try

@vbatts
Copy link
Contributor Author

vbatts commented Sep 25, 2017

while that will show that it builds in such an environment, that line change would affect the ISO release process, no?

@r2d4
Copy link
Contributor

r2d4 commented Sep 25, 2017

Ah, you're right. I thought our release process built the ISO in docker also, but it doesn't. The makefile rules around the ISO could use some refactoring.

diff --git a/Makefile b/Makefile
index a3f7e48b4..3abad1a3e 100755
--- a/Makefile
+++ b/Makefile
@@ -288,7 +288,7 @@ $(ISO_BUILD_IMAGE): deploy/iso/minikube-iso/Dockerfile
        @echo "$(@) successfully built"
 
 .PHONY: release-iso
-release-iso: minikube_iso checksum
+release-iso: out/minikube.iso checksum
        gsutil cp out/minikube.iso gs://$(ISO_BUCKET)/minikube-$(ISO_VERSION).iso
        gsutil cp out/minikube.iso.sha256 gs://$(ISO_BUCKET)/minikube-$(ISO_VERSION).iso.sha256
 
diff --git a/hack/jenkins/build_iso.sh b/hack/jenkins/build_iso.sh
index 32cc67131..6d76be548 100644
--- a/hack/jenkins/build_iso.sh
+++ b/hack/jenkins/build_iso.sh
@@ -20,4 +20,4 @@
 #      ISO_VERSION = the suffix for the iso (i.e. minikube-$(ISO_VERSION).iso)
 
 set -e
-${ARGS} make release-iso
+${ARGS} IN_DOCKER=1 make release-iso

That patch should fix the release job also

@vbatts vbatts force-pushed the crio-bin branch 3 times, most recently from fe882ac to 55194b1 Compare September 26, 2017 14:32
@vbatts
Copy link
Contributor Author

vbatts commented Sep 26, 2017

hmm. Still appears to be failing due to go1.7 compiler

@r2d4
Copy link
Contributor

r2d4 commented Sep 26, 2017

Ok, suspiciously there isn't even a go tool chain in the docker image we use to build the iso. I'm not even sure why we use a different image to build the iso, and I don't think that image is being build everytime.

I'll just go ahead and update the build slave to go 1.8, since that seems like the path of least resistance here. I'll update this issue when its upgraded.

@r2d4
Copy link
Contributor

r2d4 commented Sep 26, 2017

upgraded

@minikube-bot retest this please

@vbatts
Copy link
Contributor Author

vbatts commented Sep 26, 2017 via email

https://github.com/kubernetes-incubator/cri-o

Updated the runc version to its latest master commit.
Got crio into the automounter to get off the tmpfs

This feature bubbles up to the minikube command by reusing the
`--container-runtime=` flag, by enabling the value of "`crio`"
(`minikube start --container-runtime=crio`), while the flags/config
passed to localkube are more like k8s (`--container-runtime=remote
--remote-runtime-endpoint=/var/run/crio.sock`)

This is mostly ready for review. It is still lacking having
--insecure-registry plumbed through, but for now the policy.json is
open.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts
Copy link
Contributor Author

vbatts commented Sep 27, 2017

i've removed that IN_DOCKER=1 make out/minikube.iso commit.

Also, with cri-o/cri-o#940 merged, I've switched back to cri-o master commit (not pointing to my branch).

Not sure why the windows environment failed, but we'll see if it works this next time.

@r2d4
Copy link
Contributor

r2d4 commented Sep 27, 2017

the windows machine is in a bad state and we've had some problems with it in the past, since the tests passed on most of the other platforms, I think it should be fine.

@r2d4
Copy link
Contributor

r2d4 commented Sep 27, 2017

After this is merged I'll create an integration test using the --container-runtime=crio flag, since theres a little bit of setup on our side for adding a new integration test.

Pulled it down locally and ran it, seems to be working fine.

Is this expected on kubectl describe node:

Container Runtime Version: runc://Unknown?

@vbatts
Copy link
Contributor Author

vbatts commented Sep 27, 2017 via email

image_volumes = "mkdir"

# insecure_registries is used to skip TLS verification when pulling images.
insecure_registries = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to write this file dynamically like we do the docker systemd unit to pass through things like insecure registries. I think this is fine for now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once i saw that there were dynamic service files generated, I thought the same thing.

@r2d4 r2d4 merged commit a90b6a4 into kubernetes:master Sep 27, 2017
@r2d4
Copy link
Contributor

r2d4 commented Sep 27, 2017

Thanks for working through this!

@vbatts vbatts deleted the crio-bin branch September 27, 2017 17:18
@vbatts
Copy link
Contributor Author

vbatts commented Sep 27, 2017

@r2d4 when I run:

kubectl run httpd --image=httpd
kubectl describe pod httpd

I see

[...]
    Container ID:       runc://1d753ae46099f0a2dca86b1d658d913bcabbc9d48c96b4d2bd56b8857481c413
[...]

@vbatts
Copy link
Contributor Author

vbatts commented Sep 27, 2017

oh sorry, you said describe node, yes i see that same thing. I'll check into that.

@rhatdan
Copy link

rhatdan commented Sep 27, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants