flyte icon indicating copy to clipboard operation
flyte copied to clipboard

[Bug] fix ArrayNode state's TaskPhase reset

Open pvditt opened this issue 1 year ago • 1 comments

Tracking issue

Why are the changes needed?

When a NodePhase change is detected, the current implementation resets the TaskPhaseVersion to 0 prior to emitting the event. This would create a duplicate event since the TaskPhase isn't also updated causing for the event to not get emitted/persisted to admin.

What changes were proposed in this pull request?

  • Bump the task phase version after emitting the event when a NodePhase change is detected in the ArrayNode handler.

We opt to do this instead of bumping the task phase as that can cause issues. Example issue:

  • updating the task phase to a terminal phase and then emitting that event to admin would set that task execution to a terminal state
  • admin does not persist follow up event when a task execution is already in a terminal state
  • the next propeller loop that handles cleanup wouldn't be able to persist new task events such as aborting subnodes on faliure cleanup.

Also this seems to be consistent with letting the proceeding propeller loop handling the next state.

How was this patch tested?

Ran through different failing and succeeding scenarios and ensured that the subnodes phases transitioned to the correct terminal phase.

Setup process

Screenshots

Check all the applicable boxes

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

Related PRs

Docs link

pvditt avatar Jun 05 '24 23:06 pvditt

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.92%. Comparing base (fc1c92c) to head (8a5362b). Report is 132 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5451   +/-   ##
=======================================
  Coverage   60.91%   60.92%           
=======================================
  Files         796      796           
  Lines       51689    51689           
=======================================
+ Hits        31488    31493    +5     
+ Misses      17294    17289    -5     
  Partials     2907     2907           
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.73% <ø> (+0.04%) :arrow_up:
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 67.41% <ø> (ø)
unittests-flyteidl 79.04% <ø> (ø)
unittests-flyteplugins 61.85% <ø> (ø)
unittests-flytepropeller 57.25% <100.00%> (ø)
unittests-flytestdlib 65.57% <ø> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 05 '24 23:06 codecov[bot]