-
Notifications
You must be signed in to change notification settings - Fork 279
Conversation
Hey @OmerKahani, I think what you are hitting is an Argo CD issue... I have seen that if you:
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
name: cluster1-guestbook
namespace: argocd-e2e
spec:
destination:
name: cluster1
namespace: guestbook
project: default
source:
path: guestbook
repoURL: https://github.com/argoproj/argocd-example-apps.git
targetRevision: HEAD
Then Argo CD seems to get stuck in a state where it keeps trying to process that Application over and over again (eg bounded only by network I/O bandwidth) in a reconciliation loop. Which means it never gets around to deleting it (and the test fails on missing deletion) IMHO it seems like the correct behaviour is for Argo CD to allow invalid Applications (Applications that point to clusters w/o a corresponding secret) to be deleted. The deletion finalizer and/or reconciler shouldn't prevent this operation. |
You just saved me hours of debugging :) I will split this PR with the tests that are OK, and open an issue in ArgoCD |
I investigated the Argo CD behaviour a bit more, and opened an Argo CD bug with steps to reproduce: argoproj/argo-cd#5817 |
@jgwest maybe we should add a finalizer on the secret. Then the applicationset would delete the application prior to the deletion of the app. If the applicationset created the apps because of the secret, and the secret is been deleted I think that we should also delete the apps - what do you think? |
f5601fb
to
76c0190
Compare
Sorry for the review delay, @OmerKahani, in the process of reviewing and testing your changes I discovered and investigated race conditions in the cluster creation/deletion logic of ApplicationSet/ArgoCD, and your tests were super helpful in helping test them. These tests have helped with:
So thanks for writing these! The code looks good to go, but I would like to hold off on merging it until #176 is fixed (merging it now would destabilize the E2E tests until the race condition is fixed.) |
Thank you! These are probably the best tests I had ever written 😆 |
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.
As mentioned above, looks great and very helpful for finding bugs! Thanks for taking this on, and thanks for your patience in the time it took to get it merged 😄 .
Add two more tests:
closes #80