Use annotations instead of delete CRDs when terminate execution #5789
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5789 +/- ##
=======================================
Coverage 36.31% 36.31%
=======================================
Files 1304 1304
Lines 110048 110058 +10
=======================================
+ Hits 39964 39972 +8
+ Misses 65928 65927 -1
- Partials 4156 4159 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andres Gomez Ferrer <[email protected]>
andresgomezfrr
force-pushed
the
fix-fast-terminations
branch
from
74adb22 to
70123c9
Compare
Signed-off-by: Andres Gomez Ferrer <[email protected]>
Signed-off-by: Andres Gomez Ferrer <[email protected]>
Signed-off-by: Andres Gomez Ferrer <[email protected]>
| @@ -103,7 +110,8 @@ func (p *Propeller) TryMutateWorkflow(ctx context.Context, originalW *v1alpha1.F | |||
| ctx = contextutils.WithResourceVersion(ctx, mutableW.GetResourceVersion()) | |||
|
|
|||
| maxRetries := uint32(p.cfg.MaxWorkflowRetries) | |||
| if IsDeleted(mutableW) || (mutableW.Status.FailedAttempts > maxRetries) { | |||
|
|
|||
| if IsAborted(mutableW) || (mutableW.Status.FailedAttempts > maxRetries) { | |||
There was a problem hiding this comment.
we could still have the case that the CRD gets deleted by some other process and should still account for that here.
There was a problem hiding this comment.
mmmmh could be? Looking inside the if, the function there looks like is only for aborting case
There was a problem hiding this comment.
I added back the isDeleted(...) to manage this case here too, now it is backward compatible, wdyt?
| w.ObjectMeta.Annotations = make(map[string]string) | ||
| } | ||
|
|
||
| w.ObjectMeta.Annotations[AbortedWorkflowAnnotation] = "true" |
There was a problem hiding this comment.
concern here is if there's a case that this delete is required to delete dangling CRDs (not picked up by propeller)
There was a problem hiding this comment.
I guess.. that the dangling CRDs should be removed by propeller GC, right?
There was a problem hiding this comment.
It is the same case as completed workflows/failed workflows.
There was a problem hiding this comment.
concern here is if there's a case that this delete is required to delete dangling CRDs (not picked up by propeller)
If the concern is about deleting CRDs if the propeller is down.. what do you think about having a flag in the terminate to force the delete from the admin? Similar to kubectl delete --force which force the deletion of a CRD.
Signed-off-by: Andres Gomez Ferrer <[email protected]>
Signed-off-by: Andres Gomez Ferrer <[email protected]>
Signed-off-by: Andres Gomez Ferrer <[email protected]>
Tracking issue
Closes #5786
Alternative to: #5788
Why are the changes needed?
To make sure that the propeller can handle the CRD before they are deleted if a terminated execution is called very fast.
New behavior:
Instead of propagating the terminate execution to the propeller deleting the CRD, the admin now updates a new aborted annotation, the propeller will check this annotation instead of the
deletedTimestamp. If the annotation is there the propeller will send theABORTevent to the admin and remove the finalizer from the CRD. The propeller garbage-collector will clean up this CRD when it is executed.What changes were proposed in this pull request?
Unit test + manual test in a staging environment with real flyte execution.
How was this patch tested?
Check all the applicable boxes