-
Notifications
You must be signed in to change notification settings - Fork 671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Write abort event for aborted k8s workflows #5785
Write abort event for aborted k8s workflows #5785
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5785 +/- ##
=======================================
Coverage 36.31% 36.32%
=======================================
Files 1304 1304
Lines 110048 110067 +19
=======================================
+ Hits 39964 39981 +17
- Misses 65928 65930 +2
Partials 4156 4156
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6f4c335
to
bcba038
Compare
Signed-off-by: Andres Gomez Ferrer <[email protected]> Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
Signed-off-by: Rafael Raposo <[email protected]>
142ed5a
to
358151e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lean against adding this eventing in admin. I feel as though this updating of status should be handled in propeller.
FlyteWorkflow CRDs get finalizers set by default. In your scenario, it seems the delete occurs before propeller is able to set finalizers on the FlyteWorkflow. Let me check if there'd be any issues setting this in the same loop that the resource is created - either in the propeller side admin launcher or just set it by default in admin. Unsure why this isn't just set when the FlyteWorkflow is created.
I think getting the finalizer set would be a better path forward.
Closing in favour of #5788 |
Tracking issue
Potentially closes #5547 and #5786
Why are the changes needed?
To make sure Workflows are able to transition from
Aborting
toAborted
if flytepropeller is not able to do it.What changes were proposed in this pull request?
As proposed in #5547 we're emitting an event after sending a Delete call to Flyte k8s cluster
How was this patch tested?
With unit tests and staging environment
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link