Skip to content

Add per-service mutex to prevent concurrent reconciliation #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mweibel wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add per-service mutex to prevent concurrent reconciliation #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
262c040
Add per-service mutex to prevent concurrent reconciliation
mweibel Aug 28, 2026
92f8e55
fix: do not proceed if no zone found
mweibel Sep 7, 2026
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
40 changes: 39 additions & 1 deletion pkg/cloudscale_ccm/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@ import (
"fmt"
"slices"
"strings"
"sync"
"time"

"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 +311,20 @@ 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)
start := time.Now()
klog.V(4).InfoS("acquiring service lock", "uid", uid)
mu.Lock()

return func() {
klog.V(4).InfoS("releasing service lock", "uid", uid, "duration", time.Since(start))
mu.Unlock()
}
Comment on lines +317 to +327

@alakae alakae Aug 31, 2026

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.

I compared it with: https://github.com/cloudscale-ch/terraform-provider-cloudscale/blob/master/cloudscale/mutex_kv.go

  • any reason why you did not use the channel pattern you suggested in that repo?
  • I just now realized that both implementations do never actually delete map entries. In Terraform this should not be a problem, as the whole process is short-lived, in the CCM the process could live for weeks/months. Do you have an idea how we can address it? I don't, at least not a trivial one :)

@mweibel mweibel Sep 1, 2026 •
edited
Loading

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.

any reason why you did not use the channel pattern you suggested in that repo?

the mutex_kv.go is a slightly different case: it used a loop with timer for waiting until the lock is there. It also supports cancellation with context to support terraform-native timeouts (and ctrl+c). It used TryLock for this and we changed it to a channel-based approach to get rid of the loop and timer and instead use "native" Go polling.

This case here is different and much simpler: we just acquire the lock and nobody else waits on it. We don't really need context cancellation here since it's running as a service. While it may happen that the parent context could be cancelled for some reason, the lock/unlock should not be the point where this gets respected (and hasn't been so far).

I just now realized that both implementations do never actually delete map entries. In Terraform this should not be a problem, as the whole process is short-lived, in the CCM the process could live for weeks/months. Do you have an idea how we can address it? I don't, at least not a trivial one :)

That's true and I tried to address this in the commit message:

Locks are not cleaned up on service deletion to avoid issues with late-arriving goroutines.

How often this happens is questionable but it avoids potential issues without much cost: Even large clusters won't have more than a couple loadbalancers. Keeping them around would be a couple 100 bytes (one entry is ~150B). Of course if somebody creates and deletes services 1000s of times it eventually could crash the CCM, but that would rather speak for a misuse of the system than anything else TBH.

Still, We could remove the entry for a service in two ways:

  1. once EnsureLoadBalancerDeleted is successfully done
  2. Add a goroutine which periodically removes unused mutexes. This would need to track which services still exist.

point 1 is rather trivial, I just wonder if there are any edge cases. I don't think so, but I didn't add this change because the benefit is not 100% clear to me.

Let me know what you think - happy to add the deletion logic if you feel it's valuable.

@mweibel mweibel Sep 1, 2026

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.

As an aside: we could also use a plain map with an accompanying single mutex in this case. I think either approach would work well.

@mweibel mweibel Sep 7, 2026

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.

FTR we discussed this offline and also considered using a simple sync.Mutex instead.

sync.Mutex can't be used because of the way the reconciliation works at this point: a single loadbalancer reconcile can take up to two minutes until it returns, fully blocking the mutex during that time.
Then we tried to reduce the scope of the Mutex, but because refetch is done not at the right places, the issue then can still happen.

We settled on the current approach with the intention to refactor reconcililation using api.RetryError instead of sleeps instead.

}

// GetLoadBalancer returns whether the specified load balancer exists, and
Expand Down Expand Up @@ -391,6 +408,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 +516,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 +577,17 @@ func (l *loadbalancer) EnsureLoadBalancerDeleted(
clusterName string,
service *v1.Service,
) error {
unlock := l.lockForService(service.UID)
serviceDeleted := false
defer func() {
unlock()

if serviceDeleted {
// clean up
klog.V(4).InfoS("cleaning up service lock", "uid", service.UID)
l.muMap.Delete(service.UID)
}
}()

// Detect configuration issues and abort if they are found
serviceInfo := newServiceInfo(service, clusterName)
Expand All @@ -564,11 +596,17 @@ func (l *loadbalancer) EnsureLoadBalancerDeleted(
}

// Reconcile with a desired state of "nothing"
return reconcileLbState(ctx, l.lbs.client, func() (*lbState, error) {
err := reconcileLbState(ctx, l.lbs.client, func() (*lbState, error) {
return &lbState{}, nil
}, func() (*lbState, error) {
return actualLbState(ctx, &l.lbs, serviceInfo)
})

if err == nil {
serviceDeleted = true
}

return err
}

// loadBalancerStatus generates the v1.LoadBalancerStatus for the given
Expand Down
128 changes: 128 additions & 0 deletions pkg/cloudscale_ccm/loadbalancer_test.go
View file
Open in desktop
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
10 changes: 8 additions & 2 deletions pkg/cloudscale_ccm/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ import (
"strings"
"time"

"github.com/cloudscale-ch/cloudscale-cloud-controller-manager/pkg/internal/actions"
"github.com/cloudscale-ch/cloudscale-cloud-controller-manager/pkg/internal/compare"
"github.com/cloudscale-ch/cloudscale-go-sdk/v6"
v1 "k8s.io/api/core/v1"
"k8s.io/klog/v2"

"github.com/cloudscale-ch/cloudscale-cloud-controller-manager/pkg/internal/actions"
"github.com/cloudscale-ch/cloudscale-cloud-controller-manager/pkg/internal/compare"
)

type lbState struct {
Expand Down Expand Up @@ -74,6 +75,11 @@ func desiredLbState(
}
zone = s.Zone.Slug
}
if zone == "" {
return nil, errors.New(
"no loadbalancer zone set and no server zone information available - nodes may not be provisioned yet",
)
}
}

// Parse the loadbalancer VIP addresses
Expand Down
31 changes: 24 additions & 7 deletions pkg/cloudscale_ccm/reconcile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,18 @@ func TestDesiredName(t *testing.T) {
nodes := []*v1.Node{}
servers := []cloudscale.Server{}

// No name is given, generate one
state, err := desiredLbState(i, nodes, servers)
assert.NoError(t, err)
assert.Equal(t, state.lb.Name, "k8s-service-deadbeef")
// Empty servers without zone annotation should error
_, err := desiredLbState(i, nodes, servers)
assert.Error(t, err)
assert.Contains(t, err.Error(), "no loadbalancer zone set")

// This can be overridden
s.Annotations = make(map[string]string)
s.Annotations[LoadBalancerName] = "foo"

state, err = desiredLbState(i, nodes, servers)
assert.NoError(t, err)
assert.Equal(t, state.lb.Name, "foo")
// Still errors because zone is missing
_, err = desiredLbState(i, nodes, servers)
assert.Error(t, err)
}

func TestDesiredZone(t *testing.T) {
Expand Down Expand Up @@ -92,6 +92,23 @@ func TestDesiredZone(t *testing.T) {
assert.Equal(t, "rma1", state.lb.Zone.Slug)
}

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

s := testkit.NewService("service").V1()
s.Annotations = make(map[string]string)
s.Annotations[LoadBalancerZone] = "lpg1"
i := newServiceInfo(s, "")

nodes := []*v1.Node{}
servers := []cloudscale.Server{}

// Zone is explicitly set, so empty servers should be OK
state, err := desiredLbState(i, nodes, servers)
assert.NoError(t, err)
assert.Equal(t, "lpg1", state.lb.Zone.Slug)
}

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

Expand Down
10 changes: 10 additions & 0 deletions pkg/internal/testkit/api.go
View file
Open in desktop
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