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

Make VersionMatch follow upstream + configure list semantics in watcher::Config #1171

Merged
merged 14 commits into from
Apr 3, 2023

Conversation

clux
Copy link
Member

@clux clux commented Mar 28, 2023

Effectively makes the VersionMatch an exact duplicate of the upstream, and adds some builders here and there to compensate for the missing Any.

Have changed the watcher interface and made a new enum that maps onto VersionMatch instead, because it does not make sense to run a watcher against a pinned resource version; it will always use 0.

(at least unless/until we build pagination into it).

We could keep the enum that wraps resource version, but this is awkward; it would be the only one that bundles resourceVersion that way;

  • watch api calls get it as a str (outside watchparams)
  • get api calls (in a follow-up pr) will also not use the enum

so altogether we are just making it awkward for ourselves by trying to make it nice :(

Follow-up to #1162 before releasing.

Questions

Q: should we make NotOlderThan("0") a new default for ListParams? we want people to use the new-faster/less-taxing variant, although it is also slightly less consistent. I feel the people who want strong consistency guarantees should opt-in rather than people who don't want strong consistency to opt-out.

A: ListParams should follow the api. Opt-in is up to the user. This avoids breaking changes.

Q: should watcher default to a less consistent listsemantic for relists? it's much less of an issue because a few extra events will eventually get cleaned up anyway due to the nature of it being an infinite watch stream. Given the reports that people are passing on upstream complaining about our default watch semantics, it might better to provide a stronger opinion here so that we are not seen as a "taxing runtime to use"

A: Not changing defaults. While it makes sense for Controller use where things are eventually consistent and meant to deal with a few redundant reconciles anyway, it's a little more iffy for watcher. We emphasise this in the docs, in particular for Semantic. People like opting into performance a lot more than they have to debug less consistency (if that happens).

Effectively makes the `VersionMatch` an exact duplicate of the upstream,
and adds some builders here and there to compensate for the missing `Any`.

Have changed the watcher interface and made a new enum that maps onto `VersionMatch` instead,
because it does not make sense to run a watcher against a pinned resource version; it will always use 0.

(at least unless/until we build pagination into it).

We could keep the enum that wraps resource version, but this is awkward;
it would be the only one that bundles resourceVersion that way;

- watch api calls get it as a str (outside watchparams)
- get api calls (in a follow-up pr) will also not use the enum

so altogether we are just making it awkward for ourselves by trying to make it nice :(

Signed-off-by: clux <[email protected]>
@clux clux added the changelog-change changelog change category for prs label Mar 28, 2023
@clux clux added this to the 0.81.0 milestone Mar 28, 2023
@clux clux changed the title Make VersionMatch follow upstream + decouple enum from watcher Make VersionMatch follow upstream + new semantic interface watcher::Config Mar 28, 2023
@nightkr
Copy link
Member

nightkr commented Mar 28, 2023

I'd argue that slow is a vastly better default than incorrect. If shit's slow then you can always profile and fix that afterwards. If shit's broken then, well, it has probably been broken for a long time already by the time that you discover the problem.

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@aed07be). Click here to learn what that means.
The diff coverage is 87.27%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1171   +/-   ##
=======================================
  Coverage        ?   72.75%           
=======================================
  Files           ?       67           
  Lines           ?     5201           
  Branches        ?        0           
=======================================
  Hits            ?     3784           
  Misses          ?     1417           
  Partials        ?        0           
Impacted Files Coverage Δ
kube-runtime/src/watcher.rs 43.38% <50.00%> (ø)
kube-core/src/params.rs 79.42% <85.71%> (ø)
kube-core/src/request.rs 96.27% <100.00%> (ø)

@clux
Copy link
Member Author

clux commented Mar 28, 2023

I'd argue that slow is a vastly better default than incorrect.

It's not really about it being incorrect (as it will eventually be correct), but i guess that's how it will be perceived. Ok, will leave the ListParams default.

Are you OK with the default Semantic being re-list from cache for watchers? or would you rather keep the full quorum read default there as well.

@clux clux changed the title Make VersionMatch follow upstream + new semantic interface watcher::Config Make VersionMatch follow upstream + configure list semantics in watcher::Config Mar 28, 2023
@clux
Copy link
Member Author

clux commented Mar 28, 2023

Ok. Have left default (strongly consistent) list semantics for the default Api, but made more opinionated defaults for watcher and edited in my reasoning for it in the main post.

Have cleaned up a bunch of builders, and shortened some tests as a result, but I am happy with this now. cc @nabokihms

@clux clux marked this pull request as ready for review March 28, 2023 21:16
@clux clux requested a review from nightkr March 28, 2023 21:16
kube-core/src/params.rs Outdated Show resolved Hide resolved
kube-core/src/params.rs Outdated Show resolved Hide resolved
kube-core/src/params.rs Outdated Show resolved Hide resolved
kube-core/src/request.rs Outdated Show resolved Hide resolved
kube-core/src/request.rs Show resolved Hide resolved
Copy link
Contributor

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

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

From my perspective, everything seems great.

@clux clux enabled auto-merge (squash) March 31, 2023 08:31
@clux
Copy link
Member Author

clux commented Apr 3, 2023

Any other comments on this @nightkr ?
Keen to get some merges happening and this is the most public facing thing, whereas most other things are unstable features.

Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

LGTM

@clux clux merged commit 4f633ee into main Apr 3, 2023
@clux clux deleted the unroll-version-match branch April 3, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants