tests: migrate hashkv tests(e2e/integration) to common test framework #20584
Conversation
|
Hi @yagikota. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
yagikota
changed the title
yagikota
force-pushed
the
migrate-hashkv-test
branch
2 times, most recently
from
f6f8936 to
c30ceeb
Compare
| if tc.config.ClusterSize < 2 { | ||
| t.Skip("Skipping test for single-member cluster") | ||
| } |
There was a problem hiding this comment.
The test scope is across cluster members, so it can be skipped in the single-member case.
| if tc.config.ClusterSize < 2 { | ||
| t.Skip("Skipping test for single-member cluster") | ||
| } |
There was a problem hiding this comment.
The test scope is across cluster members, so it can be skipped in the single-member case.
| if tc.config.ClusterSize < 2 { | ||
| t.Skip("Skipping test for single-member cluster") | ||
| } |
There was a problem hiding this comment.
The test scope is across cluster members, so it can be skipped in the single-member case.
yagikota
force-pushed
the
migrate-hashkv-test
branch
from
c30ceeb to
c417fbb
Compare
|
|
||
| // TestVerifyHashKVAfterTwoCompactsOnTombstone tests that HashKV is consistent | ||
| // across all members after two physical compactions on tombstone revisions. | ||
| func TestVerifyHashKVAfterTwoCompactsOnTombstone(t *testing.T) { |
There was a problem hiding this comment.
Migrate this e2e test:
Line 102 in 991ea7e
There was a problem hiding this comment.
Before migration, this test covered only mixed-version, but now covers all cluster cases in e2eClusterTestCases or integrationClusterTestCases
Issues around compaction/tombstones/HashKV can occur even in same-binary clusters, so I think we do not need to restrict these tests to mixed-version only. 🤔
|
|
||
| // TestVerifyHashKVAfterCompactOnLastTombstone tests that HashKV is consistent | ||
| // across all members after a physical compaction on the last tombstone revision. | ||
| func TestVerifyHashKVAfterCompactOnLastTombstone(t *testing.T) { |
There was a problem hiding this comment.
Migrate this e2e test:
Line 139 in 991ea7e
There was a problem hiding this comment.
| defer clus.Close() | ||
|
|
||
| cc := testutils.MustClient(clus.Client()) | ||
| tombstoneRevs, _ := populateDataForHashKV(t, cc, []string{"key0"}) |
There was a problem hiding this comment.
In the previous integration test, the number of operations to populate sample data was more than 1,000.
etcd/server/storage/mvcc/testutil/hash.go
Lines 31 to 83 in 991ea7e
For now, we keep it at ~ 40 operations using populateDataForHashKV.
This test validates an invariant rather than throughput, so higher operation counts don't improve the quality of the test.
yagikota
changed the title
|
Would you review this PR? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, serathius, yagikota 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 |
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted filessee 26 files with indirect coverage changes @@ Coverage Diff @@
## main #20584 +/- ##
==========================================
- Coverage 69.15% 68.78% -0.37%
==========================================
Files 420 418 -2
Lines 34776 34689 -87
==========================================
- Hits 24049 23862 -187
- Misses 9333 9437 +104
+ Partials 1394 1390 -4 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I'll fix it tomorrow. |
|
/retest-required
|
hashkv_test.go:175:
Error Trace: /home/prow/go/src/github.com/etcd-io/etcd/tests/common/hashkv_test.go:175
Error: Received unexpected error:
context done before matching log found: context deadline exceeded
Test: TestVerifyHashKVAfterCompactAndDefrag/ClientAutoTLS-> Use |
Signed-off-by: Kota <[email protected]>
yagikota
force-pushed
the
migrate-hashkv-test
branch
from
c417fbb to
56231c5
Compare
|
cc. @serathius @ahrtr I fixed the failed test. Would you check it again? |
What
Migrate HashKV tests to the common test framework and remove legacy e2e/integration suites.
Test Migration Mapping
Cluster-wide consistency check:
TestVerifyHashKVAfterCompact: Ensure that HashKV is consistent across all members after a physical compactionTestVerifyHashKVAfterTwoCompactsOnTombstone: Ensure that HashKV is consistent across all members after two physical compactions on tombstone revisions.TestVerifyHashKVAfterCompactOnLastTombstone: Ensure that HashKV is consistent across all members after a physical compaction on the last tombstone revision.Member-local consistency check:
TestVerifyHashKVAfterCompactAndDefrag: Ensure that HashKV is consistent within a member before and after a physical compaction and defragmentation.Other note
context.WithTimeout(t.Context(), 30*time.Second)(30s timeouts) to reduce flakiness due to CI variance.Why
This migration is part of #13637
How to test?
Requirement
Check the
./bin/etcdversion-> minor version of
/etcd-last-releasecan be calculated like this: 7 - 1 = 6Check the latest
v3.6.*versionInstall etcd
v3.6.4as/etcd-last-releaseCheck versions
Before Migration
E2E
Integration
After Migration
E2E
Integration
Ref
close #20551