Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSMachine #2272

Karthik-K-N · 2025-03-26T04:39:18Z

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:

Tried to keep different commits for each use case

/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 IBMPowerVSMachine object.

netlify · 2025-03-26T04:39:35Z

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

Name	Link
🔨 Latest commit	`62f9e9f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/67f3cae35e29b70008fc0cb8
😎 Deploy Preview	https://deploy-preview-2272.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-03-26T06:06:43Z

/test pull-cluster-api-provider-ibmcloud-apidiff

Karthik-K-N · 2025-03-26T06:12:16Z

Output from testing

kubectl get ibmpowervsmachine karthikkn-capi-powervs-control-plane-4vfvx -o yaml

status:
  addresses:
  - address: karthikkn-capi-powervs-control-plane-4vfvx
    type: InternalDNS
  - address: karthikkn-capi-powervs-control-plane-4vfvx
    type: Hostname
  - address: 192.168.0.12
    type: InternalIP
  conditions:
  - lastTransitionTime: "2025-03-26T06:06:11Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2025-03-26T06:06:11Z"
    status: "True"
    type: InstanceReady
  health: WARNING
  instanceID: b13dc26f-d491-442d-afde-83487aed2628
  instanceState: ACTIVE
  ready: true
  v1beta2:
    conditions:
    - lastTransitionTime: "2025-03-26T06:06:11Z"
      message: ""
      observedGeneration: 2
      reason: Ready
      status: "True"
      type: Ready
    - lastTransitionTime: "2025-03-26T06:06:11Z"
      message: ""
      observedGeneration: 2
      reason: InstanceReady
      status: "True"
      type: InstanceReady
    - lastTransitionTime: "2025-03-26T05:59:04Z"
      message: ""
      observedGeneration: 2
      reason: NotPaused
      status: "False"
      type: Paused

$ kubectl get machines
NAME                                         CLUSTER                  NODENAME                                     PROVIDERID                                                                                         PHASE         AGE     VERSION
karthikkn-capi-powervs-control-plane-4vfvx   karthikkn-capi-powervs   karthikkn-capi-powervs-control-plane-4vfvx   ibmpowervs://osa/osa21/ca6af7ac-75bd-41b3-9c73-61f8cde01aea/b13dc26f-d491-442d-afde-83487aed2628   Running       13m     v1.29.3
karthikkn-capi-powervs-control-plane-7ncq6   karthikkn-capi-powervs                                                ibmpowervs://osa/osa21/ca6af7ac-75bd-41b3-9c73-61f8cde01aea/2bb05c0b-4507-4fb7-86ad-c38052385bea   Provisioned   4m34s   v1.29.3
karthikkn-capi-powervs-control-plane-pbxld   karthikkn-capi-powervs   karthikkn-capi-powervs-control-plane-pbxld   ibmpowervs://osa/osa21/ca6af7ac-75bd-41b3-9c73-61f8cde01aea/b200a882-d711-4324-80a2-bcaa839d8d9c   Running       52m     v1.29.3
karthikkn-capi-powervs-md-0-4bt5m-l65ms      karthikkn-capi-powervs   karthikkn-capi-powervs-md-0-4bt5m-l65ms      ibmpowervs://osa/osa21/ca6af7ac-75bd-41b3-9c73-61f8cde01aea/825139af-3f01-4b38-8cf4-35f238825650   Running       52m     v1.29.3

Karthik-K-N · 2025-03-26T06:15:51Z

$ kubectl get ibmpowervsmachines.v1beta1.infrastructure.cluster.x-k8s.io karthikkn-capi-powervs-control-plane-4vfvx -o yaml

status:
  addresses:
  - address: karthikkn-capi-powervs-control-plane-4vfvx
    type: InternalDNS
  - address: karthikkn-capi-powervs-control-plane-4vfvx
    type: Hostname
  - address: 192.168.0.12
    type: InternalIP
  conditions:
  - lastTransitionTime: "2025-03-26T06:06:11Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2025-03-26T06:06:11Z"
    status: "True"
    type: InstanceReady
  health: WARNING
  instanceID: b13dc26f-d491-442d-afde-83487aed2628
  instanceState: ACTIVE
  ready: true

$ kubectl get ibmpowervsmachines.v1beta2.infrastructure.cluster.x-k8s.io karthikkn-capi-powervs-control-plane-4vfvx -o yaml

status:
  addresses:
  - address: karthikkn-capi-powervs-control-plane-4vfvx
    type: InternalDNS
  - address: karthikkn-capi-powervs-control-plane-4vfvx
    type: Hostname
  - address: 192.168.0.12
    type: InternalIP
  conditions:
  - lastTransitionTime: "2025-03-26T06:06:11Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2025-03-26T06:06:11Z"
    status: "True"
    type: InstanceReady
  health: WARNING
  instanceID: b13dc26f-d491-442d-afde-83487aed2628
  instanceState: ACTIVE
  ready: true
  v1beta2:
    conditions:
    - lastTransitionTime: "2025-03-26T06:06:11Z"
      message: ""
      observedGeneration: 2
      reason: Ready
      status: "True"
      type: Ready
    - lastTransitionTime: "2025-03-26T06:06:11Z"
      message: ""
      observedGeneration: 2
      reason: InstanceReady
      status: "True"
      type: InstanceReady
    - lastTransitionTime: "2025-03-26T05:59:04Z"
      message: ""
      observedGeneration: 2
      reason: NotPaused
      status: "False"
      type: Paused

Karthik-K-N · 2025-03-28T05:16:03Z

@Prajyot-Parab @Amulyam24 @dharaneeshvrd please take a look when you get chance.

Prajyot-Parab

/lgtm

Prajyot-Parab · 2025-04-03T06:29:43Z

/cc @dharaneeshvrd

dharaneeshvrd

I see few logs have been completely removed, hope that is intentional
Overall LGTM.
I don't see any changes that would affect the hypershift flow. Please think once from that perspective too. Thanks!

dharaneeshvrd · 2025-04-07T04:28:45Z

cloud/scope/powervs_machine.go

@@ -635,7 +617,7 @@ func (m *PowerVSMachineScope) GetRawBootstrapData() ([]byte, error) {
 	secret := &corev1.Secret{}
 	key := types.NamespacedName{Namespace: m.Machine.Namespace, Name: *m.Machine.Spec.Bootstrap.DataSecretName}
 	if err := m.Client.Get(context.TODO(), key, secret); err != nil {
-		return nil, fmt.Errorf("failed to retrieve bootstrap data secret for IBMPowerVSMachine %s/%s: %w", m.Machine.Namespace, m.Machine.Name, err)


Any reason to remove this info?

We are embedding the machine names, namespaces, cluster name so on into logger and thats added to context, Like

log = log.WithValues("Machine", klog.KObj(machine)) ctx = ctrl.LoggerInto(ctx, log)

These can be found in function
func (r *IBMPowerVSMachineReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {

So the log line will by default contains these information and we need not explicitly add it again.

Karthik-K-N · 2025-04-07T05:07:32Z

I see few logs have been completely removed, hope that is intentional Overall LGTM. I don't see any changes that would affect the hypershift flow. Please think once from that perspective too. Thanks!

Thank you, Yes, I just refactored logs to avoid unnecessary or duplicate lines.

dharaneeshvrd · 2025-04-07T05:17:07Z

/lgtm

Amulyam24

Minor comments, overall LGTM @Karthik-K-N !

Amulyam24 · 2025-04-07T06:50:50Z

cloud/scope/powervs_machine.go


 	instanceReply, err := m.ensureInstanceUnique(m.IBMPowerVSMachine.Name)
 	if err != nil {
 		return nil, err
 	} else if instanceReply != nil {
-		// TODO need a reasonable wrapped error.
+		log.Info("PowerVS instance already exists")


It would be good to log the name of the machine.

The logger will automatically print the machine and other owner details as mentioned here #2272 (comment)

Amulyam24 · 2025-04-07T06:54:27Z

cloud/scope/powervs_machine.go

@@ -271,38 +255,42 @@ func (m *PowerVSMachineScope) CreateMachine() (*models.PVMInstanceReference, err
 		if con.Type == infrav1beta2.InstanceReadyCondition && con.Status == corev1.ConditionUnknown {


would we have to remove this going further as it is similar to the below check?

when CAPI completely migrates into v1beta2 we will remove the earlier one.

Amulyam24 · 2025-04-07T06:59:26Z

cloud/scope/powervs_machine.go

 	m.IBMPowerVSMachine.Status.FailureReason = &reason
 }

 // SetFailureMessage will set status FailureMessage for the machine.
 func (m *PowerVSMachineScope) SetFailureMessage(message string) {
+	//nolint:staticcheck


can you share why staticcheck is being ignored?

Because we are setting the FailureMessage which has been deprecated as per CAPI v1beta2 guidelines.

Amulyam24 · 2025-04-07T08:16:14Z

controllers/ibmpowervsmachine_controller_test.go

@@ -235,6 +236,7 @@ func TestIBMPowerVSMachineReconciler_Reconcile(t *testing.T) {
 						Name:      tc.powervsMachine.Name,
 					},
 				})
+				fmt.Println(err)


missed removing it?

Updated it in the latest commit

Amulyam24 · 2025-04-07T10:26:39Z

api/v1beta2/ibmpowervsmachine_types.go

+func (r *IBMPowerVSMachine) GetV1Beta2Conditions() []metav1.Condition {
+	if r.Status.V1Beta2 == nil {
+		return nil
+	}
+	return r.Status.V1Beta2.Conditions
+}
+
+// SetV1Beta2Conditions sets conditions for an API object.
+func (r *IBMPowerVSMachine) SetV1Beta2Conditions(conditions []metav1.Condition) {
+	if r.Status.V1Beta2 == nil {
+		r.Status.V1Beta2 = &IBMPowerVSMachineV1Beta2Status{}
+	}
+	r.Status.V1Beta2.Conditions = conditions
+}
+


@Karthik-K-N , where are we using this functions? I'm unable to find it in this PR, it is required for future changes?

These are part of Setter interface. We need to implement these interfaces to use utility functions to set the conditions, As they accept setter interface as parameter: https://github.com/kubernetes-sigs/cluster-api/blob/09e6c8a14c2dd955924e7534727974d1e89643e5/util/conditions/v1beta2/setter.go#L73

Got it, Thanks @Karthik-K-N 👍🏼

Prajyot-Parab

/lgtm

k8s-ci-robot · 2025-04-08T05:31:44Z

[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

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Mar 26, 2025

k8s-ci-robot requested review from Amulyam24 and mkumatag March 26, 2025 04:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 26, 2025

Karthik-K-N force-pushed the standardise-controller branch from 8db3c4e to e6c4127 Compare March 26, 2025 11:56

Prajyot-Parab reviewed Apr 3, 2025

View reviewed changes

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

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

k8s-ci-robot requested a review from dharaneeshvrd April 3, 2025 06:29

dharaneeshvrd reviewed Apr 7, 2025

View reviewed changes

k8s-ci-robot assigned dharaneeshvrd Apr 7, 2025

Amulyam24 reviewed Apr 7, 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 7, 2025

Karthik-K-N force-pushed the standardise-controller branch from 40ede8b to 5abb1a8 Compare April 7, 2025 09:50

Amulyam24 reviewed Apr 7, 2025

View reviewed changes

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

Karthik-K-N added 5 commits April 7, 2025 18:21

Add v1beta2 conditions to IBMPowerVSMachine

b375131

Add watch-filter flag to enable controller to watch for label

2579036

Add v1beta2 condition constants

61266a5

Standardise controller and update logs

3018183

Test fixes after controller changes

f3d4f13

Remove println in test files

62f9e9f

Karthik-K-N force-pushed the standardise-controller branch from 5abb1a8 to 62f9e9f Compare April 7, 2025 12:53

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

Prajyot-Parab approved these changes Apr 8, 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 8, 2025

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

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

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

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

Standerdise the controller workflow #2129

Open

5 tasks

		@@ -271,38 +255,42 @@ func (m PowerVSMachineScope) CreateMachine() (models.PVMInstanceReference, err
		if con.Type == infrav1beta2.InstanceReadyCondition && con.Status == corev1.ConditionUnknown {

Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSMachine #2272

Adopt CAPI v1beta2 status changes and respective controller changes for IBMPowerVSMachine #2272

Uh oh!

Conversation

Karthik-K-N commented Mar 26, 2025

Uh oh!

netlify bot commented Mar 26, 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 Mar 26, 2025

Uh oh!

Karthik-K-N commented Mar 26, 2025

Uh oh!

Karthik-K-N commented Mar 26, 2025

Uh oh!

Karthik-K-N commented Mar 28, 2025

Uh oh!

Prajyot-Parab left a comment

Choose a reason for hiding this comment

Uh oh!

Prajyot-Parab commented Apr 3, 2025

Uh oh!

dharaneeshvrd left a comment

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!

Karthik-K-N commented Apr 7, 2025

Uh oh!

dharaneeshvrd commented Apr 7, 2025

Uh oh!

Amulyam24 left a comment

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!

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!

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!

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 8, 2025

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Mar 26, 2025 •

edited

Loading