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

Switch exec to look at exit code not output status. #8011

Merged
merged 1 commit into from
May 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/kubelet/dockertools/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ type DockerInterface interface {
Info() (*docker.Env, error)
CreateExec(docker.CreateExecOptions) (*docker.Exec, error)
StartExec(string, docker.StartExecOptions) error
InspectExec(id string) (*docker.ExecInspect, error)
}

// KubeletContainerName encapsulates a pod name and a Kubernetes container name.
Expand Down
5 changes: 5 additions & 0 deletions pkg/kubelet/dockertools/fake_docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ type FakeDockerClient struct {
RemovedImages util.StringSet
VersionInfo docker.Env
Information docker.Env
ExecInspect *docker.ExecInspect
}

func (f *FakeDockerClient) ClearCalls() {
Expand Down Expand Up @@ -285,6 +286,10 @@ func (f *FakeDockerClient) StartExec(_ string, _ docker.StartExecOptions) error
return nil
}

func (f *FakeDockerClient) InspectExec(id string) (*docker.ExecInspect, error) {
return f.ExecInspect, f.popError("inspect_exec")
}

func (f *FakeDockerClient) ListImages(opts docker.ListImagesOptions) ([]docker.APIImages, error) {
err := f.popError("list_images")
return f.Images, err
Expand Down
8 changes: 8 additions & 0 deletions pkg/kubelet/dockertools/instrumented_docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,3 +153,11 @@ func (in instrumentedDockerInterface) StartExec(startExec string, opts docker.St
}()
return in.client.StartExec(startExec, opts)
}

func (in instrumentedDockerInterface) InspectExec(id string) (*docker.ExecInspect, error) {
start := time.Now()
defer func() {
metrics.DockerOperationsLatency.WithLabelValues("inspect_exec").Observe(metrics.SinceInMicroseconds(start))
}()
return in.client.InspectExec(id)
}
38 changes: 38 additions & 0 deletions pkg/kubelet/dockertools/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"strconv"
"strings"
"sync"
"time"

"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
"github.com/GoogleCloudPlatform/kubernetes/pkg/capabilities"
Expand Down Expand Up @@ -894,10 +895,47 @@ func (dm *DockerManager) RunInContainer(containerID string, cmd []string) ([]byt
RawTerminal: false,
}
err = dm.client.StartExec(execObj.ID, startOpts)
if err != nil {
return nil, err
}
tick := time.Tick(2 * time.Second)
for {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some timeout here, just to be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, we were already blocking (detach is set to 'false') so to be honest this loop is probably never actually executed.

I'd rather not have a timeout here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

inspect, err2 := dm.client.InspectExec(execObj.ID)
if err2 != nil {
return buf.Bytes(), err2
}
if !inspect.Running {
if inspect.ExitCode != 0 {
err = &dockerExitError{inspect}
}
break
}
<-tick
}

return buf.Bytes(), err
}

type dockerExitError struct {
Inspect *docker.ExecInspect
}

func (d *dockerExitError) String() string {
return d.Error()
}

func (d *dockerExitError) Error() string {
return fmt.Sprintf("Error executing in Docker Container: %d", d.Inspect.ExitCode)
}

func (d *dockerExitError) Exited() bool {
return !d.Inspect.Running
}

func (d *dockerExitError) ExitStatus() int {
return d.Inspect.ExitCode
}

// ExecInContainer uses nsenter to run the command inside the container identified by containerID.
//
// TODO:
Expand Down
9 changes: 9 additions & 0 deletions pkg/kubelet/dockertools/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -619,3 +619,12 @@ func TestProbeContainer(t *testing.T) {
}
}
}

func TestIsAExitError(t *testing.T) {
var err error
err = &dockerExitError{nil}
_, ok := err.(uexec.ExitError)
if !ok {
t.Error("couldn't cast dockerExitError to exec.ExitError")
}
}
21 changes: 11 additions & 10 deletions pkg/probe/exec/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,34 +17,35 @@ limitations under the License.
package exec

import (
"strings"

"github.com/GoogleCloudPlatform/kubernetes/pkg/probe"
uexec "github.com/GoogleCloudPlatform/kubernetes/pkg/util/exec"
"github.com/GoogleCloudPlatform/kubernetes/pkg/util/exec"

"github.com/golang/glog"
)

const defaultHealthyOutput = "ok"

func New() ExecProber {
return execProber{}
}

type ExecProber interface {
Probe(e uexec.Cmd) (probe.Result, error)
Probe(e exec.Cmd) (probe.Result, error)
}

type execProber struct{}

func (pr execProber) Probe(e uexec.Cmd) (probe.Result, error) {
func (pr execProber) Probe(e exec.Cmd) (probe.Result, error) {
data, err := e.CombinedOutput()
glog.V(4).Infof("health check response: %s", string(data))
if err != nil {
exit, ok := err.(exec.ExitError)
if ok {
if exit.ExitStatus() == 0 {
return probe.Success, nil
} else {
return probe.Failure, nil
}
}
return probe.Unknown, err
}
if !strings.HasPrefix(strings.ToLower(string(data)), defaultHealthyOutput) {
return probe.Failure, nil
}
return probe.Success, nil
}
26 changes: 25 additions & 1 deletion pkg/probe/exec/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,40 @@ type healthCheckTest struct {
err error
}

type fakeExitError struct {
exited bool
statusCode int
}

func (f *fakeExitError) String() string {
return f.Error()
}

func (f *fakeExitError) Error() string {
return "fake exit"
}

func (f *fakeExitError) Exited() bool {
return f.exited
}

func (f *fakeExitError) ExitStatus() int {
return f.statusCode
}

func TestExec(t *testing.T) {
prober := New()
fake := FakeCmd{}

tests := []healthCheckTest{
// Ok
{probe.Success, false, []byte("OK"), nil},
// Ok
{probe.Success, false, []byte("OK"), &fakeExitError{true, 0}},
// Run returns error
{probe.Unknown, true, []byte("OK, NOT"), fmt.Errorf("test error")},
// Unhealthy
{probe.Failure, false, []byte("Fail"), nil},
{probe.Failure, false, []byte("Fail"), &fakeExitError{true, 1}},
}
for _, test := range tests {
fake.out = test.output
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/pods.go
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ var _ = Describe("Pods", func() {
{
Name: "liveness",
Image: "gcr.io/google_containers/busybox",
Command: []string{"/bin/sh", "-c", "echo ok >/tmp/health; sleep 10; echo fail >/tmp/health; sleep 600"},
Command: []string{"/bin/sh", "-c", "echo ok >/tmp/health; sleep 10; rm -rf /tmp/health; sleep 600"},
LivenessProbe: &api.Probe{
Handler: api.Handler{
Exec: &api.ExecAction{
Expand Down