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

kubelet/rkt: Add routines for converting kubelet pod to rkt pod. #7543

Merged
merged 1 commit into from May 1, 2015

Conversation

Projects
None yet
4 participants
@yifan-gu
Copy link
Member

commented Apr 30, 2015

@vmarmol @dchen1107 @jonboulle

This creates the rkt pod manifest from k8s pod spec.

There are some places where RunContainerOptions should be used instead of reading the container's spec directly. But that requires some refactor on the RunContainerOptions (such as add volume/port names), so I leave a TODO for now.

@googlebot googlebot added the cla: yes label Apr 30, 2015

@yifan-gu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

This follows #7465

CAP_AUDIT_READ
)

var capalibityList = map[int]string{

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

capabilityList

caps = getCapabilities(c.Capabilities.Add)
}
if len(caps) > 0 {
value = []byte(fmt.Sprintf(`{"name":"os/linux/capabilities-retain-set","value":{"set":[%s]}`, caps))

This comment has been minimized.

Copy link
@jonboulle

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

Thanks @jonboulle

func setIsolators(app *appctypes.App, c *api.Container) error {
var isolator appctypes.Isolator
if len(c.Capabilities.Add) > 0 || len(c.Capabilities.Drop) > 0 || len(c.Resources.Limits) > 0 || len(c.Resources.Requests) > 0 {
app.Isolators = []appctypes.Isolator{}

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

isn't this the nil value anyway?

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

nope, the app is from the imageManifest, so it can have some non-empty isolators.
What I am doing here is to override the isolators if any isolators are specified in the pod spec
@jonboulle

// TODO(yifan): Return error?
}
var caps string
var value []byte

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

imho this is a scope leak, I'd just := it inline where it's used

}

// TODO(yifan): Use non-root user in the future?
// Currently it's a bug as reported https://github.com/coreos/rkt/issues/539.

This comment has been minimized.

Copy link
@jonboulle
for _, p := range c.Ports {
portName, err := appctypes.NewACName(p.Name)
if err != nil {
glog.Errorf("Cannot use the port's name %q as ACName: %v", p.Name, err)

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

should we just sanitise here or is that not safe? (how are these subsequently used?)

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

@jonboulle For example: yifan-gu@8d1d516#diff-717f367da08d0bb8eb8c45d38f164eb8R350

I think it has a potential danger when two names after being sanitised become the same. But thta's not likely to happen.
I will print a warning, and not return.

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

@jonboulle Well, I changed my mind. I think we should just stop guessing user. Let user fix the naming problem for now!

//
// https://github.com/coreos/rkt/issues/723.
//
ds, err := store.NewStore(rktDataDir)

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

nit, s#ds#s#, ds was a legacy of downloadStore

for _, port := range c.Ports {
portName, err := appctypes.NewACName(port.Name)
if err != nil {
glog.Errorf("Cannot use the volume's name %q as ACName: %v", port.Name, err)

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

s#volume#port#

return manifest, nil
}

// TODO(yifan): Need to implement image management in rkt.

This comment has been minimized.

Copy link
@jonboulle

jonboulle Apr 30, 2015

Contributor

is this a personal note? or some outstanding rkt issues?

@vmarmol vmarmol self-assigned this Apr 30, 2015

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from 8d1d516 to be5db44 Apr 30, 2015

@vmarmol

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

Needs a rebase btw

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from be5db44 to 8c15cb9 Apr 30, 2015

@yifan-gu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

Rebased @vmarmol

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from 8c15cb9 to dab1fab Apr 30, 2015

func getCapabilities(caps []api.CapabilityType) string {
var capList []string
for _, cap := range caps {
capList = append(capList, fmt.Sprintf("%q", cap))

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Won't this just print out the integer value? I think we need to do the lookup in the map above

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

This is the api.CabapilityType, which is a string.
Actually we should have the capability list defined in kubelet. Let me TODO it :)

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Ah I see, yes you're right.

CAP_AUDIT_READ
)

var capabilityList = map[int]string{

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

nit: s/capabilityList/allCapabilities/ it's a map too :)

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from dab1fab to ec65d6e Apr 30, 2015

"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
)

// TODO(yifan): Export this to higher level package.

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Do we use this anywhere? It seems like we always use the string representation no?

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

@vmarmol I mean we should define those string somewhere. At least it can help us check the user input in pod spec?

if !strings.HasPrefix(last, "sha512-") {
return "", fmt.Errorf("unexpected result: %q", last)
}
return last, err

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

nit: s/err/nil/ to be clearer

privileged = c.Privileged
} else if c.Privileged {
glog.Errorf("rkt: Privileged is disallowed globally")
// TODO(yifan): Return error?

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Yeah, lets fail here

case api.ResourceMemory:
name = "resource/memory"
default:
glog.Warningf("Resource type not supported: %v", name)

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

add a continue

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Or better yet, an error :)

default:
glog.Warningf("Resource type not supported: %v", name)
}
value := []byte(fmt.Sprintf(`"name":%q,"value":{"request":%q,"limit":%q}`, name, res.request, res.limit))

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Can we build the struct here instead of unmarshalling JSON? I don't think we have the constructor problem from above.

}

// Resources.
resources := make(map[api.ResourceName]resource)

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Since resource is not a pointer we can't edit it in place. We're modifying a copy:

http://play.golang.org/p/eYtNFuq5N8

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

Good catch.

for name, quantity := range c.Resources.Requests {
r, ok := resources[name]
if !ok {
r = resource{}

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

We never actually add it to the map here

This comment has been minimized.

Copy link
@yifan-gu

yifan-gu Apr 30, 2015

Author Member

Still use non-pointer, but update the map. @vmarmol

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from ec65d6e to 93de5d6 Apr 30, 2015

@yifan-gu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

@vmarmol Thanks for the review! Comments addressed. Probably need another look :)

for _, m := range c.VolumeMounts {
mountPointName, err := appctypes.NewACName(m.Name)
if err != nil {
glog.Errorf("rkt: Cannot use the volume mount's name %q as ACName: %v", m.Name, err)

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

I'd argue we shouldn't log here (and below) since we are returning the error. We can make a new error with more data and let higher layers log if they'd like

}
hash, err := appctypes.NewHash(imageID)
if err != nil {
glog.Errorf("rkt: Cannot create new hash from %q", imageID)

This comment has been minimized.

Copy link
@vmarmol

vmarmol Apr 30, 2015

Contributor

Yeah we probably should not log and return an error everywhere

@vmarmol

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

This looks good. I am unsure about some of the detail of the rkt config setup, but we can iterate on those with future PRs. Good to merge after the nits above.

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from 93de5d6 to 7dbb592 Apr 30, 2015

@yifan-gu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

@vmarmol Thanks! You are right, I changed all the logs with returning new errors

@vmarmol

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

LGTM, thanks @yifan-gu!

@vmarmol

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

Will need a rebase @yifan-gu

@yifan-gu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

Roger @vmarmol

@yifan-gu yifan-gu changed the title Pod manifest kubelet/rkt: Add routines for converting kubelet pod to rkt pod. Apr 30, 2015

@yifan-gu yifan-gu force-pushed the yifan-gu:pod_manifest branch from 7dbb592 to 7e8afc7 Apr 30, 2015

@vmarmol

This comment has been minimized.

Copy link
Contributor

commented May 1, 2015

Merging as this is green, thanks @yifan-gu!

vmarmol added a commit that referenced this pull request May 1, 2015

Merge pull request #7543 from yifan-gu/pod_manifest
kubelet/rkt: Add routines for converting kubelet pod to rkt pod.

@vmarmol vmarmol merged commit 262c34e into kubernetes:master May 1, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed

@yifan-gu yifan-gu deleted the yifan-gu:pod_manifest branch May 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.