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

use sidecar container to execute rbd command #13691

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@rootfs
Copy link
Member

rootfs commented Sep 8, 2015

This is a validation for proposal #14288

@googlebot googlebot added the cla: yes label Sep 8, 2015

use sidecar container to execute rbd command
Signed-off-by: Huamin Chen <hchen@redhat.com>

@rootfs rootfs force-pushed the rootfs:container_exec branch from 09779d9 to ea955f9 Sep 8, 2015

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 8, 2015

GCE e2e build/test passed for commit 09779d9.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 8, 2015

GCE e2e build/test passed for commit ea955f9.

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 8, 2015

GCE e2e build/test passed for commit 01a3b32.

attach stdout/stderr to container
Signed-off-by: Huamin Chen <hchen@redhat.com>

@rootfs rootfs force-pushed the rootfs:container_exec branch from 01a3b32 to b82fbfd Sep 9, 2015

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Sep 9, 2015

GCE e2e build/test passed for commit b82fbfd.

@rootfs

This comment has been minimized.

Copy link
Member

rootfs commented Sep 10, 2015

@dchen1107 feedback appreciated!

func (kl *Kubelet) RunContainerCommand(pod *api.Pod, container *api.Container, cmd []string) ([]byte, error) {
// Mount volumes.
podFullName := kubecontainer.GetPodFullName(pod)
podVolumes, err := kl.mountExternalVolumes(pod)

This comment has been minimized.

@pmorie

pmorie Sep 10, 2015

Member

why is the mountExternalVolumes call necessary here?

This comment has been minimized.

@rootfs

rootfs Sep 10, 2015

Member

kubelet builds volume first then creates the pod. if there is no volume, kubelet will complain.

This comment has been minimized.

@pmorie

pmorie Sep 19, 2015

Member

You might want to enforce that the pod is run-once here.

@dchen1107

This comment has been minimized.

Copy link
Member

dchen1107 commented Sep 18, 2015

cc/ @bgrant0607 on API changes.
cc/ @saad-ali on persistentvolume_recycler_controller.go

@@ -988,6 +990,12 @@ type Container struct {
// Whether this container should allocate a TTY for itself, also requires 'stdin' to be true.
// Default is false.
TTY bool `json:"tty,omitempty"`
// Whether this container should send stdout.

This comment has been minimized.

@bgrant0607

bgrant0607 Sep 18, 2015

Member

Send it where?

This comment has been minimized.

@bgrant0607

bgrant0607 Sep 18, 2015

Member

Note that we already have the ability to attach to running containers.

https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/attach.go

This comment has been minimized.

@rootfs

rootfs Sep 21, 2015

Member

stdout is to tell if container out is sent to stdout. The attach ability is not used here because the container could just exit after exec, attaching to an exited container just fails.

@bgrant0607

This comment has been minimized.

@@ -582,6 +582,8 @@ type RBDVolumeSource struct {
// Defaults to false.
// More info: http://releases.k8s.io/HEAD/examples/rbd/README.md#how-to-use-it
ReadOnly bool `json:"readOnly,omitempty"`
// Optional: Sidecar is the sidecar container's name
Sidecar string `json:"sidecar,omitempty"`

This comment has been minimized.

@bgrant0607

bgrant0607 Sep 18, 2015

Member

Neither the name of this field nor the comment helps me understand what this is or what its intended usage is. Is this a container or an image? If a container, is it Docker's name for the container, or something else? What are you trying to do?

Why is this exposed by the API at all? My interpretation of #13138 is that you wanted a container-based plugin mechanism. Kubelet plugins shouldn't be exposed via the user-facing API.

This comment has been minimized.

@pmorie

pmorie Sep 19, 2015

Member

Why is this exposed by the API at all? My interpretation of #13138 is that you wanted a container-based plugin mechanism. Kubelet plugins shouldn't be exposed via the user-facing API.

+1

This is really the name of an image to used to run the rbd client in a pod. I'm not sure I would call it a sidecar container, since it runs in a dedicated pod. I agree that we should nit expose this information via the API. I like the direction the the factoring in the volume subsystem is going, but the API changes should go away. Instead this should be a Kubelet property.

@rootfs

This comment has been minimized.

@rootfs

rootfs Sep 21, 2015

Member

Sidecar is the image name. It is part of the pod API that user can specify so the appropriate container image can be started to execute the command.

This comment has been minimized.

@markt-gh

markt-gh Sep 21, 2015

Does the image name need to vary by volume or would a single image name
suffice for all instances of the type?

The new VolumeConfig stuff is meant to easily do the latter. An admin can
configure the image to use.

On Monday, September 21, 2015, Huamin Chen notifications@github.com wrote:

In pkg/api/v1/types.go
#13691 (comment)
:

@@ -582,6 +582,8 @@ type RBDVolumeSource struct {
// Defaults to false.
// More info: http://releases.k8s.io/HEAD/examples/rbd/README.md#how-to-use-it
ReadOnly bool json:"readOnly,omitempty"

  • // Optional: Sidecar is the sidecar container's name
  • Sidecar string json:"sidecar,omitempty"

Sidecar is the image name. It is part of the pod API that user can specify
so the appropriate container image can be started to execute the command.


Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/13691/files#r39969486.

This comment has been minimized.

@rootfs

rootfs Sep 21, 2015

Member

i expect image name vary by volume. I use ceph/base in this example. This image is maintained by ceph community. There could also be a glusterfs image to do glusterfs mount, and yet another image for e.g. fibre channel/iSCSI related exec.

GenerateName: "rbd-sidecar-",
Namespace: api.NamespaceDefault,
},
Spec: api.PodSpec{

This comment has been minimized.

@pmorie

pmorie Sep 19, 2015

Member

Don't you need to set the NodeName in the pod soec to ensure this pod runs on the same node?

This comment has been minimized.

@rootfs

rootfs Sep 21, 2015

Member

since it is invoked by kubelet, the pod runs on the same node.

if err := dm.GetContainerLogs(pod, id, "all", true, &stdout, &stderr); err != nil {
return nil, err
}
out := append(stdout.Bytes(), stderr.Bytes()...)

This comment has been minimized.

@ncdc

ncdc Sep 21, 2015

Member

I don't think it makes sense to concatenate stdout and stderr. How about returning both separately?

This comment has been minimized.

@rootfs

rootfs Sep 21, 2015

Member

@ncdc yes I like your point. The idea here is to reach the parity with exec.CombinedOutput

@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Apr 26, 2016

This PR has had no meaningful activity for multiple months. If it is still valid please rebase, push a new commit and reopen the PR. Thanks!

@fejta fejta closed this Apr 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment