-
Notifications
You must be signed in to change notification settings - Fork 107
CLOUDP-82115: Implement delete retry loop #149
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
Changes from all commits
7aecda9
a66ffdd
1832036
2cc8a53
34455f2
3dd25fa
d91f972
d93ca22
0e2e4a0
525a420
23edcd4
3a00444
9d69526
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,8 +18,11 @@ package atlasproject | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "go.mongodb.org/atlas/mongodbatlas" | ||
| "go.uber.org/zap" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
|
|
@@ -134,12 +137,29 @@ func (r *AtlasProjectReconciler) Delete(e event.DeleteEvent) error { | |
| return fmt.Errorf("cannot build Atlas client: %w", err) | ||
| } | ||
|
|
||
| _, err = atlasClient.Projects.Delete(context.Background(), project.Status.ID) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot delete Atlas project: %w", err) | ||
| } | ||
|
|
||
| log.Infow("Successfully deleted Atlas project", "projectID", project.Status.ID) | ||
| go func() { | ||
| timeout := time.Now().Add(workflow.DefaultTimeout) | ||
|
|
||
| for time.Now().Before(timeout) { | ||
| _, err = atlasClient.Projects.Delete(context.Background(), project.Status.ID) | ||
| var apiError *mongodbatlas.ErrorResponse | ||
| if errors.As(err, &apiError) && apiError.ErrorCode == atlas.NotInGroup { | ||
| log.Infow("Project doesn't exist or is already deleted", "projectID", project.Status.ID) | ||
| return | ||
| } | ||
|
|
||
| if err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the same as above |
||
| log.Errorw("cannot delete Atlas project", "error", err) | ||
| time.Sleep(workflow.DefaultRetry) | ||
| continue | ||
| } | ||
|
|
||
| log.Infow("Successfully deleted Atlas project", "projectID", project.Status.ID) | ||
| return | ||
| } | ||
|
|
||
| log.Errorw("Failed to delete Atlas project in time", "projectID", project.Status.ID) | ||
| }() | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,19 +58,12 @@ var _ = Describe("AtlasCluster", func() { | |
| if createdCluster != nil { | ||
| By("Removing Atlas Cluster " + createdCluster.Name) | ||
| Expect(k8sClient.Delete(context.Background(), createdCluster)).To(Succeed()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdCluster.Name), 600, interval).Should(BeTrue()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdCluster.Spec.Name), 600, interval).Should(BeTrue()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🥇 |
||
| } | ||
|
|
||
| // TODO: CLOUDP-82115 | ||
| // By("Removing Atlas Project " + createdProject.Status.ID) | ||
| // Expect(k8sClient.Delete(context.Background(), createdProject)).To(Succeed()) | ||
| // Eventually(checkAtlasProjectRemoved(createdProject.Status.ID), 20, interval).Should(BeTrue()) | ||
|
|
||
| By("Removing Atlas Project " + createdProject.Status.ID) | ||
| // This is a bit strange but the delete request right after the cluster is removed may fail with "Still active cluster" error | ||
| // UI shows the cluster being deleted though. Seems to be the issue only if removal is done using API, | ||
| // if the cluster is terminated using UI - it stays in "Deleting" state | ||
| Eventually(removeAtlasProject(createdProject.Status.ID), 600, interval).Should(BeTrue()) | ||
| Expect(k8sClient.Delete(context.Background(), createdProject)).To(Succeed()) | ||
| Eventually(checkAtlasProjectRemoved(createdProject.Status.ID), 60, interval).Should(BeTrue()) | ||
| } | ||
| removeControllersAndNamespace() | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,12 +28,13 @@ import ( | |
| "github.com/mongodb/mongodb-atlas-kubernetes/pkg/util/testutil" | ||
| ) | ||
|
|
||
| const DevMode = false | ||
|
|
||
| const UserPasswordSecret = "user-password-secret" | ||
| const DBUserPassword = "Passw0rd!" | ||
| const UserPasswordSecret2 = "second-user-password-secret" | ||
| const DBUserPassword2 = "H@lla#!" | ||
| const ( | ||
| DevMode = false | ||
| UserPasswordSecret = "user-password-secret" | ||
| DBUserPassword = "Passw0rd!" | ||
| UserPasswordSecret2 = "second-user-password-secret" | ||
| DBUserPassword2 = "H@lla#!" | ||
| ) | ||
|
|
||
| var _ = Describe("AtlasDatabaseUser", func() { | ||
| const interval = time.Second * 1 | ||
|
|
@@ -101,20 +102,23 @@ var _ = Describe("AtlasDatabaseUser", func() { | |
|
|
||
| return | ||
| } | ||
|
|
||
| if createdProject != nil && createdProject.ID() != "" { | ||
| if createdClusterGCP != nil { | ||
| By("Removing Atlas Cluster " + createdClusterGCP.Name) | ||
| Expect(k8sClient.Delete(context.Background(), createdClusterGCP)).To(Succeed()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdClusterGCP.Name), 600, interval).Should(BeTrue()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdClusterGCP.Spec.Name), 600, interval).Should(BeTrue()) | ||
| } | ||
|
|
||
| if createdClusterAWS != nil { | ||
| By("Removing Atlas Cluster " + createdClusterAWS.Name) | ||
| Expect(k8sClient.Delete(context.Background(), createdClusterAWS)).To(Succeed()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdClusterAWS.Name), 600, interval).Should(BeTrue()) | ||
| Eventually(checkAtlasClusterRemoved(createdProject.Status.ID, createdClusterAWS.Spec.Name), 600, interval).Should(BeTrue()) | ||
| } | ||
|
|
||
| By("Removing Atlas Project " + createdProject.Status.ID) | ||
| Eventually(removeAtlasProject(createdProject.Status.ID), 600, interval).Should(BeTrue()) | ||
| Expect(k8sClient.Delete(context.Background(), createdProject)).To(Succeed()) | ||
| Eventually(checkAtlasProjectRemoved(createdProject.Status.ID), 60, interval).Should(BeTrue()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [q] do you know if we still need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some time could pass between a call to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, right, we remove the resource in K8s, not in Atlas |
||
| } | ||
| removeControllersAndNamespace() | ||
| }) | ||
|
|
@@ -289,12 +293,14 @@ func normalize(user mongodbatlas.DatabaseUser, projectID string) mongodbatlas.Da | |
| user.Password = "" | ||
| return user | ||
| } | ||
|
|
||
| func tryConnect(projectID string, cluster mdbv1.AtlasCluster, user mdbv1.AtlasDatabaseUser) func() error { | ||
| return func() error { | ||
| _, err := mongoClient(projectID, cluster, user) | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| func mongoClient(projectID string, cluster mdbv1.AtlasCluster, user mdbv1.AtlasDatabaseUser) (*mongo.Client, error) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
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.
what do you think if we check here the error that is returned in case the cluster is already removed - and break the loop in this case?
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.
Good catch!