Skip to content

Compact watch cache based on last observed etcd compaction#132876

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
serathius:watchcache-compact
Jul 11, 2025
Merged

Compact watch cache based on last observed etcd compaction#132876
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
serathius:watchcache-compact

Conversation

@serathius
Copy link
Copy Markdown
Contributor

@serathius serathius commented Jul 10, 2025 •
edited
Loading

/kind feature

Compact snapshots in watch cache based on etcd compaction

This PR adds a watch to the etcd compactor to monitor compaction done by other apiservers and adds a compactor to watch cache that every 15 seconds reads compaction revision from etcd compactor and compacts snapshots.

Ref kubernetes/enhancements#4988

Note; after merging this PR we will be reverting kubernetes/test-infra#34587 to re-enable conformance tests for compaction.
/assign @jpbetz
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 10, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 10, 2025

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 10, 2025
@serathius serathius force-pushed the watchcache-compact branch 2 times, most recently from 7c1b2b5 to b484074 Compare July 11, 2025 08:20
@serathius serathius mentioned this pull request Jul 11, 2025
Open
16 tasks
jpbetz
jpbetz reviewed Jul 11, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

A few suggestions, LGTM after those are cleared.
Comment thread staging/src/k8s.io/apiserver/pkg/storage/cacher/compactor.go Outdated
Comment thread staging/src/k8s.io/apiserver/pkg/storage/cacher/compactor.go Outdated
Comment thread staging/src/k8s.io/apiserver/pkg/storage/cacher/compactor.go Outdated
Comment thread staging/src/k8s.io/apiserver/pkg/storage/etcd3/compact.go
Comment thread staging/src/k8s.io/apiserver/pkg/storage/etcd3/compact.go
if err != nil {
return compactRev, currentRev, fmt.Errorf("get %q failed: %w", compactRevKey, err)
}
if len(resp.Kvs) != 0 {
Copy link
Copy Markdown
Contributor

@jpbetz jpbetz Jul 11, 2025

Choose a reason for hiding this comment

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

If this condition evaluates to false, it looks like compactRev will be zero valued for the following code (lines 289-). Is that safe?
Copy link
Copy Markdown
Contributor Author

@serathius serathius Jul 11, 2025

Choose a reason for hiding this comment

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

Yes, until we read the value, the initial compaction revision is set to 0, so all the code was written to handle 0. Also the compaction revision is expected to always increase, so we always take max(current, new). This is to prevent races between TXN and Watch.
@serathius serathius force-pushed the watchcache-compact branch from b484074 to debefb7 Compare July 11, 2025 16:17
@serathius serathius force-pushed the watchcache-compact branch from debefb7 to bfeaae3 Compare July 11, 2025 16:25
jpbetz
jpbetz approved these changes Jul 11, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 11, 2025

LGTM label has been added.

DetailsGit tree hash: 54202f7aa42f2d5cb186a36096255aa5af348013
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Jul 11, 2025
@serathius
Copy link
Copy Markdown
Contributor Author

serathius commented Jul 11, 2025

/retest
@k8s-ci-robot k8s-ci-robot merged commit e44ddbf into kubernetes:master Jul 11, 2025
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone Jul 11, 2025
@serathius serathius mentioned this pull request Jul 11, 2025
Merged
@AwesomePatrol AwesomePatrol mentioned this pull request Aug 13, 2025
Merged
brandond added a commit to brandond/kine that referenced this pull request Sep 3, 2025
@brandond
Redirect apiserver get/watch to substitute compact rev key
835185f 
The apiserver now watches the compact rev key and prunes caches based on
the compact revision.
Ref: kubernetes/kubernetes#132876

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to brandond/kine that referenced this pull request Sep 3, 2025
@brandond
Redirect apiserver get/watch to substitute compact rev key
206c6a9 
The apiserver now watches the compact rev key and prunes caches based on
the compact revision.
Ref: kubernetes/kubernetes#132876

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@brandond brandond mentioned this pull request Sep 3, 2025
Merged
brandond added a commit to brandond/kine that referenced this pull request Sep 3, 2025
@brandond
Redirect apiserver get/watch to substitute compact rev key
16bfbf6 
The apiserver now watches the compact rev key and prunes caches based on
the compact revision.
Ref: kubernetes/kubernetes#132876

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to k3s-io/kine that referenced this pull request Sep 4, 2025
@brandond
Redirect apiserver get/watch to substitute compact rev key
c375b6e 
The apiserver now watches the compact rev key and prunes caches based on
the compact revision.
Ref: kubernetes/kubernetes#132876

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@fusida
Copy link
Copy Markdown
Contributor

fusida commented Sep 8, 2025 •
edited
Loading

@serathius Here is a case: the apiserver compaction disabled by --etcd-compaction-interval=0, and using etcd with --auto-compaction-retention=5m to compact the etcd data. This pr can not fix #131011 entirely in the comformance test.

Do you have any good idea?
@serathius
Copy link
Copy Markdown
Contributor Author

serathius commented Sep 8, 2025

There is currently no good way get compaction revision from etcd, that's why we depend on k8s compaction to compact watch cache. There was no movement with #80513 to fully migrate K8s to etcd managed compaction.

You can still configure --etcd-compaction-interval=5m apiserver or even larger period. Etcd will compact itself, while compaction done by apiserver will fail to compact etcd, but will compact watch cache.
lizardruss pushed a commit to loft-sh/kine that referenced this pull request Sep 26, 2025
@brandond @lizardruss
Redirect apiserver get/watch to substitute compact rev key
dff380a 
The apiserver now watches the compact rev key and prunes caches based on
the compact revision.
Ref: kubernetes/kubernetes#132876

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@RainbowMango RainbowMango mentioned this pull request Oct 22, 2025
Closed
15 tasks
@serathius serathius mentioned this pull request Jan 13, 2026
Merged
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. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

4 participants

@serathius @k8s-ci-robot @fusida @jpbetz