Do not inject MigrateAction if it is not supported

[mushao999]

In #61377 we introduced a feature to let ILM migrate data between tiers. We achived this by inject a MigrateAction in warm and cold phase. However we also injected this action to phases where it is not supported such as hot, frozen and delete phase. It's better to no inject MigrateAction even though MigrateAction in these phases will be filtered out in TimeseriesLifecycleType#getOrderedActions(Phase) .
This pr updates the TimeseriesLifecycleType#shouldInjectMigrateStepForPhase(Phase) to not inject MigrateAction for phase in which MigrateAction is not supported.

[elasticsearchmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[gmarouli]

@mushao999 thank you for discovering and fixing this :)!

[gmarouli]

I would remove the changelog because there is no impact to the user. Even thought the MigrateAction was injected in the beginning it was filtered out later in https://github.com/elastic/elasticsearch/blob/be7c7415627377a1b795400fb8dfcc6cbdf0e322/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicy.java#L239/

[mushao999]

ChangeLog removed

[gmarouli]

This method is public and can be called with an invalid phase as an argument, if that happens this will throw an NPE. I think we should check this and throw an IllegalArgumentException like we do in other methods in this class.

[mushao999]

Good point, I've updated the code in this way.

[gmarouli]

What do you think about restructuring the conditionals like this:

• We start the method with this:

if (ALLOWED_ACTIONS.containsKey(phase.getName()) == false) {
    throw new IllegalArgumentException("Timeseries lifecycle does not support phase [" + phase.getName() + "]");
}

This way we make sure that we have consistent behaviour every time there is an invalid phase. The way the code is now, if there are searchable snapshots in an invalid phase it will return false instead of IllegalArgumentException.

• Then we could continue listing the phase independent cases, and finally just return if it's allowed in this phase or not:

// if the user configured the {@link MigrateAction} already we won't automatically configure it
if (phase.getActions().get(MigrateAction.NAME) != null) {
    return false;
}
        
// only inject the MigrateAction id it's supported for this phase (for example warm, cold).
return ALLOWED_ACTIONS.get(phase.getName()).contains(MigrateAction.NAME);

What do you think?

[mushao999]

I put the invalid-phase-checking there, beacase I want to make the change as smaller as possible.
Moving this checking to the head of this method will change the behaviour of invalid phase who has SearchableSnapshotAction in this method. False would be returned before, but IllegalArgumentException will be thrown for now.
But as you said, we could make sure consistent behaviour for invalid phases in this way which I think is more important for long terms.
I've update the code in this way.

[gmarouli]

Hm, I see, you are right we are changing the behaviour but I would expect if an invalid phase is provided that it would blow up anyway in a similar way since other methods do that too. I would like to ask another opinion, @dakrone do you see any red flags for throwing an IllegalArgumentException if we get an invalid phase?

[gmarouli]

Thank for joining the discussion @andreidan , in that case we would be throwing an NPE, right? Since it's a public method we cannot control that the phase has passed any check.

[andreidan]

@gmarouli that's correct. I missed we're actually adding the check that uses ALLOWED_ACTIONS.

I think it makes sense to have a check to avoid NPE. IMO returning false for an invalid phase would be preferable - ie. migrate should NOT be injected (as we're not really validating the allowed phases here), but an exception could work too.

[gmarouli]

Great, I am okay with returning false consistently when we receive a phase we do not recognize. @mushao999 if you agree too with this approach, let's replace the exception with returning false and let's ship it! 🚀

[mushao999]

@gmarouli It's ok for me, code updated.

[gmarouli]

Thanks! I will merge it with master asap.

[gmarouli]

@elasticmachine ok to test

[gmarouli]

@elasticmachine update branch

[gmarouli]

@mushao999, I merged with main so the tests will stop failing. (At least I thought this was the reason).

[gmarouli]

@elasticmachine update branch

[gmarouli]

@mushao999 , once more thank you for your contribution the swift reactions! It was a pleasure to work on this PR with you!