Repository: kubernetes-sigs/cluster-api-provider-openstack

Issue:
CAPO does not observe Member provisioning state when adding/deleting members of a pool
/kind bug

**What steps did you take and what happened:**
Currently, when adding or deleting members from a loadbalancer pool, it waits until the loadbalancer is in provisoning state ACTIVE again, before considering the the process as done. With using amphorav2 as a loadbalancer provider, we noticed cases where the loadbalancer is not in a pending_* status, when a member is. This results in CAPO thinking the member is created/deleted successfully, while it may be actually stuck.

This should be the relevant piece of code:
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/eefc873457b7b1f1823d4b2a032f3cb07bcadcfa/pkg/cloud/services/loadbalancer/loadbalancer.go#L721

This can lead to a more fatal situation:
As CAPO thinks members are correctly added, it continues to add new (control plane) nodes and remove old ones. We had cases were this resulted in a offline cluster, as all nodes were replaced by CAPI, but not correctly set in the Loadbalancer pool.

**What did you expect to happen:**
CAPO should check the provisioning status of the member resource and wait until it is active before going on.
In the fatal case, CAPO should either wait indefinitly for the member for becoming active, or should have some kind of failure/error. It should not continue the update process.

**Anything else you would like to add:**
I've already had a talk with @mnaser about this issue.

**Environment:**

- Cluster API Provider OpenStack version (Or `git rev-parse HEAD` if manually built): 0.12.3
- Cluster-API version: v1.10.2
- OpenStack version: idk
- Minikube/KIND version: not relevant
- Kubernetes version (use `kubectl version`): not relevant
- OS (e.g. from `/etc/os-release`): not relevant


Discussion:
@lentzi90: Thanks for the well described issue!
Is this something you or @mnaser could work on? I am not sure I will have time for it at the moment.

Solution patch:
diff --git a/pkg/clients/loadbalancer.go b/pkg/clients/loadbalancer.go
index 1c358239..3cf46350 100644
--- a/pkg/clients/loadbalancer.go
+++ b/pkg/clients/loadbalancer.go
@@ -51,6 +51,7 @@ type LbClient interface {
 	DeletePool(id string) error
 	CreatePoolMember(poolID string, opts pools.CreateMemberOptsBuilder) (*pools.Member, error)
 	ListPoolMember(poolID string, opts pools.ListMembersOptsBuilder) ([]pools.Member, error)
+	GetPoolMember(poolID string, lbMemberID string) (*pools.Member, error)
 	DeletePoolMember(poolID string, lbMemberID string) error
 	CreateMonitor(opts monitors.CreateOptsBuilder) (*monitors.Monitor, error)
 	ListMonitors(opts monitors.ListOptsBuilder) ([]monitors.Monitor, error)
@@ -213,6 +214,15 @@ func (l lbClient) ListPoolMember(poolID string, opts pools.ListMembersOptsBuilde
 	return pools.ExtractMembers(allPages)
 }
 
+func (l lbClient) GetPoolMember(poolID string, lbMemberID string) (*pools.Member, error) {
+	mc := metrics.NewMetricPrometheusContext("loadbalancer_member", "get")
+	member, err := pools.GetMember(context.TODO(), l.serviceClient, poolID, lbMemberID).Extract()
+	if mc.ObserveRequest(err) != nil {
+		return nil, fmt.Errorf("error getting lbmember: %s", err)
+	}
+	return member, nil
+}
+
 func (l lbClient) DeletePoolMember(poolID string, lbMemberID string) error {
 	mc := metrics.NewMetricPrometheusContext("loadbalancer_member", "delete")
 	err := pools.DeleteMember(context.TODO(), l.serviceClient, poolID, lbMemberID).ExtractErr()
diff --git a/pkg/clients/mock/loadbalancer.go b/pkg/clients/mock/loadbalancer.go
index 59024521..586f7884 100644
--- a/pkg/clients/mock/loadbalancer.go
+++ b/pkg/clients/mock/loadbalancer.go
@@ -251,6 +251,21 @@ func (mr *MockLbClientMockRecorder) GetPool(id any) *gomock.Call {
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPool", reflect.TypeOf((*MockLbClient)(nil).GetPool), id)
 }
 
+// GetPoolMember mocks base method.
+func (m *MockLbClient) GetPoolMember(poolID, lbMemberID string) (*pools.Member, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "GetPoolMember", poolID, lbMemberID)
+	ret0, _ := ret[0].(*pools.Member)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// GetPoolMember indicates an expected call of GetPoolMember.
+func (mr *MockLbClientMockRecorder) GetPoolMember(poolID, lbMemberID any) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPoolMember", reflect.TypeOf((*MockLbClient)(nil).GetPoolMember), poolID, lbMemberID)
+}
+
 // ListListeners mocks base method.
 func (m *MockLbClient) ListListeners(opts listeners.ListOptsBuilder) ([]listeners.Listener, error) {
 	m.ctrl.T.Helper()
diff --git a/pkg/cloud/services/loadbalancer/loadbalancer.go b/pkg/cloud/services/loadbalancer/loadbalancer.go
index 5c88c979..394a9ed0 100644
--- a/pkg/cloud/services/loadbalancer/loadbalancer.go
+++ b/pkg/cloud/services/loadbalancer/loadbalancer.go
@@ -52,6 +52,7 @@ const (
 const (
 	loadBalancerProvisioningStatusActive        = "ACTIVE"
 	loadBalancerProvisioningStatusPendingDelete = "PENDING_DELETE"
+	poolMemberProvisioningStatusActive          = "ACTIVE"
 )
 
 // Default values for Monitor, sync with `kubebuilder:default` annotations on APIServerLoadBalancerMonitor object.
@@ -718,7 +719,12 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
 			return err
 		}
 
-		if _, err := s.loadbalancerClient.CreatePoolMember(pool.ID, lbMemberOpts); err != nil {
+		member, err := s.loadbalancerClient.CreatePoolMember(pool.ID, lbMemberOpts)
+		if err != nil {
+			return err
+		}
+
+		if _, err := s.waitForPoolMemberActive(pool.ID, member.ID); err != nil {
 			return err
 		}
 
@@ -932,6 +938,25 @@ func (s *Service) waitForLoadBalancerActive(id string) (*loadbalancers.LoadBalan
 	return lb, nil
 }
 
+// Possible Pool Member states are documented here: https://docs.openstack.org/api-ref/load-balancer/v2/#prov-status
+func (s *Service) waitForPoolMemberActive(poolID, memberID string) (*pools.Member, error) {
+	var member *pools.Member
+
+	s.scope.Logger().Info("Waiting for pool member", "pool_id", poolID, "member_id", memberID, "targetStatus", "ACTIVE")
+	err := wait.ExponentialBackoff(backoff, func() (bool, error) {
+		var err error
+		member, err = s.loadbalancerClient.GetPoolMember(poolID, memberID)
+		if err != nil {
+			return false, err
+		}
+		return member.ProvisioningStatus == poolMemberProvisioningStatusActive, nil
+	})
+	if err != nil {
+		return nil, err
+	}
+	return member, nil
+}
+
 func (s *Service) waitForListener(id, target string) error {
 	s.scope.Logger().Info("Waiting for load balancer listener", "id", id, "targetStatus", target)
 	return wait.ExponentialBackoff(backoff, func() (bool, error) {



Test patch:
diff --git a/pkg/cloud/services/loadbalancer/loadbalancer_test.go b/pkg/cloud/services/loadbalancer/loadbalancer_test.go
index a6a04d48..9a2ec0d6 100644
--- a/pkg/cloud/services/loadbalancer/loadbalancer_test.go
+++ b/pkg/cloud/services/loadbalancer/loadbalancer_test.go
@@ -1052,8 +1052,19 @@ func Test_ReconcileLoadBalancerMember(t *testing.T) {
 				).DoAndReturn(func(_ string, got pools.CreateMemberOpts) (*pools.Member, error) {
 					// SubnetID must be empty here
 					g.Expect(got.SubnetID).To(Equal(""))
-					return &pools.Member{ID: "member-2"}, nil
+					return &pools.Member{ID: memberID}, nil
 				})
+
+				pendingMember := pools.Member{
+					ID:                 memberID,
+					Name:               poolMemberName,
+					ProvisioningStatus: "PENDING_CREATE",
+				}
+				m.GetPoolMember(poolID, memberID).Return(&pendingMember, nil)
+
+				activeMember := pendingMember
+				activeMember.ProvisioningStatus = "ACTIVE"
+				m.GetPoolMember(poolID, memberID).Return(&activeMember, nil)
 			},
 			wantError: nil,
 		},
@@ -1092,8 +1103,10 @@ func Test_ReconcileLoadBalancerMember(t *testing.T) {
 				}
 				m.ListPools(pools.ListOpts{Name: pool.Name}).Return([]pools.Pool{pool}, nil)
 
+				poolMemberName := fmt.Sprintf("%s-kubeapi-%d-%s", clusterResourceName, port, machineName)
+
 				member := pools.Member{
-					Name:    fmt.Sprintf("%s-kubeapi-%d-%s", clusterResourceName, port, machineName),
+					Name:    poolMemberName,
 					Address: wrongMemberIP,
 					ID:      memberID,
 				}
@@ -1107,8 +1120,15 @@ func Test_ReconcileLoadBalancerMember(t *testing.T) {
 				).DoAndReturn(func(_ string, got pools.CreateMemberOpts) (*pools.Member, error) {
 					// SubnetID must be empty here
 					g.Expect(got.SubnetID).To(Equal(""))
-					return &pools.Member{ID: "member-2"}, nil
+					return &pools.Member{ID: memberID}, nil
 				})
+
+				activeMember := pools.Member{
+					ID:                 memberID,
+					Name:               poolMemberName,
+					ProvisioningStatus: "ACTIVE",
+				}
+				m.GetPoolMember(poolID, memberID).Return(&activeMember, nil).AnyTimes()
 			},
 			wantError: nil,
 		},
@@ -1133,8 +1153,8 @@ func Test_ReconcileLoadBalancerMember(t *testing.T) {
 				}
 				m.ListPools(pools.ListOpts{Name: pool.Name}).Return([]pools.Pool{pool}, nil)
 
-				memberName := fmt.Sprintf("%s-kubeapi-%d-%s", clusterResourceName, port, machineName)
-				m.ListPoolMember(poolID, pools.ListMembersOpts{Name: memberName}).Return([]pools.Member{}, nil)
+				poolMemberName := fmt.Sprintf("%s-kubeapi-%d-%s", clusterResourceName, port, machineName)
+				m.ListPoolMember(poolID, pools.ListMembersOpts{Name: poolMemberName}).Return([]pools.Member{}, nil)
 
 				// Expect CreatePoolMember; capture opts to assert SubnetID is set
 				m.CreatePoolMember(
@@ -1148,8 +1168,15 @@ func Test_ReconcileLoadBalancerMember(t *testing.T) {
 					g.Expect(got.SubnetID).To(Equal(subnetID))
 					// Tags should pass through
 					g.Expect(got.Tags).To(ConsistOf("k8s", "clusterapi"))
-					return &pools.Member{ID: "member-1", Address: memberIP, ProtocolPort: port}, nil
+					return &pools.Member{ID: memberID, Address: memberIP, ProtocolPort: port}, nil
 				})
+
+				activeMember := pools.Member{
+					ID:                 memberID,
+					Name:               poolMemberName,
+					ProvisioningStatus: "ACTIVE",
+				}
+				m.GetPoolMember(poolID, memberID).Return(&activeMember, nil).AnyTimes()
 			},
 			wantError: nil,
 		},