-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Move event messages logic into task_runner #3486
Conversation
nomad/plan_apply.go
Outdated
@@ -149,6 +149,8 @@ func (s *Server) applyPlan(plan *structs.Plan, result *structs.PlanResult, snap | |||
for _, alloc := range req.Alloc { | |||
if alloc.CreateTime == 0 { | |||
alloc.CreateTime = now | |||
} else { | |||
alloc.ModifyTime = now |
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.
This is unexpected, I must have messed something up rebasing b-nomad-0.7.1 yesterday, will fix.
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.
Fixed
command/alloc_status.go
Outdated
case api.TaskTerminated: | ||
var parts []string | ||
parts = append(parts, fmt.Sprintf("Exit Code: %d", event.ExitCode)) | ||
if event.DisplayMessage != "" { |
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.
The only difference between the if and the else is the formattedDisplayMessage. Should we instead do:
msg := event.DisplayMessage
if msg == "" {
msg = buildDisplayMessage(event)
}
formattedTime := formatUnixNanoTime(event.Time)
formattedDisplayMsg := fmt.Sprintf("%s|%s|%s", formattedTime, event.Type, msg)
events[size-i] = formattedDisplayMsg
nomad/structs/structs.go
Outdated
@@ -73,6 +73,9 @@ const ( | |||
ACLTokenUpsertRequestType | |||
ACLTokenDeleteRequestType | |||
ACLTokenBootstrapRequestType | |||
|
|||
// Constant for restart events that are within policy | |||
ReasonWithinPolicy = "Restart within policy" |
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 would move this maybe to be next to the RestartPolicy struct. We try to group vars/consts logically together. Also is there a need to duplicate this? Why not just use the client version.
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.
Got an import cycle when using client.ReasonWithinPolicy so I duplicated. Didn't seem worth it to try to break the import cycle since this is one constant.
nomad/structs/structs.go
Outdated
@@ -3817,6 +3820,129 @@ type TaskEvent struct { | |||
|
|||
// GenericSource is the source of a message. | |||
GenericSource string | |||
|
|||
// DisplayMessage is a human friendly message about the event |
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.
Would you mind making a deprecation notice about the other fields explaining we are transitioning to just these two.
nomad/structs/structs.go
Outdated
|
||
func (event *TaskEvent) PopulateEventDisplayMessage() { | ||
// Build up the description based on the event type. | ||
if event == nil { //TODO PA needs investigation alloc_runner's Run method sends a nil event when sigterming nomad. Why? |
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.
Can you format TODO(preetha)
. Easier grepping if consistent.
nomad/structs/structs.go
Outdated
// Build up the description based on the event type. | ||
if event == nil { //TODO PA needs investigation alloc_runner's Run method sends a nil event when sigterming nomad. Why? | ||
return | ||
} |
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.
Can we break if event.DisplayMessage != ""
nomad/structs/structs.go
Outdated
desc = event.DriverMessage | ||
case TaskLeaderDead: | ||
desc = "Leader Task in Group dead" | ||
case TaskGenericMessage: |
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 think we may be able to deprecate this one. We can have the callers just set the type and the DisplayMessage.
api/tasks.go
Outdated
@@ -614,4 +614,6 @@ type TaskEvent struct { | |||
TaskSignalReason string | |||
TaskSignal string | |||
GenericSource string | |||
DisplayMessage string |
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.
Should put a deprecation notice here as well.
@@ -542,6 +542,7 @@ func (r *TaskRunner) DestroyState() error { | |||
|
|||
// setState is used to update the state of the task runner | |||
func (r *TaskRunner) setState(state string, event *structs.TaskEvent, lazySync bool) { | |||
event.PopulateEventDisplayMessage() |
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.
Can you put a blank line between.
…lient initiated updates
… two new fields DisplayMessage and Details.
…lso added deprecation info in comments.
239af54
to
b3631f3
Compare
Some lint errors: https://travis-ci.org/hashicorp/nomad/jobs/296833794#L546 |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This fixes #3399