linearizability tests - Add support for delete api #14802

geetasg · 2022-11-17T22:59:33Z

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

codecov-commenter · 2022-11-17T23:30:03Z

Codecov Report

Merging #14802 (e1ea963) into main (6a156bd) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14802      +/-   ##
==========================================
- Coverage   75.64%   75.44%   -0.20%     
==========================================
  Files         457      457              
  Lines       37423    37416       -7     
==========================================
- Hits        28309    28230      -79     
- Misses       7347     7403      +56     
- Partials     1767     1783      +16

Flag	Coverage Δ
all	`75.44% <ø> (-0.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/etcdserver/api/v3rpc/util.go	`67.74% <0.00%> (-12.91%)`	⬇️
server/proxy/grpcproxy/register.go	`69.76% <0.00%> (-11.63%)`	⬇️
client/v3/leasing/util.go	`91.66% <0.00%> (-6.67%)`	⬇️
client/pkg/v3/fileutil/purge.go	`68.85% <0.00%> (-6.56%)`	⬇️
client/v3/namespace/watch.go	`87.87% <0.00%> (-6.07%)`	⬇️
client/v3/concurrency/session.go	`88.63% <0.00%> (-4.55%)`	⬇️
server/etcdserver/api/v3rpc/watch.go	`84.12% <0.00%> (-3.50%)`	⬇️
server/proxy/grpcproxy/watch.go	`93.64% <0.00%> (-2.90%)`	⬇️
server/etcdserver/api/v3election/election.go	`66.66% <0.00%> (-2.78%)`	⬇️
client/v3/experimental/recipes/key.go	`75.34% <0.00%> (-2.74%)`	⬇️
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ahrtr · 2022-11-18T05:48:08Z

Please mark it ready for review when it's ready.

serathius · 2022-11-18T11:00:49Z

tests/linearizability/model.go

@@ -119,6 +129,9 @@ func initState(request etcdRequest, response etcdResponse) EtcdState {
 		} else {
 			state.FailedWrites[request.putData] = struct{}{}
 		}
+	case Delete:
+		//TODO should state have 'deleted' field?


Not needed.

tests/linearizability/model.go

tests/linearizability/model_test.go

serathius · 2022-11-18T11:34:29Z

You still need to integrate tests into ./tests/linearizability/traffic.go.
You can just change

DefaultTraffic Traffic = readWriteSingleKey{key: "key", writes: []opChance{{operation: Put, chance: 100}}}

to

DefaultTraffic Traffic = readWriteSingleKey{key: "key", writes: []opChance{{operation: Put, chance: 90}, {operation: Delete, chance: 10}}}

And add a case Delete to switch within (t readWriteSingleKey) Writefunction to that will callclient.Delete`.

serathius · 2022-11-18T11:35:40Z

We are also missing tests for failed Delete request, but those are hard to write due to how model works. I can help and add them in later PR.

tests/linearizability/model.go

serathius · 2022-11-29T09:40:43Z

Overall it looks good, just need to handle delete errors. Great job!

serathius · 2022-11-29T09:45:30Z

Checked the linearizability failure. As expected it's an issue with handling Delete error.

Linearizability tests reported error as Get returned empty string. There is a missing Delete request in the graph. We need to also track failed requests as fail can a network error from perspective from client, but etcd managed to persist it.

tests/linearizability/client.go

serathius · 2022-11-30T10:40:46Z

Can you fix DCO check?
you just need to do add a line Signed-off-by: X to your commit message.

This can be done automatically on latest commit via git commit --amend --signoff.

As there are some more commits to be updated you should be able to do that with git fetch upstream && git checkout origin/delete_api && git rebase origin/main --signoff && git push origin HEAD:delete_api -f assuming name of remotes is upstream for the etcd repo, and origin for your fork.

serathius · 2022-11-30T13:16:31Z

I started testing the model and found issues related to put request failed but was persisted and following delete checks if object was deleted. Will keep you posted if I figure out a solution.

Signed-off-by: Geeta Gharpure <[email protected]>

Signed-off-by: Marek Siarkowicz <[email protected]>

geetasg · 2022-12-01T18:37:27Z

notes -
test using

   pushd tools/mod; go install go.etcd.io/gofail; popd
    mkdir -p /tmp/linearizability
    FAILPOINTS=true make build
    cat server/etcdserver/raft.fail.go
    EXPECT_DEBUG=true GO_TEST_FLAGS=-v RESULTS_DIR=/tmp/linearizability make test-linearizability

or -
make gofail-enable
make build
GO_TEST_FLAGS=-v make test-linearizability

geetasg · 2022-12-05T04:07:46Z

Marking ready for review based on checked - https://github.com/etcd-io/etcd/pull/14802/checks
Please take a look @serathius @ahrtr

ahrtr · 2022-12-06T07:44:51Z

tests/linearizability/model.go

+
+func stepDelete(state EtcdState, request etcdRequest, response etcdResponse) (bool, EtcdState) {
+	if response.err != nil {
+		state.FailedWrites[""] = struct{}{}


Suggested change

state.FailedWrites[""] = struct{}{}

state.FailedWrites[state.Value] = struct{}{}

I think we need to record whatever value in state in FailedWrites.

ahrtr

Overall looks good to me with a minor comment.

Thank you @geetasg

This PR has some conflict with #14880. I tend to let this PR in firstly, and then update 14880 afterwards.

serathius · 2022-12-06T08:03:14Z

FYI, this PR doesn't implement correct handling of failed delete requests. I looked into fixing this, however code got so complicated that I decided to change our approach of handling errors. This PR without #14880 will introduce flakiness.

I would be ok to merge this PR first assuming that we will merge #14880 right after.

serathius · 2022-12-06T08:30:55Z

All PRs approved, let's start merging.

ahrtr marked this pull request as draft November 18, 2022 05:47