Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional eventsocket Server to ndt-server #396

Merged
merged 9 commits into from
Nov 6, 2023

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Nov 3, 2023

This change adds an eventsocket.Server to the ndt-server for the ndt7 protocol.

With this change, the ndt-server could operate as a replacement for tcp-info's eventsocket for deployments that run with standard sidecar services (e.g. uuid-annotator, or others) but without needing tcp-info to be the event source. In this configuration, the ndt-server itself becomes the event source, limiting events to verified ndt7 measurements (rather than every TCP connection).

For this change to be possible, the ndt7 handler.Handler methods now use a pointer receiver so that the Handler.Events field can be reset after ndt7test.NewNDT7Server creates a server using the default eventsocket.NullServer. As well, we add a handler_integration_test.go to allow import of the existing ndt7test package, which sets up an ndt7 server for testing.

At the same time, I've removed use of the deprecated "io/ioutil" TempDir in favor of t.TempDir().

Default server behavior is unchanged. This change adds a new flag to the ndt-server, -tcpinfo.eventsocket string. We reuse the existing eventsocket flag name, just as the sidecars do, to preserve the symmetry between the serving flag and the client flag names. Since we now have multiple event servers, ideally the eventsocket package would be moved to a generic location such as m-lab/go/eventsocket and imported by tcp-info and ndt-server, using a more general flag name in all cases and sidecars, e.g. -server.eventsocket.


This change is Reviewable

Copy link
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


ndt7/handler/handler.go line 117 at r2 (raw file):

	h.Events.FlowCreated(result.StartTime, data.UUID, id)

	// Guarantee results are written even if test functions panics.

panics?

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

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


ndt7/handler/handler.go line 117 at r2 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

panics?

Thank you. Fixed.

@stephen-soltesz stephen-soltesz merged commit ea3439c into main Nov 6, 2023
5 checks passed
@stephen-soltesz stephen-soltesz deleted the sandbox-soltesz-eventsocket branch November 6, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants