-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
Can one of the admins verify this patch? |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@minikube-bot ok to test |
sorry. I had to update the checksum. Please retest. |
@minikube-bot retest this please |
1 similar comment
@minikube-bot retest this please |
interesting that the cross build fails on a |
d0110e6
to
71b5e6d
Compare
updated and rebased |
508fd6f
to
4d4331f
Compare
@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 |
@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 |
@r2d4 i expect leaky abstractions to be going away. But while testing this, it is both docker and rkt services are started. |
ohman go1.7. Let me work on getting go1.8 into the build 🙏 |
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 the docker image will build with go 1.8 |
How does that work for the jenkins job? |
Jenkins just calls that script for the cross build job |
ah. lemme give it a try |
while that will show that it builds in such an environment, that line change would affect the ISO release process, no? |
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 |
fe882ac
to
55194b1
Compare
hmm. Still appears to be failing due to go1.7 compiler |
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. |
upgraded @minikube-bot retest this please |
I'll remove that docker variable when I get to my desk
|
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 <[email protected]>
i've removed that 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. |
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. |
After this is merged I'll create an integration test using the Pulled it down locally and ran it, seems to be working fine. Is this expected on
|
cool. Like I said, the --insecure-registry still needs to be plumbed
through. And wouldn't mind getting the provisioner logic to result in fewer
services. Those can be done in future PRs.
…On Wed, Sep 27, 2017 at 12:51 PM Gerard Braad ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1998 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEF6WqnkTOpuwq4DIkbV8Sl03BRnUSlks5smn0XgaJpZM4Pg4vc>
.
|
image_volumes = "mkdir" | ||
|
||
# insecure_registries is used to skip TLS verification when pulling images. | ||
insecure_registries = [ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Thanks for working through this! |
@r2d4 when I run: kubectl run httpd --image=httpd
kubectl describe pod httpd I see
|
oh sorry, you said describe node, yes i see that same thing. I'll check into that. |
👍 |
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
), whilethe 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 [email protected]