Watch Progress Request #212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarkResultsCurrent status |
|
The preliminary performance test with 1.31 suggests that the
|
berkayoz
reviewed
Dec 11, 2024
There was a problem hiding this comment.
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.
I assume we check if compaction happened in the backend.Watch function right?
There was a problem hiding this comment.
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.
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.
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.
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
bschimke95
reviewed
Dec 11, 2024
| go 1.22 | ||
| go 1.22.0 | ||
|
|
||
| toolchain go1.22.10 |
There was a problem hiding this comment.
why is this needed? Are we really relying on this specific patch version to build this?
There was a problem hiding this comment.
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.
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?
marco6
requested changes
Dec 13, 2024
marco6
approved these changes
Dec 20, 2024
petrutlucian94
added a commit
that referenced
this pull request
Jan 10, 2025
The e2e tests are currently failing because of this k8s-dqlite error: k8s.k8s-dqlite[1947]: time="2025-01-09T15:00:22Z" level=error msg="error while range on /registry/clusterroles/system:aggregate-to-view : unexpected limit: want 0, got 1" A separate PR is removing this check: #212 That being considered, we'll apply this change here to unblock the CI.
petrutlucian94
added a commit
that referenced
this pull request
Jan 10, 2025
* Bump CI os and Python versions The integration tests use Python 3.8 / ubuntu 20.04. The problem is that k8s-snap tests use type specifiers that require Python 3.10. For this reason, we'll switch to Python 3.10 / ubuntu 22.04. * Install lxd using k8s-snap composite action This simplifies the gh workflows and also applies the Docker iptables workaround. * Use self-hosted runners k8s-snap switched to self-hosted CI runners as the default Github ones didn't have enough resources. We'll update the k8s-dqlite CI job to use self-hosted runners as well when running k8s-snap integration tests. * Fix r install * Remove "get" checks The e2e tests are currently failing because of this k8s-dqlite error: k8s.k8s-dqlite[1947]: time="2025-01-09T15:00:22Z" level=error msg="error while range on /registry/clusterroles/system:aggregate-to-view : unexpected limit: want 0, got 1" A separate PR is removing this check: #212 That being considered, we'll apply this change here to unblock the CI. * fix self-hosted runner tag
louiseschmidtgen
force-pushed
the
KU-2290/watch-progress-request
branch
from
79047d6 to
8b0d967
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-intervalflagWhen 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