Attach connections to existing transit gateway when its not there #1901

dharaneeshvrd · 2024-08-01T07:45:42Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1887

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

netlify · 2024-08-01T07:45:59Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`b24fe96`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/66e2bb972c09f20007630d68
😎 Deploy Preview	https://deploy-preview-1901--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dharaneeshvrd · 2024-08-01T07:56:47Z

/cc @Karthik-K-N

Karthik-K-N

few doubts, will take a look again

Karthik-K-N · 2024-08-01T08:03:13Z

cloud/scope/powervs_cluster.go

@@ -394,6 +394,35 @@ func (s *PowerVSClusterScope) GetServiceInstanceID() string {
 	return ""
 }

+func (s *PowerVSClusterScope) SetTransitGatewayStatus(id *string, controllerCreated *bool, powerVSConnResource *infrav1beta2.ResourceReference, vpcConnResource *infrav1beta2.ResourceReference) {


Suggested change

func (s *PowerVSClusterScope) SetTransitGatewayStatus(id *string, controllerCreated *bool, powerVSConnResource *infrav1beta2.ResourceReference, vpcConnResource *infrav1beta2.ResourceReference) {

func (s *PowerVSClusterScope) SetTransitGatewayStatus(id *string, controllerCreated *bool, powerVSConnResource , vpcConnResource *infrav1beta2.ResourceReference) {

Karthik-K-N · 2024-08-01T09:05:18Z

cloud/scope/powervs_cluster.go

+
+	s.IBMPowerVSCluster.Status.TransitGateway = &infrav1beta2.TransitGatewayStatus{
+		ID:                id,
+		ControllerCreated: controllerCreated,


won't it set ControllerCreated to false during second reconcilation?

Second time, we won't call the SetTransitgatewayStatus(). We would check status of TG and connections with the ID set in status and return.

Karthik-K-N · 2024-08-01T09:08:39Z

cloud/scope/powervs_cluster.go

-func (s *PowerVSClusterScope) checkTransitGatewayConnections(id *string) (bool, error) {
-	requeue := false
+// checkAndUpdateTransitGatewayConnections checks given transit gateway's connections status.
+// // if update is set to true, it updates the transit gateway connections too if it is not exist already.


Suggested change

// // if update is set to true, it updates the transit gateway connections too if it is not exist already.

// if update is set to true, it updates the transit gateway connections too if it is not exist already.

Karthik-K-N

few questions. otherwise overall LGTM,

Karthik-K-N · 2024-08-02T10:31:28Z

cloud/scope/powervs_cluster.go

+	if powerVSAttached && vpcAttached {
+		if s.IBMPowerVSCluster.Status.TransitGateway == nil {
+			vpcConnResource = &infrav1beta2.ResourceReference{
+				ControllerCreated: ptr.To(false),


We intentionally not storing connections id here is status right? also if we dont set anything the default value for controller created is also false, but I think explicitly setting will enhance readability of code.

Yes Agree, refactored with returning the ID and checking the attached status based on that. Ptal!

Amulyam24

Initial comments on the create TG flow, will be taking a closer look

Amulyam24 · 2024-08-05T06:18:32Z

cloud/scope/powervs_cluster.go

 			}
-			s.V(3).Info("VPC connection successfully attached to transit gateway", "name", *conn.Name)
-			vpcAttached = true
+			s.V(3).Info("Transit gateway VPC connection OK", "name", *conn.Name)


Can we use a better logging statement?

Amulyam24 · 2024-08-05T06:36:42Z

cloud/scope/powervs_cluster.go

@@ -1627,71 +1651,66 @@ func (s *PowerVSClusterScope) ReconcileTransitGateway() (bool, error) {
 		if err != nil {
 			return false, err
 		}
-		requeue, err := s.checkTransitGateway(tg.ID)
+		requeue, _, _, err := s.checkAndUpdateTransitGateway(tg, false)


In a case where TG pre-exists and no connections exist, if we pass false here, it won't create any connections right?

Modified the logic to handle the existing resource passed via id in second block and taken care there to update the status. Ptal!

Karthik-K-N · 2024-08-12T12:22:11Z

cloud/scope/powervs_cluster.go

 	if err != nil {
-		return "", false, err
+		return "", false, nil, nil, err
 	}
 	if transitGateway == nil || transitGateway.ID == nil {


I think I dont remember, Do you know when can transitGateway can be nil, If we try to query a TG with invalid id or name which does not exist, It will throw an error right?

yes, will remove this.

Karthik-K-N · 2024-08-12T12:24:18Z

cloud/scope/powervs_cluster.go

 	if err != nil {
 		return false, err
 	}
 	if tgID != "" {
-		s.V(3).Info("Transit gateway found in IBM Cloud")
-		s.SetStatus(infrav1beta2.ResourceTypeTransitGateway, infrav1beta2.ResourceReference{ID: &tgID, ControllerCreated: ptr.To(false)})
+		s.SetTransitGatewayStatus(&tgID, ptr.To(false), powerVSConn, vpcConn)


My another query was wont this set controllerCreated to false, In the second loop?

It won't come to this block on second reconciliation since status would have already generated and try to validate the resource's status in 1st block.

Amulyam24 · 2024-08-29T10:04:18Z

cloud/scope/powervs_cluster.go

 	}
-	return *transitGateway.ID, requeue, nil
-}



In a case where spec.TG.ID is nil and given TG name doesn't exist in cloud, GetTransitGatewayByName, returns nil. Without a nil check here, it would fail going further at checkTransitGatewayStatus where *tg.Status would be nil right? @dharaneeshvrd, Please correct me if my understanding is not right..

Amulyam24 · 2024-09-02T10:49:40Z

cloud/scope/powervs_cluster.go

-		s.Info("Skipping transit gateway deletion as resource is not created by controller")
-		return false, nil
+		s.Info("Skipping transit gateway deletion as resource is not created by controller, but will check if connections are created by the controller.")
+		skipTGDeletion = true
 	}

 	if s.IBMPowerVSCluster.Status.TransitGateway.ID == nil {


@dharaneesh, while testing, came across a scenario where for any reason if TG reconcilation fails and it doesn't set the s.IBMPowerVSCluster.Status.TransitGateway, it will return a nil pointer error during deletion as ID will be nil. Though the value of s.IBMPowerVSCluster.Status.TransitGateway is being checked during isResourceCreatedByController(infrav1beta2.ResourceTypeTransitGateway), we don't return after it to allow deletion of TG connections separately.

Amulyam24

Overall LGTM!

@dharaneeshvrd, hope you have tested all possible scenarios.

Karthik-K-N

/lgtm

hamzy · 2024-09-11T13:07:29Z

When will this PR merge?

mkumatag · 2024-08-16T11:32:16Z

cloud/scope/powervs_cluster.go

+		Name:             connName,
+	})
+
+	return conn.ID, err


what happens when there is an error? don't we face a nil pointer dereference issue because conn is nil when err occurs.

mkumatag · 2024-08-16T11:33:22Z

cloud/scope/powervs_cluster.go

+
+// createTransitGatewayConnections creates PowerVS and VPC connections in the transit gateway.
+func (s *PowerVSClusterScope) createTransitGatewayConnections(tg *tgapiv1.TransitGateway, pvsServiceInstanceCRN, vpcCRN *string) (*string, *string, error) {
+	powerVSConnID, err := s.createTransitGatewayConnection(tg.ID, ptr.To(fmt.Sprintf("%s-pvs-con", *tg.Name)), ptr.To(string(powervsNetworkConnectionType)), pvsServiceInstanceCRN)


this %s-pvs-con format can be placed somewhere so that we can reuse and easy to handle as well.

mkumatag · 2024-08-16T11:33:41Z

cloud/scope/powervs_cluster.go

+		return nil, nil, fmt.Errorf("failed to create PowerVS connection in transit gateway: %w", err)
+	}
+
+	vpcConnID, err := s.createTransitGatewayConnection(tg.ID, ptr.To(fmt.Sprintf("%s-vpc-con", *tg.Name)), ptr.To(string(vpcNetworkConnectionType)), vpcCRN)


same as above

mkumatag · 2024-09-10T06:04:02Z

cloud/scope/powervs_cluster.go

 	}
-	if transitGateway == nil || transitGateway.ID == nil {
+
+	if transitGateway == nil || (transitGateway != nil && transitGateway.ID == nil) {


Suggested change

if transitGateway == nil || (transitGateway != nil && transitGateway.ID == nil) {

if transitGateway == nil || transitGateway.ID == nil {

mkumatag · 2024-09-10T06:05:05Z

cloud/scope/powervs_cluster.go

-// checkTransitGateway checks transit gateway exist in cloud.
-func (s *PowerVSClusterScope) isTransitGatewayExists() (string, bool, error) {
+// isTransitGatewayExists checks transit gateway exist in cloud and if exist with unattached connections, it attaches the connection too.
+func (s *PowerVSClusterScope) isTransitGatewayExists() (string, bool, *infrav1beta2.ResourceReference, *infrav1beta2.ResourceReference, error) {


function name misleads as you are doing attach operation as well in this function.

mkumatag · 2024-09-11T13:24:54Z

When will this PR merge?

soon, wondering if can test these changes with OCP directly pulling from the @dharaneeshvrd 's branch

mkumatag

/lgtm

k8s-ci-robot · 2024-09-12T12:14:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharaneeshvrd, mkumatag

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:

~~OWNERS~~ [mkumatag]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2024

k8s-ci-robot requested review from mkumatag and Prajyot-Parab August 1, 2024 07:45

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 1, 2024

k8s-ci-robot requested a review from Karthik-K-N August 1, 2024 07:56

dharaneeshvrd force-pushed the tg-connections branch from 21a2662 to ba86fed Compare August 1, 2024 08:13

Karthik-K-N reviewed Aug 1, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-connections branch from ba86fed to 984f3e5 Compare August 1, 2024 11:27

Karthik-K-N reviewed Aug 2, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-connections branch 3 times, most recently from 54b7576 to a261261 Compare August 5, 2024 06:13

Amulyam24 reviewed Aug 5, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-connections branch 3 times, most recently from 6229226 to 29d57ee Compare August 6, 2024 04:25

mkumatag added this to the v0.9.0 milestone Aug 6, 2024

Karthik-K-N reviewed Aug 12, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-connections branch from 29d57ee to 673438b Compare August 16, 2024 05:21

Amulyam24 reviewed Aug 29, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-connections branch from 673438b to fae53e5 Compare August 29, 2024 15:09

Amulyam24 mentioned this pull request Sep 2, 2024

Refactor reconcilation of resources #1937

Merged

Amulyam24 reviewed Sep 2, 2024

View reviewed changes

dharaneeshvrd force-pushed the tg-connections branch from fae53e5 to 1a15e21 Compare September 5, 2024 05:36

Amulyam24 reviewed Sep 9, 2024

View reviewed changes

Karthik-K-N reviewed Sep 9, 2024

View reviewed changes

k8s-ci-robot assigned Karthik-K-N Sep 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2024

mkumatag requested changes Sep 11, 2024

View reviewed changes

Attach connections to existing transit gateway when its not there

b24fe96

dharaneeshvrd force-pushed the tg-connections branch from 1a15e21 to b24fe96 Compare September 12, 2024 09:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2024

mkumatag approved these changes Sep 12, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag Sep 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2024

k8s-ci-robot merged commit 632a722 into kubernetes-sigs:main Sep 12, 2024
13 checks passed

Karthik-K-N mentioned this pull request Sep 17, 2024

EPIC - Introduce new API Specs and infra creation flow #1611

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attach connections to existing transit gateway when its not there #1901

Attach connections to existing transit gateway when its not there #1901

dharaneeshvrd commented Aug 1, 2024

netlify bot commented Aug 1, 2024 •

edited

Loading

dharaneeshvrd commented Aug 1, 2024

Karthik-K-N left a comment

Karthik-K-N Aug 1, 2024

Karthik-K-N Aug 1, 2024

dharaneeshvrd Aug 5, 2024

Karthik-K-N Aug 1, 2024

Karthik-K-N left a comment

Karthik-K-N Aug 2, 2024

dharaneeshvrd Aug 5, 2024

Amulyam24 left a comment •

edited

Loading

Amulyam24 Aug 5, 2024

Amulyam24 Aug 5, 2024

dharaneeshvrd Aug 6, 2024

Karthik-K-N Aug 12, 2024

dharaneeshvrd Aug 14, 2024

Karthik-K-N Aug 12, 2024

dharaneeshvrd Aug 14, 2024

Amulyam24 Aug 29, 2024

Amulyam24 Sep 2, 2024 •

edited

Loading

Amulyam24 left a comment

Karthik-K-N left a comment

hamzy commented Sep 11, 2024

mkumatag Aug 16, 2024

mkumatag Aug 16, 2024

mkumatag Aug 16, 2024

mkumatag Sep 10, 2024

mkumatag Sep 10, 2024

mkumatag commented Sep 11, 2024

mkumatag left a comment

k8s-ci-robot commented Sep 12, 2024

	func (s PowerVSClusterScope) SetTransitGatewayStatus(id string, controllerCreated bool, powerVSConnResource infrav1beta2.ResourceReference, vpcConnResource *infrav1beta2.ResourceReference) {
	func (s PowerVSClusterScope) SetTransitGatewayStatus(id string, controllerCreated bool, powerVSConnResource , vpcConnResource infrav1beta2.ResourceReference) {

	// // if update is set to true, it updates the transit gateway connections too if it is not exist already.
	// if update is set to true, it updates the transit gateway connections too if it is not exist already.

	if transitGateway == nil \|\| (transitGateway != nil && transitGateway.ID == nil) {
	if transitGateway == nil \|\| transitGateway.ID == nil {

Attach connections to existing transit gateway when its not there #1901

Attach connections to existing transit gateway when its not there #1901

Conversation

dharaneeshvrd commented Aug 1, 2024

netlify bot commented Aug 1, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

dharaneeshvrd commented Aug 1, 2024

Karthik-K-N left a 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

Karthik-K-N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amulyam24 left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amulyam24 Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Amulyam24 left a comment

Choose a reason for hiding this comment

Karthik-K-N left a comment

Choose a reason for hiding this comment

hamzy commented Sep 11, 2024

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

mkumatag commented Sep 11, 2024

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 12, 2024

netlify bot commented Aug 1, 2024 •

edited

Loading

Amulyam24 left a comment •

edited

Loading

Amulyam24 Sep 2, 2024 •

edited

Loading