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

feat(mads) add support for HTTP long polling #2121

Merged
merged 6 commits into from
Jun 10, 2021
Merged

Conversation

austince
Copy link
Contributor

@austince austince commented Jun 7, 2021

Summary

Adds support for HTTP long polling by making the xds rest server Fetch call block until there is an update, or the operation times out.

Currently, it just uses the configuration to define default timeouts for updates. I've got an open question in envoy
to see if there is a standard way to set this timeout in the REST-JSON spec (see envoyproxy/envoy#16859), but if they
don't get back to us soon we can just add a simple timeout query param or request header.

This will unblock the native Prometheus SD efforts here: prometheus/prometheus#8844

Full changelog

  • Implement a blocking Fetch call using context timeouts
  • Change config option monitoringAssignmentServer.fetchTimeout to monitoringAssignmentServer.defaultFetchTimeout

Issues resolved

n/a

Documentation

n/a – need to finish the Prometheus SD updates before adding docs.

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

@@ -60,8 +59,10 @@ func (s *service) handleDiscovery(req *restful.Request, res *restful.Response) {

discoveryReq.TypeUrl = mads_v1.MonitoringAssignmentType

ctx, cancel := context.WithTimeout(context.Background(), s.config.FetchTimeout)
defer cancel()
timeout := s.config.DefaultFetchTimeout
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once we decide on a way to specify this client-side, all that is needed is to parse it here and pass it along.

Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
Signed-off-by: austin ce <austin.cawley@gmail.com>
@austince austince marked this pull request as ready for review June 9, 2021 15:51
@austince austince requested a review from a team as a code owner June 9, 2021 15:51
@@ -473,7 +473,7 @@ sdsServer:
"KUMA_API_SERVER_AUTH_ALLOW_FROM_LOCALHOST": "false",
"KUMA_MONITORING_ASSIGNMENT_SERVER_GRPC_PORT": "3333",
"KUMA_MONITORING_ASSIGNMENT_SERVER_PORT": "2222",
"KUMA_MONITORING_ASSIGNMENT_SERVER_FETCH_TIMEOUT": "45s",
"KUMA_MONITORING_ASSIGNMENT_SERVER_DEFAULT_FETCH_TIMEOUT": "45s",
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need a line in Upgrade.md for this name change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check, but I don't think this has been included in a release yet. Would we still need one if that is the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, we won't need it if this is a brand new config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed this is not in 1.1.x:

// Monitoring Assignment Discovery Service (MADS) server configuration.
type MonitoringAssignmentServerConfig struct {
// Port of a gRPC server that serves Monitoring Assignment Discovery Service (MADS).
GrpcPort uint32 `yaml:"grpcPort" envconfig:"kuma_monitoring_assignment_server_grpc_port"`
// Interval for re-generating monitoring assignments for clients connected to the Control Plane.
AssignmentRefreshInterval time.Duration `yaml:"assignmentRefreshInterval" envconfig:"kuma_monitoring_assignment_server_assignment_refresh_interval"`
}

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

I think we can merge this. One remark about Configuration renaming that should land in Upgrade.md

@austince
Copy link
Contributor Author

austince commented Jun 10, 2021

Any thoughts on the timeout parameter @nickolaev / @jakubdyszkiewicz?

Since the envoy issue is still silent, I would think either:

  1. A header (x-kuma-fetch-timeout) that holds a GRPC TimeoutValue and TimeoutUnit https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests

  2. A query parameter ?fetch-timeout that holds the same

@nickolaev
Copy link
Contributor

Any thoughts on the timeout parameter @nickolaev / @jakubdyszkiewicz?

Since the envoy issue is still silent, I would think either:

  1. A header (x-kuma-fetch-timeout) that holds a GRPC TimeoutValue and TimeoutUnit https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests
  2. A query parameter ?fetch-timeout that holds the same

Are you referring to #2121 (comment) ?

if yes, I would vote for (2).

@austince
Copy link
Contributor Author

Are you referring to #2121 (comment) ?

if yes, I would vote for (2).

Yes, exactly. Ok, will merge this now and add this addition ASAP today.

@austince austince merged commit dbba76c into master Jun 10, 2021
@austince austince deleted the feat/mads/long-polling branch June 10, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants