-
Notifications
You must be signed in to change notification settings - Fork 16
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 Progress Request #212
base: master
Are you sure you want to change the base?
Conversation
BenchmarkResults
Current status
|
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.
LGTM overall, I'll do another pass once finalized. We should split these into multiple PRs(one per upstream PR) as it is really hard to read and follow which change correlates to which PR. If we have to do this in a single PR then commenting through the PR on the changed lines to provide context would be appreciated.
pkg/kine/server/watch.go
Outdated
// If the watch result has a non-zero CompactRevision, then the watch request failed due to | ||
// the requested start revision having been compacted. Pass the current and and compact | ||
// revision to the client via the cancel response, along with the correct error message. | ||
if err == ErrCompacted { |
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.
I assume we check if compaction happened in the backend.Watch
function right?
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.
Correct, this happens in the After query
which checks for any revisions that happen after the start key.
If that start key is compacted we return the compaction error and cancel the watch and send a response with the current revision and the compact revision and canceled=true.
|
||
trace := logrus.IsLevelEnabled(logrus.TraceLevel) | ||
outer := true | ||
for outer { |
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.
Can we add a comment for this block, AFAIK we are merging all the events to send it in a single watch message?
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.
I've added some comments to this block- does that help clarify?
Basically we are waiting on events but if a progress notify request comes we send a progress report whether there have been events or not.
watchQueryTimeout time.Duration | ||
etcdMode bool | ||
watchQueryTimeout time.Duration | ||
watchProgressNotifyInterval time.Duration |
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.
We make the watchProgressNotifyInterval configurable.
The watchProgressNotifyInterval is the interval in which we send progress reports on our watchers to the api-server. This interval gets used in watch.go
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.
did a first pass - great work @louiseschmidtgen
go 1.22 | ||
go 1.22.0 | ||
|
||
toolchain go1.22.10 |
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.
why is this needed? Are we really relying on this specific patch version to build 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.
I believe go mod tidy
started adding this, see golang/go#65847
pkg/kine/server/get.go
Outdated
if r.Limit != 0 { | ||
return nil, fmt.Errorf("unexpected limit: want 0, got %d", r.Limit) | ||
if r.Limit != 0 && len(r.RangeEnd) != 0 { | ||
err := fmt.Errorf("invalid combination of rangeEnd and limit, limit should be 0 got %d", r.Limit) |
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.
just curious here: This basically enforces no limit on a range request, right?
Why can't we have a range request with a limit which would just return the first X entries?
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.
LGTM
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.
Hi Louise! Thanks for the effort on this new requirement. First pass below:
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.
Looking way better, thanks Louise!
Watch Progress Request
This PR adds support for watch progress notifications. Progress notifications ensure api-server's watch cache is consistent with Dqlite while reducing the need for resource-intensive quorum reads from Dqlite.
The api-server builds a watch history on its side based on an initial list and then a watch stream of events (update/create/delete). Reading from this cache can lead to issues from stale reads. This is where the progress notifications come into play: Upon a progress request to k8s-dqlite's watch-server we return the current revision (in other words the revision of the last event received). If the api-server's cache is not out of date then it can serve reads from the cache instead of from Dqlite.
Support for Upstream Features:
Through progress notifications support, upstream k8s WatchList and ConsistentListFromCache features are enabled.
For more details read feature-gates
Kine reference
The PR takes inspiration from kine’s watcher PRs:
Note to reviewer: I would recommend taking a look at each one of these PRs before diving into this PR.
Implementation Details
Emulated Etcd version
Support for etcd version check that evaluates whether RequestWatchProgress is supported by the current version of etcd endpoint from status request: https://github.com/kubernetes/kubernetes/blob/beb696c2c9467dbc44cbaf35c5a4a3daf0321db3/staging/src/k8s.io/apiserver/pkg/storage/feature/feature_support_checker.go#L157
Backport of k3s-io/kine#316.
The PR bumps the emulated etcd version to signal support of the api-server's
RequestWatchProgress
(v3.4.31+ or v3.5.13+).Watch Channels
For this implementation we need two channels for the watchers:
watch-progress-notify-interval
flagWhen a watch is requested on a revision that has been compacted we return a cancel response, along with the message that the key has been compacted and provide the current revision as well as the compact revision.
Testing
1.32 rc & test_config_propagation passed https://github.com/canonical/k8s-dqlite/actions/runs/12257281237/workflow
1.32 rc & full integration suite https://github.com/canonical/k8s-dqlite/actions/runs/12293052446/job/34304958695
Unit test: requesting and receiving ProgressNotify message