Add support for bytes limit

[robertodauria]

The bytes querystring parameter allows the client to specify the maximum number of bytes the server will send/receive before terminating the connection.

The limit is only enabled on the server, since the server is almost always guaranteed to have access to the connection's TCPInfo.

The check is implemented in Protocol.sendCounterflow() for upload tests (where it checks TCPInfo.BytesReceived) and in Protocol.sender() for download tests (where it checks TCPInfo.BytesAcked).

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 7353368436

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.6%) to 69.807%

---

Totals
Change from base Build 7117003081:	0.6%
Covered Lines:	867
Relevant Lines:	1242

---

💛 - Coveralls

[cristinaleonr]

Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)

---

cmd/msak-client/client.go line 51 at r1 (raw file):

		},
		NoVerify:   *flagNoVerify,
		BytesLimit: *flagBytesLimit,

nit: ByteLimit is probably more consistent with the comments throughout.

---

internal/handler/handler.go line 146 at r1 (raw file):

				"invalid-byte-limit").Inc()
			log.Info("Received request with an invalid byte limit",
				"source", req.RemoteAddr, "value", bytesLimit)

Is bytesLimit set at this point? Should this be requestBytesLimit?

---

pkg/throughput1/protocol.go line 231 at r1 (raw file):

			// End the test once enough bytes have been received.
			if p.bytesLimit > 0 && m.TCPInfo != nil && m.TCPInfo.BytesReceived >= int64(p.bytesLimit) {

Do we need to cast to int64 for every check?

Should we store the int64 value in a variable, or should bytesLimit be of int64 type?

[cristinaleonr]

@robertodauria Thanks for this change!

I pushed the changes to address the comments in your absence. Feel free to also make changes as you see fit.

I tried running the client to test out the flag using a 150KB limit.

 go run client.go -locate.url=https://locate-dot-mlab-sandbox.appspot.com/v2/nearest/?region=US-NY -streams=1 -bytes=150000000 -duration=1s

It looks like the stream completes successfully, but the last output line says "failed to write prepared message (ctx: 0xc000016140, err: write tcp 192.168.0.223:50674->4.14.3.49:443: use of closed network connection)". Can you let me know if it's expected?

@stephen-soltesz can you please be the reviewer while I'm out?

Thank you both!

Reviewable status: 0 of 1 approvals obtained

---

cmd/msak-client/client.go line 51 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

nit: ByteLimit is probably more consistent with the comments throughout.

Done.

---

internal/handler/handler.go line 146 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Is bytesLimit set at this point? Should this be requestBytesLimit?

Done.

---

pkg/throughput1/protocol.go line 231 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Do we need to cast to int64 for every check?

Should we store the int64 value in a variable, or should bytesLimit be of int64 type?

Done.

[stephen-soltesz]

These changes lgtm -- there is an open question whether polling vs byte counting in the sender is preferable.

Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)

---

pkg/throughput1/protocol.go line 293 at r2 (raw file):

			// End the test once enough bytes have been acked.
			if p.byteLimit > 0 && m.TCPInfo != nil && m.TCPInfo.BytesAcked >= p.byteLimit {

@cristinaleonr will polling the bytes sent meet the requirements of the integrator you're working with? if not, what are the requirements?

[cristinaleonr]

I meant to say -bytes=150000 above.

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

pkg/throughput1/protocol.go line 293 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

@cristinaleonr will polling the bytes sent meet the requirements of the integrator you're working with? if not, what are the requirements?

The requirement is to use small payloads (e.g., 150KB) and test durations (e.g., < 10s) to test speeds up to 20 Mbps.

Testing it out, it does not seem like it will meet the requirements given the sample frequency. For example, using a 150,000 byte limit, the recorded bytes sent under ServerMeasurements ends up being a much larger number (562,567,663).

So, we might need tighter bounds over the bytes sent.

[stephen-soltesz]

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

---

pkg/throughput1/protocol.go line 293 at r2 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

The requirement is to use small payloads (e.g., 150KB) and test durations (e.g., < 10s) to test speeds up to 20 Mbps.

Testing it out, it does not seem like it will meet the requirements given the sample frequency. For example, using a 150,000 byte limit, the recorded bytes sent under ServerMeasurements ends up being a much larger number (562,567,663).

So, we might need tighter bounds over the bytes sent.

I recommend considering only supporting byte limits equal to one of the steps in the cumulative total of all scaled messages sent in the default write loop below, and stopping once that limit is reached. This will be imprecise equal to the overhead of transport layers, but much more predictable than polling.

[cristinaleonr]

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

pkg/throughput1/protocol.go line 293 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I recommend considering only supporting byte limits equal to one of the steps in the cumulative total of all scaled messages sent in the default write loop below, and stopping once that limit is reached. This will be imprecise equal to the overhead of transport layers, but much more predictable than polling.

I updated it to apply the limits to the cumulative total bytes sent in the write loop.

I don't want to be overly restrictive on the supported parameters since clients might want to experiment with different settings.

[stephen-soltesz]

Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)

---

internal/handler/handler.go line 137 at r3 (raw file):

	var byteLimit int
	if requestByteLimit != "" {
		if byteLimit, err = strconv.Atoi(requestByteLimit); err != nil {

For the future, we should probably prohibit negative values as well.

---

pkg/throughput1/protocol.go line 293 at r2 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

I updated it to apply the limits to the cumulative total bytes sent in the write loop.

I don't want to be overly restrictive on the supported parameters since clients might want to experiment with different settings.

Okay, I was imagining that sub-buffer sizes (or, ultimately sub-packet sizes) won't make any performance difference and invite a pseudo-configurability. But, maybe it's a thing to observe experimentally instead!

---

pkg/throughput1/protocol.go line 303 at r3 (raw file):

			bytesSent := int(p.applicationBytesSent.Load())
			if p.byteLimit > 0 && bytesSent >= p.byteLimit {
				p.close(ctx)

Potentially the connection will be closed twice: here and in sendCounterFlow -- @robertodauria is this a problem? What is the preferred close sequence?

[stephen-soltesz]

@robertodauria please confirm these changes LGTY? There's one open question as far as I'm concerned.

Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)

---

pkg/throughput1/protocol.go line 303 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Potentially the connection will be closed twice: here and in sendCounterFlow -- @robertodauria is this a problem? What is the preferred close sequence?

Of course after I send, I see pre-existing double close is possible between sender and sendCounterFlow. If this is fine, then lgtm.

[robertodauria]

It looks like the stream completes successfully, but the last output line says "failed to write prepared message (ctx: 0xc000016140, err: write tcp 192.168.0.223:50674->4.14.3.49:443: use of closed network connection)". Can you let me know if it's expected?

@cristinaleonr It is, in the sense that after control is passed to the gorilla/websocket library by calling WritePreparedMessage() writing the whole message to the network will take some time. In most cases the connection is closed while the library is doing so, which causes that (benign) error.

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

pkg/throughput1/protocol.go line 303 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Of course after I send, I see pre-existing double close is possible between sender and sendCounterFlow. If this is fine, then lgtm.

They do not run at the same time -SenderLoop uses sender while ReceiverLoop uses sendCounterflow.

[stephen-soltesz]

Thank you!

Reviewed 1 of 7 files at r1, 5 of 6 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained

[stephen-soltesz]

@robertodauria would you be willing to prepare an update to k8s-support with this newest version?