🐛 reconciler/apibinding: make group resources of APIBindings + CRD race-free #3251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kcp-ci-bot
added
do-not-merge/work-in-progress
sttts
force-pushed
the
sttts-multiple-identities
branch
3 times, most recently
from
90885fe to
e338b59
Compare
kcp-ci-bot
added
size/XXL
sttts
changed the title
sttts
force-pushed
the
sttts-multiple-identities
branch
4 times, most recently
from
93f12c1 to
8ba5890
Compare
sttts
changed the title
kcp-ci-bot
removed
the
do-not-merge/work-in-progress
|
/retest |
sttts
force-pushed
the
sttts-multiple-identities
branch
4 times, most recently
from
3f47600 to
facb4eb
Compare
kcp-ci-bot
added
release-note
|
/build-image |
kcp-ci-bot
added
the
needs-rebase
|
/cc |
mjudeikis
reviewed
Feb 10, 2025
| @@ -78,35 +97,60 @@ func (p *crdNoOverlappingGVRAdmission) Validate(ctx context.Context, a admission | |||
| if a.GetKind().GroupKind() != apiextensions.Kind("CustomResourceDefinition") { | |||
| return nil | |||
| } | |||
| cluster, err := request.ClusterNameFrom(ctx) | |||
| if a.GetOperation() != admission.Create { | |||
There was a problem hiding this comment.
Do we need to care about server side patch to create?
There was a problem hiding this comment.
That should also be a create, no?
Looking on the possible values:
Create Operation = "CREATE"
Update Operation = "UPDATE"
Delete Operation = "DELETE"
Connect Operation = "CONNECT"
|
Question: Couldn't we use |
Don't we have that already for all of our controllers? |
… exports Signed-off-by: Dr. Stefan Schimanski <[email protected]>
…lCluster Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
sttts
force-pushed
the
sttts-multiple-identities
branch
from
facb4eb to
c132269
Compare
kcp-ci-bot
removed
the
needs-rebase
|
/lgtm Feeling lucky today :) |
|
LGTM label has been added. Git tree hash: 30533bd26f109dab9b0de99549fe3a3d28dbc860
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjudeikis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
approved
|
/kind bug |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The conflict checker in the APIBinding reconciler computed a list of CRDs, both real and bound ones. It took the APIExport as reference instead of the bindings directly. This seems wrong. If a schema is removed from an export, or the whole export is deleted, the binding still exists, but it didn't contribute the conflict checker anymore.
This PR does the following:
APIBinding.status.boundResourcesas reference.apis.kcp.io/resource-bindingsannotation on theLogicalCluster. It records the APIBinding or whether it is a CRD. This annotation (a JSON map) is updated BEFORE (a) a CRD is created (admission) or (b) an APIBinding bind the group resource:The tricky case is that we can only update the annotation during CRD creation via admission. And creation can fail. Then we have a dangling group resource reservation. This PR adds an expery timestamp to the group resource entries for CRD which matches the maximum request duration of 30s. For that time an APIBinding of the same group resource would fail (delayed). Don't know a better way to solve that.
Downside: CRD creation implies update of the
LogicalCluster, with potential for conflicts. This might make CRD creation slower. So if you install 1000 CRDs, this PR will have an impact.Related issue(s)
Fixes #3252
Release Notes