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

[core] Add a channel argument to set DSCP on streams #28322

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Dec 9, 2021

This adds a new channel argument GRPC_ARG_DSCP which allows users to create classified gRPC streams with a
Differentiated Services Code Point (DSCP) marking on the IP frames.

The channel argument is handled on both clients and servers, but currently only on posix based systems.

Fixes #17225

Background:
In addition to what is already described is #17225, when gRPC is used in telco systems there is often a need to classify streams of importance. There can be multiple hops between two endpoints (e.g. between 2 telecom operators) and some streams that are more important than others (e.g. emergency call related or similar). By marking the IP packets using DSCP the aware routers can make a sound decision of the prioritization.

This PR propose to use DSCP as the configuration value since its common for both IPv4/IPv6, an alternative would be to use a config name that includes TOS and Traffic Class.
There might be more needed regarding documentation and end2end testing, but there I need some advice.

References
https://datatracker.ietf.org/doc/html/rfc2474
https://www.iana.org/assignments/dscp-registry/dscp-registry.xhtml

@yashykt

@bjosv
Copy link
Contributor Author

bjosv commented Jan 12, 2022

Are there any thoughts about this proposal, or is there an alternative plan how to classify streams?

@yashykt
Copy link
Member

yashykt commented May 17, 2022

why not just use a socket mutator for this?

@bjosv
Copy link
Contributor Author

bjosv commented May 24, 2022

why not just use a socket mutator for this?

The issue I have is how to handle the declarations of the socket mutator types needed by SetSocketMutator() in ChannelArguments. Currently the socket_mutator.h is located in src/ and not installed together with the library, so it was a bit vague to me if this could be used as a public api.
I have found #18989 related to this, but it got stale and closed.

Are there any plans to make it available for public consumption?
If no plans; what would the common or recommended way be to handle the missing types? Modify the grpc installation or having an own copy of socket_mutator.h in the project?
Thanks!

@yashykt yashykt assigned veblush and drfloob and unassigned veblush Jun 8, 2022
@yashykt
Copy link
Member

yashykt commented Jun 8, 2022

@drfloob For considering making this API public

@drfloob
Copy link
Member

drfloob commented Sep 10, 2022

@bjosv Sorry for the delay. Some changes are underway in gRPC's low-level I/O code, and we plan to replace the SocketMutator API with an equivalent. SocketMutator is, as you pointed out, not really a public thing even though it exists in the surface API, the only sanctioned uses are internal to Google. We expect to provide something that the public can use as well.

@rAlexandre00
Copy link

rAlexandre00 commented Feb 6, 2023

@drfloob Any updates on whether the SocketMutator (or equivalent) API will be made public?
I have a use case for it. I need to make sure the packets leave from a specific interface.
Thanks.

@drfloob
Copy link
Member

drfloob commented Mar 16, 2023

Sorry for the long delay, @bjosv. We have been discussing implementation alternatives for this feature the past few months. Since gRPC has a strong commitment to maintaining a stable public API, and because we're undergoing some significant changes to the way I/O is done in gRPC, we hadn't wanted to make any hasty decisions and risk being burdened with mistakes we'd need to maintain long-term.

Ultimately, since this is a widely-used standard, a channel argument like this is probably the right approach. When we introduce this feature, we'll want 4 implementations: Posix iomgr and EventEngine implementations (near identical to each other in regards to this work), and ideally Windows iomgr and EventEngine implementations as well (similarly identical to each other). Would you like to take on the task?

@bjosv
Copy link
Contributor Author

bjosv commented Mar 16, 2023

@drfloob Sure, sounds good. I'll start by lifting this and implement an EventEngine variant.

I need to look into the Windows parts a bit more first since I'm not that familiar how it handles DSCP/QoS.
It seems that IP_TOS can be used, but is only handled by some versions of Windows (Win 10 and 8).
The README states that gRPC C++ supports "Windows 10+" so maybe the current IP_TOS variant should work fine then.
The alternative seems to be using a Windows provided QOS API.

@drfloob
Copy link
Member

drfloob commented Mar 16, 2023

Sure, sounds good. I'll start by lifting this and implement an EventEngine variant.

Wonderful, thank you!

It seems that IP_TOS can be used, but is only handled by some versions

I see a strong recommendation against using IP_TOS, and a few reports that IP_TOS does not work without registry hacking. I'm not familiar with the Windows QoS APIs, but they will probably be needed here based on my initial understanding.

Regarding the PosixEventEngine, I believe the iomgr-equivalent spot is here:

absl::Status PrepareTcpClientSocket(PosixSocketWrapper sock,
const EventEngine::ResolvedAddress& addr,
const PosixTcpOptions& options) {
bool close_fd = true;
auto sock_cleanup = absl::MakeCleanup([&close_fd, &sock]() -> void {
if (close_fd and sock.Fd() >= 0) {
close(sock.Fd());
}
});
GRPC_RETURN_IF_ERROR(sock.SetSocketNonBlocking(1));
GRPC_RETURN_IF_ERROR(sock.SetSocketCloexec(1));
if (reinterpret_cast<const sockaddr*>(addr.address())->sa_family != AF_UNIX) {
// If its not a unix socket address.
GRPC_RETURN_IF_ERROR(sock.SetSocketLowLatency(1));
GRPC_RETURN_IF_ERROR(sock.SetSocketReuseAddr(1));
sock.TrySetSocketTcpUserTimeout(options, true);
}
GRPC_RETURN_IF_ERROR(sock.SetSocketNoSigpipeIfPossible());
GRPC_RETURN_IF_ERROR(sock.ApplySocketMutatorInOptions(
GRPC_FD_CLIENT_CONNECTION_USAGE, options));
// No errors. Set close_fd to false to ensure the socket is not closed.
close_fd = false;
return absl::OkStatus();
}

@bjosv
Copy link
Contributor Author

bjosv commented Apr 17, 2023

@drfloob I pushed a re-take of the PR to include both iomgr and event_engine for both Linux/posix and Windows.
Windows required a different take as you mentioned by using the Windows provided qwave library and the QoS Api.
I had to experiment a lot to get and understanding of it, but it works fine when taking the quirks mentioned in the added docs into account.

I'm not sure adding an additional grpc library is accepted. An alternative for Windows event_engine can be to put the DSCP handling in the WindowsEndpoint constructor, which takes the needed peer address, socket and EndpointConfig.

I added the qwave dependency to the CMake builds, but I'm not sure if it also is needed in templates/grpc.gyp.template for node(?) and templates/Makefile.template for MINGW32. I don't know the steps to build them yet.
Another thought I had was if we should add a CMake flag to enable the functionality and the new library dependency.

@bjosv
Copy link
Contributor Author

bjosv commented Apr 17, 2023

I now see typos in the commit message...unless PRs are squshed there should be a s/event_mananger/event_engine/ in 2 commit messages..

@drfloob
Copy link
Member

drfloob commented May 10, 2023

I'm sorry this slipped through the cracks. Thank you for the hard work! I've replied to your comments below, and I'll do another review pass.

I'm not sure adding an additional grpc library is accepted.

Is the qwave library available by default on Windows 10 and later? I haven't found any instructions on how to acquire it, and it's not popping up as a separate installation component for visual studio, so I assume it's available by default but it's worth asking. gRPC currently supports Windows 10+, Visual Studio 2019 and later. If it's available everywhere we support, I don't see any harm here.

I'm not sure if it also is needed in templates/grpc.gyp.template for node(?) and templates/Makefile.template for MINGW32. I don't know the steps to build them yet.

You don't have to worry about those. They are both used for wrapped languages, which is safely out of scope for this feature addition.

I think you will, however, want to add qwave.lib in the windows_qos bazel build target's linkopts to get bazel support.

Another thought I had was if we should add a CMake flag to enable the functionality and the new library dependency.

Most things in gRPC are available in all builds, sometimes configured via runtime flags or arguments. This simplifies testing among other things, since compilation time can be expensive. Unless this adds an intolerable amount of bloat, I'd opt to leave it in all builds.

I now see typos in the commit message...unless PRs are squshed there should be a s/event_mananger/event_engine/ in 2 commit messages..

PRs are squashed, no worries.

@bjosv bjosv changed the title Add a channel argument to set DSCP on streams [core] Add a channel argument to set DSCP on streams May 24, 2023
@bjosv
Copy link
Contributor Author

bjosv commented Jun 15, 2023

Shall we skip Windows support?

It became a bit complex and we don't need the windows support, maybe it can be implemented by a eager user when the time comes. I have updated the PR to only include Linux/posix support now.

The implementation in Mac/BSD require the usage of IPPROTO_IPV6 for IPV6
while the Linux implementation is more relaxed. Correcting the testcase
to check the socket options IPPROTO_IPV6 for IPv6 sockets.
@bjosv
Copy link
Contributor Author

bjosv commented Jun 16, 2023

Running CI on multiple platforms is sure great..
Have fixed the testcase to check the correct sockopt so it works on OSX now. Linux is a bit relaxed regarding which option you can get the DSCP value from regarding IPv6 sockets.

@drfloob drfloob added the release notes: yes Indicates if PR needs to be in release notes label Jun 21, 2023
Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for all the hard work, @bjosv!

@markdroth would you mind providing a second review?

@drfloob drfloob requested review from yashykt and removed request for markdroth June 30, 2023 18:59
@yashykt
Copy link
Member

yashykt commented Jun 30, 2023

The PR itself looks good to me, but I wonder whether this needs a gRFC

Copy link
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

@drfloob I'm gonna approve this but could you drive the conversation internally on whether this needs a grfc please?

@drfloob
Copy link
Member

drfloob commented Jun 30, 2023

The PR itself looks good to me, but I wonder whether this needs a gRFC

I've already had that discussion with the core/C++ team TLs, and we decided it isn't necessary here.

@drfloob drfloob merged commit ac874c2 into grpc:master Jun 30, 2023
66 checks passed
HannahShiSFB pushed a commit to HannahShiSFB/grpc that referenced this pull request Jul 3, 2023
This adds a new channel argument `GRPC_ARG_DSCP` which allows users to
create classified gRPC streams with a
Differentiated Services Code Point (DSCP) marking on the IP frames.

The channel argument is handled on both clients and servers, but
currently only on posix based systems.

Fixes grpc#17225

**Background**:
In addition to what is already described is grpc#17225, when gRPC is used in
telco systems there is often a need to classify streams of importance.
There can be multiple hops between two endpoints (e.g. between 2 telecom
operators) and some streams that are more important than others (e.g.
emergency call related or similar). By marking the IP packets using DSCP
the aware routers can make a sound decision of the prioritization.

This PR propose to use DSCP as the configuration value since its common
for both IPv4/IPv6, an alternative would be to use a config name that
includes TOS and Traffic Class.
There might be more needed regarding documentation and end2end testing,
but there I need some advice.

**References**
https://datatracker.ietf.org/doc/html/rfc2474
https://www.iana.org/assignments/dscp-registry/dscp-registry.xhtml

<!--

Your pull request will be routed to the following person by default for
triaging.
If you know who should review your pull request, please remove the
mentioning below.

-->

@yashykt
@bjosv bjosv deleted the add-dscp branch July 4, 2023 16:22
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jul 5, 2023
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jul 5, 2023
This adds a new channel argument `GRPC_ARG_DSCP` which allows users to
create classified gRPC streams with a
Differentiated Services Code Point (DSCP) marking on the IP frames.

The channel argument is handled on both clients and servers, but
currently only on posix based systems.

Fixes grpc#17225

**Background**:
In addition to what is already described is grpc#17225, when gRPC is used in
telco systems there is often a need to classify streams of importance.
There can be multiple hops between two endpoints (e.g. between 2 telecom
operators) and some streams that are more important than others (e.g.
emergency call related or similar). By marking the IP packets using DSCP
the aware routers can make a sound decision of the prioritization.

This PR propose to use DSCP as the configuration value since its common
for both IPv4/IPv6, an alternative would be to use a config name that
includes TOS and Traffic Class.
There might be more needed regarding documentation and end2end testing,
but there I need some advice.

**References**
https://datatracker.ietf.org/doc/html/rfc2474
https://www.iana.org/assignments/dscp-registry/dscp-registry.xhtml

<!--

Your pull request will be routed to the following person by default for
triaging.
If you know who should review your pull request, please remove the
mentioning below.

-->

@yashykt
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jul 13, 2023
This adds a new channel argument `GRPC_ARG_DSCP` which allows users to
create classified gRPC streams with a
Differentiated Services Code Point (DSCP) marking on the IP frames.

The channel argument is handled on both clients and servers, but
currently only on posix based systems.

Fixes grpc#17225

**Background**:
In addition to what is already described is grpc#17225, when gRPC is used in
telco systems there is often a need to classify streams of importance.
There can be multiple hops between two endpoints (e.g. between 2 telecom
operators) and some streams that are more important than others (e.g.
emergency call related or similar). By marking the IP packets using DSCP
the aware routers can make a sound decision of the prioritization.

This PR propose to use DSCP as the configuration value since its common
for both IPv4/IPv6, an alternative would be to use a config name that
includes TOS and Traffic Class.
There might be more needed regarding documentation and end2end testing,
but there I need some advice.

**References**
https://datatracker.ietf.org/doc/html/rfc2474
https://www.iana.org/assignments/dscp-registry/dscp-registry.xhtml

<!--

Your pull request will be routed to the following person by default for
triaging.
If you know who should review your pull request, please remove the
mentioning below.

-->

@yashykt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/low imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing C++ API to set IP_TOS (DSCP) on a gRPC server stream
6 participants