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

extension: User space event #14712

Merged
merged 17 commits into from
Feb 1, 2021
Merged

extension: User space event #14712

merged 17 commits into from
Feb 1, 2021

Conversation

lambdai
Copy link
Contributor

@lambdai lambdai commented Jan 15, 2021

Commit Message:
UserSpaceFileEventImpl provides 3 functions

  1. setEnabled(events) declared by FileEvent. The events are registered for callback. Each call detects fired events once, and IO handle will notify the new events in the long run.
  2. activate(events). Explicitly activate the events. The above enabling events are not honored.
  3. activateIfEnabled(events). It should only be used by IO handle. Unlike activate, poll honors the setEnabled. Will schedule callback if there is no pending callback.

Additional Description:
Part of #13418
Risk Level: LOW all codes are in extension
Testing: Mocked userspace io handle to unit test userspace event.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
Base automatically changed from master to main January 15, 2021 23:02
Copy link
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Thanks so much for splitting this off and sending it out for review before the full change.

Copy link
Contributor Author

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

1 round update. Namespace name dedup and inner class comes later

Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Contributor Author

lambdai commented Jan 22, 2021

@antoniovicente ready to be reviewed now

Signed-off-by: Yuchen Dai <[email protected]>
Signed-off-by: Yuchen Dai <[email protected]>
antoniovicente
antoniovicente previously approved these changes Jan 25, 2021
Copy link
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

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

Thanks!

test/per_file_coverage.sh Show resolved Hide resolved
source/extensions/io_socket/user_space/file_event_impl.cc Outdated Show resolved Hide resolved
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Contributor Author

lambdai commented Jan 26, 2021

/assign @mattklein123

@lambdai
Copy link
Contributor Author

lambdai commented Jan 26, 2021

The latest commit dismissed the approval though. That commit is merely a trivial comment update.

antoniovicente
antoniovicente previously approved these changes Jan 27, 2021
@antoniovicente
Copy link
Contributor

Matt is on paternity leave. @envoyproxy/senior-maintainers could one of you take a look?

@ggreenway ggreenway assigned ggreenway and unassigned mattklein123 Jan 28, 2021
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Looks good overall. I'm excited to see this feature moving forward!

@lambdai
Copy link
Contributor Author

lambdai commented Jan 29, 2021

@ggreenway Thanks! I address the included headers and also cleaned the bazel deps.

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM

@lambdai
Copy link
Contributor Author

lambdai commented Feb 1, 2021

@ggreenway I believe the failing macos test is unrelated. It's failing consistently on other merged PRs.
What is the missing part to merge this PR? Thanks!

@ggreenway
Copy link
Contributor

Mac CI fix was merged an hour ago. Please merge main and try again, and it should work.

@lambdai
Copy link
Contributor Author

lambdai commented Feb 1, 2021

@ggreenway All green now!

@ggreenway ggreenway merged commit 8e2286b into envoyproxy:main Feb 1, 2021
@moderation
Copy link
Contributor

Interested in what this functionally is for. Userspace QUIC for HTTP/3? For use with gVisor's userspace Netstack? Something else?

@antoniovicente
Copy link
Contributor

See #11725

This is a step towards generalized connect handling; one use case could be access to internal listeners which are reachable via HTTP CONNECT, and support of the CONNECT method over HTTP2.

ggreenway pushed a commit that referenced this pull request Mar 2, 2021
Introduce user space io socket handle on top of user space event at #14712

Userspace Io socket handles are always created with a peering handle.
The write() methods populate the buffer in the peer handle.
The read() methods consume the owned buffer.

Signed-off-by: Yuchen Dai <[email protected]>
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.

5 participants