Add validation of the SLM schedule frequency #64452
Conversation
joegallo
added
>enhancement
WIP
:Data Management/ILM+SLM
|
Pinging @elastic/es-core-features (:Core/Features/ILM+SLM) |
elasticmachine
added
the
Team:Data Management
mostly just whitespace
It represents the minimum allowable scheduled time from when one snapshot starts to when the next snapshot following it will start. Defaults to 15 minutes.
joegallo
force-pushed
the
validate-slm-schedule-frequency
branch
from
ff5aeb5
to
a931f11
Compare
This comment has been minimized.
This comment has been minimized.
|
Additional question for review: one could argue that this is a breaking change w.r.t. 7.x, what kind of additional changelog notes or the like are appropriate for on-the-edge changes like these? |
|
The 'next interval' calculation is naive, there's no guarantee that somebody hasn't entered something 'fun' like
which would be okay or not okay by this current logic depending on exactly when you tried to validate it. 😬 edit: if we wanted to get more serious about checking for this possibility, which maybe we do, then I suppose we could run through the next hour's worth of intervals returning the minimum one found. I don't think it would be too difficult to implement that. |
There was a problem hiding this comment.
Thanks Joe, I left some comments about this but it looks good in general.
one could argue that this is a breaking change w.r.t. 7.x, what kind of additional changelog notes or the like are appropriate for on-the-edge changes like these?
I think this fits squarely in the "fixing cluster stability" bucket, not really a breaking change (insert disclaimer about any change being a breaking change). I don't think we need to explicitly add anything about it (there is a workaround regardless)
if we wanted to get more serious about checking for this possibility, which maybe we do, then I suppose we could run through the next hour's worth of intervals returning the minimum one found. I don't think it would be too difficult to implement that.
How about we do this as a follow-up?
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotLifecyclePolicy.java
Outdated
Show resolved
Hide resolved
| long minimumIntervalMillis = minimumInterval.getMillis(); | ||
| long nextInterval = lifecycle.calculateNextInterval(); | ||
| if (nextInterval >= 0 && minimumIntervalMillis > 0 && nextInterval < minimumIntervalMillis) { | ||
| throw new IllegalArgumentException("invalid schedule [" + lifecycle.getSchedule() + "]: too frequent"); |
There was a problem hiding this comment.
We should include what the limit for frequency is in the error message, and perhaps something like:
invalid schedule [*/1 * * * ?]: schedule would be too frequent, executing more frequently than [15m] interval
(the wording doesn't have to be exactly that, just an example)
There was a problem hiding this comment.
👍, how about 2798b78?
x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleService.java
Show resolved
Hide resolved
x-pack/plugin/ilm/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/ilm/11_basic_slm.yml
Outdated
Show resolved
Hide resolved
and check for that message in the test
This comment has been minimized.
This comment has been minimized.
I'm going to poke at it for a bit -- if it seems sane enough I might keep it on this PR, but we'll see. (edit: Ehhhh, yeah, I'm punting on this one for now.) |
+1, sounds fair to me. |
joegallo
added
the
Team:Deployment Management
|
Pinging @elastic/es-ui (Team:UI) |
|
/cc @elastic/es-ui long story short, at the very least, this is going to mean that
|
|
Thanks for the heads up @joegallo! I created elastic/kibana#83918 to track this. |
Closes #55450
Adds the new
slm.minimum_intervalsetting (default: 15 minutes) and validates that SLM policies are not more frequent than that minimum interval./cc @original-brownbear