tests/robustness: Split model code into deterministic and non-deterministic #15819

serathius · 2023-05-03T18:09:14Z

Big refactor to separate deterministic etcd model (assumes all request succeed) and non-deterministic (considers failed request both persisted and not). This way a non-deterministic model just layer above deterministic one.

cc @ahrtr @ptabor

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>

serathius · 2023-05-04T14:18:57Z

PTAL @ahrtr
This refactor makes code much clearer, however it's nightmare to rebase against.

tests/robustness/model/base.go

tests/robustness/model/model.go

tests/robustness/model/base.go

ahrtr · 2023-05-05T03:01:25Z

tests/robustness/model/model.go

-// applyFailedRequest handles a failed requests, one that it's not known if it was persisted or not.
-func applyFailedRequest(states PossibleStates, request EtcdRequest) PossibleStates {
+// stepFailedRequest duplicates number of states by considering request persisted and lost.
+func (states nonDeterministicState) stepFailedRequest(request EtcdRequest) nonDeterministicState {


You want change the receiver's state, but it isn't a pointer receiver. So a function makes more sense than a method here. This comment applies the stepSuccessfulRequest as well.

I think it's useful for clarity to have a both models implement Step method. It makes it more natural to understand when compared to have stepDeterministicModel and stepNonDeterministicModel.

Fact that I don't use pointer receiver and return new state is just a design decision that state is non mutable. I think it's ok that methods to be non mutable.

ahrtr · 2023-05-05T03:08:00Z

This's a huge PR, and it's hard to carefully review each line of code change. Please try to breakdown such huge PR as possible as you can next time.

I will approve this PR for now given:

It's just test code;
You have deep understanding on the robustness test;
This PR will definitely conflict with any other PRs which update robustness test, and you or others will have to do the boring rebasing.

ahrtr · 2023-05-05T03:09:14Z

Before I approve this PR, please other members also take a look. cc @chaochn47 @fuweid @jmhbnz @ptabor if possible.

lavacat · 2023-05-05T05:50:51Z

tests/robustness/model/model.go

-func applyFailedRequest(states PossibleStates, request EtcdRequest) PossibleStates {
+// stepFailedRequest duplicates number of states by considering request persisted and lost.
+func (states nonDeterministicState) stepFailedRequest(request EtcdRequest) nonDeterministicState {
+	newStates := make(nonDeterministicState, 0, len(states)*2)


you are doubling number of states, is there a scenario that the number will grow very quickly and checker won't be able to handle it?

I guess stepSuccessfulRequest would remove possible states. One successful response should clean up all previous possible states generated back to back.

You are both right. NonDeterministic model can grow exponentially with number of operations. This however mostly happens on failed/error (wanna guess when it can happen on request success?). As @chaochn47 mentioned successful requests will cleanup previous possible states.

So the problem is that we should avoid long sequences of failed requests as they cause complexity to explode. I have mitigated the problem by introducing history patching #15078 #15598

lavacat · 2023-05-05T05:53:48Z

tests/robustness/model/model.go

 		if !reflect.DeepEqual(newState, s) {
-			states = append(states, newState)


nit: I see that the check was added in different PR, but can you add a comment why we need !reflect.DeepEqual(newState, s)

I guess it is just used to avoid same states. Please confirm.

If the request doesn't impact the state then there is no reason to fork it. Example is failed defrag request.

lavacat · 2023-05-05T05:55:44Z

tests/robustness/model/history.go

@@ -295,11 +295,15 @@ func (h *AppendableHistory) AppendDefragment(start, end time.Duration, resp *cli
 		h.appendFailed(request, start, err)
 		return
 	}
+	var revision int64


adding revision to defragmentResponse is nice to have, not critical for this PR. I that correct? Just trying to understand

It is based on other code changes due to some shortcuts I took. By separating models I separate EtcdResponse and EtcdNonDeterministicResponse which means that now EtcdResponse needs to have accurate Revision, which means that I need to make sure that defragmentResponse returns response with accurate revision.

chaochn47 · 2023-05-05T06:43:40Z

LGTM, It's a clean refactor from my point of view. The changed lines look scary though. No regression spotted from my eyes.

tests/robustness/model/base.go

chaochn47 · 2023-05-05T05:27:00Z

tests/robustness/model/base.go

+		if response.Txn.TxnResult {
+			return state
+		}


For my understanding, could you please explain why the initial state is empty if TxnResult is true?

I understand it is the same as before.

Ehm, embarrassing story, apparently the old me decided that TxnResult = !success, because I wanted default Txn struct to reflect transaction success.

chaochn47 · 2023-05-05T05:37:14Z

tests/robustness/model/base.go

+	newKVs := map[string]ValueRevision{}
+	for k, v := range s.KeyValues {
+		newKVs[k] = v
+	}
+	s.KeyValues = newKVs


It seems redundant between newKVs and s.KeyValues. Could you please clarify what's the usage of newKVs?

Avoid modifying the s.KeyValues, I need to copy whole map. Would love to get snapshotable map to clean this up.

chaochn47 · 2023-05-05T05:52:49Z

tests/robustness/model/base.go

+	case LeaseRevoke:
+		//Delete the keys attached to the lease
+		keyDeleted := false
+		for key := range s.Leases[request.LeaseRevoke.LeaseID].Keys {


For my understanding, lease in robustness base model has indefinite TTL, correct?

Yes for now we don't test lease expiration by time, just by revoke call.

chaochn47 · 2023-05-05T05:56:51Z

tests/robustness/model/base.go

+}
+
+type TxnResponse struct {
+	TxnResult bool


It would be better if you can document the meaning of this field in future PRs. Thanks ~

Will fix it in following PR, sorry for this abomination in naming.

chaochn47 · 2023-05-05T06:38:26Z

tests/robustness/model/model.go

 		if !reflect.DeepEqual(newState, s) {
-			states = append(states, newState)


I guess it is just used to avoid same states. Please confirm.

chaochn47 · 2023-05-05T06:41:06Z

tests/robustness/model/model.go

-func applyFailedRequest(states PossibleStates, request EtcdRequest) PossibleStates {
+// stepFailedRequest duplicates number of states by considering request persisted and lost.
+func (states nonDeterministicState) stepFailedRequest(request EtcdRequest) nonDeterministicState {
+	newStates := make(nonDeterministicState, 0, len(states)*2)


I guess stepSuccessfulRequest would remove possible states. One successful response should clean up all previous possible states generated back to back.

serathius · 2023-05-05T10:24:04Z

Sorry for large PR, I didn't have time to split it and was worried that it would only make review take longer. As code structure clarifies it should be easier to make small independent changes.

FYI I'm pretty sure that this change is correct as etcd model have absurd number of tests and any error would result would immediately result in robustness tests failing.

Main motivation for this was exponential growth of tests for non-deterministic model. If you look through scenarios I test every combination of requests both their success and failure. Separating deterministic model tests allows me to just implement tests for request success.

…nistic Signed-off-by: Marek Siarkowicz <siarkowicz@google.com> Co-authored-by: Benjamin Wang <wachao@vmware.com> Co-authored-by: chao <54131596+chaochn47@users.noreply.github.com>

ahrtr · 2023-05-05T10:40:45Z

@serathius Please ping me if you resolve all comments, I will take another look so that this PR can merged today.

serathius · 2023-05-05T11:10:40Z

I resolved all comments, however I didn't mark all of them as "done", as some of them included questions and I want to give people chance to read answers.

serathius force-pushed the robustness-non-deterministic branch 2 times, most recently from 37b2d89 to 49f84cb Compare May 3, 2023 18:45

tests/robustness: Separate describe model functions to dedicated file

cfe1542

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>

serathius force-pushed the robustness-non-deterministic branch from 49f84cb to 9965bc1 Compare May 4, 2023 12:30

ahrtr reviewed May 5, 2023

View reviewed changes

lavacat reviewed May 5, 2023

View reviewed changes

chaochn47 reviewed May 5, 2023

View reviewed changes

tests/robustness: Split model code into deterministic and non-determi…

92366a5

…nistic Signed-off-by: Marek Siarkowicz <siarkowicz@google.com> Co-authored-by: Benjamin Wang <wachao@vmware.com> Co-authored-by: chao <54131596+chaochn47@users.noreply.github.com>

serathius force-pushed the robustness-non-deterministic branch from 62c69ae to 92366a5 Compare May 5, 2023 10:25

ahrtr approved these changes May 5, 2023

View reviewed changes

serathius merged commit 79eabc1 into etcd-io:main May 5, 2023

serathius deleted the robustness-non-deterministic branch June 15, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/robustness: Split model code into deterministic and non-deterministic #15819

tests/robustness: Split model code into deterministic and non-deterministic #15819

serathius commented May 3, 2023

serathius commented May 4, 2023

ahrtr May 5, 2023

serathius May 5, 2023

ahrtr commented May 5, 2023

ahrtr commented May 5, 2023

lavacat May 5, 2023

chaochn47 May 5, 2023

serathius May 5, 2023

lavacat May 5, 2023

chaochn47 May 5, 2023

serathius May 5, 2023

lavacat May 5, 2023

serathius May 5, 2023

chaochn47 commented May 5, 2023

chaochn47 May 5, 2023 •

edited

Loading

serathius May 5, 2023 •

edited

Loading

chaochn47 May 5, 2023

serathius May 5, 2023

chaochn47 May 5, 2023

serathius May 5, 2023 •

edited

Loading

chaochn47 May 5, 2023

serathius May 5, 2023 •

edited

Loading

chaochn47 May 5, 2023

chaochn47 May 5, 2023

serathius commented May 5, 2023

ahrtr commented May 5, 2023

serathius commented May 5, 2023

		if !reflect.DeepEqual(newState, s) {
		states = append(states, newState)

tests/robustness: Split model code into deterministic and non-deterministic #15819

tests/robustness: Split model code into deterministic and non-deterministic #15819

Conversation

serathius commented May 3, 2023

serathius commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented May 5, 2023

ahrtr commented May 5, 2023

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

chaochn47 commented May 5, 2023

chaochn47 May 5, 2023 • edited Loading

Choose a reason for hiding this comment

serathius May 5, 2023 • 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

serathius May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius commented May 5, 2023

ahrtr commented May 5, 2023

serathius commented May 5, 2023

chaochn47 May 5, 2023 •

edited

Loading

serathius May 5, 2023 •

edited

Loading

serathius May 5, 2023 •

edited

Loading

serathius May 5, 2023 •

edited

Loading