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.

Sign up for GitHub

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

Jump to bottom

rocket support #5518

Closed
wants to merge 15 commits into from
Closed

rocket support #5518

wants to merge 15 commits into from

Conversation

yifan-gu
Copy link
Contributor

OVERVIEW

  • This PR is an initial attempt at adding support for Rocket to Kubernetes (Support Rocket #2725). It is largely functional but not fully complete. We are seeking feedback on the approach and for some of the outstanding issues. More details below.

CURRENT STATE

  • The kubelet is able to launch/stop/reconcile pods via Rocket (also with specified mount volumes)
  • Pods/Containers are managed by systemd: the pod and container info is stored in unit files as a JSON string, which can be fetched to do comparison and reconciliation.

TODO

  • Health Probing:
    Exec in container, via nsenter directly, we have code in k8s doing this, seems to only need minor changes.
    Or we can use rkt enter.
    Edit:
    Added rocket's implementation for RunInContainer. But need to wait for refactoring ContainerCommandRunner interface.
  • Restart Policy:
    Since we use systemd to supervise container process, we can bake the restart policy in the unit file (which simplifies the sync pod logic). Unfortunately this solution is not generic for other container runtimes like docker or other future runtimes.
  • Image Puller and Security Configuration:
    Implement the image puller/throttled image puller interface for Rocket. Also need to add command line options to allow Rocket to trust image sources.
  • Port Mappings:
    This needs support from Rocket: networking: expose ports to the host rkt/rkt#624
  • Log Retrieval:
    Get log for each container in the pod.
  • Support More docker run Options:
    Such as capability add/drop (which is approachable now).
  • Inspect the Pod/Container, Get Status:
    Get more runtime information about the container, like the creation time, the exit code, etc. I think most of them can be done via systemd.
  • Add e2e Tests For K8S/Rocket:
    TBD.

ISSUE

  • TheVolumeMounts([{Name, Readonly, MountPath}, ...]) in a kubernetes container should also appear in ACI, otherwise we cannot mount the volumes to the path in the rkt container.

NEXT PLAN

  • Add the missing features for the current rocket implementation.
  • Refactor on the existing docker code to make it convergent with the new container runtime.

Thank you!

Yifan Gu added 15 commits March 16, 2015 13:02
kubelet: add container runtime interface.
6faf695
kubelet: add container runtime cache.
8823d31
kubelet: add rocket implementation for container runtime interface.
863c610
kubelet: add tests for rocket runtime.
ab58560
Godeps: update godeps for rocket.
8c692ad
kubelet: Add new sync path for rocket.
101fb77
kubelet: add manageRocketPodLoop for pod_workers.go.
699a055
kubelet: update the interface of newPodWorkers() to support rocket.
fe62e86
cmd and kubelet: add jump wire for running rocket.
a0b68fe
kubelet: update cleanupOrphanedVolumes() to take only pods as input.
800a98b
rocket: support volume mounts.
764c4e9
rocket: add test for run with mounted volumes.
3e2eedb
rocket: change the test aci.
d4c363c
kubelet: rename SyncPods and syncPod.
441e1eb 
Rename SyncPods to SyncDockerPods.
Rename syncPod to syncDockerPods.
Rename SyncRocketPods to SyncPods.
Rename syncRocketPod to syncPod.
godep: change back to older go-systemd to be compatible to libcontainer.
274701d
@yifan-gu yifan-gu changed the title rkt support rocket support Mar 16, 2015
@j3ffml j3ffml added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Mar 16, 2015
@j3ffml
Copy link
Contributor

j3ffml commented Mar 16, 2015

@dchen1107 to review/reassign

@vmarmol
Copy link
Contributor

vmarmol commented Mar 16, 2015

Happy to help @dchen1107 :)

@dchen1107
Copy link
Member

@yifan-gu Thanks for the PR.

Haven't read PR in detail yet, and here are my initial comments by reading through changes under pkg/kublet:

  1. I like runtime abstraction introduced here
  2. But I don't like make a pair of SyncDockerPods and syncDockerPod for docker containers, and introduced a new set of sync methods for Rocket. Actually the basic logic in both supporting Docker and Rocket should be same / similar. Why not coverting pkg/kubelet/dockertool to pkg/kublet/runtimetool which contains 3 different runtime libraries / plugins: docker, rocket and fakeone for testing, and let pkg/kubelet dealing with the main logic without pollution from real runtime choice?

@bgrant0607 @thockin

@yifan-gu
Copy link
Contributor Author

@dchen1107 Thanks for the comments! I definitely agree with you! I think we are on the same page that the final goal should move the docker runtime out of the kubelet's code. But that's not easy giving that we are doing optimization around the infra/network container stuff in the kubelet's sync logic, and we are tightly coupled with docker's data types in the kubelet.

So this PR is a short term solution to make k8s run rocket without breaking the docker piece. I am now trying to make the rocket's container runtime implementation and the new syncloop path more complete, and by the mean time, I am trying to figure out how to refactor the docker code to make it convergent to the runtime interface.

@dchen1107 dchen1107 mentioned this pull request Mar 16, 2015
Closed
@dchen1107
Copy link
Member

ACK on your last comment. I am ok with refactoring later. I also filed #5526 to eliminate those coupling from kubelet which makes implementing runtime plugins easier.

To make the review go smoother and faster, could you please break this PR into several smaller ones?

@yifan-gu yifan-gu mentioned this pull request Mar 17, 2015
Merged
@yifan-gu
Copy link
Contributor Author

@dchen1107
Thank you! I splitted the interface part first.

@philips
Copy link
Contributor

philips commented Mar 18, 2015

@dchen1107 @vmarmol After splitting this all out and getting incremental changes together the idea is to get this merged into master, right? We already agree that adding this abstraction for multiple runtimes is good, right?

@vmarmol
Copy link
Contributor

vmarmol commented Mar 18, 2015

@philips yes, we're all on-board getting this in :) Sorry for the delay, large PRs are scary, the smaller ones should be faster to review.

@dchen1107
Copy link
Member

@philips Yes, this is the plan.

Also @yifan-gu started to send out smaller pr #5572 based on this one. Meanwhile, there is a kubelet internal architecture issue. Ideally, I want this be introduced an another runtime plugin. The problem is that kubelet currently tightly coupled with docker runtime. see #5526 for detail, which requires a deep cleanup first, then introduce a framework for this. I definitely don't want those internal refactor and cleanup block this feature; but on another hand, I also want to make sure there is a way to get there without paying larger debt later.

@philips
Copy link
Contributor

philips commented Mar 18, 2015

@dchen1107 Absolutely, I want to make sure we get this in in the right way. I was just checking that everyone agrees that we are in fact trying to get it in with these PRs.

@bgrant0607
Copy link
Member

@philips We do want this in. We're somewhat bottlenecked on review bandwidth right now, but #5572 was reviewed and appears to be close to being merged. Please ping me by direct email if you have any concerns -- replying through github may get lost in the noise.

@dchen1107 @vmarmol @yifan-gu Consider arranging a Hangout if you're not converging in the PR discussion.

@yifan-gu
Copy link
Contributor Author

@bgrant0607 Thank you! Cleaning up the kubelet definitely needs some time (I am willing to help). Just need to make sure that temporal second code path for rocket is OK, right? @dchen1107 @vmarmol

@yifan-gu yifan-gu mentioned this pull request Mar 24, 2015
Closed
@brendandburns
Copy link
Contributor

I think that this is obsoleted by #5877? If so, I'd like to close this PR.

@yifan-gu
Copy link
Contributor Author

@brendanburns OK. Let me close

@yifan-gu yifan-gu closed this Apr 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dchen1107 dchen1107

Labels
area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yifan-gu @j3ffml @vmarmol @dchen1107 @philips @bgrant0607 @brendandburns @googlebot