Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions pkg/cloudscale_ccm/loadbalancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import (
"fmt"
"slices"
"strings"
"sync"

"github.com/cloudscale-ch/cloudscale-go-sdk/v6"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/record"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -308,6 +310,19 @@ type loadbalancer struct {
srv serverMapper
k8s kubernetes.Interface
recorder record.EventRecorder
muMap sync.Map
}

func (l *loadbalancer) lockForService(uid types.UID) func() {
rawMu, _ := l.muMap.LoadOrStore(string(uid), new(sync.Mutex))
mu := rawMu.(*sync.Mutex)
klog.V(4).InfoS("acquiring service lock", "uid", uid)
mu.Lock()

return func() {
klog.V(4).InfoS("releasing service lock", "uid", uid)
mu.Unlock()
}
}

// GetLoadBalancer returns whether the specified load balancer exists, and
Expand Down Expand Up @@ -391,6 +406,8 @@ func (l *loadbalancer) EnsureLoadBalancer(
service *v1.Service,
nodes []*v1.Node,
) (*v1.LoadBalancerStatus, error) {
unlock := l.lockForService(service.UID)
defer unlock()

// Detect configuration issues and abort if they are found
serviceInfo := newServiceInfo(service, clusterName)
Expand Down Expand Up @@ -497,6 +514,8 @@ func (l *loadbalancer) UpdateLoadBalancer(
service *v1.Service,
nodes []*v1.Node,
) error {
unlock := l.lockForService(service.UID)
defer unlock()

// Detect configuration issues and abort if they are found
serviceInfo := newServiceInfo(service, clusterName)
Expand Down Expand Up @@ -556,6 +575,8 @@ func (l *loadbalancer) EnsureLoadBalancerDeleted(
clusterName string,
service *v1.Service,
) error {
unlock := l.lockForService(service.UID)
defer unlock()

// Detect configuration issues and abort if they are found
serviceInfo := newServiceInfo(service, clusterName)
Expand Down
128 changes: 128 additions & 0 deletions pkg/cloudscale_ccm/loadbalancer_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package cloudscale_ccm

import (
"encoding/json"
"net/http"
"sync"
"testing"
"time"

"github.com/cloudscale-ch/cloudscale-go-sdk/v6"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -179,6 +183,130 @@ func TestLoadBalancer_EnsureLoadBalancer(t *testing.T) {
}
}

func TestLoadBalancer_ConcurrentCreate(t *testing.T) {
t.Parallel()

apiServer := testkit.NewMockAPIServer()

createCount := 0
var lbs []cloudscale.LoadBalancer
var mu sync.Mutex

// Custom handler for /v1/load-balancers to track creates.
// The sleep before appending to lbs increases the race window so that
// both goroutines can see an empty list before either creates.
apiServer.HandleFunc("/v1/load-balancers", func(w http.ResponseWriter, r *http.Request) {
switch r.Method {
case http.MethodPost:
time.Sleep(200 * time.Millisecond)

mu.Lock()
createCount++
lb := cloudscale.LoadBalancer{
HREF: "/v1/load-balancers/lb-uuid-1",
UUID: "lb-uuid-1",
Name: "k8s-service-test-uid",
Status: "running",
ZonalResource: cloudscale.ZonalResource{
Zone: cloudscale.Zone{Slug: "rma1"},
},
Flavor: cloudscale.LoadBalancerFlavorStub{Slug: "lb-standard"},
}
lbs = append(lbs, lb)
mu.Unlock()

w.WriteHeader(http.StatusCreated)
_ = json.NewEncoder(w).Encode(lb)
case http.MethodGet:
mu.Lock()
defer mu.Unlock()
w.WriteHeader(http.StatusOK)
_ = json.NewEncoder(w).Encode(lbs)
}
})

// Mock server endpoint for node mapping.
serverUUID := "08d56bfe-40d0-4c68-a915-54f846c28c9e"
apiServer.WithServers([]cloudscale.Server{{
UUID: serverUUID,
Name: "node-1",
ZonalResource: cloudscale.ZonalResource{
Zone: cloudscale.Zone{Slug: "rma1"},
},
Interfaces: []cloudscale.Interface{{
Type: "private",
Addresses: []cloudscale.Address{{
Address: "10.0.0.1",
Subnet: cloudscale.SubnetStub{UUID: "subnet-uuid-1"},
}},
}},
}})

// Mock the remaining LB endpoints so reconciliation can proceed.
apiServer.On("/v1/load-balancers/pools", 200, []cloudscale.LoadBalancerPool{})
apiServer.On("/v1/load-balancers/listeners", 200, []cloudscale.LoadBalancerListener{})
apiServer.On("/v1/load-balancers/health-monitors", 200, []cloudscale.LoadBalancerHealthMonitor{})
apiServer.On("/v1/floating-ips", 200, []cloudscale.FloatingIP{})

apiServer.Start()
defer apiServer.Close()

client := fake.NewSimpleClientset()
fakeDiscovery, ok := client.Discovery().(*fakediscovery.FakeDiscovery)
require.True(t, ok, "couldn't convert Discovery() to *FakeDiscovery")
fakeDiscovery.FakedServerVersion = &version.Info{
Major: "1",
Minor: "34",
}

l := &loadbalancer{
lbs: lbMapper{client: apiServer.Client()},
srv: serverMapper{client: apiServer.Client()},
k8s: client,
recorder: record.NewFakeRecorder(10),
}

service := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "test-service",
Namespace: "default",
UID: "test-uid",
Annotations: map[string]string{
LoadBalancerName: "k8s-service-test-uid",
LoadBalancerFlavor: "lb-standard",
LoadBalancerZone: "rma1",
},
},
Spec: v1.ServiceSpec{
Type: v1.ServiceTypeLoadBalancer,
Ports: []v1.ServicePort{
{Protocol: v1.ProtocolTCP, Port: 80, NodePort: 80},
},
},
}

_, _ = l.k8s.CoreV1().Services("default").Create(t.Context(), service, metav1.CreateOptions{})

nodes := []*v1.Node{{
ObjectMeta: metav1.ObjectMeta{Name: "node-1"},
Spec: v1.NodeSpec{ProviderID: "cloudscale://" + serverUUID},
}}

var wg sync.WaitGroup
wg.Add(2)
go func() {
defer wg.Done()
_, _ = l.EnsureLoadBalancer(t.Context(), "test-cluster", service, nodes)
}()
go func() {
defer wg.Done()
_, _ = l.EnsureLoadBalancer(t.Context(), "test-cluster", service, nodes)
}()
wg.Wait()

assert.Equal(t, 1, createCount, "expected exactly one LB creation")
}

func TestFilterNodesBySelector(t *testing.T) {
t.Parallel()

Expand Down
5 changes: 1 addition & 4 deletions pkg/cloudscale_ccm/reconcile.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"encoding/json"
"errors"
"fmt"
"math/rand"
"slices"
"strings"
"time"
Expand Down Expand Up @@ -749,9 +748,7 @@ func reconcileLbState(
break
}

// Wait between 5-7.5 seconds between state fetches
// #nosec G404
wait := time.Duration(5000+rand.Intn(2500)) * time.Millisecond
wait := 500 * time.Millisecond

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is too aggressive. Open to discuss and test going form 5-7.5s to lets say 2-4s, but I'd recommend against 500ms.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that we had more integration test failures with this: I'll revert this change.


select {
case <-ctx.Done():
Expand Down
15 changes: 14 additions & 1 deletion pkg/internal/actions/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/cloudscale-ch/cloudscale-go-sdk/v6"
"k8s.io/klog/v2"
)

type Action interface {
Expand Down Expand Up @@ -50,6 +51,18 @@ func (a *CreateLbAction) Label() string {
func (a *CreateLbAction) Run(
ctx context.Context, client *cloudscale.Client) (Control, error) {

existing, err := client.LoadBalancers.List(ctx)
if err == nil {
for _, lb := range existing {
if lb.Name == a.lb.Name {
klog.InfoS("lb already exists, skipping create",
"name", a.lb.Name, "uuid", lb.UUID)

return Refresh, nil
}
}
}

Comment on lines +54 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the mutex in the other PR works correctly, when would you expected that this case is still relevant?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's more of a safety check. We can also remove it.

addrs := make([]cloudscale.VIPAddressRequest, 0, len(a.lb.VIPAddresses))
for _, addr := range a.lb.VIPAddresses {
addrs = append(addrs, cloudscale.VIPAddressRequest{
Expand All @@ -58,7 +71,7 @@ func (a *CreateLbAction) Run(
})
}

_, err := client.LoadBalancers.Create(ctx, &cloudscale.LoadBalancerRequest{
_, err = client.LoadBalancers.Create(ctx, &cloudscale.LoadBalancerRequest{
Name: a.lb.Name,
Flavor: a.lb.Flavor.Slug,
VIPAddresses: &addrs,
Expand Down
10 changes: 10 additions & 0 deletions pkg/internal/testkit/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,16 @@ func (m *MockAPIServer) Start() {
m.server = httptest.NewServer(m.mux)
}

// HandleFunc registers a custom handler for the given pattern.
// This allows intercepting requests dynamically, e.g. to track invocations.
func (m *MockAPIServer) HandleFunc(pattern string, handler http.HandlerFunc) {
if m.mux == nil {
m.mux = http.NewServeMux()
m.On("/", 404, "{}")
}
m.mux.HandleFunc(pattern, handler)
}

// Close stops/closes the server and resets it.
func (m *MockAPIServer) Close() {
if m.server != nil {
Expand Down
Loading