Fix service.check_restart stanza propagation #3718
Conversation
schmichael
force-pushed
the
b-3713-fix-check-restart
branch
from
1810ff9 to
dff64ef
Compare
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ | |||
| BUG FIXES: | |||
| * core: Fix search endpoint forwarding for multi-region clusters [[GH-3680](https://github.com/hashicorp/nomad/issues/3680)] | |||
| * config: Revert minimum CPU limit back to 20 from 100. | |||
| * discovery: Fix handling of `service.check_restart` [[GH-3718](https://github.com/hashicorp/nomad/issues/3718)] | |||
There was a problem hiding this comment.
Allow check_restart to be specified in the service stanza.
There was a problem hiding this comment.
Move to Feature or leave as Bug Fix?
I'm going to move to Feature since we removed it from the docs and it never worked but let me know if Bug Fix is preferred.
| @@ -136,7 +136,7 @@ func (c *CheckRestart) Merge(o *CheckRestart) *CheckRestart { | |||
| nc.Grace = o.Grace | |||
There was a problem hiding this comment.
This merge seems backwards. Usually we prefer the passed in object's values. https://github.com/hashicorp/nomad/blob/master/api/jobs.go#L389
api/tasks.go
Outdated
| @@ -136,7 +136,7 @@ func (c *CheckRestart) Merge(o *CheckRestart) *CheckRestart { | |||
| nc.Grace = o.Grace | |||
| } | |||
|
|
|||
| if nc.IgnoreWarnings { | |||
| if !nc.IgnoreWarnings { | |||
There was a problem hiding this comment.
if o.IgnoreWarnings { nc.IgnoreWarnings = o.IgnoreWarnings }
api/tasks.go
Outdated
| @@ -189,9 +189,9 @@ func (s *Service) Canonicalize(t *Task, tg *TaskGroup, job *Job) { | |||
|
|
|||
| // Canonicallize CheckRestart on Checks and merge Service.CheckRestart | |||
| // into each check. | |||
| for _, c := range s.Checks { | |||
| for i, c := range s.Checks { | |||
| s.Checks[i].CheckRestart = c.CheckRestart.Merge(s.CheckRestart) | |||
There was a problem hiding this comment.
Can we make sure the merge is correct for check restart too
jobspec/parse.go
Outdated
| @@ -1042,7 +1043,7 @@ func parseChecks(service *api.Service, checkObjs *ast.ObjectList) error { | |||
| if ot, ok := co.Val.(*ast.ObjectType); ok { | |||
| checkRestartList = ot.List | |||
| } else { | |||
| return fmt.Errorf("check_restart '%s': should be an object", check.Name) | |||
| return fmt.Errorf("check '%s': should be an object", check.Name) | |||
There was a problem hiding this comment.
Not sure I get why you changed this? It is the check restart object that is failing the cast.
There was a problem hiding this comment.
Argh, I think the check.Name interpolation confused me. Fixing.
schmichael
changed the title
schmichael
force-pushed
the
b-3713-fix-check-restart
branch
from
dff64ef to
84a1013
Compare
schmichael
force-pushed
the
b-3713-fix-check-restart
branch
from
84a1013 to
b658ac3
Compare
|
Should be ready to review again. |
| } | ||
|
|
||
| service.Canonicalize(task, tg, job) | ||
| assert.Equal(t, service.Checks[0].CheckRestart.Limit, 22) |
There was a problem hiding this comment.
In this case I want it to continue checking in case it's just one field off.
|
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. |
There was a bug in jobspec parsing, a bug in CheckRestart merging, and a
bug in CheckRestart canonicalization. All are now tested.