-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Windows] Onboard Envoy to SCM #14352
Conversation
Creating as draft to have an idea of what the CI is thinking and get some early feedback |
@envoyproxy/windows-dev want to do an early pass to see if this going to the correct direction? |
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First high level pass looks good overall, some changes requested in comments
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
/retest |
Retrying Azure Pipelines: |
@wrowe this is ready for review. Please take a look when you have time |
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@wrowe can we get this on track to get merged since 1.17 is out now |
@wrowe just a ping for a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed with two key concerns, the suggested sc.exe args, and whether we want to utilize preshutdown, see the description of preshutdown in https://docs.microsoft.com/en-us/windows/win32/services/service-control-handler-function
Signed-off-by: davinci26 <sotirisnan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for the contribution and iterating through all our nits.
@envoyproxy/senior-maintainers could someone give this new facility a final pass?
@envoyproxy/senior-maintainers anyone interested in reviewing this one? |
@envoyproxy/senior-maintainers anyone interested in doing a win32 code review to get this one merged? |
I can take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM at a high level (I skimmed the win32 code) with some small comments. Thank you!
/wait
Signed-off-by: davinci26 <sotirisnan@gmail.com>
Signed-off-by: davinci26 <sotirisnan@gmail.com>
Signed-off-by: davinci26 <sotirisnan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
picking up main to fix the CI |
char data[] = {'a'}; | ||
Buffer::RawSlice buffer{data, 1}; | ||
auto result = handler->writev(&buffer, 1); | ||
RELEASE_ASSERT(result.rc_ == 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davinci26 how come we are using RELEASE_ASSERT
here for error handling? This doesn't match error handling policy in https://github.com/envoyproxy/envoy/blob/main/STYLE.md#error-handling I believe. CC @asraa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe it's fine because you are on the way out anyway? I'd be interested in your take @davinci26 on all the RELEASE_ASSERTs in this module relative to the policy. I actually think it might be fine, but it's hard to tell without understanding the Windows API expectations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe it's fine because you are on the way out anyway?
Exactly, we have already received a SIGTERM from the service control manager which means that for some reason we need to shutdown anyway. We try to do it in a graceful way but if the write fails we might as well crash.
The rest of the release asserts are guarding against a developer shooting themselves at the foot. i.e. someone manually calling delete the pointers on the global scope. I added the release asserts for them based on or an obvious crash on a subsequent line via null pointer dereference.
. No strong opinion though.
Do you want me to document these in code with comments so it's clear for anyone reading the code?
@@ -1,5 +1,9 @@ | |||
#include "exe/main_common.h" | |||
|
|||
#ifdef WIN32 | |||
#include "exe/service_base.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a new hand to envoy, but there seems no service_base.h
header file in ./source/exe/
. Should it be #include "exe/win32/service_base.h"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you encountering any issues compiling this or just curious? We use the strip_prefix
tag on here https://github.com/davinci26/envoy/blob/6650ba0799592927005f59fc00472ce67c03da37/source/exe/BUILD#L196 and this is why win32
is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious about it, thanks a lot for your help : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No prob, happy to help.
Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com
Commit Message:
Fixes #13188
Additional Description:
Utilizes the same inter process communication as
signal_impl
to handle close.Pending work:
Risk Level: Low (Windows Only)
Testing: Unit/Integration
Docs Changes: Added on how to run Envoy
Release Notes: N/A