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
use container ports in tasks SRV records #519
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
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 for the PR. Couple of suggestions:
- Please add unit tests that cover the port mapping case. Record generation is complicated and we rely on unit tests to help guard against accidental breakage.
- Does this work the same for both Docker- and UCR-containerized tasks? If not, can you make it do so?
- This will break in clusters where people are assuming SRV always points to the host port. Please gate this functionality behind a configuration flag that defaults to the current, host-port based behavior.
- Is there a relationship between this code and the value of the config
IPSources
? If so, that should be very clearly documented so that people aren't fighting confusing configuration combinations.
|
SRVPreferContainerPorts ?
…On Tue, May 8, 2018 at 8:16 AM, vixns ***@***.***> wrote:
UseContainerPorts is not a good name, SRVTaskRecordsUseContainerPort
sWithNetinfo has a better semantic, but may be too long, any idea for a
better option name ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#519 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLNVFzvxHjE-X_sbjCPGLdDcaPes0ks5twYy7gaJpZM4T2Y79>
.
|
Can one of the admins verify this patch? |
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.
made another pass over this PR. @vixns are you still interested in seeing this land?
Mesos has deprecated/removed the /state.json endpoint and has replaced it with /state. This patch updates the url path from /state.json to /state
oops rebased on wrong branch, sorry. Re-rebased and force pushed. |
No description provided.