Handling external deletion of the resources in the clusterresources group #584

worryg0d · 2023-10-03T11:42:37Z

Issue 572

Added:

handling of the external deletion of the resources on Instaclustr
fixed external changes for VPC peering resources when Instaclustr API returns a list of subnets in a different order

closes #572

OleksiienkoMykyta · 2023-10-03T13:04:30Z

pkg/instaclustr/mock/client.go

@@ -375,3 +375,7 @@ func (c *mockClient) GetResizeOperationsByClusterDataCentreID(cdcID string) ([]*
 func (c *mockClient) GetAWSVPCPeering(peerID string) (*models.AWSVPCPeering, error) {
 	panic("GetAWSVPCPeering: is not implemented")
 }
+
+func (c *mockClient) CheckIfAWSEndpointServicePrincipalExists(id string) (bool, error) {
+	panic("CheckIfAWSEndpointServicePrincipalExists: is not implemented")


Everything is great, but I think we should cover it with tests in future

OleksiienkoMykyta · 2023-10-03T13:08:13Z

I agree with your statment, we should rename "state" to "instaclustrResourceState"

testisnullus · 2023-10-04T21:11:17Z

pkg/instaclustr/client.go

@@ -2215,6 +2215,31 @@ func (c *Client) UpdateClusterSettings(clusterID string, settings *models.Cluste
 	return nil
 }

+func (c *Client) CheckIfAWSEndpointServicePrincipalExists(id string) (bool, error) {
+	url := c.serverHostname + AWSEndpointServicePrincipalEndpoint + "/" + id


AWSEndpointServicePrincipalEndpoint already has a backslash at the end, why did you add an additional one?

testisnullus · 2023-10-04T21:14:22Z

pkg/instaclustr/client.go

@@ -2215,6 +2215,31 @@ func (c *Client) UpdateClusterSettings(clusterID string, settings *models.Cluste
 	return nil
 }

+func (c *Client) CheckIfAWSEndpointServicePrincipalExists(id string) (bool, error) {


Let's rename this method to GetAWSEndpointServicePrincipal and will return the service principal entity itself and an error. We can check the existence of the principal by using errors.Is(err, instaclustr.NotFound) func also

testisnullus · 2023-10-04T21:16:37Z

controllers/clusterresources/awsvpcpeering_controller.go

@@ -49,6 +48,8 @@ type AWSVPCPeeringReconciler struct {
 	EventRecorder record.EventRecorder
 }

+const awsVPCPeeringStatusCodeDeleted = "deleted"


Let's use const from package models instead of this variable

Moved to the models pkg

ribaraka · 2023-10-10T11:35:33Z

I want to hear your opinion about the added state field which indicates the state of the resource on Instaclustr. Maybe changing it to something like instaclustrResourceState will be better.

The source of truth is the Instaclustr Console. So whatever the state/status of the Instaclustr resource is, it should be directly reflected on the k8s state field without any deviation. For now, I don't see a good reason to create a new field or give a new name to the existing state field of the resource.

Because some resources also have their status codes like AWS VPC peering. So it looks a bit weird when there is the following status:
status:
  state: CREATED
  statusCode: deleted
The field state indicates that the resource is created on Instaclustr, but statusCode: deleted indicates that the VPC peering is deleted on AWS.

In this example, I suggest you maintain consistency between the state and statusCode fields, or, even better, you can consider using only the state field to represent the statusCode.

ribaraka · 2023-10-10T11:44:08Z

controllers/clusterresources/awsencryptionkey_controller.go

@@ -129,6 +129,7 @@ func (r *AWSEncryptionKeyReconciler) handleCreate(
 		)

 		encryptionKey.Status = *encryptionKeyStatus
+		encryptionKey.Status.State = models.CreatedStatus


try the thing I suggested by replacing statusCode with state or vice versa, as you prefer.

Started using existing Status field

ribaraka · 2023-10-10T11:45:18Z

controllers/clusterresources/awsencryptionkey_controller.go

+	l := log.FromContext(ctx)
+
+	patch := key.NewPatch()
+	key.Status.State = models.DeletedStatus


again, the thing with displaying the statuscode in the state

ribaraka · 2023-10-10T11:48:02Z

controllers/clusterresources/awsendpointserviceprincipal_controller.go

+func (r *AWSEndpointServicePrincipalReconciler) newWatchStatusJob(ctx context.Context, principal *clusterresourcesv1beta1.AWSEndpointServicePrincipal) scheduler.Job {
+	return func() error {
+		_, err := r.API.GetAWSEndpointServicePrincipal(principal.Status.ID)
+		if err != nil {
+			if errors.Is(err, instaclustr.NotFound) {
+				return r.handleExternalDelete(ctx, principal)
+			}
+
+			return err
+		}
+
+		return nil
+	}
+}


add a checker here for the existence of k8s resource

Added here and for other resources

…mplemented

worryg0d self-assigned this Oct 3, 2023

worryg0d added the enhancement New feature or request label Oct 3, 2023

worryg0d marked this pull request as draft October 3, 2023 11:42

worryg0d force-pushed the issue-572-clusterresources branch 4 times, most recently from c118958 to 659e937 Compare October 3, 2023 12:24

worryg0d marked this pull request as ready for review October 3, 2023 12:26

worryg0d requested review from ribaraka, testisnullus, DoodgeMatvey, RostislavPorohnya, tengu-alt and OleksiienkoMykyta October 3, 2023 12:26

OleksiienkoMykyta approved these changes Oct 3, 2023

View reviewed changes

tengu-alt approved these changes Oct 3, 2023

View reviewed changes

testisnullus requested changes Oct 4, 2023

View reviewed changes

worryg0d force-pushed the issue-572-clusterresources branch from 659e937 to 3fb5ed9 Compare October 5, 2023 08:00

worryg0d requested a review from testisnullus October 5, 2023 08:01

testisnullus approved these changes Oct 8, 2023

View reviewed changes

ribaraka requested changes Oct 10, 2023

View reviewed changes

worryg0d force-pushed the issue-572-clusterresources branch from 3fb5ed9 to 2f9b933 Compare October 11, 2023 07:45

worryg0d requested a review from ribaraka October 11, 2023 07:46

issue-572-2, handling of external deletion for clusterresources was i…

8d99391

…mplemented

worryg0d force-pushed the issue-572-clusterresources branch from 2f9b933 to 8d99391 Compare October 12, 2023 12:27

ribaraka approved these changes Oct 17, 2023

View reviewed changes

testisnullus merged commit 4d5d744 into main Oct 17, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling external deletion of the resources in the clusterresources group #584

Handling external deletion of the resources in the clusterresources group #584

worryg0d commented Oct 3, 2023 •

edited

Loading

OleksiienkoMykyta Oct 3, 2023

OleksiienkoMykyta commented Oct 3, 2023

testisnullus Oct 4, 2023

worryg0d Oct 5, 2023

testisnullus Oct 4, 2023

worryg0d Oct 5, 2023

testisnullus Oct 4, 2023

worryg0d Oct 5, 2023

ribaraka commented Oct 10, 2023

ribaraka Oct 10, 2023

worryg0d Oct 11, 2023

ribaraka Oct 10, 2023

worryg0d Oct 11, 2023

ribaraka Oct 10, 2023

worryg0d Oct 11, 2023

Handling external deletion of the resources in the clusterresources group #584

Handling external deletion of the resources in the clusterresources group #584

Conversation

worryg0d commented Oct 3, 2023 • edited Loading

Issue 572

Choose a reason for hiding this comment

OleksiienkoMykyta commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ribaraka commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worryg0d commented Oct 3, 2023 •

edited

Loading