MULTIARCH-3667: Loadbalancer integration support for control plane machines #41

Karthik-K-N · 2023-03-10T07:49:44Z

With the introduction of cluster-control-plane-machine-set-operator(CPMS operator) now we can replace the control-plane machines in openshift cluster.
Control-plance machines will be attached to two loadbalancer in case of public cluster during the cluster creation and it will be done by the installer component.
When there is change in cpms object, the cpms operator will trigger machine replacement and during this time we need to update the loadbalancer with newly created vm ip and remove the ip of machine that is going to be deleted.

The sample control-plane machine object looks like this

apiVersion: machine.openshift.io/v1beta1
kind: Machine
metadata:
  creationTimestamp: null
  labels:
    machine.openshift.io/cluster-api-cluster: rdr-kn01032033-gjnlr
    machine.openshift.io/cluster-api-machine-role: master
    machine.openshift.io/cluster-api-machine-type: master
  name: karthik-master-1
  namespace: openshift-machine-api
spec:
  providerSpec:
    value:
      apiVersion: machine.openshift.io/v1
      credentialsSecret:
        name: powervs-credentials
      image:
        name: rhcos-rdr-kn08031125-vhwtg
        type: Name
      keyPairName: vhwtg-key
      kind: PowerVSMachineProviderConfig
      loadBalancers:
      - name: rdr-kn08031125-vhwtg-loadbalancer-int
        type: Application
      - name: rdr-kn08031125-vhwtg-loadbalancer
        type: Application
      memoryGiB: 32
      network:
        regex: ^DHCPSERVER.*rdr-kn08031125-vhwtg.*_Private$
        type: RegEx
      processorType: Shared
      processors: "0.5"
      serviceInstance:
        id: df1a3112-5067-4366-8a44-fdfcec
        type: ID
      systemType: s922
      userDataSecret:
        name: master-user-data

Proposed api changes: openshift/api#1415

Pending work

Unit testcases
Use paging mechanism

Karthik-K-N · 2023-03-10T07:50:14Z

@Prajyot-Parab @dharaneeshvrd Please take a look and provide your feedback.

pkg/actuators/machine/reconciler.go

dharaneeshvrd · 2023-03-14T12:49:22Z

pkg/actuators/machine/reconciler.go

+			}
+			if !alreadyRegistered {
+				// make sure that LoadBalancer is in active state
+				loadBalancer, _, err := powerVSClient.GetLoadBalancer(&vpcv1.GetLoadBalancerOptions{


Do we need another Get call here?

In our case a loadbalancer will have 2 pools, So after updating one pool, if we try to update second one it will fail as the loadbalancer wont be in active state, so instead of throwing ugly error added a check.

dharaneeshvrd · 2023-03-14T12:50:32Z

pkg/actuators/machine/reconciler.go

+		klog.Infof("Deleting IP %s from LoadBalancer %s ", internalIP, lbName)
+
+		// Fetch the LoadBalancer details
+		loadBalancer, _, err := powerVSClient.GetLoadBalancer(&vpcv1.GetLoadBalancerOptions{


Same as previous comment

Karthik-K-N · 2023-03-17T04:27:08Z

@dharaneeshvrd @Prajyot-Parab Please take a look when you get a chance, If the approach looks good I will go ahead and fix CI errors and add UT.

Prajyot-Parab · 2023-03-21T06:25:56Z

pkg/actuators/machine/reconciler.go


+	internalIP := r.getMachineInternalIP()
+	if internalIP != "" {
+		klog.Infof("deregister ip %s of machine %s form LoadBalancer", internalIP, r.machine.Name)


Minor Nit:

Suggested change

klog.Infof("deregister ip %s of machine %s form LoadBalancer", internalIP, r.machine.Name)

klog.Infof("deregister ip %s of machine %s from LoadBalancer", internalIP, r.machine.Name)

JoelSpeed

I don't have any feedback here apart from styling nits, I'd suggested always wrapping errors with additional context where possible, it tends to help with the flow of errors when debugging IMO, I'll leave the call to this repos maintainers though

JoelSpeed · 2023-04-05T09:52:04Z

pkg/actuators/machine/reconciler.go

+	internalIP := r.getMachineInternalIP()
+	if internalIP != "" {
+		klog.Infof("deregister ip %s of machine %s form LoadBalancer", internalIP, r.machine.Name)
+		if err = r.removeFromApplicationLoadBalancers(internalIP); err != nil {


Nit: no need to shadow the err var when you do this inline if style

Suggested change

if err = r.removeFromApplicationLoadBalancers(internalIP); err != nil {

if err := r.removeFromApplicationLoadBalancers(internalIP); err != nil {

JoelSpeed · 2023-04-05T09:52:24Z

pkg/actuators/machine/reconciler.go

+		err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP)
+		if err != nil {


Nit: should inline this

Suggested change

err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP)

if err != nil {

if err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP); err != nil {

JoelSpeed · 2023-04-05T09:53:00Z

pkg/actuators/machine/reconciler.go

+		err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP)
+		if err != nil {
+			klog.Errorf("%s: Failed to register application load balancers: %v", r.machine.Name, err)
+			return err


Should wrap this error with additional context before returning

@Karthik-K-N please take care of this comment..

JoelSpeed · 2023-04-05T09:53:14Z

pkg/actuators/machine/reconciler.go

+func registerWithApplicationLoadBalancers(powerVSClient client.Client, loadBalancerNames []string, internalIP string) error {
+	lbMap, err := getLoadBalancers(powerVSClient, loadBalancerNames)
+	if err != nil {
+		return err


Should wrap this error with additional context before returning

JoelSpeed · 2023-04-05T09:54:50Z

pkg/actuators/machine/reconciler.go

+	}
+
+	if err := utils.PagingHelper(f); err != nil {
+		return nil, err


Should wrap the error

JoelSpeed · 2023-04-05T09:55:06Z

pkg/actuators/machine/reconciler.go

+	}
+
+	if loadBalancersList == nil {
+		return nil, fmt.Errorf("no loadbalancer is retrieved")


No format string here

Suggested change

return nil, fmt.Errorf("no loadbalancer is retrieved")

return nil, errors.New("no loadbalancer is retrieved")

JoelSpeed · 2023-04-05T09:55:27Z

pkg/actuators/machine/reconciler.go

+					ID: &lbOptions.id,
+				})
+				if err != nil {
+					return err


Should add context

JoelSpeed · 2023-04-05T10:12:47Z

pkg/actuators/machine/reconciler.go

+				})
+				response, _, err := powerVSClient.CreateLoadBalancerPoolMember(options)
+				if err != nil {
+					return errors.Wrapf(err, "error creating LoadBalacner pool member")


Use fmt.Errorf for consistency with the rest of this PR

mkumatag · 2023-04-06T00:42:20Z

I don't have any feedback here apart from styling nits, I'd suggested always wrapping errors with additional context where possible, it tends to help with the flow of errors when debugging IMO, I'll leave the call to this repos maintainers though

Agree, @Karthik-K-N lets fix this wherever possible as a separate PR.

dharaneeshvrd · 2023-04-20T07:04:04Z

pkg/actuators/machine/reconciler.go

+		}
+	}
+	if len(lbMap) != len(loadBalancerNames) {
+		return nil, fmt.Errorf("not able to find %s loadbalancer in cloud", loadBalancerNames)


There is a possibility that this will have load balancers that are exist too in case of more than one load balancer is on search.

dharaneeshvrd

Overall looks good to me!

Prajyot-Parab

lgtm

mkumatag · 2023-04-27T16:34:12Z

pkg/client/powervs_client.go

 	c.ImageClient = instance.NewIBMPIImageClient(ctx, c.session, cloudInstanceID)
 	c.DHCPClient = instance.NewIBMPIDhcpClient(ctx, c.session, cloudInstanceID)
+
+	vpcZone, err := utils.VPCRegionForPowerVSRegion(c.region)


should be a region

Suggested change

vpcZone, err := utils.VPCRegionForPowerVSRegion(c.region)

vpcRegion, err := utils.VPCRegionForPowerVSRegion(c.region)

mkumatag · 2023-04-27T16:35:35Z

go.mod

+require (
+	github.com/IBM/vpc-go-sdk v0.35.0
+	github.com/onsi/ginkgo/v2 v2.8.1
+)


Any specific reason for creating a different block? if not - can this be merged with above block?

mkumatag · 2023-04-27T16:40:55Z

pkg/actuators/machine/reconciler.go

+		err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP)
+		if err != nil {
+			klog.Errorf("%s: Failed to register application load balancers: %v", r.machine.Name, err)
+			return err


@Karthik-K-N please take care of this comment..

mkumatag · 2023-04-27T16:41:25Z

pkg/actuators/machine/reconciler.go

+func registerWithApplicationLoadBalancers(powerVSClient client.Client, loadBalancerNames []string, internalIP string) error {
+	lbMap, err := getLoadBalancers(powerVSClient, loadBalancerNames)
+	if err != nil {
+		return err


JoelSpeed · 2023-05-02T16:54:28Z

@Karthik-K-N Just wanted to check if you need any additional assistance or review here from me

Karthik-K-N · 2023-05-03T03:20:21Z

@Karthik-K-N Just wanted to check if you need any additional assistance or review here from me

Thank you, I tried to address all the review comments and its ready for next round of review.

JoelSpeed · 2023-05-03T10:05:05Z

General structure LGTM, will defer to @mkumatag to apply the label when they're happy with this

mkumatag · 2023-05-18T01:25:52Z

@Karthik-K-N overall lgtm, can you squash the commits and split into, one for go mods and another for code changes?

mkumatag · 2023-05-18T14:12:45Z

@Karthik-K-N but I see Add loadbalancer pool logic contains the go mod and the vendor files.

mkumatag · 2023-06-07T09:49:40Z

/lgtm

@Karthik-K-N lets create a jira ticket if doesn't exist and map to this issue

openshift-ci · 2023-06-07T09:51:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Karthik-K-N,mkumatag]

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

openshift-ci-robot · 2023-06-07T10:21:56Z

@Karthik-K-N: This pull request references MULTIARCH-3667 which is a valid jira issue.

Details

In response to this:

With the introduction of cluster-control-plane-machine-set-operator(CPMS operator) now we can replace the control-plane machines in openshift cluster.
Control-plance machines will be attached to two loadbalancer in case of public cluster during the cluster creation and it will be done by the installer component.
When there is change in cpms object, the cpms operator will trigger machine replacement and during this time we need to update the loadbalancer with newly created vm ip and remove the ip of machine that is going to be deleted.

The sample control-plane machine object looks like this
apiVersion: machine.openshift.io/v1beta1
kind: Machine
metadata:
 creationTimestamp: null
 labels:
   machine.openshift.io/cluster-api-cluster: rdr-kn01032033-gjnlr
   machine.openshift.io/cluster-api-machine-role: master
   machine.openshift.io/cluster-api-machine-type: master
 name: karthik-master-1
 namespace: openshift-machine-api
spec:
 providerSpec:
   value:
     apiVersion: machine.openshift.io/v1
     credentialsSecret:
       name: powervs-credentials
     image:
       name: rhcos-rdr-kn08031125-vhwtg
       type: Name
     keyPairName: vhwtg-key
     kind: PowerVSMachineProviderConfig
     loadBalancers:
     - name: rdr-kn08031125-vhwtg-loadbalancer-int
       type: Application
     - name: rdr-kn08031125-vhwtg-loadbalancer
       type: Application
     memoryGiB: 32
     network:
       regex: ^DHCPSERVER.*rdr-kn08031125-vhwtg.*_Private$
       type: RegEx
     processorType: Shared
     processors: "0.5"
     serviceInstance:
       id: df1a3112-5067-4366-8a44-fdfcec
       type: ID
     systemType: s922
     userDataSecret:
       name: master-user-data
Proposed api changes: openshift/api#1415

Pending work

Unit testcases

Use paging mechanism

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/test-infra repository.

openshift-ci · 2023-06-19T08:28:14Z

@Karthik-K-N: all tests passed!

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2023

openshift-ci bot requested review from JoelSpeed and mkumatag March 10, 2023 07:52

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2023

dharaneeshvrd reviewed Mar 14, 2023

View reviewed changes

Prajyot-Parab reviewed Mar 21, 2023

View reviewed changes

JoelSpeed reviewed Apr 5, 2023

View reviewed changes

Karthik-K-N force-pushed the loadbalancer-support branch from 2bc3202 to 24d105c Compare April 6, 2023 10:50

Karthik-K-N changed the title ~~[WIP][DNM] Loadbalancer integration support for control plane machines~~ Loadbalancer integration support for control plane machines Apr 6, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2023

dharaneeshvrd reviewed Apr 20, 2023

View reviewed changes

Prajyot-Parab reviewed Apr 20, 2023

View reviewed changes

mkumatag requested changes Apr 27, 2023

View reviewed changes

openshift-ci bot assigned mkumatag Apr 27, 2023

Karthik-K-N force-pushed the loadbalancer-support branch from 24d105c to 7f36f86 Compare April 28, 2023 10:14

Karthik-K-N requested a review from mkumatag May 17, 2023 06:27

Karthik-K-N force-pushed the loadbalancer-support branch 2 times, most recently from 046fe75 to a642495 Compare May 18, 2023 05:25

Karthik-K-N added 2 commits May 19, 2023 08:54

Add vendor files

0815959

Add loadbalancer logic

c3d71ca

Karthik-K-N force-pushed the loadbalancer-support branch from a642495 to c3d71ca Compare May 19, 2023 03:24

Karthik-K-N mentioned this pull request Jun 2, 2023

MULTIARCH-3667: Add support for CPMSO for Power VS openshift/installer#7226

Merged

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2023

Karthik-K-N changed the title ~~Loadbalancer integration support for control plane machines~~ MULTIARCH-3667: Loadbalancer integration support for control plane machines Jun 7, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 7, 2023

openshift-merge-robot merged commit 68477f0 into openshift:main Jun 19, 2023

	klog.Infof("deregister ip %s of machine %s form LoadBalancer", internalIP, r.machine.Name)
	klog.Infof("deregister ip %s of machine %s from LoadBalancer", internalIP, r.machine.Name)

	if err = r.removeFromApplicationLoadBalancers(internalIP); err != nil {
	if err := r.removeFromApplicationLoadBalancers(internalIP); err != nil {

		err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP)
		if err != nil {

	err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP)
	if err != nil {
	if err := registerWithApplicationLoadBalancers(r.powerVSClient, applicationLoadBalancerNames, internalIP); err != nil {

	return nil, fmt.Errorf("no loadbalancer is retrieved")
	return nil, errors.New("no loadbalancer is retrieved")

	vpcZone, err := utils.VPCRegionForPowerVSRegion(c.region)
	vpcRegion, err := utils.VPCRegionForPowerVSRegion(c.region)

MULTIARCH-3667: Loadbalancer integration support for control plane machines #41

MULTIARCH-3667: Loadbalancer integration support for control plane machines #41

Uh oh!

Conversation

Karthik-K-N commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karthik-K-N commented Mar 10, 2023

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!

Karthik-K-N commented Mar 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed 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!

mkumatag commented Apr 6, 2023

Uh oh!

dharaneeshvrd Apr 20, 2023 • 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!

dharaneeshvrd left a comment

Choose a reason for hiding this comment

Uh oh!

Prajyot-Parab 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!

JoelSpeed commented May 2, 2023

Uh oh!

Karthik-K-N commented May 3, 2023

Uh oh!

JoelSpeed commented May 3, 2023

Uh oh!

mkumatag commented May 18, 2023

Uh oh!

mkumatag commented May 18, 2023

Uh oh!

mkumatag commented Jun 7, 2023

Uh oh!

openshift-ci bot commented Jun 7, 2023

Uh oh!

openshift-ci-robot commented Jun 7, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Karthik-K-N commented Mar 10, 2023 •

edited

Loading

dharaneeshvrd Apr 20, 2023 •

edited

Loading

openshift-ci-robot commented Jun 7, 2023 •

edited by openshift-ci bot

Loading