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

Streaming lists documentation #39993

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

wojtek-t
Copy link
Member

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 14, 2023
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 14, 2023
@netlify
Copy link

netlify bot commented Mar 14, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit f02bfac
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/64199bf8f2ff270008903625
😎 Deploy Preview https://deploy-preview-39993--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sftim
Copy link
Contributor

sftim commented Mar 14, 2023

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 14, 2023
@wojtek-t wojtek-t force-pushed the send_initial_events branch from f1aab87 to 0d4cd27 Compare March 20, 2023 09:14
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 20, 2023
@wojtek-t wojtek-t force-pushed the send_initial_events branch from 0d4cd27 to 2cd0599 Compare March 20, 2023 09:15
@wojtek-t wojtek-t changed the title [WIP] Streaming lists documentation Streaming lists documentation Mar 20, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023
@wojtek-t
Copy link
Member Author

/assign @lavalamp

@lavalamp - can you please review from the technical POV?

@lavalamp
Copy link
Member

/lgtm

looks great, just grammar nits :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 015155382d9e65681ab441470d253f45f2b6c3b0

@wojtek-t wojtek-t force-pushed the send_initial_events branch from 2cd0599 to cf4a450 Compare March 21, 2023 09:15
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2023
@k8s-ci-robot k8s-ci-robot requested a review from lavalamp March 21, 2023 09:15
@wojtek-t
Copy link
Member Author

@lavalamp - thanks! comments applied, PTAL

@wojtek-t
Copy link
Member Author

@kubernetes/sig-docs-en-owners - PTAL

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6dde35a44617a60d7096c4d03aad019ba5a03bec

Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

Not needed for this PR, but the api-concepts page is getting quite long. I think it might make sense to pull all the watch content out to a separate page.

Provided that the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled, this can be achieved by specifying `sendInitialEvents=true` as query string parameter
in a **watch** request. If set, the API server starts the watch stream with synthetic init
events to build the whole state of all existing objects followed by a `BOOKMARK` event
Copy link
Member

Choose a reason for hiding this comment

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

Add an anchor link to #watch-bookmarks here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 244 to 245
(if requested via `allowWatchBookmarks=true` option). The bookmark event includes the resource for that
watch bookmark. After sending the bookmark event, the API server continues as for any other **watch**
Copy link
Member

Choose a reason for hiding this comment

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

The bookmark event includes the resource for that watch bookmark

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

rephrased

Comment on lines 244 to 245
(if requested via `allowWatchBookmarks=true` option). The bookmark event includes the resource for that
watch bookmark. After sending the bookmark event, the API server continues as for any other **watch**
Copy link
Member

Choose a reason for hiding this comment

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

Is the apiserver guaranteed to send all initial events before any other watch events?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes


Provided that the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled, this can be achieved by specifying `sendInitialEvents=true` as query string parameter
in a **watch** request. If set, the API server starts the watch stream with synthetic init
Copy link
Member

Choose a reason for hiding this comment

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

synthetic init events

Are these always type: ADDED? Maybe add a note about that here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - added

Content-Type: application/json

{
"type": "ADDED",
Copy link
Member

Choose a reason for hiding this comment

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

(aside) Did you consider adding a new event type for the synthetic init events? I could imagine a case where a controller might want to use WatchList to initialize it's cache, but also takes a special action when resources are added that shouldn't be performed on the initialization events.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - it's not a good idea.
Because technically watch with RV=0 was supported from k8s 1.0 version and that also had ADDED events. Changing that would introduce more confusion or incompatibility than it's worth it.

And the bookmark give you exactly what you need in case you want to handle them differently (which btw isn't clear - we changed reflector to it (I mean optionally now) and having ADDED is good enough).

@wojtek-t wojtek-t force-pushed the send_initial_events branch from 748d837 to 94b1049 Compare March 27, 2023 08:53
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023
@k8s-ci-robot k8s-ci-robot requested a review from tengqm March 27, 2023 08:53
@netlify
Copy link

netlify bot commented Mar 27, 2023

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 2bf6e39
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/64219ccfea664b0008be5b4e

Copy link
Member Author

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

PTAL

@sftim - can you please approve if you're ok with it?


Provided that the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled, this can be achieved by specifying `sendInitialEvents=true` as query string parameter
in a **watch** request. If set, the API server starts the watch stream with synthetic init
Copy link
Member Author

Choose a reason for hiding this comment

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

yes - added

Provided that the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled, this can be achieved by specifying `sendInitialEvents=true` as query string parameter
in a **watch** request. If set, the API server starts the watch stream with synthetic init
events to build the whole state of all existing objects followed by a `BOOKMARK` event
Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 244 to 245
(if requested via `allowWatchBookmarks=true` option). The bookmark event includes the resource for that
watch bookmark. After sending the bookmark event, the API server continues as for any other **watch**
Copy link
Member Author

Choose a reason for hiding this comment

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

rephrased

Comment on lines 244 to 245
(if requested via `allowWatchBookmarks=true` option). The bookmark event includes the resource for that
watch bookmark. After sending the bookmark event, the API server continues as for any other **watch**
Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Content-Type: application/json

{
"type": "ADDED",
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - it's not a good idea.
Because technically watch with RV=0 was supported from k8s 1.0 version and that also had ADDED events. Changing that would introduce more confusion or incompatibility than it's worth it.

And the bookmark give you exactly what you need in case you want to handle them differently (which btw isn't clear - we changed reflector to it (I mean optionally now) and having ADDED is good enough).

On large clusters, retrieving the collection of some resource types may result in
a significant increase of resource usage (primarily RAM) on the control plane.
In order to alleviate its impact and simplify the user experience of the **list** + **watch**
pattern, Kubernetes - including version {{< skew currentVersion >}} - provides alpha support
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to make this statement ever-green? For example,

Suggested change
pattern, Kubernetes - including version {{< skew currentVersion >}} - provides alpha support
pattern, Kubernetes v1.27 introduces as alpha feature the support

content/en/docs/reference/using-api/api-concepts.md Outdated Show resolved Hide resolved
@wojtek-t wojtek-t force-pushed the send_initial_events branch from 94b1049 to 023a0ee Compare March 27, 2023 09:42
@wojtek-t
Copy link
Member Author

@tengqm - thanks PTAL

@tengqm
Copy link
Contributor

tengqm commented Mar 27, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6a0605a876c835898310f4c0009a46e330e1350a

@@ -226,6 +226,64 @@ As a client, you can request `BOOKMARK` events by setting the
assume bookmarks are returned at any specific interval, nor can clients assume that
the API server will send any `BOOKMARK` event even when requested.

## Streaming lists {#watch-bookmarks}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will shadow the ## Watch bookmarks heading earlier.
I have a suspicion this was my suggestion; anyway, I suggest:

Suggested change
## Streaming lists {#watch-bookmarks}
### Streaming lists {#watch-bookmark-initial-events}

The new text becomes part of “Efficient detection of changes”.


If we need to reframe that 2nd-level heading, perhaps to be “Data streams”, “Streamed collection data” or something else more generic, then we can do so.

Is there a case where a client does a watch with sendInitialEvents other than to end up running the watch? (Is there a reasonable case where client might want to abort the watch as soon as it sees the first BOOKMARK event?)

Copy link
Member Author

Choose a reason for hiding this comment

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

That wasn't your suggestion - it was @tallclair 's one.

But I messed it up - it should be for "watch bookmarks" section. Should be fixed now.

Is there a reasonable case where client might want to abort the watch as soon as it sees the first BOOKMARK event?

There might be a reasonable usecase where we actually want to stream the data without storing everything in memory. So yes - there are usecases (although we don't want to endorse them yet - probably in the future).

@wojtek-t wojtek-t force-pushed the send_initial_events branch from 023a0ee to 2bf6e39 Compare March 27, 2023 13:40
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023
@k8s-ci-robot k8s-ci-robot requested a review from tengqm March 27, 2023 13:40
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Given that this won't be part of efficient detection of changes, we should help readers learn about watches. They may land directly on this section without seeing the context earlier in the page.

/lgtm
this is already alpha quality

It'd be nice to tweak further

Comment on lines +236 to +238
pattern, Kubernetes v1.27 introduces as an alpha feature the support
for requesting the initial state (previously requested via the **list** request) as part of
the **watch** request.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pattern, Kubernetes v1.27 introduces as an alpha feature the support
for requesting the initial state (previously requested via the **list** request) as part of
the **watch** request.
pattern, Kubernetes {{< skew currentVersion >}} allows you to fetch the contents of a collection
as part of a **watch** request. See [Efficient detection of changes](#efficient-detection-of-changes)
to learn more about watches in Kubernetes.
As a client, you can either continue the **watch** to subscribe to further event updates, or you can
use this mechanism purely as a more efficient alternative to **list** a collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

pattern, Kubernetes {{< skew currentVersion >}}

Please figure it out - I got the opposite comment from @tengqm - I'm with this project for 8y+, but it can be extremely annoying if different reviewers are giving contradicting comments.

Also - as I mentioend in the other comment I don't want to endorse this as a more efficient alternative to LIST yet - we're not ready for that. So let's not try to recommend things we're not ready for.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the recommend vs. not, feel free to frame this as experimental rather than as more efficient. It's good if a reader can still see under what conditions they might try it out.

We don't have a clear style guideline for when to mention the current version or not. Our overall guidance is to aim for timeless documentation.
Sorry about the mixed messages. I know I've got a particular way of writing for maintainability, and because this is text not code, there's no single right way to do it.

Here's a test: imagine that what you write is still present in Kubernetes 1.42 and the feature is still alpha. Does it make sense and feel current for a 1.42 release? If it clearly does, it's probably fine.

Comment on lines +240 to +241
Provided that the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled, this can be achieved by specifying `sendInitialEvents=true` as query string parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Provided that the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
is enabled, this can be achieved by specifying `sendInitialEvents=true` as query string parameter
Provided your cluster is running at least Kubernetes v1.27 and you have enabled the
the `WatchList` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/), you can
perform a streaming list by specifying `sendInitialEvents=true` as query string parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

The 1.27 version is mentioned 2 lines above (in addition to the header of the section). I don't think it makes sense to constantly repeat it - for me as a reader it would be more annoying than useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm proposing that this (and the feature state shortcode, which lots of readers don't fully notice) is where we make the requirements clear.

That way, we're documenting in one place both:

  • the version requirement
  • the need to explicitly enable a feature gate

My experience suggests putting these details in 2 places is likely to lead to more GitHub issues telling us the docs are wrong (even when the docs are strictly correct and have been misunderstood).

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 406e5ebd1042de1a64d850222582429fa62a9b93

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

Comfortably good enough for alpha

@tengqm
Copy link
Contributor

tengqm commented Mar 27, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2023
@k8s-ci-robot k8s-ci-robot merged commit f23cc95 into kubernetes:dev-1.27 Mar 27, 2023
@k8s-ci-robot k8s-ci-robot added this to the 1.27 milestone Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants