Fix PowerVS cluster's load balancer status check #2029

dharaneeshvrd · 2024-10-28T08:07:55Z

Need to depend on load balancer's state to set the cluster's ready status instead of using requeue logic which is designed to block the reconciliation.

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

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-10-28T08:08:13Z

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

Name	Link
🔨 Latest commit	`87fe803`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/671f888fc972bc0008077418
😎 Deploy Preview	https://deploy-preview-2029--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-10-28T08:42:43Z

/cc @Karthik-K-N

Karthik-K-N · 2024-10-28T08:47:08Z

cloud/scope/powervs_cluster.go

-			if requeue := s.checkLoadBalancerStatus(*loadBalancer); requeue {
-				return requeue, nil
+			if isReady := s.checkLoadBalancerStatus(*loadBalancer); !isReady {
+				s.V(3).Info("LoadBalancer is still not Active", "name", *loadBalancer.Name, "state", *loadBalancer.ProvisioningStatus)


*loadbalancer.Name can be nil when user sets only id. so better avoid it.

It's coming from cloud API resp right, would be able to create a LB without name?

oh yeah, sorry, I overlooked loadbalancer from

for index, loadBalancer := range loadBalancers {

Karthik-K-N · 2024-10-28T08:53:55Z

cloud/scope/powervs_cluster.go

 		}

-		// check VPC load balancer exist in cloud
-		loadBalancerStatus, err := s.checkLoadBalancer(loadBalancer)


why is this removed?

checkLoadBalancer checks load balancer by the user provided name, which we already do in prev block here by recently added code via this.

s.GetLoadBalancerID(loadBalancer.Name) just checks only from status not from cloud!

Sry, did not look properly, reverted the changes.

Karthik-K-N

/lgtm
/assign @Amulyam24 @mkumatag

Amulyam24

/lgtm

Thank you for the quick fix @dharaneeshvrd!

Karthik-K-N · 2024-10-28T09:30:57Z

/hold

@KeerthanaAP is doing some testing, will update here once done.

KeerthanaAP · 2024-10-28T10:35:44Z

@dharaneeshvrd I have tried deploying the cluster with your changes and tried to verify the loadbalancer status.
I could see one loadbalancer is in active state and other is in create_pending state but still the ready is set to true

 status:
    conditions:
    - lastTransitionTime: "2024-10-28T10:09:43Z"
      status: "True"
      type: COSInstanceCreated
    - lastTransitionTime: "2024-10-28T10:06:06Z"
      status: "True"
      type: LoadBalancerReady
    - lastTransitionTime: "2024-10-28T10:14:23Z"
      status: "True"
      type: NetworkReady
    - lastTransitionTime: "2024-10-28T10:05:58Z"
      status: "True"
      type: ServiceInstanceReady
    - lastTransitionTime: "2024-10-28T10:09:34Z"
      status: "True"
      type: TransitGatewayReady
    - lastTransitionTime: "2024-10-28T10:05:08Z"
      status: "True"
      type: VPCReady
    - lastTransitionTime: "2024-10-28T10:05:30Z"
      status: "True"
      type: VPCSecurityGroupReady
    - lastTransitionTime: "2024-10-28T10:05:30Z"
      status: "True"
      type: VPCSubnetReady
    cosInstance:
      controllerCreated: true
      id: 2535acaf-64b4-43af-b62f-0d04ef5d802a
    dhcpServer:
      controllerCreated: true
      id: 7d5d4674-568f-4095-bb2f-d88a1ed86caa
    loadBalancers:
      keer-test-r2tmb-loadbalancer:
        controllerCreated: true
        hostname: eecc5943-eu-gb.lb.appdomain.cloud
        id: r018-eecc5943-6cba-4801-9f8d-d1f364c0d662
        state: active
      keer-test-r2tmb-loadbalancer-int:
        controllerCreated: true
        hostname: fed61d1c-eu-gb.lb.appdomain.cloud
        id: r018-fed61d1c-4b28-4a31-b5be-707e1c5099eb
        state: create_pending
    network:
      controllerCreated: true
      id: a3780531-b7ff-4a2e-b795-4fde4aad5297
    ready: true

Karthik-K-N · 2024-10-28T10:39:26Z

Thanks @KeerthanaAP for testing it quickly.
@dharaneeshvrd i think we discussed about this but forgot , we need to make sure all the LBs are active before infra is set to true.

Need to depend on load balancer's state to set the cluster's ready status instead of using requeue logic which is designed to block the reconciliation.

dharaneeshvrd · 2024-10-28T12:55:03Z

@KeerthanaAP Can you please double check this? It is working for me when I tested in local and in the log you have provided for this deployment does not have my changes.
@Karthik-K-N I have considered and did the changes earlier and it was working properly, but it was creating the LBs serially, now I have added a flag to create them without blocking further LBs reconcile. PTAL now.

Karthik-K-N · 2024-10-28T13:34:57Z

@KeerthanaAP Can you please double check this? It is working for me when I tested in local and in the log you have provided for this deployment does not have my changes.

Checking with @ hamzy about this in slack

@Karthik-K-N I have considered and did the changes earlier and it was working properly, but it was creating the LBs serially, now I have added a flag to create them without blocking further LBs reconcile. PTAL now.

Got it earlier will work but there was no gurantee that in case of multiple LBs all will be in Active as we used to return once we found one LB in active. But I think with this change we should be good

Karthik-K-N

Its been verified and works as expected.
/lgtm

mkumatag

/unhold

k8s-ci-robot · 2024-10-29T04:57:01Z

[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 Oct 28, 2024

k8s-ci-robot requested review from mkumatag and Prajyot-Parab October 28, 2024 08:08

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 28, 2024

k8s-ci-robot requested a review from Karthik-K-N October 28, 2024 08:42

Karthik-K-N reviewed Oct 28, 2024

View reviewed changes

dharaneeshvrd force-pushed the fix-lb-status branch from 148a645 to 647e9da Compare October 28, 2024 09:19

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2024

Karthik-K-N reviewed Oct 28, 2024

View reviewed changes

k8s-ci-robot assigned Amulyam24, mkumatag and Karthik-K-N Oct 28, 2024

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

Amulyam24 reviewed Oct 28, 2024

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 Oct 28, 2024

Fix PowerVS cluster's load balancer status check

87fe803

Need to depend on load balancer's state to set the cluster's ready status instead of using requeue logic which is designed to block the reconciliation.

dharaneeshvrd force-pushed the fix-lb-status branch from 647e9da to 87fe803 Compare October 28, 2024 12:50

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

Karthik-K-N reviewed Oct 29, 2024

View reviewed changes

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

mkumatag added this to the v0.9.0 milestone Oct 29, 2024

mkumatag approved these changes Oct 29, 2024

View reviewed changes

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

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

k8s-ci-robot merged commit 9b07704 into kubernetes-sigs:main Oct 29, 2024
13 checks passed

hamzy mentioned this pull request Nov 5, 2024

OCPBUGS-44247: PowerVS: Update 4.17 CAPI ibmcloud to 9b077049 openshift/installer#9182

Merged

Fix PowerVS cluster's load balancer status check #2029

Fix PowerVS cluster's load balancer status check #2029

Uh oh!

Conversation

dharaneeshvrd commented Oct 28, 2024

Uh oh!

netlify bot commented Oct 28, 2024 • 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!

dharaneeshvrd commented Oct 28, 2024

Uh oh!

Karthik-K-N Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

dharaneeshvrd Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N left a comment

Choose a reason for hiding this comment

Uh oh!

Amulyam24 left a comment

Choose a reason for hiding this comment

Uh oh!

Karthik-K-N commented Oct 28, 2024

Uh oh!

KeerthanaAP commented Oct 28, 2024

Uh oh!

Karthik-K-N commented Oct 28, 2024

Uh oh!

dharaneeshvrd commented Oct 28, 2024

Uh oh!

Karthik-K-N commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karthik-K-N 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!

mkumatag left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Oct 28, 2024 •

edited

Loading

Karthik-K-N commented Oct 28, 2024 •

edited

Loading

Karthik-K-N left a comment •

edited

Loading