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

Add Marathon guide. #1578

Merged
merged 1 commit into from
May 17, 2017
Merged

Add Marathon guide. #1578

merged 1 commit into from
May 17, 2017

Conversation

gsemet
Copy link
Contributor

@gsemet gsemet commented May 10, 2017

@timoreimann
Copy link
Contributor

Refs #1250.

@timoreimann timoreimann self-requested a review May 10, 2017 12:56
@timoreimann timoreimann changed the title [DOC] Marathon service rotation explanation Add Marathon guide. May 10, 2017
@timoreimann
Copy link
Contributor

hey @stibbons, I went crazy and expanded what you have started to a full guide. Would you mind taking a look and tell me if it makes sense?

@timoreimann
Copy link
Contributor

@argeiger @gnur @tcolgate if you're still using Marathon along with Traefik and would be willing to prove-read the Marathon guide put together here, I'd be grateful. :-)

@timoreimann timoreimann removed their request for review May 10, 2017 14:27
@ldez
Copy link
Contributor

ldez commented May 10, 2017

ping @juliens

@gsemet
Copy link
Contributor Author

gsemet commented May 10, 2017

Great! I don't know what I did but my review does not appear on Github "File changed". It is here: 2f7d104
Just a few questions ! This doc is great!

@timoreimann
Copy link
Contributor

@stibbons

I don't know what I did but my review does not appear on Github "File changed".

Looks like you commented on a single commit instead of the entire change set. But don't worry, I see them. :-)

Thanks for the feedback -- will address it soon!

@timoreimann
Copy link
Contributor

@stibbons Feedback incorporated. :-)

@argeiger
Copy link
Contributor

LGTM. You might just want to point out the version of marathon that implements readiness checks

Thanks for putting this together.

@ldez
Copy link
Contributor

ldez commented May 10, 2017

@stibbons could you fix spelling? https://travis-ci.org/containous/traefik/jobs/230798308#L829

@timoreimann
Copy link
Contributor

@argeiger

You might just want to point out the version of marathon that implements readiness checks.

Good point -- I added the info for both the readiness checks and graceful termination handler.

@gsemet
Copy link
Contributor Author

gsemet commented May 10, 2017

+1

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

Great work.
If only I could have had these documents 2 months earlier ;)


Traefik tries to detect the configured mode and route traffic to the right IP addresses. It is possible to force using task hosts with the `forceTaskHostname` option.

Given the complexity of the subject, it is possible that the heuristic fails. Apart from filing an issue and waiting for the feature request / bug report to get addressed, one workaround for such situations is to customize the Marathon template file to the individual needs. (Note that this does _not_ require rebuilding Traefik but only to point the `filename` configuration parameter to a customized version of the `marathon.tmpl` file on Traefik startup.)
Copy link
Member

Choose a reason for hiding this comment

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

We may need an example. I'm not sure all users can see what to do with template.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree but would rather see the example in a more generic spot of the Traefik documentation since it applies to all providers. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, and I realize yesterday that I can't find documentation on this.
One more todo ! 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created #1590 to track the effort.

@timoreimann
Copy link
Contributor

@juliens anything else you think is missing? Otherwise, I'd kindly ask for your LGTM. :-)

@timoreimann
Copy link
Contributor

/cc @containous/traefik for potential other reviewers.

@juliens
Copy link
Member

juliens commented May 11, 2017

LGTM

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks a lot @stibbons !
LGTM
I suggest we change the base branch to v1.3 to get this right into the next release :)

@emilevauge emilevauge changed the base branch from master to v1.3 May 17, 2017 12:32
@emilevauge emilevauge changed the base branch from v1.3 to master May 17, 2017 12:37
@emilevauge emilevauge changed the base branch from master to v1.3 May 17, 2017 12:46
@emilevauge emilevauge changed the base branch from v1.3 to master May 17, 2017 12:47
Copy/pasted from very comprehensive slack response from @ttr
https://traefik.slack.com/archives/C0CDT22PJ/p1494347929571784?thread_ts=1494339388.375916&cid=C0CDT22PJ

Signed-off-by: Gaetan Semet <gaetan@xeberon.net>
@emilevauge emilevauge changed the base branch from master to v1.3 May 17, 2017 13:03
@emilevauge
Copy link
Member

Changed base branch to containous:v1.3 and rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants