tests/robustness: Add safeguards to client and history

[serathius]

cc @ahrtr @ptabor

[chaochn47]

LGTM.

The CI test was not kicked off due to github outage.

[jmhbnz]

Forgive me if this is a silly question, still learning... Does using a non standard client for robustness create any risk that the robustness tests will not behave as real world etcd will behave and therefore miss potential issues?

[chaochn47]

Good question. From my understanding, current k8s/etcd traffic run in robustness test does not support concurrent read/write for each recording client.

The nuance between robustness test and k8s usage is

1. k8s re-uses the same etcd client per resource type to achieve concurrent r/w.
2. robustness test uses multiple etcd clients to achieve concurrent r/w.

To conform robustness test to k8s usage requires some re-factoring on appending to history sequentially that might complicates the code base. Could you please comment if there is a plan to do that? @serathius

[serathius]

No, this is not a limitation of robustness test, nor k8s client. What I implemented here are safeguards over a naive workaround around limitation of linearizability checking. "There can be only one concurrent request in a request stream". For now I assign to client only one request stream at a time. I could assign each request a unique stream, but that would make visualizations not readable.

[chaochn47]

Thanks for the explanation! That's really helpful. Code usually tells how but not why.

[serathius]

Hoped my comments will help :(
I think the problem is with clash of term client. Etcd uses client as a library to call etcd. For porcupine it's more like a identifier for sequential stream of operations. For example on failed requests we rotate stream id for client, to continue executing requests, because from client perspective we don't know when the requests has finished.

[chaochn47]

@serathius Please bear me with too many question~

1. Why you name it as we rotate stream id, looks like h.idProvider.ClientId() always adds up by 1.
2. What if we keep adding new records into the same stream and what will break?

[chaochn47]

Just to correlate between statement and code for other readers.

h.id is updated with the next stream ID. (I guess porcupine calls it client ID but it is actually a stream from visualization).

For porcupine it's more like a identifier for sequential stream of operations. For example on failed requests we rotate stream id for client, to continue executing requests

etcd/tests/robustness/model/history.go

Lines 335 to 346 in 7c68be4

	func (h *AppendableHistory) appendFailed(request EtcdRequest, start time.Duration, err error) {
	h.failed = append(h.failed, porcupine.Operation{
	ClientId: h.id,
	Input: request,
	Call: start.Nanoseconds(),
	Output: failedResponse(err),
	Return: 0, // For failed writes we don't know when request has really finished.
	})
	// Operations of single client needs to be sequential.
	// As we don't know return time of failed operations, all new writes need to be done with new client id.
	h.id = h.idProvider.ClientId()
	}

[serathius]

@serathius Please bear me with too many question~

1. Why you name it as we rotate stream id, looks like h.idProvider.ClientId() always adds up by 1.

Yep, by rotate I mean pick another unique value. Porcupine requires it to be a counter from 0 to render nicely.

2. What if we keep adding new records into the same stream and what will break?

I think you might get incorrect result from porcupine linearizer and porcupine visualization will render two blocks overlapping making it not readable. This was a big problem with failed requests that took me a long time to figure out. See anishathalye/porcupine#8 (comment), #14880

[serathius]

Tried to document answers in #15880

[ahrtr]

LGTM with a couple of minor comments.