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.

Sign up for GitHub

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

Jump to bottom

Fix a problem where if a minion went missing, we still thought the pod was running. #1347

Merged
merged 1 commit into from
Sep 18, 2014
Merged

Fix a problem where if a minion went missing, we still thought the pod was running. #1347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion pkg/client/fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type Fake struct {
Ctrl api.ReplicationController
ServiceList api.ServiceList
EndpointsList api.EndpointsList
Minions api.MinionList
Err error
Watch watch.Interface
}
Expand Down Expand Up @@ -144,5 +145,5 @@ func (c *Fake) ServerVersion() (*version.Info, error) {

func (c *Fake) ListMinions() (*api.MinionList, error) {
c.Actions = append(c.Actions, FakeAction{Action: "list-minions", Value: nil})
return &api.MinionList{}, nil
return &c.Minions, nil
}
1 change: 1 addition & 0 deletions pkg/master/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ func (m *Master) init(cloud cloudprovider.Interface, podInfoGetter client.PodInf
PodCache: podCache,
PodInfoGetter: podInfoGetter,
Registry: m.podRegistry,
Minions: m.client,
}),
"replicationControllers": controller.NewREST(m.controllerRegistry, m.podRegistry),
"services": service.NewREST(m.serviceRegistry, cloud, m.minionRegistry),
Expand Down
42 changes: 34 additions & 8 deletions pkg/registry/pod/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,15 @@ type REST struct {
podInfoGetter client.PodInfoGetter
podPollPeriod time.Duration
registry Registry
minions client.MinionInterface
Copy link
Member

Choose a reason for hiding this comment

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

This is much better, but I'd prefer even more if we just put in an

type MinionLister interface {
  ListMinions(selector) (*api.MinionList, error)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already exactly what MinionInterface is, why should we introduce a different interface?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, you're right-- fine for now.

}

type RESTConfig struct {
CloudProvider cloudprovider.Interface
PodCache client.PodInfoGetter
PodInfoGetter client.PodInfoGetter
Registry Registry
Minions client.MinionInterface
}

// NewREST returns a new REST.
Expand All @@ -61,6 +63,7 @@ func NewREST(config *RESTConfig) *REST {
podInfoGetter: config.PodInfoGetter,
podPollPeriod: time.Second * 10,
registry: config.Registry,
minions: config.Minions,
}
}

Expand Down Expand Up @@ -101,7 +104,11 @@ func (rs *REST) Get(id string) (runtime.Object, error) {
}
if rs.podCache != nil || rs.podInfoGetter != nil {
rs.fillPodInfo(pod)
pod.CurrentState.Status = getPodStatus(pod)
status, err := getPodStatus(pod, rs.minions)
if err != nil {
return pod, err
}
pod.CurrentState.Status = status
}
pod.CurrentState.HostIP = getInstanceIP(rs.cloudProvider, pod.CurrentState.Host)
return pod, err
Expand All @@ -113,7 +120,11 @@ func (rs *REST) List(selector labels.Selector) (runtime.Object, error) {
for i := range pods.Items {
pod := &pods.Items[i]
rs.fillPodInfo(pod)
pod.CurrentState.Status = getPodStatus(pod)
status, err := getPodStatus(pod, rs.minions)
if err != nil {
return pod, err
}
pod.CurrentState.Status = status
pod.CurrentState.HostIP = getInstanceIP(rs.cloudProvider, pod.CurrentState.Host)
}
}
Expand Down Expand Up @@ -202,9 +213,24 @@ func getInstanceIP(cloud cloudprovider.Interface, host string) string {
return addr.String()
}

func getPodStatus(pod *api.Pod) api.PodStatus {
func getPodStatus(pod *api.Pod, minions client.MinionInterface) (api.PodStatus, error) {
if pod.CurrentState.Info == nil || pod.CurrentState.Host == "" {
return api.PodWaiting
return api.PodWaiting, nil
}
res, err := minions.ListMinions()
if err != nil {
glog.Errorf("Error listing minions: %v", err)
return "", err
}
found := false
for _, minion := range res.Items {
if minion.ID == pod.CurrentState.Host {
found = true
break
}
}
if !found {
return api.PodTerminated, nil
}
running := 0
stopped := 0
Expand All @@ -222,13 +248,13 @@ func getPodStatus(pod *api.Pod) api.PodStatus {
}
switch {
case running > 0 && stopped == 0 && unknown == 0:
return api.PodRunning
return api.PodRunning, nil
case running == 0 && stopped > 0 && unknown == 0:
return api.PodTerminated
return api.PodTerminated, nil
case running == 0 && stopped == 0 && unknown > 0:
return api.PodWaiting
return api.PodWaiting, nil
default:
return api.PodWaiting
return api.PodWaiting, nil
}
}

Expand Down
156 changes: 98 additions & 58 deletions pkg/registry/pod/rest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
"github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider/fake"
"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest"
Expand Down Expand Up @@ -257,6 +258,15 @@ func TestGetPodCloud(t *testing.T) {
}

func TestMakePodStatus(t *testing.T) {
fakeClient := client.Fake{
Minions: api.MinionList{
Items: []api.Minion{
{
JSONBase: api.JSONBase{ID: "machine"},
},
},
},
}
desiredState := api.PodState{
Manifest: api.ContainerManifest{
Version: "v1beta1",
Expand All @@ -269,12 +279,6 @@ func TestMakePodStatus(t *testing.T) {
currentState := api.PodState{
Host: "machine",
}
pod := &api.Pod{DesiredState: desiredState, CurrentState: currentState}
status := getPodStatus(pod)
if status != api.PodWaiting {
t.Errorf("Expected 'Waiting', got '%s'", status)
}

runningState := docker.Container{
State: docker.State{
Running: true,
Expand All @@ -286,67 +290,103 @@ func TestMakePodStatus(t *testing.T) {
},
}

// All running.
pod = &api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
"containerB": runningState,
tests := []struct {
pod *api.Pod
status api.PodStatus
test string
}{
{&api.Pod{DesiredState: desiredState, CurrentState: currentState}, api.PodWaiting, "waiting"},
{
&api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
"containerB": runningState,
},
Host: "machine",
},
},
Host: "machine",
api.PodRunning,
"all running",
},
}
status = getPodStatus(pod)
if status != api.PodRunning {
t.Errorf("Expected 'Running', got '%s'", status)
}

// All stopped.
pod = &api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": stoppedState,
"containerB": stoppedState,
{
&api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
"containerB": runningState,
},
Host: "machine-two",
},
},
Host: "machine",
api.PodTerminated,
"all running but minion is missing",
},
}
status = getPodStatus(pod)
if status != api.PodTerminated {
t.Errorf("Expected 'Terminated', got '%s'", status)
}

// Mixed state.
pod = &api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
"containerB": stoppedState,
{
&api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": stoppedState,
"containerB": stoppedState,
},
Host: "machine",
},
},
Host: "machine",
api.PodTerminated,
"all stopped",
},
}
status = getPodStatus(pod)
if status != api.PodWaiting {
t.Errorf("Expected 'Waiting', got '%s'", status)
}

// Mixed state.
pod = &api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
{
&api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": stoppedState,
"containerB": stoppedState,
},
Host: "machine-two",
},
},
Host: "machine",
api.PodTerminated,
"all stopped but minion missing",
},
{
&api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
"containerB": stoppedState,
},
Host: "machine",
},
},
api.PodWaiting,
"mixed state #1",
},
{
&api.Pod{
DesiredState: desiredState,
CurrentState: api.PodState{
Info: map[string]docker.Container{
"containerA": runningState,
},
Host: "machine",
},
},
api.PodWaiting,
"mixed state #2",
},
}
status = getPodStatus(pod)
if status != api.PodWaiting {
t.Errorf("Expected 'Waiting', got '%s'", status)
for _, test := range tests {
if status, err := getPodStatus(test.pod, &fakeClient); status != test.status {
t.Errorf("In test %s, expected %v, got %v", test.test, test.status, status)
if err != nil {
t.Errorf("In test %s, unexpected error: %v", test.test, err)
}
}
}
}

Expand Down