-
Notifications
You must be signed in to change notification settings - Fork 52
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
e2e: Enable appset DR actions #1443
Conversation
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.
This works, and we can look to enhance the binding and configmap creation as independent of each deployment as the next step. If so we can add it to the tracker here #1398 and once this PR is rebased it can be merged.
e2e/deployers/applicationset.go
Outdated
return err | ||
} | ||
|
||
err = deleteManagedClusterSetBinding(McsbName, namespace) |
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.
In a parallel test run another AppSet maybe using the same namespace to deploy, at which point this binding becomes a more common setup need than per invocation of Deploy|Undeploy
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.
yes, currently appset-deployment-0-busybox and appset-deployment-1-busybox both use the same mcsb "default" and both are in argocd ns. Need improve the create and delete for mcsb.
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 added code to first check if only 1 appset remaining before deleting the mcsb in the argocd ns.
return err | ||
} | ||
|
||
err = deleteConfigMap(name, namespace) |
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.
Same comment as the binding creation/deletion
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.
currently for configmap, different cm name are used, like "appset-deployment-0-busybox" and "appset-deployment-1-busybox". So configmap should be ok.
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.
The config map should be part of drenv setup. We setup ArgoCD to be able to deliver AppSets to different ManagedCluster using decisions from PlacementDecision. This CM as a result should be part of the ArgoCD plugin configuration and not part of e2e.
Signed-off-by: jacklu <jilu@redhat.com>
6744cef
to
85f2369
Compare
Also did code refactor to use a function to get dr action namespace Signed-off-by: jacklu <jilu@redhat.com>
33f1e63
to
734fe39
Compare
also remove repeated retry logs Signed-off-by: jacklu <jilu@redhat.com>
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.
LGTM, some minor things to consider as enhancements down the line.
return err | ||
} | ||
|
||
err = deleteConfigMap(name, namespace) |
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.
The config map should be part of drenv setup. We setup ArgoCD to be able to deliver AppSets to different ManagedCluster using decisions from PlacementDecision. This CM as a result should be part of the ArgoCD plugin configuration and not part of e2e.
|
||
// multiple appsets could use the same mcsb in argocd ns. | ||
// so delete mcsb if only 1 appset is in argocd ns | ||
lastAppset, err := isLastAppsetInArgocdNs(namespace) |
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.
This can be slightly racy, let's assume that a prallel invocation has progressed to line 24 above, it would have not yet created the AppSet and this check would hence pass and delete the binding.
A simple fix would be to create the binding after creating the AppSet in the Deploy function.
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.
this still could not completely remove the race condition. Thread 1 could know it is last appset, and before it deletes mcsb, thread 2 could create appset and mcsb, and then thread 1 deletes the mcsb.
If we want to fully avoid delete mcsb conflict, a simple way is to just leave mcsb not removed in argocd ns. Since this mcsb only has one in the argocd ns, I do not see much harm to just leave it there.
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.
Also the PlacementDecisionConfigMap is needed by each applicationset. Currently each appset uses different configmap but the cm content is same.
It should be ok if all appsets use same configmap. But since this need drenv change to create such configmap, I would prefer to use a separate PR to address this.
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.
Or, do you want drenv to create both mcsb and PlacementDecisionConfigMap in argocd ns, so that e2e do not need manage them for appset?
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.
ack on the race, just something to remember I think.
On the drenv change for the config map, agree that it would be handled in a separate PR.
The mcsb I am not sure, it is common for Argo, but for Subscriptions it is per namespace, so it could be part of Ramen setup where we add the binding to the Argo NS, but this can wait.
No description provided.