Configure Loadbalancer back-end pool only for controlplane machines #2004

carmal891 · 2024-10-12T19:26:34Z

What this PR does / why we need it:

Adds UTs to cover existing reconcile operations of VPC loadbalancer pool member creation
Adds check to ensure Loadbalancer back-end pool is only for controlplane machines

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

linux-foundation-easycla · 2024-10-12T19:26:38Z

The committers listed above are authorized under a signed CLA.

✅ login: carmal891 / name: Carmal (a469601)

k8s-ci-robot · 2024-10-12T19:26:43Z

Hi @carmal891. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2024-10-12T19:26:51Z

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

Name	Link
🔨 Latest commit	`a469601`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/671b88021d63a3000841d378
😎 Deploy Preview	https://deploy-preview-2004--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.

Amulyam24 · 2024-10-14T04:47:14Z

/ok-to-test

Karthik-K-N · 2024-10-14T06:59:57Z

controllers/ibmpowervsmachine_controller.go

-	if poolMember != nil && *poolMember.ProvisioningStatus != string(infrav1beta2.VPCLoadBalancerStateActive) {
-		return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil
+
+	if poolMemberReconcileResult, err := r.handleLoadBalancerPoolMemberConfiguration(machineScope); err != nil || poolMemberReconcileResult.RequeueAfter > 0 {


How about machineScope.handleLoadBalancerPoolMemberConfiguration()? or just calling util.IsControlPlaneMachine(machineScope.Machine) here itself.

@Karthik-K-N The handleLoadBalancerPoolMemberConfiguration was introduced more on the lines of a helper func that handles 1. controlPlaneMachine check, 2. machineScope.CreateVPCLoadBalancerPoolMember and 3. check if poolmember provisioning status is not active. So may not be apt to add as a single responsibility function that is similar to the existing machineScope methods.

To your second question, we could go with something like this
if util.IsControlPlaneMachine(machineScope.Machine) { return r.handleLoadBalancerPoolMemberConfiguration(machineScope) }

because adding all the conditions on the parent will cause the current cyclomatic complexity to cross threshold. Let me know your opinion

Got it thanks.

Updated as discussed

Karthik-K-N · 2024-10-14T07:01:23Z

controllers/ibmpowervsmachine_controller.go

+// handleLoadBalancerPoolMemberConfiguration handles loadbalancer pool member creation flow.
+func (r *IBMPowerVSMachineReconciler) handleLoadBalancerPoolMemberConfiguration(machineScope *scope.PowerVSMachineScope) (ctrl.Result, error) {
+	if !util.IsControlPlaneMachine(machineScope.Machine) {
+		return ctrl.Result{}, nil


May be lets use log.V(3) to log that we are skipping loadbalancer configuration as its not control plane machine

Karthik-K-N · 2024-10-14T07:02:25Z

controllers/ibmpowervsmachine_controller.go

+	if !util.IsControlPlaneMachine(machineScope.Machine) {
+		return ctrl.Result{}, nil
+	}
+	machineScope.Info("Configuring control plane machine to backend LoadBalancer pool", "machine name", machineScope.IBMPowerVSMachine.Name)


I think its better use single name for key in logs, like machineName instead of machine name, It might be easy to filter out in observability tools.

Karthik-K-N · 2024-10-14T08:10:46Z

controllers/ibmpowervsmachine_controller_test.go

+			g := NewWithT(t)
+			setup(t)
+			t.Cleanup(teardown)
+			secret := &corev1.Secret{


Since secret, pvsmachine, machine used at multiple tests, can we move this to helper function?

Karthik-K-N · 2024-10-14T08:12:26Z

controllers/ibmpowervsmachine_controller_test.go

+			}
+
+			mockclient := fake.NewClientBuilder().WithObjects([]client.Object{secret, pvsmachine, machine}...).Build()
+			machineScope = &scope.PowerVSMachineScope{


Can't we call NewPowerVSMachineScope()?

Correct me if I’m wrong, but based on issue #1937, it’s preferred for UT not to rely on the New functions, as their flow shouldn't be within the scope of the parent test.

ok, It would be good if can reduce the number of duplicate lines if possible.

reduced overall lines of code

Karthik-K-N · 2024-10-21T10:32:31Z

controllers/ibmpowervsmachine_controller.go

 func (r *IBMPowerVSMachineReconciler) reconcileNormal(machineScope *scope.PowerVSMachineScope) (ctrl.Result, error) {
+	ctx := context.Background()
+	log := ctrl.LoggerFrom(ctx)


Suggested change

log := ctrl.LoggerFrom(ctx)

log := ctrl.LoggerFrom(context.Background())

Karthik-K-N · 2024-10-21T10:34:57Z

controllers/ibmpowervsmachine_controller.go

+
+	return ctrl.Result{}, nil
+}
+
 func (r *IBMPowerVSMachineReconciler) reconcileNormal(machineScope *scope.PowerVSMachineScope) (ctrl.Result, error) {


We should create another issue for refactoring these functions to accept the context from caller, like reconcileNormal(ctx context.Context, machineScope *scope.PowerVSMachineScope)
We are not fully utilizing the contextual logging, May be this can be the first step.

Issue created - #2018.
Updated portions using machinescope.Info to contextual logging

Karthik-K-N · 2024-10-21T10:40:28Z

controllers/ibmpowervsmachine_controller.go

-		return ctrl.Result{RequeueAfter: 1 * time.Minute}, nil
+
+	if util.IsControlPlaneMachine(machineScope.Machine) {
+		log.V(3).Info("skipping loadbalancer configuration as it is not control plane machine", "machineName", machineScope.IBMPowerVSMachine.Name)


In case of control plane machines we are configuring LB, So lets log that here and move this out of if block for better debugging.

misplaced the logging statement. Corrected

Karthik-K-N

Thank you

LGTM

controllers/ibmpowervsmachine_controller.go

Karthik-K-N · 2024-10-22T06:19:17Z

/assign @Amulyam24

Karthik-K-N · 2024-10-23T05:54:35Z

/easycla

carmal891 · 2024-10-23T07:44:29Z

/easycla

Karthik-K-N · 2024-10-23T08:32:30Z

Please squash the commits. LGTM from my side. Thank you.

carmal891 · 2024-10-23T10:06:33Z

Please squash the commits. LGTM from my side. Thank you.

done

Karthik-K-N · 2024-10-23T11:45:49Z

/lgtm

/assign @Amulyam24 @Prajyot-Parab

Prajyot-Parab · 2024-10-23T15:46:45Z

controllers/ibmpowervsmachine_controller.go

 	}
+	machineScope.Info("skipping loadbalancer configuration as it is not control plane machine", "machineName", machineScope.IBMPowerVSMachine.Name)


Suggested change

machineScope.Info("skipping loadbalancer configuration as it is not control plane machine", "machineName", machineScope.IBMPowerVSMachine.Name)

machineScope.Info("skipping loadbalancer configuration for worker machine", "machineName", machineScope.IBMPowerVSMachine.Name)

Prajyot-Parab

/lgtm

k8s-ci-robot · 2024-10-25T12:26:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carmal891, 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 Oct 12, 2024

k8s-ci-robot requested review from Amulyam24 and Prajyot-Parab October 12, 2024 19:26

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 12, 2024

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

carmal891 force-pushed the LB-BackendPool-Update-1990 branch from ded3687 to f735079 Compare October 13, 2024 04:00

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 14, 2024

Karthik-K-N reviewed Oct 14, 2024

View reviewed changes

carmal891 force-pushed the LB-BackendPool-Update-1990 branch from 1948e33 to 36c6127 Compare October 20, 2024 14:03

carmal891 requested a review from Karthik-K-N October 20, 2024 14:10

Karthik-K-N reviewed Oct 21, 2024

View reviewed changes

carmal891 requested a review from Karthik-K-N October 21, 2024 12:26

carmal891 force-pushed the LB-BackendPool-Update-1990 branch from 719512a to f7a9497 Compare October 21, 2024 14:02

Karthik-K-N reviewed Oct 22, 2024

View reviewed changes

controllers/ibmpowervsmachine_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned Amulyam24 Oct 22, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 23, 2024

carmal891 force-pushed the LB-BackendPool-Update-1990 branch from 368001b to d00e568 Compare October 23, 2024 08:37

k8s-ci-robot assigned Prajyot-Parab and Karthik-K-N Oct 23, 2024

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

carmal891 requested a review from Karthik-K-N October 25, 2024 11:52

Prajyot-Parab suggested changes Oct 25, 2024

View reviewed changes

Configure Loadbalancer back-end pool only for controlplane machines

a469601

carmal891 force-pushed the LB-BackendPool-Update-1990 branch from d00e568 to a469601 Compare October 25, 2024 11:58

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

carmal891 requested a review from Prajyot-Parab October 25, 2024 12:00

Prajyot-Parab approved these changes Oct 25, 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 25, 2024

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

k8s-ci-robot merged commit 803b6de into kubernetes-sigs:main Oct 25, 2024
12 of 13 checks passed

	log := ctrl.LoggerFrom(ctx)
	log := ctrl.LoggerFrom(context.Background())

		}
		machineScope.Info("skipping loadbalancer configuration as it is not control plane machine", "machineName", machineScope.IBMPowerVSMachine.Name)

Configure Loadbalancer back-end pool only for controlplane machines #2004

Configure Loadbalancer back-end pool only for controlplane machines #2004

Conversation

carmal891 commented Oct 12, 2024 • edited Loading

linux-foundation-easycla bot commented Oct 12, 2024 • edited Loading

k8s-ci-robot commented Oct 12, 2024

netlify bot commented Oct 12, 2024 • edited Loading

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

Amulyam24 commented Oct 14, 2024

Choose a reason for hiding this comment

carmal891 Oct 14, 2024 • 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

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

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

Karthik-K-N commented Oct 22, 2024

Karthik-K-N commented Oct 23, 2024

carmal891 commented Oct 23, 2024

Karthik-K-N commented Oct 23, 2024

carmal891 commented Oct 23, 2024

Karthik-K-N commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prajyot-Parab left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 25, 2024

carmal891 commented Oct 12, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 12, 2024 •

edited

Loading

netlify bot commented Oct 12, 2024 •

edited

Loading

carmal891 Oct 14, 2024 •

edited

Loading