fix: CI flakes #127
fix: CI flakes #127
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mateusoliveira43 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mateusoliveira43
force-pushed
the
fix/ci-flakes
branch
from
4bde54e to
ec40c69
Compare
Signed-off-by: Mateus Oliveira <[email protected]>
mateusoliveira43
force-pushed
the
fix/ci-flakes
branch
from
ec40c69 to
e6d775b
Compare
| } else { | ||
| logger.V(1).Info("NonAdminBackup status unchanged during deletion") | ||
| } | ||
| if nab.ObjectMeta.DeletionTimestamp.IsZero() { | ||
| logger.V(1).Info("Marking NonAdminBackup for deletion", constant.NameString, nab.Name) | ||
| if err := r.Delete(ctx, nab); err != nil { |
There was a problem hiding this comment.
once this is called, update predicate is triggered. So, this should be last step to avoid running multiple reconciles
There was a problem hiding this comment.
But if you just return false, nil then reconcile will enter another step and will not exit as last step?
There was a problem hiding this comment.
I think we may need a 3rd state as it was in the very initial implementation which means "exit cleanly now" to avoid complexity moving parts to the steps where they don't belong. Moving this to the last step e.g. removeNabFinalizerUponVeleroBackupDeletion also doesn't makes sense to me as this step is actually to remove finalizer and not call delete on an object.
There was a problem hiding this comment.
what I did was add this as the last step of deletion, so return false will not do anything more
There was a problem hiding this comment.
How this works with the path where Deletion happens via DeleteBackupRequest
There was a problem hiding this comment.
it should be as it was, only change was the order of steps
|
@mateusoliveira43 is this flake still visible? I can see the log from the issue #93 is from 27th of September. There were so many changes in the reconcile paths since then, that I wonder if it's even still reproducible? |
|
@mpryc do not remember seeing it lately But with the approach of removing |
|
@mateusoliveira43 right, I am just not comfortable with reworking reconcile to remove one sleep as this PR is much more then just one issue fix and I am still not sold on the rework here. |
Why the changes were made
TODO
How to test the changes made
Check CI logs