[YUNIKORN-3245] Reset queue properties on config update#1086
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
+ Coverage 81.47% 81.50% +0.03%
==========================================
Files 103 103
Lines 13963 13978 +15
==========================================
+ Hits 11376 11393 +17
+ Misses 2310 2309 -1
+ Partials 277 276 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ayubpathan this seems to be a different solution for the same issue as is fixed in #1088. The merge code for the properties should handle the reset also. |
Reset queue fields derived from properties before applying the current effective property map. This lets removed queue properties revert to their defaults during config updates.
ayubpathan
force-pushed
the
YUNIKORN-3245-reset-queue-properties
branch
from
5be7636 to
d360e9c
Compare
|
Thanks @wilfred-s, I checked #1088. The inherited-property reload part of my previous patch overlaps with YUNIKORN-3285, so I removed the ApplyConf merge change and the related inherited-property test from this PR. This PR is now narrowed back to YUNIKORN-3245: resetting runtime fields derived from the effective queue properties in UpdateQueueProperties(), so removed properties revert to their defaults on config update. I kept the mergeProperties cleanup out of this revision to avoid duplicating #1088. If you prefer that cleanup to be handled here instead, I can adjust. |
| sq.sortType = policies.FifoSortPolicy | ||
| return | ||
| } | ||
| // Reset fields derived from properties so removed properties revert to defaults. |
There was a problem hiding this comment.
Can we move setting all these defaults into a single function: sq.resetProperties() ?
| if quotaPreemptionDelaySet || oldDelay != sq.quotaPreemptionDelay { | ||
| if sq.quotaPreemptionDelay == 0 { | ||
| sq.quotaPreemptionStartTime = time.Time{} | ||
| return | ||
| } | ||
| sq.setPreemptionTime(oldMaxResource, oldDelay) | ||
| } |
There was a problem hiding this comment.
This can be hidden inside the sq.setPreemptionTime(oldMaxResource, oldDelay) call and always call that method with a small change inside that method:
- if preemption is running the start time will be cleaned up automatically
- if preemption is not running and delay is not set now clear out the start time
- all other cases let it flow as it was
Removes the need to track quotaPreemptionDelaySet
2 participants
What is this PR for?
Reset queue fields derived from properties before applying the current effective property map during queue config updates. This lets removed properties return to defaults while preserving inherited parent properties for existing child queues on reload.
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-3245
How should this be tested?
Screenshots (if appropriate)
N/A
Questions: