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

Use single API call to fetch Marathon resources. #1815

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Jul 2, 2017

Change Marathon provider to make just one API call instead of two per configuration update by means of specifying embedded resources, which enable retrieving multiple response types from the API at once. Apart from the obvious savings in API calls, we primarily gain a consistent view on both applications and tasks that allows us to drop a lot of correlation logic. Additionally, it will serve as the basis for the introduction of readiness checks which require application/task consistency for correct leverage on the proxy end.

Additional changes:

marathon.go:

  • Filter on tasks now embedded inside the applications.
  • Reduce/simplify signature on multiple template functions as we do not need to check for proper application/task correlation anymore.
  • Remove getFrontendBackend in favor of just getBackend.
  • Move filtering on enabled/exposed applications from taskFilter to applicationFilter. (The task filter just reached out to the applications anyway, so it never made sense to locate it with the
    tasks where the filter was called once for every task even though the result would never change.)
  • Remove duplicate constraints filter in tasks, where it neither made sense to keep as it operates on the application level only.
  • Add context to rendering error.

marathon_test.go:

  • Simplify and reduce numerous tests.
  • Convert tests with high number of cases into parallelized sub-tests.
  • Improve readability/structure for several tests.
  • Add missing test for enabled/exposed applications.
  • Simplify the mocked Marathon server.

marathon.tmpl:

  • Update application/task iteration.
  • Replace getFrontendBackend by getBackend.

Fixes #1846

@timoreimann
Copy link
Contributor Author

timoreimann commented Jul 2, 2017

Note that I have explicitly tried to keep the changes to the tests as close to the bare minimum as possible, reflecting just the changes in the production code. Specifically, the Marathon tests badly need a builder pattern to reduce the amount of boilerplate.

I'll tackle that in a follow-up PR in order to keep this PR simple-ish.

@ldez ldez changed the title [marathon] Use single API call to fetch Marathon resources. Use single API call to fetch Marathon resources. Jul 2, 2017
@ldez ldez requested a review from juliens July 2, 2017 20:39
@timoreimann timoreimann force-pushed the marathon-use-single-api-call branch from 37b0d20 to 50e98eb Compare July 3, 2017 08:46
@emilevauge
Copy link
Member

Great work @timoreimann, design LGTM 👏

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 Job !
well spotted "embed=app.tasks" !! 😄
LGTM

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.

Maybe you can add some integration tests (or in another PR)? 😉

},
tasks: &marathon.Tasks{
Tasks: []marathon.Task{
task: marathon.Task{
Copy link
Contributor

@ldez ldez Jul 7, 2017

Choose a reason for hiding this comment

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

Could you reduce tests definition with some helpers builder?

// ...
    task: aMarathonTask(withDefaultIPAddresses),
// ...
func aMarathonTask(builders ...func(marathon.Task)) marathon.Task {
	task := marathon.Task{
		Host:  "localhost",
		Ports: []int{80},
	}

	for _, builder := range builders {
		builder(task)
	}

	return task
}

func withDefaultIPAddresses(task marathon.Task) {
	task.IPAddresses =  []*marathon.IPAddress{
		{
			IPAddress: "127.0.0.1",
			Protocol:  "tcp",
		},
	}
}

I think you can do the same for marathon.Application, types.Backend, types.Frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted, I intentionally kept modifications to the tests as minimal as possible in order to not risk changing both production code and the our test coverage safety net at the same time.

I'd like to do the test improvements in a follow-up PR. In fact, I'm already working on that and have started building a builder very similar to what you outlined.

Okay for you?

Copy link
Contributor

@ldez ldez Jul 7, 2017

Choose a reason for hiding this comment

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

I don't really like to have 2 PRs for the same subject when 2-3 commits make the job: you can create a temporary test file, with the new tests, ensure all old tests always pass and replace old tests file by the temporary file. (with commit [and push to run CI] at each step)

But tests passed and code build.

Forget my request.

@emilevauge
Copy link
Member

@ldez Marathon integration tests will be done #1406 ;)

@timoreimann
Copy link
Contributor Author

Having at least some minimal integration test would actually be nice.

I'll wipe off the dust of #1406 and see if I can put something basic together. This PR doesn't change functionality, so ideally #1406 should provide immediate benefit.

@ldez
Copy link
Contributor

ldez commented Jul 7, 2017

@emilevauge I know, it's just a joke because @timoreimann and me have already speaking about Marathon integration tests. 😃

@ldez ldez added this to the 1.4 milestone Jul 7, 2017
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.

"Approval in disapproval." ©️

@ldez
Copy link
Contributor

ldez commented Jul 7, 2017

@timoreimann Could you rebase and resolve conflicts?

@timoreimann timoreimann force-pushed the marathon-use-single-api-call branch 2 times, most recently from 996574c to e16255f Compare July 8, 2017 19:26
@timoreimann
Copy link
Contributor Author

Rebased.

@juliens
Copy link
Member

juliens commented Jul 10, 2017

@timoreimann Could you resolve conflicts? Thx

Change Marathon provider to make just one API call instead of two per
configuration update by means of specifying embedded resources, which
enable retrieving multiple response types from the API at once. Apart
from the obvious savings in API calls, we primarily gain a consistent
view on both applications and tasks that allows us to drop a lot of
correlation logic.  Additionally, it will serve as the basis for the
introduction of readiness checks which require application/task
consistency for correct leverage on the proxy end.

Additional changes:

marathon.go:
- Filter on tasks now embedded inside the applications.
- Reduce/simplify signature on multiple template functions as we do not
  need to check for proper application/task correlation anymore.
- Remove getFrontendBackend in favor of just getBackend.
- Move filtering on enabled/exposed applications from `taskFilter` to
  `applicationFilter`. (The task filter just reached out to the
  applications anyway, so it never made sense to locate it with the
  tasks where the filter was called once for every task even though the
  result would never change.)
- Remove duplicate constraints filter in tasks, where it neither made
  sense to keep as it operates on the application level only.
- Add context to rendering error.

marathon_test.go:
- Simplify and reduce numerous tests.
- Convert tests with high number of cases into parallelized sub-tests.
- Improve readability/structure for several tests.
- Add missing test for enabled/exposed applications.
- Simplify the mocked Marathon server.

marathon.tmpl:
- Update application/task iteration.
- Replace `getFrontendBackend` by `getBackend`.
@timoreimann
Copy link
Contributor Author

@juliens Rebased.

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@juliens juliens merged commit 779eeba into traefik:master Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants