-
Notifications
You must be signed in to change notification settings - Fork 424
CachedResources: replicate full objects #3683
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.
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
CachedResources: replicate full objects #3683
Conversation
| localExists := !apierrors.IsNotFound(err) | ||
|
|
||
| // we only replicate objects that match the label selector. | ||
| if localExists && r.localLabelSelector != nil && !r.localLabelSelector.Matches(labels.Set(localCopy.GetLabels())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This early-return with selector mismatch is an additional opportunity to optimize away the GET call: we could feed r.getLocalCopy with the selector, and skip on mismatch before getting the full object.
Let me know if you'd like to do it still in this PR or separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do that, and rename the function so its something clearer names what it actually does now
|
/retest |
| }, | ||
| } | ||
| // r.getLocalCopy is defined separately because it calls r.getGlobalCopy internally. | ||
| r.getLocalCopy = func(cluster logicalcluster.Name, namespace, name string) (*unstructured.Unstructured, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name now does now make sense. Can we rename to something better as it calls both now?
e614097 to
41fb874
Compare
428f555 to
cc73264
Compare
|
/retest |
…urce CachedResource Each instance of the replication controller is dedicated to a specific CachedResource object, and so its CachedObjects handler should only react on objects that are coming from that CachedResource. On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
DDSIF, the informer for retrieving local objects, yields only PartialMetadataObjects. This commit adds a live GET call to retrieve the full object for replication. On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
On-behalf-of: @SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>
cc73264 to
a56886f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
| } | ||
|
|
||
| // Skip CachedObjects that are not coming from the source CachedResource. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dont need these new lines :)
|
[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 |
|
LGTM label has been added. Git tree hash: eca7d391de255e7b5579f711971ef735084b820a
|
|
/retest |
2 similar comments
|
/retest |
|
/retest |
Summary
DDSIF, the informer for retrieving local objects, yields only PartialMetadataObjects. This PR adds a live GET call to retrieve the full object for replication.
What Type of PR Is This?
/kind bug
Related Issue(s)
Fixes #3478
Release Notes