Skip to content

Split annotation for multiple-writers#4102

Open
mjudeikis wants to merge 2 commits into
kcp-dev:mainfrom
mjudeikis:split.annotations
Open

Split annotation for multiple-writers#4102
mjudeikis wants to merge 2 commits into
kcp-dev:mainfrom
mjudeikis:split.annotations

Conversation

@mjudeikis
Copy link
Copy Markdown
Contributor

@mjudeikis mjudeikis commented May 5, 2026 •
edited
Loading

Summary

Currently, we have a lot of write conflicts on logicalcluster as 3 operators write to the same annotations.
This PR moves all of them to separate annoations so we can use SSA and field ownerships

What Type of PR Is This?

/kind cleanup

Related Issue(s)

Fixes #4089

Release Notes

NONE

@kcp-ci-bot kcp-ci-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/chore Categorizes issue or PR as related to maintenance and other usually non-code changes. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 5, 2026
@mjudeikis
Copy link
Copy Markdown
Contributor Author

mjudeikis commented May 13, 2026

/retest

@mjudeikis mjudeikis changed the title [WIP] Split annotation for multiple-writers Split annotation for multiple-writers May 13, 2026
@kcp-ci-bot kcp-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 May 13, 2026
ntnn
ntnn approved these changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ntnn ntnn left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2026
@kcp-ci-bot
Copy link
Copy Markdown
Contributor

kcp-ci-bot commented May 13, 2026

LGTM label has been added.

DetailsGit tree hash: d7ba0a0d69992fa6117e370a6eca9d93dc0b8e85

@kcp-ci-bot
Copy link
Copy Markdown
Contributor

kcp-ci-bot commented May 13, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ntnn
Once this PR has been reviewed and has the lgtm label, please assign embik for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@ntnn
Copy link
Copy Markdown
Member

ntnn commented May 13, 2026

/retest

flake?

=== FAIL: test/e2e/proxy TestMappingWithClusterContext (31.18s)
    proxy_test.go:49: Shared kcp server will target configuration "/home/prow/go/src/github.com/kcp-dev/kcp/.kcp/admin.kubeconfig"
    proxy_test.go:49: Waiting for readiness for server at https://127.0.0.1:6444/
    proxy_test.go:84: Waiting for tenancy.kcp.io/v1alpha1 to be served
    proxy_test.go:84: Waiting for tenancy.kcp.io/v1alpha1 to be served
    proxy_test.go:84: Waiting for tenancy.kcp.io/v1alpha1 to be served
    proxy_test.go:84: Waiting for topology.kcp.io/v1alpha1 to be served
    proxy_test.go:84: Waiting for topology.kcp.io/v1alpha1 to be served
    proxy_test.go:84: Created root:universal workspace root:e2e-workspace-oidc-cvcjh as /clusters/qukrku7l79v8csda on shard "root"
    mockoidc.go:187: Creating WorkspaceAuthenticationConfguration mockoidc-5456868631789275145...
    authfixtures.go:52: Creating WorkspaceType with-oidc...
    proxy_test.go:103: Creating Workspace...
    proxy_test.go:104: Created root:e2e-workspace-oidc-cvcjh:with-oidc workspace root:e2e-workspace-oidc-cvcjh:team-a as /clusters/14jsb23pvjb2qv6h on shard "shard-1"
    authfixtures.go:71: Creating ClusterRoleBinding...
    proxy_test.go:148: 
        	Error Trace:	/home/prow/go/src/github.com/kcp-dev/kcp/test/e2e/proxy/proxy_test.go:148
        	            				/usr/local/go/src/runtime/asm_arm64.s:1268
        	Error:      	Not equal: 
        	            	expected: "cluster:14jsb23pvjb2qv6h"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-cluster:14jsb23pvjb2qv6h
        	            	+
        	Test:       	TestMappingWithClusterContext
    proxy_test.go:139: 
        	Error Trace:	/home/prow/go/src/github.com/kcp-dev/kcp/test/e2e/proxy/proxy_test.go:139
        	Error:      	Condition never satisfied
        	Test:       	TestMappingWithClusterContext
DONE 278 tests, 5 skipped, 1 failure in 373.368s
I0513 08:37:30.503413   22846 shard.go:248] "gathering shard metrics" shard="kcp-0"
I0513 08:37:30.642486   22846 shard.go:272] "writing metrics file" shard="kcp-0" path="/logs/artifacts/kcp/kcp-0-metrics.txt"
I0513 08:37:30.644374   22846 shard.go:277] "wrote metrics file" shard="kcp-0" path="/logs/artifacts/kcp/kcp-0-metrics.txt"
I0513 08:37:30.644415   22846 shard.go:248] "gathering shard metrics" shard="kcp-1"
I0513 08:37:30.775585   22846 shard.go:272] "writing metrics file" shard="kcp-1" path="/logs/artifacts/kcp/kcp-1-metrics.txt"
I0513 08:37:30.777499   22846 shard.go:277] "wrote metrics file" shard="kcp-1" path="/logs/artifacts/kcp/kcp-1-metrics.txt"
make: *** [Makefile:379: test-e2e-sharded-minimal] Error 1

@ntnn ntnn added this to tbd May 13, 2026
@ntnn ntnn moved this to Reviewing in tbd May 13, 2026
@mjudeikis
Copy link
Copy Markdown
Contributor Author

mjudeikis commented May 13, 2026

/retest

@sttts
Copy link
Copy Markdown
Member

sttts commented May 15, 2026

Isn't this one annotation meant as a lock and splitting it up would defeat it's purpose? Better be extra careful understanding why it exists.

@sttts
Copy link
Copy Markdown
Member

sttts commented May 15, 2026

Compare #3251. The conflict was intentional I believe. I don't remember all the details from the time.

sttts
sttts reviewed May 15, 2026
View reviewed changes
Comment thread pkg/admission/crdnooverlappinggvr/crdnooverlappinggvr_admission.go
},
},
}
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.

@sttts
Copy link
Copy Markdown
Member

sttts commented May 15, 2026

/hold

@kcp-ci-bot kcp-ci-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sttts sttts sttts left review comments

@ntnn ntnn ntnn approved these changes

Assignees

@ntnn ntnn

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/chore Categorizes issue or PR as related to maintenance and other usually non-code changes. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Convert all writers of internal.apis.kcp.io/resource-bindings to SSA so Force: true can be removed

4 participants

@mjudeikis @kcp-ci-bot @ntnn @sttts