Change RunOnce duration plugin to act as a limit instead of override #8304
Conversation
|
@liggitt @smarterclayton ptal |
| // seconds that a run-once pod can be active in that project | ||
| const ActiveDeadlineSecondsOverrideAnnotation = "openshift.io/active-deadline-seconds-override" | ||
| const ActiveDeadlineSecondsLimitAnnotation = "openshift.io/active-deadline-seconds-limit" |
There was a problem hiding this comment.
any doc need to be changed for this?
There was a problem hiding this comment.
Yes, this will require a follow-up doc PR
|
@smarterclayton given that the existing config has been in since v1.1.2, is it acceptable to change the name of the config field? |
csrwng
force-pushed
the
runonce_limit_fix
branch
from
61a45e8
to
2d2601a
Compare
|
Generally no. On Wed, Mar 30, 2016 at 2:45 PM, Cesar Wong [email protected]
|
|
So... would it be ok to still call it override, even though it's more like a max now? |
|
Add a TODO that we want to rename it in the future. We can't rename On Wed, Mar 30, 2016 at 3:34 PM, Cesar Wong [email protected]
|
|
Added a commit to keep existing external names. |
csrwng
force-pushed
the
runonce_limit_fix
branch
2 times, most recently
from
68ab166
to
f12de06
Compare
|
[test] |
| return true, nil | ||
| } | ||
|
|
||
| func minInt64(a, b *int64) *int64 { |
There was a problem hiding this comment.
csrwng
force-pushed
the
runonce_limit_fix
branch
from
f12de06
to
5b19689
Compare
|
@Kargakis thx, using the kube function now. @smarterclayton should this be considered for 3.2? |
|
What's the risk to online? @bparees can you weigh in? |
|
This is more @abhgupta's area |
|
@csrwng Online is fine with the current limit of 1 hour for runonce pods. If a user cannot set it any lower, that is fine for now. The only impacted use case is that users are trying to optimize their resource usage and want to restrict their runonce pod(s) to a smaller duration so that they can do early reclamation pod/cpu/memory quota for other purposes. I am not too concerned about this use case for Online on day 1 and if users really want to terminate their pods, they still have the ability to do so to reclaim their quota/resources. |
|
@csrwng @smarterclayton Just realized that deployment cancellations are handled by explicitly setting the ActiveDeadlineSeconds on the deployer/hook pods to 0. Since this is prevented by the plugin, the functionality is broken and users won't be able to cancel a wedged deployment and be stuck with it for the full hour, preventing new deployments from being triggered. I believe this is a blocker. |
|
Approved for 1.2 |
| return true, nil | ||
| } | ||
|
|
||
| func Int64MinP(a, b *int64) *int64 { |
|
just a couple comments, needs rebase, LGTM otherwise |
csrwng
force-pushed
the
runonce_limit_fix
branch
from
5b19689
to
cb9d7d2
Compare
|
thx, made review changes |
csrwng
force-pushed
the
runonce_limit_fix
branch
from
cb9d7d2
to
1730b2a
Compare
|
[test] |
|
[test] |
|
Evaluated for origin test up to 1730b2a |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2751/) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5540/) (Image: devenv-rhel7_3929) |
|
Evaluated for origin merge up to 1730b2a |
|
@csrwng Works. But I must use activeDeadlineSecondsOverride in master config file. If use activeDeadlineSecondsLimit, seems it is not recognized and the setting does not take effect. |
|
@xingxingxia I only changed it to activeDeadlineSecondsLimit in the internal API but left it the same as before in the external API, which means you still need to use activeDeadlineSecondsOverride in the master-config.yaml. Therefore, existing documentation should still be valid. |
|
OK, got it |
Makes the following changes to the RunOnceDuration plugin: