馃悰Fix multiple CRDs/workspaces/versions #2751
Merged
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
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
2 times, most recently
from
cbb0219
to
4e453a8
Compare
ncdc
changed the title
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
2 times, most recently
from
8d8048b
to
6d4f676
Compare
|
Got a green test run! Doing 1 more with a slight update to the kube PR |
|
/approve |
openshift-ci
bot
added
the
approved
ncdc
requested review from
s-urbaniak,
stevekuznetsov and
sttts
and removed request for
kylape and
shawn-hurley
|
/retest |
p0lyn0mial
reviewed
Feb 9, 2023
| @@ -143,3 +150,232 @@ func TestPartialMetadataCRD(t *testing.T) { | |||
| require.True(t, enabled) | |||
| } | |||
| } | |||
|
|
|||
| func TestPartialMetadataSameCRDMultipleWorkspaces(t *testing.T) { | |||
There was a problem hiding this comment.
This test exercises the partialmetadata issue.
Do you think we should also cover the watch-cache issue with a new test?
There was a problem hiding this comment.
We can probably add to the existing watch cache test?
|
/retest |
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
from
6d4f676
to
a996da8
Compare
|
Updated to pull in the latest kube |
|
@p0lyn0mial let's merge this and then follow up with a watch cache test? |
|
Ugh I really need to nail down what's going on with quota & gc |
sgtm, thanks |
|
Same test failure as what this PR is trying to fix, but a different root cause now:
Need to figure out why it's getting a 404 |
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
5 times, most recently
from
2716cf2
to
80879a5
Compare
ncdc
commented
Feb 9, 2023
pkg/syncer/spec/spec_process.go
Outdated
| @@ -187,6 +188,18 @@ func (c *Controller) process(ctx context.Context, gvr schema.GroupVersionResourc | |||
| return nil, fmt.Errorf("object to synchronize is expected to be Unstructured, but is %T", obj) | |||
| } | |||
|
|
|||
| if actualVersion := upstreamObj.GetAnnotations()[conversion.KCPOriginalAPIVersionAnnotation]; actualVersion != "" { | |||
ncdc
commented
Feb 9, 2023
| @@ -352,7 +353,20 @@ func (c *Controller) processResource(ctx context.Context, key string) error { | |||
| } | |||
| unstr = unstr.DeepCopy() | |||
|
|
|||
| return c.reconcileResource(ctx, lclusterName, unstr, gvr) | |||
| actualGVR := gvr | |||
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
from
80879a5
to
9782f80
Compare
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
from
9782f80
to
bdf1d7e
Compare
Add an e2e test that verifies that we can support multiple CRDs for the same group+resource in different workspaces, with different API versions. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Pull in changes that fix wildcard partial metadata requests when there are CRDs serving different versions of the same group/resource in different workspaces. Pull in changes that fix the watch cache for normal CRDs - it was breaking because each workspace has its own watch cache per normal CRD, yet the cache was listing/watching CRs from all workspaces, leading to conversion errors. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
The dynamic discovery shared informer factory does a wildcard partial metadata list/watch against specific API versions. When an informer event comes in for a specific item, it's for a specific API version (whatever the factory used), but that doesn't necessarily mean it's the API version of the actual object being retrieved (the API version doesn't matter because we're only getting partial metadata). But if you want to UPDATE the object, you must know its appropriate API version. This is now presented via annotations in kcp.io/original-api-version. This updates the permission claim reconcilers to use the annotation when making dynamic client requests. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
The dynamic discovery shared informer factory does a wildcard partial metadata list/watch against specific API versions. When an informer event comes in for a specific item, it's for a specific API version (whatever the factory used), but that doesn't necessarily mean it's the API version of the actual object being retrieved (the API version doesn't matter because we're only getting partial metadata). But if you want to UPDATE/DELETE the object, you must know its appropriate API version. This is now presented via annotations in kcp.io/original-api-version. This updates the reconciler to use the annotation when making dynamic client requests. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
The dynamic discovery shared informer factory does a wildcard partial metadata list/watch against specific API versions. When an informer event comes in for a specific item, it's for a specific API version (whatever the factory used), but that doesn't necessarily mean it's the API version of the actual object being retrieved (the API version doesn't matter because we're only getting partial metadata). But if you want to UPDATE/DELETE the object, you must know its appropriate API version. This is now presented via annotations in kcp.io/original-api-version. This updates the spec syncer to use the annotation when making dynamic client requests against the upstream. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
from
bdf1d7e
to
b82da8a
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc, stevekuznetsov 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 |
|
Pushed a commit similar to #2684 to deflake the status half of the test. Flake was kcp-dev/contrib-tmc#53 |
Make sure the informer factories have synced the specific GVRs we care about in the test before clearing test client actions. This way, we know we won't get watch actions from the informers after we've cleared the actions, which is what was causing the flakiness. Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
ncdc
force-pushed
the
fix-multiple-crds-different-versions
branch
from
59fb6f1
to
fbcfc28
Compare
|
/lgtm |
kcp-ci-bot
added
the
size/XL
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
k8s PRs:
Related issue(s)
Fixes #2738