Skip to content

api: Use TaskStatus Err field for non-terminal errors #2287

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

Merged
nishanttotla merged 1 commit into from
Oct 9, 2017
Merged

api: Use TaskStatus Err field for non-terminal errors #2287

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions agent/exec/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ func Resolve(ctx context.Context, task *api.Task, executor Executor) (Controller
// we always want to proceed to accepted when we resolve the controller
status.Message = "accepted"
status.State = api.TaskStateAccepted
status.Err = ""
}

return ctlr, status, err
Expand Down Expand Up @@ -158,6 +159,7 @@ func Do(ctx context.Context, task *api.Task, ctlr Controller) (*api.TaskStatus,
current := status.State
status.State = state
status.Message = msg
status.Err = ""

if current > state {
panic("invalid state transition")
Expand Down
7 changes: 6 additions & 1 deletion api/types.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion api/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,12 +481,17 @@ message TaskStatus {
// field.
string message = 3;

// Err is set if the task is in an error state.
// Err is set if the task is in an error state, or is unable to
// progress from an earlier state because a precondition is
// unsatisfied.
//
// The following states should report a companion error:
//
// FAILED, REJECTED
//
// In general, messages that should be surfaced to users belong in the
// Err field, and notes on routine state transitions belong in Message.
//
// TODO(stevvooe) Integrate this field with the error interface.
string err = 4;

Expand Down
8 changes: 5 additions & 3 deletions manager/allocator/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1159,9 +1159,11 @@ func PredefinedNetworks() []networkallocator.PredefinedNetworkData {

// updateTaskStatus sets TaskStatus and updates timestamp.
func updateTaskStatus(t *api.Task, newStatus api.TaskState, message string) {
t.Status.State = newStatus
t.Status.Message = message
t.Status.Timestamp = ptypes.MustTimestampProto(time.Now())
t.Status = api.TaskStatus{
State: newStatus,
Message: message,
Timestamp: ptypes.MustTimestampProto(time.Now()),
}
}

// IsIngressNetwork returns whether the passed network is an ingress network.
Expand Down
3 changes: 2 additions & 1 deletion manager/orchestrator/constraintenforcer/constraint_enforcer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ loop:
// restarting the task on another node
// (if applicable).
t.Status.State = api.TaskStateRejected
t.Status.Message = "assigned node no longer meets constraints"
t.Status.Message = "task rejected by constraint enforcer"
t.Status.Err = "assigned node no longer meets constraints"
Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I feel the Message/Err should be swapped here. The error on the task should be related to the task, so "task rejected by constraint enforcer" seems more relevant.

Also, does it make sense to be more specific around what constraints were failed ?

Copy link
Copy Markdown
Contributor

@stevvooe stevvooe Sep 29, 2017

Choose a reason for hiding this comment

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

The problem is that we don't display Err on the command line interface. Message is used to provided better reporting to the user.

t.Status.Timestamp = ptypes.MustTimestampProto(time.Now())
return store.UpdateTask(tx, t)
})
Expand Down
10 changes: 6 additions & 4 deletions manager/scheduler/scheduler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,9 @@ func (s *Scheduler) applySchedulingDecisions(ctx context.Context, schedulingDeci
continue
}

if t.Status.State == decision.new.Status.State && t.Status.Message == decision.new.Status.Message {
if t.Status.State == decision.new.Status.State &&
t.Status.Message == decision.new.Status.Message &&
t.Status.Err == decision.new.Status.Err {
// No changes, ignore
continue
}
Expand Down Expand Up @@ -506,7 +508,7 @@ func (s *Scheduler) taskFitNode(ctx context.Context, t *api.Task, nodeID string)
if !s.pipeline.Process(&nodeInfo) {
// this node cannot accommodate this task
newT.Status.Timestamp = ptypes.MustTimestampProto(time.Now())
newT.Status.Message = s.pipeline.Explain()
newT.Status.Err = s.pipeline.Explain()
s.allTasks[t.ID] = &newT

return &newT
Expand Down Expand Up @@ -706,9 +708,9 @@ func (s *Scheduler) noSuitableNode(ctx context.Context, taskGroup map[string]*ap
newT := *t
newT.Status.Timestamp = ptypes.MustTimestampProto(time.Now())
if explanation != "" {
newT.Status.Message = "no suitable node (" + explanation + ")"
newT.Status.Err = "no suitable node (" + explanation + ")"
} else {
newT.Status.Message = "no suitable node"
newT.Status.Err = "no suitable node"
}
s.allTasks[t.ID] = &newT
schedulingDecisions[t.ID] = schedulingDecision{old: t, new: &newT}
Expand Down
16 changes: 8 additions & 8 deletions manager/scheduler/scheduler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,7 +1145,7 @@ func TestSchedulerNoReadyNodes(t *testing.T) {
defer scheduler.Stop()

failure := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node", failure.Status.Message)
assert.Equal(t, "no suitable node", failure.Status.Err)

err = s.Update(func(tx store.Tx) error {
// Create a ready node. The task should get assigned to this
Expand Down Expand Up @@ -1435,7 +1435,7 @@ func TestSchedulerResourceConstraint(t *testing.T) {
defer scheduler.Stop()

failure := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (2 nodes not available for new tasks; insufficient resources on 1 node)", failure.Status.Message)
assert.Equal(t, "no suitable node (2 nodes not available for new tasks; insufficient resources on 1 node)", failure.Status.Err)

err = s.Update(func(tx store.Tx) error {
// Create a node with enough memory. The task should get
Expand Down Expand Up @@ -1695,7 +1695,7 @@ func TestSchedulerResourceConstraintDeadTask(t *testing.T) {
assert.NoError(t, err)

failure := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (insufficient resources on 1 node)", failure.Status.Message)
assert.Equal(t, "no suitable node (insufficient resources on 1 node)", failure.Status.Err)

err = s.Update(func(tx store.Tx) error {
// The task becomes dead
Expand Down Expand Up @@ -1989,7 +1989,7 @@ func TestSchedulerCompatiblePlatform(t *testing.T) {
})
assert.NoError(t, err)
failure := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (unsupported platform on 3 nodes)", failure.Status.Message)
assert.Equal(t, "no suitable node (unsupported platform on 3 nodes)", failure.Status.Err)

// add task3
err = s.Update(func(tx store.Tx) error {
Expand Down Expand Up @@ -2451,7 +2451,7 @@ func TestSchedulerPluginConstraint(t *testing.T) {
assert.NoError(t, err)

failure := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (missing plugin on 1 node)", failure.Status.Message)
assert.Equal(t, "no suitable node (missing plugin on 1 node)", failure.Status.Err)

// Now add the second node
err = s.Update(func(tx store.Tx) error {
Expand All @@ -2474,7 +2474,7 @@ func TestSchedulerPluginConstraint(t *testing.T) {
assert.NoError(t, err)

failure = watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (missing plugin on 2 nodes)", failure.Status.Message)
assert.Equal(t, "no suitable node (missing plugin on 2 nodes)", failure.Status.Err)

// Now add the node3
err = s.Update(func(tx store.Tx) error {
Expand All @@ -2498,7 +2498,7 @@ func TestSchedulerPluginConstraint(t *testing.T) {

// check that t4 has been assigned
failure2 := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (missing plugin on 3 nodes)", failure2.Status.Message)
assert.Equal(t, "no suitable node (missing plugin on 3 nodes)", failure2.Status.Err)

err = s.Update(func(tx store.Tx) error {
assert.NoError(t, store.CreateNode(tx, n4))
Expand Down Expand Up @@ -2803,5 +2803,5 @@ func TestSchedulerHostPort(t *testing.T) {
assert.NoError(t, err)

failure := watchAssignmentFailure(t, watch)
assert.Equal(t, "no suitable node (host-mode port already in use on 2 nodes)", failure.Status.Message)
assert.Equal(t, "no suitable node (host-mode port already in use on 2 nodes)", failure.Status.Err)
}