Skip to content

Split annotation for multiple-writers #4102

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
mjudeikis wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Split annotation for multiple-writers #4102

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
8ea5b98
Split annotation for multiple-writers
mjudeikis May 5, 2026
cdbd87b
simplify code
mjudeikis May 13, 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
32 changes: 30 additions & 2 deletions pkg/admission/crdnooverlappinggvr/crdnooverlappinggvr_admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package crdnooverlappinggvr

import (
"context"
"encoding/json"
"fmt"
"io"
"strings"
Expand All @@ -27,6 +28,7 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/client-go/util/retry"
Expand Down Expand Up @@ -80,7 +82,33 @@ func (p *crdNoOverlappingGVRAdmission) SetKcpInformers(local, global kcpinformer
// SetKcpClusterClient sets the client for kcp resources. It's part of WantsKcpClusterClient.
func (p *crdNoOverlappingGVRAdmission) SetKcpClusterClient(c kcpclientset.ClusterInterface) {
p.updateLogicalCluster = func(ctx context.Context, logicalCluster *corev1alpha1.LogicalCluster, opts metav1.UpdateOptions) (*corev1alpha1.LogicalCluster, error) {
return c.CoreV1alpha1().LogicalClusters().Cluster(logicalcluster.From(logicalCluster).Path()).Update(ctx, logicalCluster, opts)
// Use SSA to write only to the pending locks annotation key
patchObj := &corev1alpha1.LogicalCluster{
TypeMeta: metav1.TypeMeta{
APIVersion: corev1alpha1.SchemeGroupVersion.String(),
Kind: "LogicalCluster",
},
ObjectMeta: metav1.ObjectMeta{
Name: logicalCluster.Name,
Annotations: map[string]string{
apibinding.LocksPendingAnnotationKey: logicalCluster.Annotations[apibinding.LocksPendingAnnotationKey],
},
},
}
patchBytes, err := json.Marshal(patchObj)
Copy link
Copy Markdown
Member

@sttts sttts May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I am pretty sure this is wrong. The update was intentionally a consistent write to ensure that the passed logicalCluster and the one in etcd are the same resource version. Patching blindly with server side apply defeats the purpose of the whole approach.

if err != nil {
return nil, err
}
return c.CoreV1alpha1().LogicalClusters().Cluster(logicalcluster.From(logicalCluster).Path()).Patch(
ctx,
logicalCluster.Name,
types.ApplyPatchType,
patchBytes,
metav1.PatchOptions{
FieldManager: apibinding.FieldManagerPending,
// Force is not needed because each writer uses its own annotation key
},
)
}
}

Expand Down Expand Up @@ -136,7 +164,7 @@ func (p *crdNoOverlappingGVRAdmission) Validate(ctx context.Context, a admission
}

var updated *corev1alpha1.LogicalCluster
updated, _, skipped, err = apibinding.WithLockedResources(nil, time.Now(), lc, []schema.GroupResource{gr}, apibinding.ExpirableLock{
updated, _, skipped, err = apibinding.WithLockedResourcesForPending(nil, time.Now(), lc, []schema.GroupResource{gr}, apibinding.ExpirableLock{
Lock: apibinding.Lock{CRD: true},
CRDExpiry: ptr.To(p.now()),
})
Expand Down
15 changes: 8 additions & 7 deletions pkg/admission/crdnooverlappinggvr/crdnooverlappinggvr_admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func TestValidate(t *testing.T) {
},

{
name: "fails without resource binding annotation on LogicalCluster",
name: "succeeds without resource binding annotation on LogicalCluster - creates pending lock",
attr: createAttr(&apiextensions.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Spec: apiextensions.CustomResourceDefinitionSpec{
Expand All @@ -150,7 +150,7 @@ func TestValidate(t *testing.T) {
}),
clusterName: "root:acme",
initialObjects: []runtime.Object{createLogicalCluster("root:acme")},
wantErr: true,
wantAnnotation: `{"foo.acme.dev":{"c":true,"e":"2022-01-01T00:00:00Z"}}`,
},
{
name: "fails without LogicalCluster",
Expand Down Expand Up @@ -241,7 +241,8 @@ func TestValidate(t *testing.T) {
if updatedLogicalCluster == nil {
t.Fatal("expected LogicalCluster to be updated, got nil")
}
if got := updatedLogicalCluster.Annotations[apibinding.ResourceBindingsAnnotationKey]; got != scenario.wantAnnotation {
// With SSA split, the admission plugin writes to the pending locks annotation key
if got := updatedLogicalCluster.Annotations[apibinding.LocksPendingAnnotationKey]; got != scenario.wantAnnotation {
t.Errorf("expected LogicalCluster annotation %q, got %q", scenario.wantAnnotation, got)
}
}
Expand Down Expand Up @@ -310,7 +311,7 @@ func createLogicalCluster(clusterName string) *corev1alpha1.LogicalCluster {

func withCRD(lc *corev1alpha1.LogicalCluster, gr schema.GroupResource, expiry *metav1.Time) *corev1alpha1.LogicalCluster {
rbs := make(apibinding.ResourceBindingsAnnotation)
if v := lc.Annotations[apibinding.ResourceBindingsAnnotationKey]; v != "" {
if v := lc.Annotations[apibinding.LocksPendingAnnotationKey]; v != "" {
if err := json.Unmarshal([]byte(v), &rbs); err != nil {
panic(err)
}
Expand All @@ -325,13 +326,13 @@ func withCRD(lc *corev1alpha1.LogicalCluster, gr schema.GroupResource, expiry *m
if err != nil {
panic(err)
}
lc.Annotations[apibinding.ResourceBindingsAnnotationKey] = string(bs)
lc.Annotations[apibinding.LocksPendingAnnotationKey] = string(bs)
return lc
}

func withBinding(lc *corev1alpha1.LogicalCluster, binding string, boundResources []apisv1alpha2.BoundAPIResource) *corev1alpha1.LogicalCluster {
rbs := make(apibinding.ResourceBindingsAnnotation)
if v := lc.Annotations[apibinding.ResourceBindingsAnnotationKey]; v != "" {
if v := lc.Annotations[apibinding.LocksPendingAnnotationKey]; v != "" {
if err := json.Unmarshal([]byte(v), &rbs); err != nil {
panic(err)
}
Expand All @@ -347,6 +348,6 @@ func withBinding(lc *corev1alpha1.LogicalCluster, binding string, boundResources
if err != nil {
panic(err)
}
lc.Annotations[apibinding.ResourceBindingsAnnotationKey] = string(bs)
lc.Annotations[apibinding.LocksPendingAnnotationKey] = string(bs)
return lc
}
21 changes: 11 additions & 10 deletions pkg/reconciler/apis/apibinding/apibinding_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import (
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/util/workqueue"
"k8s.io/klog/v2"
"k8s.io/utils/ptr"

kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
kcpapiextensionsclientset "github.com/kcp-dev/client-go/apiextensions/client"
Expand All @@ -63,8 +62,7 @@ import (
)

const (
ControllerName = "kcp-apibinding"
LocksFieldManager = "apibinding-reconciler-locks"
ControllerName = "kcp-apibinding"
)

var (
Expand Down Expand Up @@ -171,7 +169,7 @@ func NewController(
ObjectMeta: metav1.ObjectMeta{
Name: lc.Name,
Annotations: map[string]string{
ResourceBindingsAnnotationKey: lc.Annotations[ResourceBindingsAnnotationKey],
LocksBindingsAnnotationKey: lc.Annotations[LocksBindingsAnnotationKey],
},
},
}
Expand All @@ -185,8 +183,8 @@ func NewController(
types.ApplyPatchType,
patchBytes,
metav1.PatchOptions{
FieldManager: LocksFieldManager,
Force: ptr.To(true),
FieldManager: FieldManagerBindings,
// Force is not needed because each writer uses its own annotation key
},
)
return err
Expand Down Expand Up @@ -292,10 +290,13 @@ func NewController(
c.enqueueLogicalCluster(tombstone.Obj[*corev1alpha1.LogicalCluster](obj), logger, "")
},
UpdateFunc: func(old, obj interface{}) {
was := old.(*corev1alpha1.LogicalCluster).Annotations[ResourceBindingsAnnotationKey]
is := obj.(*corev1alpha1.LogicalCluster).Annotations[ResourceBindingsAnnotationKey]
if was != is {
c.enqueueLogicalCluster(tombstone.Obj[*corev1alpha1.LogicalCluster](obj), logger, "")
oldLC := old.(*corev1alpha1.LogicalCluster)
newLC := obj.(*corev1alpha1.LogicalCluster)
for _, key := range []string{LocksBindingsAnnotationKey, LocksCRDsAnnotationKey, LocksPendingAnnotationKey} {
if oldLC.Annotations[key] != newLC.Annotations[key] {
c.enqueueLogicalCluster(tombstone.Obj[*corev1alpha1.LogicalCluster](obj), logger, "")
return
}
}
},
}))
Expand Down
7 changes: 4 additions & 3 deletions pkg/reconciler/apis/apibinding/apibinding_reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,9 @@ func (r *bindingReconciler) reconcile(ctx context.Context, apiBinding *apisv1alp
// TODO(sttts): removed schemas never get unlocked. We need a distinguishable way
// for intentional removal of schemas, versus movement of schemas
// to another APIExport.
previousResourceBindings := lc.Annotations[ResourceBindingsAnnotationKey]
lc, _, skipped, err = WithLockedResources(crds, time.Now(), lc, grs.UnsortedList(), ExpirableLock{
// Read current bindings annotation to check if we need to update
bindingsBefore := lc.Annotations[LocksBindingsAnnotationKey]
lc, _, skipped, err = WithLockedResourcesForBindings(crds, time.Now(), lc, grs.UnsortedList(), ExpirableLock{
Lock: Lock{Name: apiBinding.Name},
})
if err != nil {
Expand Down Expand Up @@ -311,7 +312,7 @@ func (r *bindingReconciler) reconcile(ctx context.Context, apiBinding *apisv1alp
return err
}

if lc.Annotations[ResourceBindingsAnnotationKey] != previousResourceBindings {
if lc.Annotations[LocksBindingsAnnotationKey] != bindingsBefore {
if err := r.updateLogicalCluster(ctx, lc); err != nil {
return err
}
Expand Down
21 changes: 15 additions & 6 deletions pkg/reconciler/apis/apibinding/apibinding_reconcile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,13 +285,21 @@ func TestReconcileBinding(t *testing.T) {
},
wantError: true,
},
"LogicalCluster without resource binding annotation": {
logicalCluster: newLogicalCluster(),
"LogicalCluster with empty resource binding annotation - binding succeeds": {
// With split annotations, the controller can now lock resources even when
// starting with an empty bindings annotation. This tests the successful
// binding flow with the new SSA-based split annotation design.
logicalCluster: withResourceBindings(newLogicalCluster(), ResourceBindingsAnnotation{}),
apiBinding: binding.Build(),
wantCreateCRD: true,
wantAPIExportValid: wantAPIExportValid{
valid: true,
},
wantInitialBindingComplete: wantInitialBindingComplete{
resourceConflict: true,
// CRD is created, so we wait for it to be established
waitingForEstablished: true,
},
wantError: true,
wantBoundResources: nil, // not yet established
},
"LogicalCluster update error": {
logicalCluster: withResourceBindings(newLogicalCluster(), ResourceBindingsAnnotation{}),
Expand Down Expand Up @@ -730,7 +738,7 @@ func TestReconcileBinding(t *testing.T) {
lc.TypeMeta = metav1.TypeMeta{}
}
if tc.wantUpdatedResourceBindings != nil {
got, err := GetResourceBindings(lc)
got, err := GetBindingsLocks(lc)
require.NoError(t, err)
require.Equal(t, tc.wantUpdatedResourceBindings, got)
}
Expand Down Expand Up @@ -1329,7 +1337,8 @@ func withResourceBindings(lc *corev1alpha1.LogicalCluster, rbs ResourceBindingsA

lc = lc.DeepCopy()
lc.Annotations = make(map[string]string)
lc.Annotations[ResourceBindingsAnnotationKey] = string(bs)
// Use the new bindings annotation key for tests
lc.Annotations[LocksBindingsAnnotationKey] = string(bs)
return lc
}

Expand Down
Loading