Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ metadata-only watches #1174

Merged
merged 4 commits into from Oct 20, 2020
Merged

✨ metadata-only watches #1174

merged 4 commits into from Oct 20, 2020

Conversation

DirectXMan12
Copy link
Contributor

This introduces metadata-only support (PartialObjectMetadata) to the client, cache, and controller builder. It works much like unstructured does, with an end-use of:

.Owns(&corev1.Pod{}, builder.OnlyMetadata)
// or
.Owns(&metav1.PartialObjectMetadata{/* GVK for corev1.Pod filled out here */})

// ...

var pods metav1.PartialObjectMetadataList
pods.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("PodList"))
cl.List(ctx, &pods, client.MatchingLabels{...})

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 24, 2020
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2020
@DirectXMan12
Copy link
Contributor Author

/assign @vincepri

since he expressed interest

@DirectXMan12
Copy link
Contributor Author

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 24, 2020
@vincepri
Copy link
Member

/milestone v0.7.x

Will take a look tomorrow :)

@k8s-ci-robot k8s-ci-robot added this to the v0.7.x milestone Sep 24, 2020
vincepri
vincepri reviewed Sep 24, 2020
View reviewed changes
pkg/builder/controller.go Outdated Show resolved Hide resolved
pkg/client/client.go
Comment on lines +257 to +258
case *metav1.PartialObjectMetadata:
return sw.client.metadataClient.PatchStatus(ctx, obj, patch, opts...)
Copy link
Member

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? 🤔

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

Copy link
Member

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?

Copy link
Member

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?

vincepri
vincepri reviewed Sep 24, 2020
View reviewed changes
pkg/client/metadata_client.go
Comment on lines +31 to +33
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2020
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 29, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2020
@DirectXMan12 DirectXMan12 reopened this Oct 1, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2020
@vincepri
Copy link
Member

@DirectXMan12 Are you able to rebase so we can get this merged in v0.7.x?

@DirectXMan12
Copy link
Contributor Author

yeah, will do later today or tomorrow

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020
@DirectXMan12
Add low-level metadata-only informer support
0160ff5 
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.
@DirectXMan12
Support metadata-only client operations
f89d3f0 
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.
@DirectXMan12
Copy link
Contributor Author

/retest

shutdown data race, still working on a fix

@DirectXMan12
Copy link
Contributor Author

@vincepri should be good to merge

@DirectXMan12
Copy link
Contributor Author

huh, that's actually a different test race this time. Will produce a fix, just a test race.

vincepri
vincepri reviewed Oct 16, 2020
View reviewed changes
pkg/builder/controller.go
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"

Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, yeah, bad merge

@DirectXMan12
Support "projections" in the controller builder
762bea7 
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",
        },
})
```
@DirectXMan12
Disable the interfacer lint
5de9250 
It's overzealous and doesn't understand that interfaces have semantic
meaning (and has just bad results on occaison).
@vincepri
Copy link
Member

/retest

@vincepri
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2020
@k8s-ci-robot k8s-ci-robot merged commit bae8fdb into kubernetes-sigs:master Oct 20, 2020
@akutz akutz mentioned this pull request Oct 29, 2020
Closed
This was referenced Nov 2, 2020
Closed
Merged
@DirectXMan12 DirectXMan12 deleted the feature/metadata-only-watch branch November 19, 2020 21:59
@timuthy timuthy mentioned this pull request Nov 30, 2020
Closed
@timebertt timebertt mentioned this pull request Feb 2, 2021
Closed
@timuthy timuthy mentioned this pull request Feb 18, 2021
Merged
@maelvls maelvls mentioned this pull request Sep 10, 2021
Closed
@dbarrosop dbarrosop mentioned this pull request Mar 2, 2022
Closed
@tesshuflower tesshuflower mentioned this pull request Jun 7, 2023
Open
@erikgb erikgb mentioned this pull request Sep 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

Assignees

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.x
Development

Successfully merging this pull request may close these issues.

Metadata-only list/watch support
3 participants
@DirectXMan12 @k8s-ci-robot @vincepri