scheduler: prevent nil pointer ref when reschedule policy is missing #24893
Conversation
When upgrading from older versions of Nomad, the reschedule policy block may be nil. There is logic to handle this safely in the `NextRescheduleTimeByTime` used for allocs on disconnected clients, but it's missing from the `NextRescheduleTime` method used by more typical allocations. Return an empty time object in this case. Fixes: #24846
| //If reschedule is disabled, return early | ||
| if reschedulePolicy.Attempts == 0 && !reschedulePolicy.Unlimited { | ||
| // If reschedule is disabled, return early | ||
| if reschedulePolicy == nil || (reschedulePolicy.Attempts == 0 && !reschedulePolicy.Unlimited) { |
There was a problem hiding this comment.
Note for reviewers: this behavior is lifted from NextRescheduleTimeByTime but I'm not 100% convinced it's correct there either. This effectively disables rescheduling rather than falling back to the default behavior. But we should never have a nil reschedule policy because we canonicalize the allocation on restore, which in turn canonicalizes the job, which in turns sets the reschedule policy to the default for the job type. So it's not clear to me how we could get into this situation.
There was a problem hiding this comment.
I pushed what I think is the root cause of the panic: TaskGroup.Canonicalize would set a nil ReschedulePolicy for system batch jobs! We never updated the NewRestartPolicy func when we added those! I pushed that fix and switched the logic around to prevent nil panics in the future (although we obviously very rarely add job types).
There was a problem hiding this comment.
In Slack you pointed out this is not the root cause as the panic mentions the GenericScheduler while sysbatch jobs use the SystemScheduler.
tgross
added
backport/ent/1.7.x+ent
schmichael
force-pushed
the
b-scheduler-panic-nil-reschedule-policy
branch
from
3c91c19 to
df1a994
Compare
When upgrading from older versions of Nomad, the reschedule policy block may be nil. There is logic to handle this safely in the
NextRescheduleTimeByTimeused for allocs on disconnected clients, but it's missing from theNextRescheduleTimemethod used by more typical allocations. Return an empty time object in this case.Fixes: #24846