fix: don't clean up old offloaded records during save. Fixes: #13220 #13286
Conversation
Signed-off-by: shuangkun <[email protected]>
agilgur5
changed the title
agilgur5
changed the title
agilgur5
added
area/gc
| if err != nil { | ||
| return "", err | ||
| } | ||
| logCtx.WithField("rowsAffected", rowsAffected).Debug("Deleted offloaded nodes") |
There was a problem hiding this comment.
Makes sense what you're saying. So, I notice that Save() can be called in either of these cases. Just want to confirm that the normal deletion method will take care of deleting the old Archived Workflows in either of those cases?
There was a problem hiding this comment.
Don‘t take care of.
We already have a periodic cleanup mechanism, so I think we can remove the cleanup during the save phase, because it does sometimes delete things that should not be deleted.
There was a problem hiding this comment.
I understand. But I'm just wondering if you tested both of these cases: if packer.IsTooLargeError(err) as well as alwaysOffloadNodeStatus. Those are the ones being called where Save() is being called here, so I'm just paranoid and want to make sure we'd still be calling Delete() in the periodic job.
There was a problem hiding this comment.
Yes, both cases have passed the test.
…j#13220 (argoproj#13286) Signed-off-by: shuangkun <[email protected]>
…13286) Signed-off-by: shuangkun <[email protected]>
don't clean up old records when save
Fixes #13220
Motivation
Modifications
Verification