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

Update etcdutl migrate command: load wal records from the latest snapshot #19128

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Jan 6, 2025

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 57.89474% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.85%. Comparing base (d5231c7) to head (7f7ebde).
Report is 3 commits behind head on main.

Current head 7f7ebde differs from pull request most recent head b99b962

Please upload reports for the commit b99b962 to get more accurate results.

Files with missing lines Patch % Lines
etcdutl/etcdutl/migrate_command.go 52.94% 6 Missing and 2 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
server/etcdserver/apply/apply.go 78.47% <100.00%> (+0.44%) ⬆️
server/etcdserver/bootstrap.go 63.13% <100.00%> (-0.47%) ⬇️
etcdutl/etcdutl/migrate_command.go 9.78% <52.94%> (+9.78%) ⬆️

... and 24 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19128      +/-   ##
==========================================
+ Coverage   68.77%   68.85%   +0.08%     
==========================================
  Files         420      420              
  Lines       35640    35656      +16     
==========================================
+ Hits        24510    24550      +40     
+ Misses       9704     9684      -20     
+ Partials     1426     1422       -4     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3388e2b...b99b962. Read the comment docs.

@ahrtr ahrtr marked this pull request as draft January 7, 2025 08:10
@serathius
Copy link
Member

/retest

serathius
serathius previously approved these changes Jan 7, 2025
@ahrtr ahrtr force-pushed the etcdutl_migrate_20250106 branch from 5386eef to 44cb583 Compare January 7, 2025 14:00
@ahrtr ahrtr marked this pull request as ready for review January 7, 2025 14:00
@serathius serathius dismissed their stale review January 7, 2025 14:06

Changes in server/etcdserver/apply/apply.go

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr

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

@ahrtr ahrtr force-pushed the etcdutl_migrate_20250106 branch from 44cb583 to 7f7ebde Compare January 7, 2025 16:18
@k8s-ci-robot
Copy link

@ahrtr: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-etcd-e2e-arm64 7f7ebde link true /test pull-etcd-e2e-arm64
pull-etcd-e2e-386 7f7ebde link true /test pull-etcd-e2e-386
pull-etcd-e2e-amd64 7f7ebde link true /test pull-etcd-e2e-amd64

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@@ -424,8 +423,9 @@ func (a *applierMembership) ClusterVersionSet(r *membershippb.ClusterVersionSetR
zap.String("new-cluster-version", newVersion.String()),
)
}
a.snapshotServer.ForceSnapshot()
Copy link
Member

Choose a reason for hiding this comment

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

The case TestV2DeprecationSnapshotMatches failed caused by this change.

ahrtr added 2 commits January 8, 2025 16:01
When downgrading or offline migration, etcd read the maximum
version from the WAL files, so as to ensure the operation is
allowed. It also reads the verson from `ClusterVersionSet` request,
so we should create a snapshot when cluster version chanages,
so as not to block the downgrade or migration operations.

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr ahrtr force-pushed the etcdutl_migrate_20250106 branch from 7f7ebde to b99b962 Compare January 8, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Etcdutl migrate should read WAL from last snapshot
4 participants