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

WIP: kubelet/rkt: Support rkt for container runtime. (Splitted into other PRs) #7244

Closed
wants to merge 14 commits into from

Conversation

yifan-gu
Copy link
Contributor

Let's use this PR to track the missing functions for experimental rkt support in k8s:
(Expected version of rkt: 0.5.4)

What I am missing? @vmarmol @dchen1107 @jonboulle

Update:

  • Get container info

@yifan-gu
Copy link
Contributor Author

#7202

@dchen1107 dchen1107 self-assigned this Apr 23, 2015
@dchen1107
Copy link
Member

PR description SGTM. Is PR itself ready for review?

@yifan-gu
Copy link
Contributor Author

@dchen1107 No... I will remove WIP and split the commits once ready :)

@yifan-gu yifan-gu changed the title WIP: kubelet/rkt: Support rkt for container runtime. WIP: kubelet/rkt: Support rkt for container runtime. (Not ready for review) Apr 23, 2015
@yifan-gu
Copy link
Contributor Author

/cc @bakins

@smarterclayton
Copy link
Contributor

@pweil- please be aware of this w.r.t. security context so that the changes you are making are appropriately abstractable for Yifan

@pmorie
Copy link
Member

pmorie commented Apr 25, 2015

We also need to accommodate rocket with pre-start hooks
On Sat, Apr 25, 2015 at 1:53 PM Clayton Coleman notifications@github.com
wrote:

@pweil- https://github.com/pweil- please be aware of this w.r.t.
security context so that the changes you are making are appropriately
abstractable for Yifan


Reply to this email directly or view it on GitHub
#7244 (comment)
.

@bcbroussard
Copy link
Contributor

@yifan-gu thanks for the rkt support. I was experimenting with rkt on k8s through a privileged docker container a few days ago. Nice to see it coming in as a first class citizen.

Are you planning to accommodate per namespace trusted keys, maybe through secrets or some other means?

@smarterclayton
Copy link
Contributor

Cc @liggitt re: pull secrets - the delivery method for pull secrets to the kubelet will differ between rocket and docker unless rocket supports .dockercfg. I think we can let supporting pull secrets be a follow on for the rocket plugin.

On Apr 25, 2015, at 3:30 PM, BC Broussard notifications@github.com wrote:

@yifan-gu thanks for the rkt support. I was experimenting with rkt on k8s through a privileged docker container a few days ago. Nice to see it coming in as a first class citizen.

Are you planning to accommodate per namespace trusted keys, maybe through secrets or some other means?


Reply to this email directly or view it on GitHub.

@bcbroussard
Copy link
Contributor

Sounds good, thanks @smarterclayton

On Apr 25, 2015, at 12:44 PM, Clayton Coleman notifications@github.com wrote:

Cc @liggitt re: pull secrets - the delivery method for pull secrets to the kubelet will differ between rocket and docker unless rocket supports .dockercfg. I think we can let supporting pull secrets be a follow on for the rocket plugin.

On Apr 25, 2015, at 3:30 PM, BC Broussard notifications@github.com wrote:

@yifan-gu thanks for the rkt support. I was experimenting with rkt on k8s through a privileged docker container a few days ago. Nice to see it coming in as a first class citizen.

Are you planning to accommodate per namespace trusted keys, maybe through secrets or some other means?


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

@pweil-
Copy link
Contributor

pweil- commented Apr 27, 2015

@smarterclayton - will read through this today. Thanks for the heads up

@vmarmol
Copy link
Contributor

vmarmol commented Apr 27, 2015

btw can we get the Godeps merged first as a separate PR? GitHub chokes trying to review such a large change and doesn't let us see the important parts of this PR.

@pmorie
Copy link
Member

pmorie commented Apr 27, 2015

Do you expect to use secrets for credentials for image pull in this PR?

@yifan-gu
Copy link
Contributor Author

btw can we get the Godeps merged first as a separate PR? GitHub chokes trying to review such a large change and doesn't let us see the important parts of this PR.

Good idea. Will do after one more commit (trying to get pid, exit code in status)

@dchen1107
Copy link
Member

+1 on separate PR on Godeps. Thanks!

@yifan-gu
Copy link
Contributor Author

@pmorie Good question. I haven't plan for using secrets in this PR. I'm not sure what's the plan for secrets for image pulling. Are we going to add a credential provider for secret volumes ??

@pmorie
Copy link
Member

pmorie commented Apr 28, 2015

@yifan-gu excellent question. We definitely need to support secrets from a
service account, but I don't think you should have to make a volume in a
pod for a secret used for pull.
On Mon, Apr 27, 2015 at 10:02 PM Yifan Gu notifications@github.com wrote:

@pmorie https://github.com/pmorie Good question. I haven't plan for
using secrets in this PR. I'm not sure what's the plan for secrets for
image pulling. Are we going to add a credential provider for secret volumes
??


Reply to this email directly or view it on GitHub
#7244 (comment)
.

@smarterclayton
Copy link
Contributor

The plan is no, at this moment - pull secrets are outside of the volumes (because they are managed outside of the containers)

On Apr 27, 2015, at 10:20 PM, Paul Morie notifications@github.com wrote:

@yifan-gu excellent question. We definitely need to support secrets from a
service account, but I don't think you should have to make a volume in a
pod for a secret used for pull.
On Mon, Apr 27, 2015 at 10:02 PM Yifan Gu notifications@github.com wrote:

@pmorie https://github.com/pmorie Good question. I haven't plan for
using secrets in this PR. I'm not sure what's the plan for secrets for
image pulling. Are we going to add a credential provider for secret volumes
??


Reply to this email directly or view it on GitHub
#7244 (comment)
.


Reply to this email directly or view it on GitHub.

@yifan-gu
Copy link
Contributor Author

Got it, thanks! @smarterclayton @pmorie

@pmorie
Copy link
Member

pmorie commented Apr 28, 2015

@yifan-gu @vmarmol Is the plan still to use ContainerRuntime as the interface to docker/rkt from kubelet?

@vmarmol
Copy link
Contributor

vmarmol commented Apr 28, 2015

@pmorie yes, it may take some time to do completely though.

@yifan-gu
Copy link
Contributor Author

@vmarmol @dchen1107 I have most the missing functions implemented. Will do a cleanup and rebase.
I will try to group related functions in separate files for easy review.

Note that get container log now mixed up all container's logs within a pod. We are working on that.

Also a few functions' signature needs to sync with runtime.

@yifan-gu yifan-gu changed the title WIP: kubelet/rkt: Support rkt for container runtime. (Not ready for review) WIP: kubelet/rkt: Support rkt for container runtime. (Splitted into other PRs) Apr 29, 2015
@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 1, 2015

Splitted into:

#7605
#7599
#7589
#7555
#7553
#7550
#7549
#7545
#7543
#7526

The tests requires host to have rkt and systemd. So I think we need to put it off for now.

Closing!

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.

None yet

8 participants