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

localkube: add rkt support #511

Merged
merged 1 commit into from
Sep 9, 2016
Merged

localkube: add rkt support #511

merged 1 commit into from
Sep 9, 2016

Conversation

s-urbaniak
Copy link

@s-urbaniak s-urbaniak commented Aug 23, 2016

This enables other container runtimes in minikube. While the changes here are pretty small, a bigger effort was to create an ISO that contains docker, rkt, and systemd, being ~67MB in size.

The https://buildroot.org/ based config for that image is available here: https://github.com/coreos/minikube-iso. I can include those configs in-tree in this PR too.

We are also investigating a "CoreOS-slim" based ISO image which could replace the aforementioned buildroot ISO, but as long as that is not available we could go with this solution first.

What I did for testing locally was to start minikube as follows (after building my PR branch):

$ ./out/minikube start \
    --kubernetes-version=file:///home/sur/src/minikube/src/k8s.io/minikube/out/localkube \
    --iso-url=https://github.com/coreos/minikube-iso/releases/download/v0.0.2/minikube-v0.0.2.iso \
    --vm-driver=kvm

/cc @dlorenc This PR also introduces more than I envisioned initially ;-) I added a small provisioner for the buildroot based "minikube" image, and the ISO image includes docker as well.

TODOs:

  • Use rkt with the cni plugin rather than the default no-op
  • use rkt v1.14.0 (landing on 9/1) fixing a bug in conjunction with minikube
  • Rethink/configure persisting /var/lib/kubelet

Fixes #168

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@dlorenc
Copy link
Contributor

dlorenc commented Aug 23, 2016

Nice! Looks like you need to run make gendocs to update the help text for the new flag.

Will take a detailed look today.

@dlorenc
Copy link
Contributor

dlorenc commented Aug 23, 2016

@minikube-bot ok to test

@euank
Copy link

euank commented Aug 23, 2016

Could we use rkt with the cni plugin rather than the default no-op? This would help in that the network config between docker and rkt could be identical and will allow use of podIP in the downward api correctly.

@s-urbaniak
Copy link
Author

Note to myself: rkt does not seem to detect overlay in /var/lib/rkt.

@s-urbaniak
Copy link
Author

@dlorenc short question: Is there a specific reason why /var/lib/kubelet is not being linked to the persistent volume? (see https://github.com/kubernetes/minikube/blob/master/deploy/iso/bootlocal.sh)

@dlorenc
Copy link
Contributor

dlorenc commented Aug 24, 2016

@dlorenc short question: Is there a specific reason why /var/lib/kubelet is not being linked to the persistent volume? (see https://github.com/kubernetes/minikube/blob/master/deploy/iso/bootlocal.sh)
View changes

I don't think there's any specific reason, we erred on the side of putting everything in tmpfs at the start. This makes it slightly more likely a broken setup will work after a reboot :)

Would it help any to move /var/lib/kubelet to the persistent disk? Maybe for the plugins?

@s-urbaniak
Copy link
Author

progress today:

@s-urbaniak
Copy link
Author

@euank sure, that's possible, this will have to be hard-coded though here in minikube, since this is not "default" behavior.

@codecov-io
Copy link

codecov-io commented Aug 24, 2016

Current coverage is 32.37% (diff: 52.38%)

Merging #511 into master will increase coverage by 0.15%

@@             master       #511   diff @@
==========================================
  Files            44         44          
  Lines          1859       1878    +19   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            599        608     +9   
- Misses         1138       1146     +8   
- Partials        122        124     +2   

Powered by Codecov. Last update cca3ff3...3945c3b

@s-urbaniak
Copy link
Author

@dlorenc I see /var/lib/kubelet according to the docs beng the "Directory path for managing kubelet files (volume mounts,etc).", whatever that means ;-) I see not only plugin related data in there, but also pod-related data, like tokens, secret. I think this is worth persisting. I'll recheck, and change the ISO accordingly if necessary.

@dlorenc
Copy link
Contributor

dlorenc commented Aug 25, 2016

Hey @s-urbaniak, this code looks reasonable to me. A few questions:

Is it possible to support both runtimes on the same node, or does the kubelet only support one?

Would it be possible for us to use your new ISO all the time, or does it cause trouble with the standard Docker runtime?

@euank
Copy link

euank commented Aug 25, 2016

@dlorenc Only a single container runtime may be configured for a given kubelet (though a cluster may be mixed, since this cluster is only one node it can't be).

I believe the ISO in question does include the docker daemon and work with it as well now. For making it the defualt, a quick size and boot-time comparison might be cool to have!

@s-urbaniak I'm still 👍 for cni for rkt (and docker too should be fine unless someone has a reason not to); I know it's not the default behaviour but it's the better behaviour.

@dlorenc
Copy link
Contributor

dlorenc commented Aug 25, 2016

Looking through the buildroot config, filed one issue so far: https://github.com/coreos/minikube-iso/issues/1

@s-urbaniak
Copy link
Author

Status update:

I will prepare a size/boot time comparison next week.

@dlorenc we could use the minikube ISO all the time. I am testing docker in parallel with rkt.

@pires
Copy link

pires commented Aug 28, 2016

@s-urbaniak superb!

@dlorenc
Copy link
Contributor

dlorenc commented Sep 1, 2016

@s-urbaniak thanks again for the help here! I haven't used buildroot before, but I'm trying to follow the tutorial now. Would you be willing to send me a description of how you generated that config? A gist or email would work.

@s-urbaniak
Copy link
Author

@dlorenc nearly there, only CNI is missing. I uploaded another ISO https://github.com/coreos/minikube-iso/releases/tag/v0.0.2 having today's rkt v1.14.0 release, and 9P kernel modules. I also added some more instructions in https://github.com/coreos/minikube-iso#hacking on how to change the buildroot/kernel config.

@s-urbaniak
Copy link
Author

@dlorenc I added CNI support, which can be enabled using --network-plugin=cni. The minikube-iso image also provides the necessary additional plugins, as well as some default config, see https://github.com/coreos/minikube-iso/releases/tag/v0.0.3.

So what do you think, could we land this?

@s-urbaniak
Copy link
Author

@euank /cc I made minikube configurable to use --network-plugin=cni, it works now in both modes with rtknetes@minikube.

@dlorenc
Copy link
Contributor

dlorenc commented Sep 8, 2016

Nice! Yeah, let's get this going :)

I'd like to move the iso code into our tree at some point, but we can do that later.

I'll make a final pass on this today.

@dlorenc dlorenc added this to the 0.10 Release milestone Sep 8, 2016
@r2d4
Copy link
Contributor

r2d4 commented Sep 8, 2016

this is looking cool, thanks @s-urbaniak!

@euank
Copy link

euank commented Sep 8, 2016

This LGTM, though I'm not super familiar with the docker-machine/provisioner bits so I couldn't really review that part well.

@@ -29,7 +29,7 @@ var stopCommand = "sudo killall localkube || true"

var startCommandFmtStr = `
# Run with nohup so it stays up. Redirect logs to useful places.
sudo sh -c 'PATH=/usr/local/sbin:$PATH nohup /usr/local/bin/localkube %s --generate-certs=false --logtostderr=true --node-ip=%s > %s 2> %s < /dev/null &'
sudo sh -c 'PATH=/usr/local/sbin:$PATH nohup /usr/local/bin/localkube %s --network-plugin=%s --container-runtime=%s --generate-certs=false --logtostderr=true --node-ip=%s > %s 2> %s < /dev/null &'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind only inserting these new flags if the user has set them?

We have some alpha builds of localkube published that will crash if these flags are passed. See #512 for the bug tracking the main issue.

If this makes the formatting get a bit unwieldy it might be a good idea to switch to a text/template.

@dlorenc
Copy link
Contributor

dlorenc commented Sep 8, 2016

Just one nit about flag passing.

@s-urbaniak
Copy link
Author

@dlorenc addressed the review comment, PTAL.

@jonboulle
Copy link
Contributor

👍

@dlorenc
Copy link
Contributor

dlorenc commented Sep 9, 2016

LGTM thanks!

@dlorenc dlorenc merged commit 270f182 into kubernetes:master Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support rkt.
9 participants