[Profiling] Allow to upgrade managed ILM policy #96550
Conversation
With this commit we amend the default lifecycle policy for Universal Profiling with a version field in the metadata section and add infrastructure to determine whether a lifecycle policy should be upgraded to a newer version.
danielmitterdorfer added >enhancement v8.9.0 :UniversalProfiling/Application 
| Pinging @elastic/profiling (Team:Universal Profiling) |
| Hi @danielmitterdorfer, I've created a changelog YAML for you. |
| @gmarouli I've assigned you as reviewer as this PR also touches |
| protected boolean isUpgradeRequired(LifecyclePolicy currentPolicy, LifecyclePolicy newPolicy) { | ||
| try { | ||
| return getVersion(currentPolicy) < getVersion(newPolicy); | ||
| } catch (NumberFormatException ex) { |
There was a problem hiding this comment.
Nit: I would move this to the getVersion method and I would add the name of the policy in the log line, for example, Invalid version for profiling lifecycle policy 'my-policy-with-invalid-version'.. I know it's unlikely that this will happen and effectively there is only one policy, but I think it's a small change that in the future might prove to be helpful.
There was a problem hiding this comment.
I've restructured the code now in a90bf43. To avoid complicating the control flow I kept the catch block here but raise a specific exception now on parse errors in #getVersion().
| }); | ||
| | ||
| ClusterChangedEvent newEvent = createClusterChangedEvent( | ||
| Collections.emptyMap(), |
There was a problem hiding this comment.
Nit: (delayed comment) I just noticed this, we prefer to use Map.of() in the code base since it is supported.
There was a problem hiding this comment.
Good to know. Corrected in a90bf43.
| if (Objects.isNull(currentPolicy)) { | ||
| logger.debug("adding lifecycle policy [{}] for [{}], because it doesn't exist", policy.getName(), getOrigin()); | ||
| putPolicy(policy, creationCheck); | ||
| } else if (isUpgradeRequired(currentPolicy, policy)) { |
There was a problem hiding this comment.
I can see that this functionality is tested in the ProfilingIndexTemplateRegistryTests but since this is implemented and it would remain even if ProfilingIndexTemplateRegistry would be deprecated/removed, I think it would be good to add a test also in IndexTemplateRegistryTests. What do you think?
There was a problem hiding this comment.
I've amended test cases for the base class in a90bf43.
| @elasticmachine merge upstream |
| Many thanks for the review! |
4 participants
With this commit we amend the default lifecycle policy for Universal Profiling with a version field in the metadata section and add infrastructure to determine whether a lifecycle policy should be upgraded to a newer version.