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

Upgrade go-marathon to 15ea23e. #1635

Merged

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented May 19, 2017

Our vendored copy contains a bug that causes unavailable Marathon nodes to never be marked as available again due to a misconstruction in the URL to the Marathon health check / ping endpoint used by go-marathon internally.

A fix has been published.

Targeting 1.3 since the impact on a production deployment could be quite severe: For long-running Traefik instances, it only takes as many failed attempts to reach the Marathon API endpoint as there are Marathon nodes in the cluster for the Marathon provider to stop working properly.

@timoreimann timoreimann added this to the 1.3 milestone May 19, 2017
@timoreimann timoreimann force-pushed the upgrade-go-marathon-to-15ea23e branch 2 times, most recently from ac59149 to 98f1f4d Compare May 19, 2017 17:18
@ldez
Copy link
Contributor

ldez commented May 19, 2017

+ go test -cover -coverprofile=cover.out ./provider/marathon
# github.com/containous/traefik/mocks
mocks/Marathon.go:856: undefined: marathon.GetAppOpts
mocks/Marathon.go:866: undefined: marathon.Queue
mocks/Marathon.go:876: undefined: marathon.GetGroupOpts
mocks/Marathon.go:881: undefined: marathon.GetGroupOpts
FAIL	github.com/containous/traefik/provider/marathon [build failed]

@ldez ldez added kind/bug/fix a bug fix and removed kind/bug/confirmed a confirmed bug (reproducible). labels May 19, 2017
@timoreimann
Copy link
Contributor Author

Interesting. I wonder why the mock fails now but did not do on the last go-marathon upgrade, which is ~2 commits behind and no API changes were introduced in the meantime AFAIR.

Will take a look. Thanks, @ldez!

// GetAppOpts contains a payload for Application method
// embed: Embeds nested resources that match the supplied path.
// You can specify this parameter multiple times with different values
type GetAppOpts struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is here

@timoreimann timoreimann force-pushed the upgrade-go-marathon-to-15ea23e branch from 98f1f4d to c514def Compare May 19, 2017 21:32
@timoreimann
Copy link
Contributor Author

timoreimann commented May 19, 2017

Fixed, thanks to @ldez and glide cache-clear. :)

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM Marty 😃

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 @timoreimann !
LGTM :)

Our vendored copy contains a bug that causes unavailable Marathon nodes
to never be marked as available again due to a misconstruction in the
URL to the Marathon health check / ping endpoint used by go-marathon
internally.

A fix[1] has been published.

[1]gambol99/go-marathon#283
@ldez ldez force-pushed the upgrade-go-marathon-to-15ea23e branch from c514def to 9aeffdf Compare May 22, 2017 18:13
@timoreimann timoreimann merged commit 219a637 into traefik:v1.3 May 22, 2017
@timoreimann timoreimann deleted the upgrade-go-marathon-to-15ea23e branch May 22, 2017 18:52
@timoreimann timoreimann removed the request for review from vdemeester May 22, 2017 18:52
@ldez ldez mentioned this pull request Jun 11, 2017
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.

3 participants