-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ metadata-only watches #1174
✨ metadata-only watches #1174
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12 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 |
/assign @vincepri since he expressed interest |
/kind feature |
/milestone v0.7.x Will take a look tomorrow :) |
case *metav1.PartialObjectMetadata: | ||
return sw.client.metadataClient.PatchStatus(ctx, obj, patch, opts...) |
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.
How would patching the status on a partial metadata object work in this case? 🤔
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.
metadata is modifiable on status...
... except certain things on certain kinds where the fields silently fail to update:
Kind | Fields |
---|---|
Deployments | Labels |
CRD | Labels, Annotations, OwnerReferences, Generation, SelfLink |
APIService | Labels, Annotations, Finalizers, OwnerReferences |
which is uuuhh not the most internally consistent.
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.
(FWIW, the client-go client allows it, which is mainly why I added it here)
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.
Should we consider not allowing patching status fields on partial objects to avoid confusion?
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.
Do we still want to do this?
// TODO(directxman12): we could rewrite this on top of the low-level REST | ||
// client to avoid the extra shallow copy at the end, but I'm not sure it's | ||
// worth it -- the metadata client deals with falling back to loading the whole | ||
// object on older API servers, etc, and we'd have to reproduce that. |
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.
👍
e6ed824
to
afc4e2a
Compare
afc4e2a
to
42e7c67
Compare
@DirectXMan12 Are you able to rebase so we can get this merged in v0.7.x? |
yeah, will do later today or tomorrow |
42e7c67
to
1b6c04b
Compare
This adds support for informers that communicate with the API server in metadata-only form. They are *completely* separate from normal informers -- that is: just like unstructured, if you ask for both a "normal" informer & a metadata-only informer, you'll get two copies of the cache.
1b6c04b
to
c43ad2e
Compare
This adds support for a metadata-only client. It only implements the operations supported by metadata (delete, deleteallof, patch, get, list, status.patch). The higher-level client will now delegate to this for when a PartialObjectMetadata object is passed in.
87bdded
to
9a82f9f
Compare
/retest shutdown data race, still working on a fix |
@vincepri should be good to merge |
huh, that's actually a different test race this time. Will produce a fix, just a test race. |
"k8s.io/apimachinery/pkg/runtime/schema" | ||
"k8s.io/client-go/rest" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
|
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.
Remove this?
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.
whoops, yeah, bad merge
This adds options to "project" watches as only metadata to the builder, making it more convienient to use these forms. For instance: ```go .Owns(&corev1.Pod{}, builder.OnlyMetadata) ``` is equivalent to ```go .Owns(&metav1.PartialObjectMetadata{ TypeMeta: metav1.TypeMeta{ APIVersion: "v1", Kind: "Pod", }, }) ```
It's overzealous and doesn't understand that interfaces have semantic meaning (and has just bad results on occaison).
9a82f9f
to
5de9250
Compare
/retest |
/lgtm |
This introduces metadata-only support (PartialObjectMetadata) to the client, cache, and controller builder. It works much like unstructured does, with an end-use of:
Note that like unstructured, the caches are completely separate -- asking for an unstructured object or concrete object will end up constructing a separate cache.
Fixes #1159