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

Filter out unhealthy node in scheduler #3389

Merged
merged 1 commit into from
Jan 13, 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
37 changes: 32 additions & 5 deletions plugin/pkg/scheduler/factory/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ func (f *ConfigFactory) CreateFromKeys(predicateKeys, priorityKeys util.StringSe
// Watch minions.
// Minions may be listed frequently, so provide a local up-to-date cache.
if false {
// Disable this code until minions support watches.
// Disable this code until minions support watches. Note when this code is enabled,
// we need to make sure minion ListWatcher has proper FieldSelector.
Copy link
Member

Choose a reason for hiding this comment

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

Do minions support watches now? Can we enable this and remove the poller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no. Basic node watch is implemented in #2662, but it only enables watch by label (no watch by status).

We can either:

  1. Watch all nodes, and filter out unhealthy nodes in some custom watch handler. Not sure if the listwatch pkg support it.
    or
  2. Do not enable watch, wait until node status is fully understood by the system.

cache.NewReflector(f.createMinionLW(), &api.Node{}, f.MinionLister.Store).Run()
} else {
cache.NewPoller(f.pollMinions, 10*time.Second, f.MinionLister.Store).Run()
Expand Down Expand Up @@ -168,14 +169,40 @@ func (factory *ConfigFactory) createMinionLW() *cache.ListWatch {
}
}

// pollMinions lists all minions and returns an enumerator for cache.Poller.
// pollMinions lists all minions and filter out unhealthy ones, then returns
// an enumerator for cache.Poller.
func (factory *ConfigFactory) pollMinions() (cache.Enumerator, error) {
list := &api.NodeList{}
err := factory.Client.Get().Resource("minions").Do().Into(list)
allNodes := &api.NodeList{}
err := factory.Client.Get().Resource("minions").Do().Into(allNodes)
if err != nil {
return nil, err
}
return &nodeEnumerator{list}, nil
nodes := &api.NodeList{
TypeMeta: allNodes.TypeMeta,
ListMeta: allNodes.ListMeta,
}
for _, node := range allNodes.Items {
conditionMap := make(map[api.NodeConditionKind]*api.NodeCondition)
for i := range node.Status.Conditions {
cond := node.Status.Conditions[i]
conditionMap[cond.Kind] = &cond
}
if condition, ok := conditionMap[api.NodeReady]; ok {
if condition.Status == api.ConditionFull {
nodes.Items = append(nodes.Items, node)
}
} else if condition, ok := conditionMap[api.NodeReachable]; ok {
if condition.Status == api.ConditionFull {
nodes.Items = append(nodes.Items, node)
}
} else {
// If no condition is set, either node health check is disabled (master
// flag "healthCheckMinions" is set to false), or we get unknown condition.
// In such cases, we add nodes unconditionally.
nodes.Items = append(nodes.Items, node)
}
}
return &nodeEnumerator{nodes}, nil
}

func (factory *ConfigFactory) makeDefaultErrorFunc(backoff *podBackoff, podQueue *cache.FIFO) func(pod *api.Pod, err error) {
Expand Down
102 changes: 97 additions & 5 deletions plugin/pkg/scheduler/factory/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,105 @@ func TestCreate(t *testing.T) {

func TestPollMinions(t *testing.T) {
table := []struct {
minions []api.Node
minions []api.Node
expectedCount int
}{
{
minions: []api.Node{
{ObjectMeta: api.ObjectMeta{Name: "foo"}},
{ObjectMeta: api.ObjectMeta{Name: "bar"}},
{
ObjectMeta: api.ObjectMeta{Name: "foo"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReady, Status: api.ConditionFull},
},
},
},
{
ObjectMeta: api.ObjectMeta{Name: "bar"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReachable, Status: api.ConditionFull},
},
},
},
{
ObjectMeta: api.ObjectMeta{Name: "baz"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReady, Status: api.ConditionFull},
{Kind: api.NodeReachable, Status: api.ConditionFull},
},
},
},
{
ObjectMeta: api.ObjectMeta{Name: "baz"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReady, Status: api.ConditionFull},
{Kind: api.NodeReady, Status: api.ConditionFull},
},
},
},
},
expectedCount: 4,
},
{
minions: []api.Node{
{
ObjectMeta: api.ObjectMeta{Name: "foo"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReady, Status: api.ConditionFull},
},
},
},
{
ObjectMeta: api.ObjectMeta{Name: "bar"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReady, Status: api.ConditionNone},
},
},
},
},
expectedCount: 1,
},
{
minions: []api.Node{
{
ObjectMeta: api.ObjectMeta{Name: "foo"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReady, Status: api.ConditionFull},
{Kind: api.NodeReachable, Status: api.ConditionNone}},
},
},
},
expectedCount: 1,
},
{
minions: []api.Node{
{
ObjectMeta: api.ObjectMeta{Name: "foo"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{
{Kind: api.NodeReachable, Status: api.ConditionFull},
{Kind: "invalidValue", Status: api.ConditionNone}},
},
},
},
expectedCount: 1,
},
{
minions: []api.Node{
{
ObjectMeta: api.ObjectMeta{Name: "foo"},
Status: api.NodeStatus{
Conditions: []api.NodeCondition{},
},
},
},
expectedCount: 1,
Copy link
Member

Choose a reason for hiding this comment

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

Please add test cases exercising these two things, either of them should turn up a bug:

  {Kind: api.NodeReady, Status: api.ConditionFull},
  {Kind: api.NodeReachable, Status: api.ConditionNone},
  {Kind: api.NodeReachable, Status: api.ConditionFull},
  {Kind: "invalidValue", Status: api.ConditionNone},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! Didn't realize range will copy values :(

},
}

Expand All @@ -80,8 +172,8 @@ func TestPollMinions(t *testing.T) {
}
handler.ValidateRequest(t, "/api/"+testapi.Version()+"/minions", "GET", nil)

if e, a := len(item.minions), ce.Len(); e != a {
t.Errorf("Expected %v, got %v", e, a)
if a := ce.Len(); item.expectedCount != a {
t.Errorf("Expected %v, got %v", item.expectedCount, a)
}
}
}
Expand Down