-
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 logic for event display message to task_runner #3476
Conversation
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.
Did you mean to commit the generated code?
nomad/structs/structs.go
Outdated
@@ -73,6 +73,7 @@ const ( | |||
ACLTokenUpsertRequestType | |||
ACLTokenDeleteRequestType | |||
ACLTokenBootstrapRequestType | |||
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.
This seems out of place... At least put a space between it and the "enum" above + a comment describing its intended purpose.
|
||
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.
Hm, interesting. I don't see anywhere in here that taskDestroyEvent
could still be nil
by the time r.destroyTaskRunners(taskDestroyEvent)
is called.
However I do feel like event being nil can intentionally happen (perhaps to cause a sync without changing the task's state?) -- I just can't find where 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.
I saw it only when shutting down, and it always consistently happened during shutdown. Haven't found the root cause yet..
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.
Hm if it's dev mode we destroy/GC all allocations on shutdown, so Nomad would be breaking out of that OUTER
loop in AllocRunner.Run
and hitting the AllocRunner.handleDestroy()
code path if that helps.
Time: time.Now().UnixNano(), | ||
Type: event, | ||
Time: time.Now().UnixNano(), | ||
Details: make(map[string]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.
Why is this a map of type -> details
instead of just being a string containing the details? It doesn't seem like we'd ever have an event with multiple detail entries but maybe I'm missing something?
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.
(If we want to keep this a map perhaps we should create a global static map of "Types -> String Keys" instead of hardcoding the strings below?)
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.
Created this because we want to get rid of the many fields like ErrorReason/KillReason etc and there can be more than one entry for a single event (e.g kill_signal and kill_reason)
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.
(Oops, left a comment in the wrong place; editing)
Ah! Makes sense. I must have missed the descriptive comment on the field.
command/alloc_status.go
Outdated
parts = append(parts, fmt.Sprintf("Exit Code: %d", event.ExitCode)) | ||
if event.DisplayMessage != "" { | ||
formattedTime := formatUnixNanoTime(event.Time) | ||
events[size-i] = fmt.Sprintf("%s|%s|%s", formattedTime, event.Type, 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.
Maybe make a named variable
Details map[string]string | ||
} | ||
|
||
func (event *TaskEvent) 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.
Maybe have a couple basic tests for a sanity check.
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.
Done
@schmichael yes I meant to commit generated code, I've added new fields to the API. This is needed until we make the workflow change we talked about. |
3b7090e
to
ef98e24
Compare
Publishing metrics for job summary
14b601b
to
af7ff0c
Compare
merge nomad 0.7.1 branch
Fixes a typo in the check restart doc.
…lient initiated updates
… two new fields DisplayMessage and Details.
ef98e24
to
a8211b1
Compare
Closing this one, will open a new one against master |
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. |
Adds two new fields DisplayMessage and Details.
Needs some more unit test updates
This fixes #3399