[fix] #4198 : Introduce PhaseInfoFailureWithCleanup for Handling Non-Recoverable Pod States

[adarsh-jha-dev]

Tracking issue

Closes #4198

Describe your changes

• Added a new method PhaseInfoFailureWithCleanup in the PodSetup package to handle non-recoverable pod states.
• Modified the existing DemystifyFailure function to utilize the new PhaseInfoFailureWithCleanup method when non-recoverable pod states are encountered. This ensures that such pods are marked as failed and cleaned up properly.

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Screenshots

N/A

Note to reviewers

Please review this PR and provide feedback on whether it effectively addresses the issue mentioned in the context.

[Future-Outlier]

Hi, just a reminder, if the function is added, the test should be added too, thanks a lot!

[Future-Outlier]

.

[Future-Outlier]

Can you also provide a screenshot or some test for this case?
I mean you can mock the scenario and ensure the pod is cleaned up correctly

[adarsh-jha-dev]

Hi, just a reminder, if the function is added, the test should be added too, thanks a lot!

Thanks for the reminder, and sorry for missing that , I have added the required test.

[adarsh-jha-dev]

Hey @Future-Outlier , i have implemented the changes, could you please review it and let me know if the changes are relevant?

[Future-Outlier]

@adarsh-jha-dev I will try to spend time on this issue in 2 days, thank you very much.
Love your attitude.

[adarsh-jha-dev]

@adarsh-jha-dev I will try to spend time on this issue in 2 days, thank you very much. Love your attitude.

Thanks a lot

[hamersaw]

I think this is a little more than the issue needs. This PR introduced a cleanupOnFailure flag to the PhaseInfo and then a new function for PhaseInfoRetryableFailureWithCleanup to enable it. The result is that these failures will mark the Flyte task as failed, but also delete the Pod in question.

The goal here was to add a similar PhaseInfoFailureWithCleanup that can be used here. For example, if there is a Pod created with an invalid name, the Flyte task should be a failure, but Flyte should also cleanup the Pod by deleting it..

[adarsh-jha-dev]

I think this is a little more than the issue needs. This PR introduced a cleanupOnFailure flag to the PhaseInfo and then a new function for PhaseInfoRetryableFailureWithCleanup to enable it. The result is that these failures will mark the Flyte task as failed, but also delete the Pod in question.

The goal here was to add a similar PhaseInfoFailureWithCleanup that can be used here. For example, if there is a Pod created with an invalid name, the Flyte task should be a failure, but Flyte should also cleanup the Pod by deleting it..

Then should i do any changes or it will be fine with this?

[hamersaw]

Then should i do any changes or it will be fine with this?

If you would be willing to update this, it would be very much appreciated. I think a PhaseFailureWithCleanup function very similar to this one and then replacing this and testing that Pods that are created with invalid names are cleaned up would be awesome!

[adarsh-jha-dev]

Then should i do any changes or it will be fine with this?

If you would be willing to update this, it would be very much appreciated. I think a PhaseFailureWithCleanup function very similar to this one and then replacing this and testing that Pods that are created with invalid names are cleaned up would be awesome!

Hey @hamersaw , I tried but was unable to implement this , could you please let me know that if the current changes are equivalent to the desired ones or not? Like will these also work?

[hamersaw]

Then should i do any changes or it will be fine with this?

If you would be willing to update this, it would be very much appreciated. I think a PhaseFailureWithCleanup function very similar to this one and then replacing this and testing that Pods that are created with invalid names are cleaned up would be awesome!

Hey @hamersaw , I tried but was unable to implement this , could you please let me know that if the current changes are equivalent to the desired ones or not? Like will these also work?

@adarsh-jha-dev unfortunately I think this PR will introduce unnecessary complexity. Also, without setting the cleanupOnFailure flag it is not clear to me that this will actually clean up resources.

I tried but was unable to implement this

Can you elaborate on the difficulties? I would be happy to help!