Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSCluster #2312

Karthik-K-N · 2025-04-17T05:30:03Z

What this PR does / why we need it:

This PR consists of following changes

Adopt CAPI v1beta2 status changes
Standardize the controller flow by utilising CAPI provided helper functions
Removed usage of deprecated github.com/pkg/errors package.

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 #2264

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:

Adopt CAPI v1beta2 status changes into IBMPowerVSCluster object.

netlify · 2025-04-17T05:30:19Z

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

Name	Link
🔨 Latest commit	`0ecada7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/680f43e5519aba00084a187d
😎 Deploy Preview	https://deploy-preview-2312.cluster-api-ibmcloud.sigs.k8s.io
📱 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.

Karthik-K-N · 2025-04-17T05:31:08Z

IBMPowerVSCluster object's status field looks like this on various stages now.

  v1beta2:
    Conditions:
      Last Transition Time:  2025-04-16T06:35:36Z
      Message:               
        * NetworkReady: Condition not yet reported
        * LoadBalancerReady: Condition not yet reported
        * TransitGatewayReady: Condition not yet reported
      Observed Generation:   1
      Reason:                ReadyUnknown
      Status:                Unknown
      Type:                  Ready
      Last Transition Time:  2025-04-16T06:35:39Z
      Message:
      Observed Generation:   1
      Reason:                Ready
      Status:                True
      Type:                  VPCReady
      Last Transition Time:  2025-04-16T06:35:45Z
      Message:
      Observed Generation:   1
      Reason:                Ready
      Status:                True
      Type:                  VPCSecurityGroupReady
      Last Transition Time:  2025-04-16T06:35:45Z
      Message:
      Observed Generation:   1
      Reason:                Ready
      Status:                True
      Type:                  VPCSubnetReady
      Last Transition Time:  2025-04-16T06:38:49Z
      Message:
      Observed Generation:   1
      Reason:                Ready
      Status:                True
      Type:                  WorkspaceReady
      Last Transition Time:  2025-04-16T06:35:23Z
      Message:
      Observed Generation:   1
      Reason:                NotPaused
      Status:                False
      Type:                  Paused


  v1beta2:
    Conditions:
      Last Transition Time:  2025-04-16T06:46:20Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  Ready
      Last Transition Time:  2025-04-16T06:45:56Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  LoadBalancerReady
      Last Transition Time:  2025-04-16T06:46:17Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  NetworkReady
      Last Transition Time:  2025-04-16T06:42:52Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  TransitGatewayReady
      Last Transition Time:  2025-04-16T06:35:39Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  VPCReady
      Last Transition Time:  2025-04-16T06:35:45Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  VPCSecurityGroupReady
      Last Transition Time:  2025-04-16T06:35:45Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  VPCSubnetReady
      Last Transition Time:  2025-04-16T06:38:49Z
      Message:
      Observed Generation:   2
      Reason:                Ready
      Status:                True
      Type:                  WorkspaceReady
      Last Transition Time:  2025-04-16T06:35:23Z
      Message:
      Observed Generation:   2
      Reason:                NotPaused
      Status:                False
      Type:                  Paused





v1beta2:
    Conditions:
      Last Transition Time:  2025-04-16T08:29:23Z
      Message:               * LoadBalancerReady: Deleting
* TransitGatewayReady: Deleting
      Observed Generation:   3
      Reason:                NotReady
      Status:                False
      Type:                  Ready
      Last Transition Time:  2025-04-16T08:32:39Z
      Message:
      Observed Generation:   3
      Reason:                Deleting
      Status:                False
      Type:                  LoadBalancerReady

cloud/scope/powervs_cluster.go

Amulyam24 · 2025-04-21T13:38:24Z

cloud/scope/powervs_cluster.go

 		}
 		if vpcDetails == nil {
-			return false, fmt.Errorf("failed to get VPC with ID %s", *vpcID)
+			return false, fmt.Errorf("vpc not found with ID %s", *vpcID)


Suggested change

return false, fmt.Errorf("vpc not found with ID %s", *vpcID)

return false, fmt.Errorf("VPC with ID %s not found", *vpcID)

Amulyam24 · 2025-04-21T13:39:17Z

cloud/scope/powervs_cluster.go

 	if err != nil {
-		return false, err
+		return false, fmt.Errorf("error checking VPC: %w", err)


Suggested change

return false, fmt.Errorf("error checking VPC: %w", err)

return false, fmt.Errorf("failed to check if VPC exists: %w", err)

Amulyam24 · 2025-04-21T13:41:11Z

cloud/scope/powervs_cluster.go

 	}
 	if len(vpcZones) == 0 {
 		return false, fmt.Errorf("failed to fetch VPC zones, no zone found for region %s", *s.VPC().Region)
 	}
 	// check whether user has set the vpc subnets
 	if len(s.IBMPowerVSCluster.Spec.VPCSubnets) == 0 {
 		// if the user did not set any subnet, we try to create subnet in all the zones.
+		log.V(3).Info("VPC subnets are not set, constructing one")


Suggested change

log.V(3).Info("VPC subnets are not set, constructing one")

log.V(3).Info("VPC subnets details are not set in spec, creating subnets in all zones in the given region")

cloud/scope/powervs_cluster.go

Amulyam24 · 2025-04-21T13:45:39Z

cloud/scope/powervs_cluster.go

-			return false, err
+		log.V(3).Info("Only VPC connection not exist in transit gateway, creating it")
+		if err := s.createTransitGatewayConnection(ctx, transitGateway.ID, ptr.To(getTGVPCConnectionName(*transitGateway.Name)), vpcCRN, vpcNetworkConnectionType); err != nil {
+			return false, fmt.Errorf("failed to create VPC transit gateway connections: %w", err)


Suggested change

return false, fmt.Errorf("failed to create VPC transit gateway connections: %w", err)

return false, fmt.Errorf("failed to create VPC transit gateway connection: %w", err)

Amulyam24 · 2025-04-21T13:45:51Z

cloud/scope/powervs_cluster.go

-			return false, err
+		log.V(3).Info("Only PowerVS connection not exist in transit gateway, creating it")
+		if err := s.createTransitGatewayConnection(ctx, transitGateway.ID, ptr.To(getTGPowerVSConnectionName(*transitGateway.Name)), pvsServiceInstanceCRN, powervsNetworkConnectionType); err != nil {
+			return false, fmt.Errorf("failed to create PowerVS transit gateway connections: %w", err)


Suggested change

return false, fmt.Errorf("failed to create PowerVS transit gateway connections: %w", err)

return false, fmt.Errorf("failed to create PowerVS transit gateway connection: %w", err)

Maybe missed to address in latest commits, please take care!

cloud/scope/powervs_cluster.go

controllers/ibmpowervsmachine_controller.go

Amulyam24 · 2025-04-21T14:07:32Z

Not to be handled in this PR but for future, we should consider

Move naming to PowerVS workspace instead of service instance to keep up with the cloud terms
Error format is incosistent with error fetching/creating and failed to fetch/create, should follow one format
wdyt?

Karthik-K-N · 2025-04-24T06:56:41Z

Not to be handled in this PR but for future, we should consider
1. Move naming to PowerVS workspace instead of service instance to keep up with the cloud terms

Yes, We are planning to do the same in our next api version where we plan to support multi-workspace,

2. Error format is incosistent with `error fetching/creating` and `failed to fetch/create`, should follow one format
   wdyt?

Yes, Agree, next iteration will fix the correct verb

Prajyot-Parab

Minor nits.
overall lgtm.

api/v1beta2/conditions_consts.go

cloud/scope/powervs_cluster.go

controllers/ibmpowervscluster_controller.go

Prajyot-Parab · 2025-04-25T11:18:14Z

/lgtm
/hold for @Amulyam24

dharaneeshvrd

Minor nits on logging
Few places you have reduced the logging level, please recheck them once
When ever we check resource in cloud, we say found in cloud. IMO just found also makes sense. e.g.: Found PowerVS service instance in cloud -> Found PowerVS service instance

cloud/scope/powervs_cluster.go

dharaneeshvrd · 2025-04-28T02:13:54Z

cloud/scope/powervs_cluster.go

 	if err != nil {
-		return "", false, err
+		return "", false, fmt.Errorf("failed to check for service instance state: %w", err)


Suggested change

return "", false, fmt.Errorf("failed to check for service instance state: %w", err)

return "", false, fmt.Errorf("failed to check service instance state: %w", err)

cloud/scope/powervs_cluster.go

dharaneeshvrd · 2025-04-28T02:34:01Z

cloud/scope/powervs_cluster.go

-			return false, err
+		log.V(3).Info("Only PowerVS connection not exist in transit gateway, creating it")
+		if err := s.createTransitGatewayConnection(ctx, transitGateway.ID, ptr.To(getTGPowerVSConnectionName(*transitGateway.Name)), pvsServiceInstanceCRN, powervsNetworkConnectionType); err != nil {
+			return false, fmt.Errorf("failed to create PowerVS transit gateway connections: %w", err)


Maybe missed to address in latest commits, please take care!

dharaneeshvrd · 2025-04-28T02:36:09Z

cloud/scope/powervs_cluster.go

 		err = s.checkLoadBalancerPort(loadBalancer)
 		if err != nil {


Suggested change

err = s.checkLoadBalancerPort(loadBalancer)

if err != nil {

if err = s.checkLoadBalancerPort(loadBalancer); err != nil {

cloud/scope/powervs_cluster.go

dharaneeshvrd · 2025-04-28T02:38:48Z

cloud/scope/powervs_cluster.go

 	if err != nil {
-		return err
+		return fmt.Errorf("failed to check if cos instance in cloud: %w", err)


Suggested change

return fmt.Errorf("failed to check if cos instance in cloud: %w", err)

return fmt.Errorf("failed to check if COS instance in cloud: %w", err)

controllers/ibmpowervscluster_controller.go

Karthik-K-N · 2025-04-28T05:25:00Z

Minor nits on logging Few places you have reduced the logging level, please recheck them once When ever we check resource in cloud, we say found in cloud. IMO just found also makes sense. e.g.: Found PowerVS service instance in cloud -> Found PowerVS service instance

Thank you for the revew, I have addressed all your comments.

Since we check for id of various resources in status, previously we kept "found in cloud" just to indicate that we are looking specifically in cloud.

Amulyam24 · 2025-04-28T05:40:05Z

/retest

Amulyam24

minor nit, LGTM!

Amulyam24 · 2025-04-28T06:59:41Z

api/v1beta2/conditions_consts.go

+	// VPCSecurityGroupDeletingV1Beta2Reason surfaces when the VPC security group is being deleted.
+	VPCSecurityGroupDeletingV1Beta2Reason = capiv1beta1.DeletingV1Beta2Reason
+
+	// TransitGatewayReadyV1Beta2Condition reports on the successful reconciliation of a PowerVS transit gateway.


Suggested change

// TransitGatewayReadyV1Beta2Condition reports on the successful reconciliation of a PowerVS transit gateway.

// TransitGatewayReadyV1Beta2Condition reports on the successful reconciliation of a transit gateway.

Prajyot-Parab · 2025-04-28T08:13:00Z

api/v1beta2/conditions_consts.go

+	IBMPowerVSClusterNotReadyV1Beta2Reason = capiv1beta1.NotReadyV1Beta2Reason
+
+	// IBMPowerVSClusterReadyUnknownV1Beta2Reason surfaces when at least one of the IBMPowerVSCluster readiness criteria is unknown
+	// and none of the IBMPowerVSCluster readiness criteria is not met.


Suggested change

// and none of the IBMPowerVSCluster readiness criteria is not met.

// and none of the IBMPowerVSCluster readiness criteria is met.

Prajyot-Parab

/lgtm

k8s-ci-robot · 2025-04-28T09:20:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Karthik-K-N, Prajyot-Parab

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~~ [Prajyot-Parab]

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

Prajyot-Parab · 2025-04-28T09:21:23Z

/unhold

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 Apr 17, 2025

k8s-ci-robot requested review from Amulyam24 and Prajyot-Parab April 17, 2025 05:30

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2025

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 17, 2025

Karthik-K-N force-pushed the update-powervs-cluster branch from 8abbc48 to 56b5d3b Compare April 17, 2025 06:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2025

Amulyam24 reviewed Apr 21, 2025

View reviewed changes

controllers/ibmpowervsmachine_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2025

Karthik-K-N added 2 commits April 24, 2025 10:38

Add v1beta2 changes to IBMPowerVSCluster

62fb845

Controller changes for v1beta2

0fa5a84

Karthik-K-N force-pushed the update-powervs-cluster branch from 56b5d3b to 849f4c6 Compare April 24, 2025 06:55

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2025

Prajyot-Parab requested changes Apr 24, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2025

k8s-ci-robot assigned Prajyot-Parab Apr 25, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2025

dharaneeshvrd reviewed Apr 28, 2025

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2025

Karthik-K-N mentioned this pull request Apr 28, 2025

Refactor reconcileNormal function to accept context from caller #2219

Closed

Amulyam24 reviewed Apr 28, 2025

View reviewed changes

Prajyot-Parab reviewed Apr 28, 2025

View reviewed changes

Update test files to accomodate controller changes

0ecada7

Karthik-K-N force-pushed the update-powervs-cluster branch from cc54762 to 0ecada7 Compare April 28, 2025 09:01

Prajyot-Parab approved these changes Apr 28, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2025

k8s-ci-robot merged commit af5ef66 into kubernetes-sigs:main Apr 28, 2025
14 checks passed

Karthik-K-N changed the title ~~Adopt CAPI v2beta2 status changes and respective controller changes for IBMPowerVSCluster~~ Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSCluster Apr 29, 2025

Karthik-K-N mentioned this pull request Apr 29, 2025

Standerdise the controller workflow #2129

Open

5 tasks

Karthik-K-N mentioned this pull request Jun 16, 2025

EPIC: Update CAPIBM resources to use v1beta2 API #2389

Open

4 tasks

	return false, fmt.Errorf("vpc not found with ID %s", *vpcID)
	return false, fmt.Errorf("VPC with ID %s not found", *vpcID)

	return false, fmt.Errorf("error checking VPC: %w", err)
	return false, fmt.Errorf("failed to check if VPC exists: %w", err)

	log.V(3).Info("VPC subnets are not set, constructing one")
	log.V(3).Info("VPC subnets details are not set in spec, creating subnets in all zones in the given region")

	return false, fmt.Errorf("failed to create VPC transit gateway connections: %w", err)
	return false, fmt.Errorf("failed to create VPC transit gateway connection: %w", err)

	return "", false, fmt.Errorf("failed to check for service instance state: %w", err)
	return "", false, fmt.Errorf("failed to check service instance state: %w", err)

	err = s.checkLoadBalancerPort(loadBalancer)
	if err != nil {
	if err = s.checkLoadBalancerPort(loadBalancer); err != nil {

	return fmt.Errorf("failed to check if cos instance in cloud: %w", err)
	return fmt.Errorf("failed to check if COS instance in cloud: %w", err)

	// TransitGatewayReadyV1Beta2Condition reports on the successful reconciliation of a PowerVS transit gateway.
	// TransitGatewayReadyV1Beta2Condition reports on the successful reconciliation of a transit gateway.

	// and none of the IBMPowerVSCluster readiness criteria is not met.
	// and none of the IBMPowerVSCluster readiness criteria is met.

Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSCluster #2312

Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSCluster #2312

Uh oh!

Conversation

Karthik-K-N commented Apr 17, 2025

Uh oh!

netlify bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Karthik-K-N commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amulyam24 commented Apr 21, 2025

Uh oh!

Karthik-K-N commented Apr 24, 2025

Uh oh!

Prajyot-Parab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Prajyot-Parab commented Apr 25, 2025

Uh oh!

dharaneeshvrd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Karthik-K-N commented Apr 28, 2025

Uh oh!

Amulyam24 commented Apr 28, 2025

Uh oh!

Amulyam24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prajyot-Parab left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 28, 2025

Uh oh!

Prajyot-Parab commented Apr 28, 2025

netlify bot commented Apr 17, 2025 •

edited

Loading

Amulyam24 left a comment •

edited

Loading