The server now speaks non-ws NDT with JSON messages. #52

pboothe · 2019-01-04T22:58:13Z

This change is

coveralls · 2019-01-04T23:05:20Z

Pull Request Test Coverage Report for Build 323

219 of 276 (79.35%) changed or added relevant lines in 6 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.0%) to 65.527%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ndt-server.go	59	60	98.33%
legacy/s2c/s2c.go	19	23	82.61%
legacy/testresponder/testresponder.go	11	15	73.33%
legacy/testrunner.go	65	77	84.42%
legacy/protocol/protocol.go	52	88	59.09%

Files with Coverage Reduction	New Missed Lines	%
ndt-server.go	1	87.14%
legacy/s2c/s2c.go	1	78.15%

Totals
Change from base Build 319:	1.0%
Covered Lines:	709
Relevant Lines:	1082

💛 - Coveralls

bassosimone · 2019-01-07T15:27:00Z

It seems regress tests are failing because of:

./web100clt

That is, the legacy client is missing. We probably don't want to compile it as part of this repository testing process. Maybe we can store it as a binary asset to download for travis?

(If this PR is still WIP, as you asked for no reviewers, please disregard :-)

pboothe · 2019-01-07T19:52:03Z

Still WIP

When it becomes reviewable, it will have reviewers assigned. :)

The command-line client doesn't work - it fails at the NDT handshake. I do not (yet) know why.

We need to put in some metrics gathering.

Test the string conversion method.

brittle test.

bassosimone · 2019-02-23T10:22:44Z

legacy/c2s/c2s.go

+		// is okay to break them.
+		ws.DrainUntil(time.Now().Add(5 * time.Second))
+		ws.Close()
+	}()


stephen-soltesz

My primary concerns are with the ramifications of the raw-to-ws pass through on performance, data collection, and monitoring.

Reviewed 2 of 10 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe and @stephen-soltesz)

ndt-server.go, line 135 at r1 (raw file):

	// The legacy protocol serving non-HTTP-based tests - forwards to WS-based
	// server if the first three bytes are "GET".
	legacyRawServer := legacy.BasicServer{

There is a lot going on in setup; we're starting 5 services.

I like the flag naming convention you've adopted, i.e. "legacy, legacy_ws, legacy_wss". The variables below do not follow this consistently. For example, it's hard to tell that "legacyServer" below is actually the setup for the "legacy_ws" port.

It looks like the legacy server is not instrumented with prometheus metrics. I think we will want to be able to measure how often the raw-to-ws proxy is operating.

TestDockerfile, line 2 at r1 (raw file):

FROM golang:1.11 as build
COPY --from=pboothe/test-runner /ndt/src/web100clt /bin

This should not reference personal container images. How was pboothe/test-runner built? How can we rebuild it if needed? Can we include that docker file in the ndt-server also?

legacy/testrunner.go, line 151 at r1 (raw file):

	}
	if string(lead) == "GET" {
		// Forward HTTP-like handshakes to the HTTP server. Note that this does NOT

Please ensure that we can count the relative frequency of 3001-raw vs 3001-ws clients using prometheus metrics.

If we never expect direct connections to 3002 then we could use the difference between connections to 3001 - connections to 3002 using the http handler instruments.

legacy/testrunner.go, line 155 at r1 (raw file):

		// HTTP server itself opens the testing port, and that server will not use
		// this TCP proxy.
		fwd, err := net.Dial("tcp", s.HTTPAddr)

Is it too late in the protocol negotiation to issue a 301 Redirect?

legacy/testrunner.go, line 164 at r1 (raw file):

		// Copy the input channel.
		go func() {
			io.Copy(fwd, input)

What impact on performance does this have?

Does this mean tcpinfo will collect two traces?

legacy/protocol/protocol.go, line 180 at r1 (raw file):

	}
	if MessageType(inbuff[0]) != expectedType {
		return nil, fmt.Errorf("Read wrong message type. Wanted %s, got %s", expectedType, MessageType(inbuff[0]))

%q?

legacy/s2c/s2c.go, line 25 at r1 (raw file):

// Result is the result object returned to S2C clients as JSON.
type Result struct {
	ThroughputValue  string

Were the types here totally wrong before? This change must impact clients that parse the value, no? A rate in json quotes is very different from not.

legacy/s2c/s2c.go, line 133 at r1 (raw file):

	}
	protocol.SendJSONMessage(protocol.TestFinalize, "", ws)
	r.Response <- 0.0

Please add comment that this is telling the client thread to shutdown. Maybe a constant like testresponder.Shutdown?

legacy/testresponder/testresponder.go, line 28 at r1 (raw file):

const (
	// RawNoJSON NDT tests correspond to the code integrated into the uTorrent client.
	RawNoJSON = ServerType(iota)

RawNoJSON is not used anywhere, is that expected?

pboothe

Performance isn't an issue - only the control channel is a pass-through. Data collection is a mild irritation - tcp-info will generate two traces for passed-through connections (but again, that's only the control channel so it's okay). Monitoring is a thing I would like to talk more about, probably in person.

Initial comments below. No need to respond yet.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe and @stephen-soltesz)

TestDockerfile, line 2 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

This should not reference personal container images. How was pboothe/test-runner built? How can we rebuild it if needed? Can we include that docker file in the ndt-server also?

Dockerizing old versions of web100clt seems like potentially a lot of work. I'll double-check this to see, but I am worried.

legacy/testrunner.go, line 155 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Is it too late in the protocol negotiation to issue a 301 Redirect?

Interesting idea. I will see.

legacy/testrunner.go, line 164 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

What impact on performance does this have?

Does this mean tcpinfo will collect two traces?

None (the control channel has minimal performance requirements) and yes, but only for the control channel, which is not the connection over which measurements take place.

legacy/testresponder/testresponder.go, line 28 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

RawNoJSON is not used anywhere, is that expected?

Yes. It is the next server type I have to implement and the last NDT client type that exists in the wild. It is the client type for uTorrent. But the current PR felt like enough of a change that i wanted to stop here and get a review.

pboothe

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe and @stephen-soltesz)

TestDockerfile, line 2 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Dockerizing old versions of web100clt seems like potentially a lot of work. I'll double-check this to see, but I am worried.

As you can see, it sucked and I did it. :P

Add libraries back into the docker image

pboothe

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone, @pboothe, and @stephen-soltesz)

TestDockerfile, line 2 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

As you can see, it sucked and I did it. :P

Done.

legacy/testrunner.go, line 164 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

None (the control channel has minimal performance requirements) and yes, but only for the control channel, which is not the connection over which measurements take place.

Done.

legacy/c2s/c2s.go, line 43 at r1 (raw file):

Previously, bassosimone (Simone Basso) wrote…

👍

Done.

legacy/protocol/protocol.go, line 180 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

%q?

Done.

legacy/s2c/s2c.go, line 133 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add comment that this is telling the client thread to shutdown. Maybe a constant like testresponder.Shutdown?

Changed to close()

pboothe · 2019-02-28T15:19:41Z

legacy/s2c/s2c.go, line 25 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Were the types here totally wrong before? This change must impact clients that parse the value, no? A rate in json quotes is very different from not.

The types were wrong before. The old server code to generate the JSON is: https://github.com/m-lab/ndt/blob/master/src/test_s2c_srv.c#L625

It turns out that the web100clt binary expects them to be strings, and javascript clients will silently convert between strings and numbers because javascript is like that. Computers are terrible.

pboothe

PTAL

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone, @pboothe, and @stephen-soltesz)

ndt-server.go, line 135 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

There is a lot going on in setup; we're starting 5 services.

I like the flag naming convention you've adopted, i.e. "legacy, legacy_ws, legacy_wss". The variables below do not follow this consistently. For example, it's hard to tell that "legacyServer" below is actually the setup for the "legacy_ws" port.

It looks like the legacy server is not instrumented with prometheus metrics. I think we will want to be able to measure how often the raw-to-ws proxy is operating.

Done and done.

stephen-soltesz

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @bassosimone)

TestDockerfile, line 2 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Done.

Thank you. This is much better.

legacy/s2c/s2c.go, line 133 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Changed to close()

Even better.

legacy/testrunner.go, line 155 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Interesting idea. I will see.

We discussed offline that while redirect might be the "right thing" we're doing this at all to support legacy clients, which evidently, may not all support redirection [which is ridiculous]. And, we would only ever consider doing something like this for the legacy protocol.

stephen-soltesz

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @bassosimone and @pboothe)

ndt-server_test.go, line 154 at r2 (raw file):

	}{
		// Test legacy raw JSON clients
		{

Please add TODO for web100clt-without-json-support

pboothe

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @bassosimone and @stephen-soltesz)

ndt-server_test.go, line 154 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add TODO for web100clt-without-json-support

Done.

pboothe added Sprint 11 Sprint 23 and removed Sprint 11 labels Jan 9, 2019

pboothe added 3 commits February 20, 2019 17:27

The server now speaks non-ws NDT with JSON messages.

d9f90ef

The command-line client doesn't work - it fails at the NDT handshake. I do not (yet) know why.

NDT-cloud can now speak to legacy clients.

cf7e960

We need to put in some metrics gathering.

Changes

fb36134

pboothe force-pushed the non-ws-with-json branch from e5babda to fb36134 Compare February 20, 2019 22:30

pboothe added 5 commits February 20, 2019 17:31

Accept the last change

4dcdde5

Short tests don't run integration tests.

ee04383

Test the string conversion method.

Verify that protocol messages turn to strings correctly.

b63651d

Legacy tests now happen inside the dockerfile.

e5fdd5e

Clean up comments and travis config

c923e10

pboothe added Sprint 4 2019 labels Feb 22, 2019

pboothe added 9 commits February 22, 2019 16:21

Merge branch 'master' into non-ws-with-json

88da892

Log test successes too.

3a732c9

Added comments and narrowed the exporting of functions.

8a21ed4

The check for hanging goroutines is inherently race-prone and makes a

537e0ab

brittle test.

Eliminate a race condition with c2s sockets.

6a08f79

A better fix for race-prone clients.

6a27aab

Improved comments

a357f6b

Delete goroutine-counter instead of just commenting it out.

de5ac96

Eliminate c2s race condition without introducing a new one

5971bb5

pboothe requested a review from stephen-soltesz February 22, 2019 23:04

pboothe added P1 8 P0 labels Feb 22, 2019

bassosimone reviewed Feb 23, 2019

View reviewed changes

legacy/c2s/c2s.go

// is okay to break them.

ws.DrainUntil(time.Now().Add(5 * time.Second))

ws.Close()

}()

Copy link

Contributor

bassosimone Feb 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

stephen-soltesz requested changes Feb 25, 2019

View reviewed changes

Build web100clt from source.

86c78e1

pboothe commented Feb 25, 2019

View reviewed changes

pboothe added 3 commits February 25, 2019 14:09

%s -> %q

c6772da

Rename things in ndt-server

307c24d

Add libraries back into the docker image

Undo the "cleanup" that incorrectly forwarded ports

ec78ae5

pboothe commented Feb 25, 2019

View reviewed changes

pboothe added 2 commits February 25, 2019 17:54

Close the channel to communicate completion.

ade0e29

liraries -> libraries

d52c23f

Keep track of client metrics.

8e5db54

pboothe commented Mar 1, 2019

View reviewed changes

stephen-soltesz approved these changes Mar 1, 2019

View reviewed changes

Updated comments to explain past and future decisions.

e321530

pboothe commented Mar 1, 2019

View reviewed changes

pboothe merged commit d111cca into master Mar 1, 2019

pboothe deleted the non-ws-with-json branch March 1, 2019 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The server now speaks non-ws NDT with JSON messages. #52

The server now speaks non-ws NDT with JSON messages. #52

pboothe commented Jan 4, 2019 •

edited by stephen-soltesz

Loading

coveralls commented Jan 4, 2019 •

edited

Loading

bassosimone commented Jan 7, 2019 •

edited

Loading

pboothe commented Jan 7, 2019

bassosimone Feb 23, 2019

stephen-soltesz left a comment

pboothe left a comment

pboothe left a comment

pboothe left a comment

pboothe commented Feb 28, 2019

pboothe left a comment

stephen-soltesz left a comment

stephen-soltesz left a comment

pboothe left a comment

The server now speaks non-ws NDT with JSON messages. #52

The server now speaks non-ws NDT with JSON messages. #52

Conversation

pboothe commented Jan 4, 2019 • edited by stephen-soltesz Loading

coveralls commented Jan 4, 2019 • edited Loading

Pull Request Test Coverage Report for Build 323

💛 - Coveralls

bassosimone commented Jan 7, 2019 • edited Loading

pboothe commented Jan 7, 2019

bassosimone Feb 23, 2019

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

pboothe left a comment

Choose a reason for hiding this comment

pboothe left a comment

Choose a reason for hiding this comment

pboothe left a comment

Choose a reason for hiding this comment

pboothe commented Feb 28, 2019

pboothe left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

stephen-soltesz left a comment

Choose a reason for hiding this comment

pboothe left a comment

Choose a reason for hiding this comment

pboothe commented Jan 4, 2019 •

edited by stephen-soltesz

Loading

coveralls commented Jan 4, 2019 •

edited

Loading

bassosimone commented Jan 7, 2019 •

edited

Loading