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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"` | ||
} | ||
|
||
// CinderVolumeSource represents a cinder volume resource in Openstack. | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Send it where? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that we already have the ability to attach to running containers. https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/attach.go There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// Default is false. | ||
Stdout bool `json:"stdout,omitempty"` | ||
// Whether this container should send stderr. | ||
// Default is false. | ||
Stderr bool `json:"stderr,omitempty"` | ||
} | ||
|
||
// Handler defines a specific action that should be taken | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -663,8 +663,10 @@ func (dm *DockerManager) runContainer( | |
WorkingDir: container.WorkingDir, | ||
Labels: labels, | ||
// Interactive containers: | ||
OpenStdin: container.Stdin, | ||
Tty: container.TTY, | ||
OpenStdin: container.Stdin, | ||
Tty: container.TTY, | ||
AttachStdout: container.Stdout, | ||
AttachStderr: container.Stderr, | ||
}, | ||
} | ||
|
||
|
@@ -1871,3 +1873,35 @@ func (dm *DockerManager) doBackOff(pod *api.Pod, container *api.Container, podSt | |
dm.clearReasonCache(pod, container) | ||
return false | ||
} | ||
|
||
// create and start a container then log the output | ||
func (dm *DockerManager) RunContainerCommand(pod *api.Pod, container *api.Container, cmd []string) ([]byte, error) { | ||
opts, err := dm.generator.GenerateRunContainerOptions(pod, container) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
utsMode := "" | ||
if pod.Spec.HostNetwork { | ||
utsMode = "host" | ||
} | ||
netNamespace := "" | ||
if pod.Spec.HostNetwork { | ||
netNamespace = "host" | ||
} | ||
|
||
// create and start container | ||
// see https://docs.docker.com/reference/api/docker_remote_api_v1.20/#3-1-inside-docker-run | ||
id, err := dm.runContainer(pod, container, opts, nil, netNamespace, "", utsMode) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer dm.KillContainerInPod("", container, pod) | ||
|
||
var stdout, stderr bytes.Buffer | ||
if err := dm.GetContainerLogs(pod, id, "all", true, &stdout, &stderr); err != nil { | ||
return nil, err | ||
} | ||
out := append(stdout.Bytes(), stderr.Bytes()...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it makes sense to concatenate stdout and stderr. How about returning both separately? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ncdc yes I like your point. The idea here is to reach the parity with exec.CombinedOutput |
||
return out, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2786,3 +2786,17 @@ func extractBandwidthResources(pod *api.Pod) (ingress, egress *resource.Quantity | |
} | ||
return ingress, egress, nil | ||
} | ||
|
||
// Runs the command in the container | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is the mountExternalVolumes call necessary here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kubelet builds volume first then creates the pod. if there is no volume, kubelet will complain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to enforce that the pod is run-once here. |
||
if err != nil { | ||
err = fmt.Errorf("Unable to mount volumes for pod %q: %v; skipping pod", podFullName, err) | ||
return nil, err | ||
} | ||
kl.volumeManager.SetVolumes(pod.UID, podVolumes) | ||
|
||
return kl.runner.RunContainerCommand(pod, container, cmd) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aglusterfs
image to do glusterfs mount, and yet another image for e.g.fibre channel/iSCSI
related exec.