Clean up termination config/defaults #1685
Conversation
internal/armada/server/submit.go
Outdated
| server.schedulingConfig.MinTerminationGracePeriod.Seconds(), | ||
| ) | ||
| if defaultTerminationGracePeriodSeconds < 0 { | ||
| panic("MinTerminationGracePeriod is negative") |
There was a problem hiding this comment.
Let's not panic here- if we want to validate config let's do it at startup
There was a problem hiding this comment.
Agreed. Let's remove the panic and later add proper config validation at startup.
| spec.TerminationGracePeriodSeconds = &defaultTerminationGracePeriod | ||
| var podTerminationGracePeriodSeconds int64 | ||
| if spec.TerminationGracePeriodSeconds != nil { | ||
| podTerminationGracePeriodSeconds = *spec.TerminationGracePeriodSeconds |
There was a problem hiding this comment.
we don't verify that *spec.TerminationGracePeriodSeconds is > 0
There was a problem hiding this comment.
We have another check that rejects pods with a value less than min.
| podTerminationGracePeriodSeconds = *spec.TerminationGracePeriodSeconds | ||
| } | ||
| if podTerminationGracePeriodSeconds == 0 { | ||
| defaultTerminationGracePeriodSeconds := int64( |
There was a problem hiding this comment.
why don't we do the conversion once at startup rather than every time? Or even make the config value be an int representing seconds rather than a duration.
There was a problem hiding this comment.
I think this is fine. I like having a duration. Since then we can put things like "5m".
For pods that set no grace period or explicitly set it to zero, automatically change it to the minimum allowed. Remove the default grace period setting and instead use the minimum as the default.