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

Remove v2 apply logic #16989

Merged
merged 1 commit into from
Nov 23, 2023
Merged

Conversation

serathius
Copy link
Member

@serathius serathius commented Nov 22, 2023

v2 store is no longer available in v3.6.
We can remove apply logic for it as they will never be used.

Only V2 PUT is needed as it applies to v3 storage and etcd v3.5 uses it for setting member attributes and cluster version.

I'm proposing etcd v3.6 to panic when encountering v2 store change. This can only happened if user has run v3.5 with --enable-v2. As documented in --v2-deprecation=write-only (default in etcd v3.6) will not allow custom v2 state.

'write-only' // Custom v2 state is not allowed (planned default in v3.6)

If etcd still has some v2 data, users are expected to either:

  • Halt the upgrade and reconsider what to do with v2 state.
  • Run etcd with --v2-deprecation=write-only-drop-data which is expected to delete the data. Implementation pending.

Note for reviewers:

  • We have dedicated tests for v2 cluster membership change and v2 cluster version setting. Add tests for setting cluster version using v2 request #16994
  • We have dedicated tests for running hybrid clusters (mixed v3.6 and v3.5)
    if fileutil.Exist(e2e.BinPath.EtcdLastRelease) {
    tcs = append(tcs, testCase{
    name: "MinorityLastVersion",
    config: config.ClusterConfig{
    ClusterSize: 3,
    ClusterContext: &e2e.ClusterContext{
    Version: e2e.MinorityLastVersion,
    },
    },
    }, testCase{
    name: "QuorumLastVersion",
    config: config.ClusterConfig{
    ClusterSize: 3,
    ClusterContext: &e2e.ClusterContext{
    Version: e2e.QuorumLastVersion,
    },
    },
    })
    }

cc @ahrtr @fuweid @chaochn47 @siyuanfoundation

@serathius serathius force-pushed the remove-v2-apply-logic branch from c9b4af3 to 2e2536c Compare November 22, 2023 09:05
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid
Copy link
Member

fuweid commented Nov 22, 2023

FAIL: (code:1):
  % (cd server && 'golangci-lint' 'run' '--config' '/home/runner/work/etcd/etcd/tools/.golangci.yaml' './...')
Error: etcdserver/apply_v2.go:119:6: func `toResponse` is unused (unused)
func toResponse(ev *v2store.Event, err error) Response {
FAIL: 'run golangci-lint run --config /home/runner/work/etcd/etcd/tools/.golangci.yaml ./...' checking failed (!=0 return code)

Need to remove the unused function toResponse.

@serathius serathius force-pushed the remove-v2-apply-logic branch 3 times, most recently from 6f78a84 to b316a4f Compare November 22, 2023 10:36
@serathius serathius marked this pull request as draft November 22, 2023 11:07
@serathius serathius force-pushed the remove-v2-apply-logic branch 6 times, most recently from 9835a60 to 32b8885 Compare November 22, 2023 12:51
@serathius serathius marked this pull request as ready for review November 22, 2023 13:09
@ahrtr
Copy link
Member

ahrtr commented Nov 22, 2023

Could you breakdown this PR into 2+ PRs?

  • This PR only focus on removing v2 apply logic (excluding Only V2 PUT, because it's still needed when in mix-version cluster). It should can be merged soon once the workflow is green.
  • Keep all other changes in separate PRs, which need careful review.

@serathius
Copy link
Member Author

Could you breakdown this PR into 2+ PRs?

  • This PR only focus on removing v2 apply logic (excluding Only V2 PUT, because it's still needed when in mix-version cluster). It should can be merged soon once the workflow is green.
  • Keep all other changes in separate PRs, which need careful review.

Done

@serathius serathius force-pushed the remove-v2-apply-logic branch 2 times, most recently from ffd42f3 to 276c811 Compare November 22, 2023 21:19
@serathius
Copy link
Member Author

serathius commented Nov 22, 2023

All the subsequent PRs were merged. ping @ahrtr for review.

For simplicity I have opted to not delete the code, just to an early check and panic. This allows to make logic change very clear and visible.

We can clean up the code slowly in subsequent PR.

@upodroid
Copy link
Contributor

/retest

@serathius
Copy link
Member Author

/retest

1 similar comment
@serathius
Copy link
Member Author

/retest

v2 store is no longer available in v3.6.
We can remove apply logic for it as they will never be used.

Only v2 PUT is neeeded as it applies to v3 storage and etcd v3.5 uses it for setting member
attributes and cluster version.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius serathius force-pushed the remove-v2-apply-logic branch from 276c811 to ed3375e Compare November 23, 2023 13:13
@@ -58,6 +58,34 @@ func TestV2DeprecationNotYet(t *testing.T) {
assert.NoError(t, err)
}

func TestV2DeprecationWriteOnlyWAL(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: this case won't work anymore if we bootstrap etcd based on v3store. But that isn't finalized yet, and may not be done in 3.6. But please let's add a comment/note.

Copy link
Member Author

Choose a reason for hiding this comment

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

We might need to update the test, however the test is still relevant.

@serathius serathius merged commit f04478f into etcd-io:main Nov 23, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants