Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support Consul Connect Envoy Command on Windows #17694
Support Consul Connect Envoy Command on Windows #17694
Changes from 16 commits
0a4c136
5e0bec2
41c02a0
c592e5d
ace9e37
7fab79f
6b1fea5
4b2fc2d
13e5433
906aaa2
2a70c2f
fb1fd8e
45f6f2a
aec2a20
4ab3bdb
aad0dab
4d62ffc
55dba95
6b0a813
6be9fcf
27073a4
b4cf245
f6424ab
c6aca86
05b3fbd
1388115
7a0b163
cd3e7ae
fd4ceed
fd75fb7
f500139
74de240
796a88a
a39760a
e142630
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If my understanding is right, this uses mmap to keep the file in memory and not write it to disk to match the behaviour of named pipe in the unix version
Maybe we can use something like this library to continue using named pipes even in windows and have an equivalent implementation on all OSes?
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.
please help me understand we need to use both libraries npipe and mmap or any one?
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 think npipe is the best choice as we would like to keep both implementation similar between windows and nix, but if that's not possible we can use mmap.
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.
This library creates a connection from which we can read lines or can create server. I am not able to understand how we can use it here. Can you please help @dhiaayachi ?
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.
Here is an example from the library. You can use it similarly to what we have in the nix implementation.
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'm not sure I understand the reasoning behind that, why don't we wait for it to be done and is this specific to Windows?
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.
Not specific to Windows. https://github.com/hashicorp/consul/blob/b0a2e33e0a6c4ec411266dfcef38a3a648fefb50/command/connect/envoy/exec_unix.go#L127C1-L129C10
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'm not sure I understand the reasoning here, why don't we defer the clean up to after the envoy process finish?
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.
This logic is not specific to windows. Please see - unix version - https://github.com/hashicorp/consul/blob/b0a2e33e0a6c4ec411266dfcef38a3a648fefb50/command/connect/envoy/exec_unix.go#L127C1-L129C10