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.

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

Watch bookmark api changes #74074

Merged
merged 2 commits into from Apr 16, 2019

Conversation

@wojtek-t
Copy link
Member

commented Feb 14, 2019

This PR contains API changes necessary for implementing this KEP: kubernetes/enhancements#819

The implementation is done in #75474

Ref: #73585
Ref: kubernetes/enhancements#956

Introduce API for watch bookmark events.
Introduce Alpha field `AllowWatchBookmarks` in ListOptions for requesting watch bookmarks from apiserver. The implementation in apiserver is hidden behind feature gate `WatchBookmark` (currently in Alpha stage).
@liggitt

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@liggitt - can you please do the API review for this?

sure, will queue it up for after 1.14 code freeze
/milestone v1.15

@yliaog

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

/cc

@k8s-ci-robot k8s-ci-robot requested a review from yliaog Feb 14, 2019

@logicalhan

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

/assign @jpbetz

@WIZARD-CXY

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

@@ -349,6 +349,13 @@ type ListOptions struct {
// add, update, and remove notifications. Specify resourceVersion.
// +optional
Watch bool `json:"watch,omitempty" protobuf:"varint,3,opt,name=watch"`
// AllowBookmarks opts-in for receiving watch events with type Bookmark.

This comment has been minimized.

Copy link
@smarterclayton

smarterclayton Feb 19, 2019

Contributor

Use the camelCase name allowBookmarks requests periodic watch events with type "BOOKMARK".

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Feb 19, 2019

Author Member

I removed periodic from your suggestion, because depending on exact logic we will choose, they not necessary have to be periodic.

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

@liggitt - I rebased and applied the comments (the godeps failure seems to be unrelated to my PR).

PTAL

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 25, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

/retest

@wojtek-t wojtek-t force-pushed the wojtek-t:watch_bookmark_api_changes branch from a52bddb to 4f8d65b Mar 26, 2019

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

API LGTM. Please update the KEP with the tweaked field name. Are we intending to build the implementation on top of this before merging?

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

API LGTM. Please update the KEP with the tweaked field name. Are we intending to build the implementation on top of this before merging?

Thanks a lot Jordan!
KEP update in: kubernetes/enhancements#922

I would prefer merging tha now if possible - there is already a WIP implementation in #75474 [I think it requires a bunch of work still, but it's really promissing] - does that sound ok for you?

@@ -57,6 +58,10 @@ type Event struct {
// Object is:
// * If Type is Added or Modified: the new state of the object.
// * If Type is Deleted: the state of the object immediately before deletion.
// * If Type is Bookmark: the object (instance of a type being watched) where
// only ResourceVersion field if set. On successful restart of watch from a

This comment has been minimized.

Copy link
@jpbetz

jpbetz Mar 29, 2019

Contributor

s/if/is?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Mar 29, 2019

Author Member

done

@wojtek-t wojtek-t force-pushed the wojtek-t:watch_bookmark_api_changes branch from 4f8d65b to 2a78fe0 Mar 29, 2019

@wojtek-t
Copy link
Member Author

left a comment

I added feature gate to this PR.

@@ -57,6 +58,10 @@ type Event struct {
// Object is:
// * If Type is Added or Modified: the new state of the object.
// * If Type is Deleted: the state of the object immediately before deletion.
// * If Type is Bookmark: the object (instance of a type being watched) where
// only ResourceVersion field if set. On successful restart of watch from a

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Mar 29, 2019

Author Member

done

@wojtek-t wojtek-t force-pushed the wojtek-t:watch_bookmark_api_changes branch from 2a78fe0 to 098f5ba Mar 29, 2019

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

/retest

// alpha: v1.15
//
// Enables support for watch bookmark events.
WatchBookmark utilfeature.Feature = "WatchBookmark"

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 12, 2019

Author Member

@liggitt - where this feature gate should be defined?
We definitely need access to it in apiserver, but we also need it in client-go (e.g. to enable bookmarks in reflector library based on that)?
Thoughts?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 12, 2019

Author Member

So thinking more about it, client-go in general shouldn't have access to feature gates.
So maybe this place is fine, because it will allow us to make both

  • kube-apiserver set server-sde support based on it
  • controllers, kubelets, etc - request bookmarks based on that
    WDYT?

This comment has been minimized.

Copy link
@hormes

hormes Apr 12, 2019

Contributor

I am afraid we can not, if we add check the feature gate in reflector, we have to import the feature package in client-go, which is what we want to avoid, right?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 12, 2019

Author Member

we don't need to import to-client-go, we can properly set ListOptions in those components themselves...

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 12, 2019

Member

this should go in staging/src/k8s.io/apiserver/pkg/features/kube_features.go, since watch behavior is part of generic apiserver

you are correct that client-go should not check the feature gate, just the option in ListOptions. see https://github.com/kubernetes/kubernetes/pull/51876/files for how @smarterclayton did this for paging

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 12, 2019

Author Member

Done. And thanks for the link.

This comment has been minimized.

Copy link
@hormes

hormes Apr 15, 2019

Contributor

The current implementation is already here.

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Apr 15, 2019

Author Member

Yes I know - but feature gates is kind of api, so I added it to this PR too.

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@liggitt - I think that we have an implementation of this API pretty much ready: #75474 (comment)

Modulo this question https://github.com/kubernetes/kubernetes/pull/74074/files#r274815161 that I just added, I think this is now ready for approval. Can you please take a look?

@wojtek-t wojtek-t force-pushed the wojtek-t:watch_bookmark_api_changes branch from 098f5ba to 2f77e8a Apr 12, 2019

@wojtek-t wojtek-t force-pushed the wojtek-t:watch_bookmark_api_changes branch from 2f77e8a to ac92c57 Apr 13, 2019

wojtek-t added some commits Feb 14, 2019

@wojtek-t wojtek-t force-pushed the wojtek-t:watch_bookmark_api_changes branch from ac92c57 to b0ff8dd Apr 15, 2019

@wojtek-t

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@liggitt - tests passing, PTAL

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 16, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

please update the release note with more details (what the list option is, that it is in alpha, the feature gate name, etc)

@k8s-ci-robot k8s-ci-robot merged commit 107595e into kubernetes:master Apr 16, 2019

19 of 20 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation wojtek-t authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@wojtek-t wojtek-t deleted the wojtek-t:watch_bookmark_api_changes branch Apr 17, 2019

@liggitt liggitt added this to Completed, 1.15 in API Reviews Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.