tests/e2e: add test cases related to HashKV #18369

fuweid · 2024-07-26T11:40:12Z

Related to #18274

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

fuweid · 2024-07-26T11:47:58Z

tests/e2e/hashkv_test.go

+	}
+}
+
+func TestVerifyHashKVAfterTwoCompactions_MixVersions(t *testing.T) {


Side note: If #18274 is accepted, the main branch always keep compacted revision. However, existing releases delete that compacted revisions if they're tombstone. The #18274 should skip previous compacted revision which is tombstone. So that HashKV result can be the same in mix versions, especially it won't file data corrupt alert when cluster is updating to new release.

serathius · 2024-07-26T11:55:13Z

tests/e2e/hashkv_test.go

+		hashKVOnRev    int64
+	}{
+		{
+			compactedOnRev: 33, // tombstone


Please don't use comments to imply some relation of number here and consts in newTestKeySetInCluster. Passing an explicit variable should always be better.

Rollback to original one. Please take a look. If it's accepted, I will squash the commits. Thanks

fuweid · 2024-07-26T12:51:29Z

ping @ahrtr

jmhbnz · 2024-07-28T17:29:26Z

/retest

serathius · 2024-07-29T08:26:46Z

tests/e2e/hashkv_test.go

+
+	scenarios := []struct {
+		ClusterVersion fe2e.ClusterVersion
+		OnlyOneKey     bool


Can you provide more context what OnlyOneKey means?

When there is only one key and new compaction revision is on tombstone, all MVCC keys will be involved in hash value, because there is empty available revision result keep.

etcd/server/storage/mvcc/hash.go

Lines 60 to 76 in e7f5729

func (h *kvHasher) WriteKeyValue(k, v []byte) {

kr := BytesToRev(k)

upper := Revision{Main: h.revision + 1}

if !upper.GreaterThan(kr) {

return

}

lower := Revision{Main: h.compactRevision + 1}

// skip revisions that are scheduled for deletion

// due to compacting; don't skip if there isn't one.

if lower.GreaterThan(kr) && len(h.keep) > 0 {

if _, ok := h.keep[kr]; !ok {

return

}

}

h.hash.Write(k)

h.hash.Write(v)

}

I want to keep previous compaction revision in #18274, no matter what type of key it is, normal revision or tombstone. Both v3.5.x and v3.4.x releases delete compaction revision if it's tombstone. So, if there is empty available revision result keep, #18274 patch could possibly involve the tombstone in hash value, while the existing releases doesn't. The scenario can be used to verify #18274 won't break compatible.

For example, in one cluster, we have two v3.5.X ETCD members and one main-branch(with #18274 patch) ETCD member. In this cluster, we only have one key foo.

// key: "foo" // modified: 9 // generations: // {{9, 0}[1]} // {{5, 0}, {6, 0}, {7, 0}, {8, 0}(t)[4]} // {{2, 0}, {3, 0}, {4, 0}(t)[3]}

First compaction is on revision {Main: 4}.

In v3.5.X member // key: "foo" // modified: 9 // generations: // {{9, 0}[1]} // {{5, 0}, {6, 0}, {7, 0}, {8, 0}(t)[4]} in main-branch(with #18274 patch) member // key: "foo" // modified: 9 // generations: // {{9, 0}[1]} // {{5, 0}, {6, 0}, {7, 0}, {8, 0}(t)[4]} // {4, 0}(t)[3]}

If HashKV is to have hash value on Revision{Main: 8}, based on v3.5.x, the hash result will involve the following keys

Revision{Main: 5}

Revision{Main: 6}

Revision{Main: 7}

Revision{Main: 8}

But for main-branch(with #18274 patch), it could compute the Revision{Main: 4} for HashKV result. It's unexpected, which could cause data corruption alert.

So, I use OnlyOneKey to represent this case. Sorry It's confusing.
However, I don't want to involve too many detail in here because it seems reasonable to have different key set to test.

serathius · 2024-07-29T08:30:11Z

tests/e2e/hashkv_test.go

+
+				compactedOnRev := dataset.tombstones[0]
+				if !tt.compactedOnTombstoneRev {
+					compactedOnRev += 3


What's the significance of number 3 here? Can we create a local const? Like numberOfRevisions or emptyWrites.

3 is random number I use to pick up a non-tombstone revision for compaction.
As your suggestion, I use afterWrites variable for that. Please take a look.

ahrtr · 2024-07-29T08:29:40Z

tests/e2e/hashkv_test.go

+	if clusVersion != fe2e.CurrentVersion {
+		if !fileutil.Exist(fe2e.BinPath.EtcdLastRelease) {
+			t.Skipf("%q does not exist", fe2e.BinPath.EtcdLastRelease)
+		}
+	}
+
+	ctx := context.Background()
+
+	fe2e.BeforeTest(t)


These should be in the test case directly instead of a helper function.

I don't think we need newClusterForHashKV at all.

OK. I removed it.

ahrtr · 2024-07-29T08:31:52Z

tests/e2e/hashkv_test.go

+}
+
+type datasetInfo struct {
+	keys map[string]struct{}


You just populate the data for keys, but it isn't used at all.

We can remove datasetInfo, and just return tombstones and latestRev in the functions of populating data.

ahrtr · 2024-07-29T08:36:16Z

tests/e2e/hashkv_test.go

+				t.Logf("HashKV on rev=%d", hashKVOnRev)
+				resp, err := cli.HashKV(ctx, hashKVOnRev)
+				require.NoError(t, err)
+
+				require.Len(t, resp, 3)
+				require.True(t, resp[0].Hash != 0)
+				t.Logf("One Hash value is %d", resp[0].Hash)
+
+				require.Equal(t, resp[0].Hash, resp[1].Hash)
+				require.Equal(t, resp[1].Hash, resp[2].Hash)


minor comment, we can add a helper function something verifyConsistentHashAcrossAllMembers

serathius · 2024-07-29T08:40:58Z

tests/e2e/hashkv_test.go

+	"go.etcd.io/etcd/client/pkg/v3/fileutil"
+	clientv3 "go.etcd.io/etcd/client/v3"
+	"go.etcd.io/etcd/tests/v3/framework/config"
+	fe2e "go.etcd.io/etcd/tests/v3/framework/e2e"


Why not just e2e? All other test just import this as e2e.

serathius · 2024-07-29T08:52:21Z

tests/e2e/hashkv_test.go

+	lastestRevision int64
+}
+
+// key: "foo"


Thanks for crafting such a meticulous testing scenario, this seems very throughout, however for reviewer it's or any future contributor it might be hard to understand why we picked this exact data shape and no other.

My question would be, how hard would it be to generalize? We know one an exact case that we know that we could break hashKV, but how sure we are that we don't miss another case? Hardcoding a exact scenario protects us against this exact mistake, but can we be sure there will not be more such cases in the future? Can we be sure that HashKV, is resilient enough?

Have you heard about property testing? https://www.mayhem.security/blog/what-is-property-based-testing. For me the etcd should always hold property that no matter the type of write, no matter the compaction revision, no matter the etcd version, no matter the revision, HashKV should always be the same on all cluster members.

I think it would be ok to merge those exact scenarios, but I also we should think how we can generalize it. For example, instead of checking hashKV on one revision, test all of them in for loop.

instead of checking hashKV on one revision, test all of them in for loop.

Good point. Will simplify data generator.

I follow @ahrtr suggestion with using populateData. Please take a look.

ahrtr · 2024-07-29T09:28:21Z

tests/e2e/hashkv_test.go

+// generations:
+//
+//	{34, 1}
+func newTestDatasetInCluster(t *testing.T, clus *fe2e.EtcdProcessCluster, cliCfg fe2e.ClientConfig, onlyOneKey bool) *datasetInfo {


It's unnecessarily complicated.

Please consider a function something like below,

// populateData populates some sample data, and return a slice of tombstone revisions and the latest revision func populateData(t *testing.T, clus *fe2e.EtcdProcessCluster, clientCfg fe2e.ClientConfig, keys []string) ([]int64, int64) { c := newClient(t, clus.EndpointsGRPC(), clientCfg) ctx := context.Background() totalOperations := 40 var ( tombStoneRevs []int64 latestRev int64 ) deleteStep := 10 // submit a delete operation on every 10 operations for i := 1; i <= totalOperations; i++ { if i%deleteStep == 0 { t.Logf("Deleting key=%s", keys[0]) // Only delete the first key for simplicity resp, derr := c.Delete(ctx, keys[0]) require.NoError(t, derr) latestRev = resp.Header.Revision tombStoneRevs = append(tombStoneRevs, resp.Header.Revision) continue } value := fmt.Sprintf("%d", i) var ops []clientv3.Op for _, key := range keys { ops = append(ops, clientv3.OpPut(key, value)) } t.Logf("Writing keys: %v, value: %s", keys, value) resp, terr := c.Txn(ctx).Then(ops...).Commit() require.NoError(t, terr) require.True(t, resp.Succeeded) require.Len(t, resp.Responses, len(ops)) latestRev = resp.Header.Revision } return tombStoneRevs, latestRev }

Thanks. I used your code in latest commit. Please take a look.

ahrtr · 2024-07-29T15:38:03Z

tests/e2e/hashkv_test.go

+	require.Len(t, resp, 3)
+	require.True(t, resp[0].Hash != 0)
+	t.Logf("One Hash value is %d", resp[0].Hash)
+
+	require.Equal(t, resp[0].Hash, resp[1].Hash)
+	require.Equal(t, resp[1].Hash, resp[2].Hash)


Suggested change

require.Len(t, resp, 3)

require.True(t, resp[0].Hash != 0)

t.Logf("One Hash value is %d", resp[0].Hash)

require.Equal(t, resp[0].Hash, resp[1].Hash)

require.Equal(t, resp[1].Hash, resp[2].Hash)

require.Greater(t, len(resp), 1)

require.True(t, resp[0].Hash != 0)

t.Logf("One Hash value is %d", resp[0].Hash)

for i := 1; i < len(resp); i++ {

require.Equal(t, resp[0].Hash, resp[i].Hash)

}

thanks for the suggestion. updated

ahrtr · 2024-07-29T15:45:50Z

tests/e2e/hashkv_test.go

+				// If compaction revision is not tombstone, select revision after 3 writes from first tombstone.
+				// And ensure it's not the following tombstone.
+				const afterWriters = int64(3)
+				if !compactedOnTombstoneRev {
+					compactedOnRev += afterWriters
+					require.True(t, tombstoneRevs[1] > compactedOnRev)
+				}


The 3 may cause unnecessary confusion. We don't care about the number at all, we just need to ensure the compaction revision isn't a tombstone.

Suggested change

// If compaction revision is not tombstone, select revision after 3 writes from first tombstone.

// And ensure it's not the following tombstone.

const afterWriters = int64(3)

if !compactedOnTombstoneRev {

compactedOnRev += afterWriters

require.True(t, tombstoneRevs[1] > compactedOnRev)

}

// If compaction revision isn't a tombstone, select a revision in the middle of two tombstones.

if !compactedOnTombstoneRev {

compactedOnRev = (tombstoneRevs[0] + tombstoneRevs[1]) / 2

require.True(t, tombstoneRevs[0] < compactedOnRev && compactedOnRev < tombstoneRevs[1])

}

ahrtr · 2024-07-29T15:46:36Z

tests/e2e/hashkv_test.go

+
+				tombstoneRevs, lastestRev := populateDataForHashKV(t, clus, cfg.Client, scenario.keys)
+
+				compactedOnRev := tombstoneRevs[0]


Can we also add a scenario which compact the last tombstone tombstoneRevs[len(tombstoneRevs) - 1]?

Added TestVerifyHashKVAfterCompactionOnLastTombstone_MixVersions

ahrtr · 2024-07-30T12:57:17Z

tests/e2e/hashkv_test.go

+				// If compaction revision is not tombstone, select revision after 3 writes from first tombstone.
+				// And ensure it's not the following tombstone.


minor comment

Suggested change

// If compaction revision is not tombstone, select revision after 3 writes from first tombstone.

// And ensure it's not the following tombstone.

// If compaction revision isn't a tombstone, select a revision in the middle of two tombstones.

my mistake. Updated

Signed-off-by: Wei Fu <fuweid89@gmail.com>

ahrtr · 2024-07-30T19:48:25Z

Ideally, we should consolidate the two cases, TestVerifyHashKVAfterCompact and TestVerifyHashKVAfterCompactionOnLastTombstone_MixVersions, into one. But I won't insist on that because the PR is already good enough.

ahrtr

LGTM

Nice work, thanks @fuweid

serathius

Looks good to give us confidence in fixing the tombstone compaction bug. Thanks @fuweid awesome work.

I think we should also consider generalizing it by adding hashKV checking to robustness tests.

k8s-ci-robot · 2024-07-31T07:34:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, fuweid, serathius

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:

~~OWNERS~~ [ahrtr,serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahrtr · 2024-07-31T08:52:15Z

tests/e2e/hashkv_test.go

+				// If compaction revision isn't a tombstone, select a revision in the middle of two tombstones.
+				if !compactedOnTombstoneRev {
+					compactedOnRev = (tombstoneRevs[0] + tombstoneRevs[1]) / 2
+					require.Greater(t, tombstoneRevs[1], compactedOnRev)


nit: It seems we partially missed the comment #18369 (comment)

require.True(t, tombstoneRevs[0] < compactedOnRev && compactedOnRev < tombstoneRevs[1])

Follow-up: etcd-io#18369 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid · 2024-08-01T10:03:05Z

Thanks @ahrtr @serathius for the review. I file pr #18387 to fix the comment #18369 (comment).

fuweid · 2024-08-21T13:06:23Z

ping @ahrtr could we remove label backport/v3.4? The #18476 covers mixed cluster versions [3.4 and 3.5].

ahrtr · 2024-08-21T13:25:03Z

ping @ahrtr could we remove label backport/v3.4? The #18476 covers mixed cluster versions [3.4 and 3.5].

Removed.

I was thinking the e2e test cases should also support different patch versions, e.g 3.4.33 vs 3.4.34+, or 3.5.15 vs 3.5.16+. But I do not see a simple/maintainable way to do it.

k8s-ci-robot added area/testing size/L labels Jul 26, 2024

fuweid force-pushed the add-hashkv-test branch from cfea1fe to a48b08b Compare July 26, 2024 11:43

fuweid commented Jul 26, 2024

View reviewed changes

fuweid changed the title ~~tests/e2e: add new test cases related to HashKV~~ tests/e2e: add test cases related to HashKV Jul 26, 2024

fuweid force-pushed the add-hashkv-test branch from a48b08b to f2636ee Compare July 26, 2024 11:54

serathius reviewed Jul 26, 2024

View reviewed changes

ahrtr self-requested a review July 26, 2024 13:08

serathius reviewed Jul 29, 2024

View reviewed changes

ahrtr reviewed Jul 29, 2024

View reviewed changes

serathius reviewed Jul 29, 2024

View reviewed changes

ahrtr reviewed Jul 29, 2024

View reviewed changes

fuweid force-pushed the add-hashkv-test branch from bcbe3ca to 2345b17 Compare July 29, 2024 12:49

ahrtr reviewed Jul 29, 2024

View reviewed changes

fuweid force-pushed the add-hashkv-test branch 2 times, most recently from 34f81e7 to c118987 Compare July 30, 2024 11:06

ahrtr reviewed Jul 30, 2024

View reviewed changes

tests/e2e: add test cases related to HashKV

6f93af8

Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid force-pushed the add-hashkv-test branch from c118987 to 6f93af8 Compare July 30, 2024 13:02

ahrtr approved these changes Jul 30, 2024

View reviewed changes

k8s-ci-robot added the approved label Jul 30, 2024

serathius approved these changes Jul 31, 2024

View reviewed changes

serathius merged commit 739a9b6 into etcd-io:main Jul 31, 2024
43 checks passed

serathius mentioned this pull request Jul 31, 2024

Add hashKV checking to robustness test #18386

Open

ahrtr reviewed Jul 31, 2024

View reviewed changes

fuweid added a commit to fuweid/etcd that referenced this pull request Aug 1, 2024

tests/e2e: check compactOnRev > tombstoneRevs[0]

182639c

Follow-up: etcd-io#18369 (comment) Signed-off-by: Wei Fu <fuweid89@gmail.com>

fuweid mentioned this pull request Aug 1, 2024

tests/e2e: check compactOnRev > tombstoneRevs[0] #18387

Merged

fuweid deleted the add-hashkv-test branch August 1, 2024 10:03

ahrtr added backport/v3.4 backport/v3.5 labels Aug 16, 2024

ahrtr mentioned this pull request Aug 16, 2024

*: keep tombstone if revision == compactAtRev #18274

Merged

fuweid mentioned this pull request Aug 21, 2024

[3.5] tests/e2e: add test cases related to HashKV #18476

Merged

ahrtr removed the backport/v3.4 label Aug 21, 2024

fuweid mentioned this pull request Aug 22, 2024

Watch dropping an event when compacting on delete #18089

Open

4 tasks

	func (h *kvHasher) WriteKeyValue(k, v []byte) {
	kr := BytesToRev(k)
	upper := Revision{Main: h.revision + 1}
	if !upper.GreaterThan(kr) {
	return
	}
	lower := Revision{Main: h.compactRevision + 1}
	// skip revisions that are scheduled for deletion
	// due to compacting; don't skip if there isn't one.
	if lower.GreaterThan(kr) && len(h.keep) > 0 {
	if _, ok := h.keep[kr]; !ok {
	return
	}
	}
	h.hash.Write(k)
	h.hash.Write(v)
	}


		tombstoneRevs, lastestRev := populateDataForHashKV(t, clus, cfg.Client, scenario.keys)

		compactedOnRev := tombstoneRevs[0]

		// If compaction revision is not tombstone, select revision after 3 writes from first tombstone.
		// And ensure it's not the following tombstone.

	// If compaction revision is not tombstone, select revision after 3 writes from first tombstone.
	// And ensure it's not the following tombstone.
	// If compaction revision isn't a tombstone, select a revision in the middle of two tombstones.

tests/e2e: add test cases related to HashKV #18369

tests/e2e: add test cases related to HashKV #18369

Conversation

fuweid commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Jul 26, 2024

jmhbnz commented Jul 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Jul 30, 2024

ahrtr left a comment

Choose a reason for hiding this comment

serathius left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 31, 2024

Choose a reason for hiding this comment

fuweid commented Aug 1, 2024

fuweid commented Aug 21, 2024

ahrtr commented Aug 21, 2024

ahrtr Jul 29, 2024 •

edited

Loading