Improve Marathon integration tests. #1406

timoreimann · 2017-04-10T07:32:17Z

This is a continuation of #1398.

The tests do not succeed yet, apparently because the external network isn't applied.

timoreimann · 2017-04-10T07:32:39Z

According to @vdemeester, it's because the version of libcompose we use does not support docker-compose v2 yet.

timoreimann · 2017-05-03T09:34:10Z

This PR is blocked because we first need to update libcompose (through libkermit) in order to have docker-compose version 2 which brings custom networking support. Otherwise, we cannot talk to the Marathon instances from the integration tests.

Updating libcompose is non-trivial, once again due to glide and our two-glide-manifests setup.

timoreimann · 2017-05-03T10:36:03Z

Note to myself: Also update go-marathon integration dependency.

timoreimann · 2017-07-10T09:15:06Z

Turns out networking aliasing isn't properly working in libcompose, which is a blocker for the required inter-container communication on the Marathon docker-compose manifest.

Our very own @vdemeester is on it. :-)

timoreimann · 2017-07-11T08:01:10Z

Currently being show-stopped by docker/libcompose#484. Sending all bug reports directly to @vdemeester. :)

ldez · 2017-07-24T19:07:31Z

How you have retry since docker/libcompose#483?

timoreimann · 2017-07-24T20:12:22Z

@ldez I did retry, and that helped us move forward a bit. However, I then ran into docker/libcompose#484. That's still the latest blocker.

timoreimann · 2017-07-28T16:31:56Z

Trying if switching to host network helps. Can only debug this properly on the CI.

ldez · 2017-08-01T10:42:39Z

integration/marathon_test.go

+	// Wait for Marathon readiness prior to creating the client so that we
+	// don't run into the "all cluster members down" state right from the
+	// start.
+	err := try.GetRequest(marathonURL+"/ping", 1*time.Minute, try.StatusCodeIs(http.StatusOK))


Could you move this into the setup like the others provider integration tests?
And maybe call /leader instead of /ping ?

Good idea (on both). Done.

ldez · 2017-08-02T12:36:16Z

integration/marathon_test.go

-	//})
-	//
-	//c.Assert(err, checker.IsNil)
+func (s *MarathonSuite) getContainerIPAddr(c *check.C, name string) string {


seems unnecessary: just put the 2 lines in the setup.

I can move it if you like; just to be sure though: did you notice that I need it once for the Mesos slave and another time for Marathon? That'd be 4 lines, so I wanted to avoid code duplication. (In fact, the function would be reusable if we turned s.composeProject into a function parameter, which would allow us to apply it in a lot of other tests as well.)

Personally, I also find getContainerIPAddr(c, name) more readable and comprehendible compared to s.composeProject.Container(c, name).NetworkSettings.IPAddress.

I understand you can create a function getContainerIPAddr in integration_test.go

func (s *BaseSuite) getContainerIPAddr(c *check.C, name string) string { ipAddr := s.composeProject.Container(c, name).NetworkSettings.IPAddress c.Assert(ipAddr, checker.Not(checker.HasLen), 0) return ipAddr }

But I think this refactor must be done in another PR.

ldez · 2017-08-02T12:43:36Z

integration/marathon_test.go

+	deploy, err := client.UpdateApplication(app, false)
+	c.Assert(err, checker.IsNil)
+	// Wait for deployment to complete.
+	c.Assert(client.WaitOnDeployment(deploy.DeploymentID, 2*time.Minute), checker.IsNil)


2 minutes ?

Cut in half. (Although a higher timeout doesn't really hurt us in the regular case where things go fast and possibly saves us if there's higher latency for whatever reason.)

ldez · 2017-08-02T12:43:42Z

integration/marathon_test.go


+	// Query application via Traefik.
+	err = try.GetRequest("http://127.0.0.1:8000/service", 1*time.Minute, try.StatusCodeIs(http.StatusOK))


1 minutes ?

Cut down to 30 seconds.

ldez · 2017-08-02T13:16:27Z

integration/marathon_test.go

+	client, err := marathon.NewClient(config)
+	c.Assert(err, checker.IsNil)
+
+	showTraefikLog := true


maybe you can add a small comment to explains that in the code.

timoreimann · 2017-08-02T19:53:15Z

I also just realized that the TestSimpleConfiguration test in its current form is pretty useless: The Marathon configuration is actually broken in that test as I don't replace the {{.MarathonURL}} template parameter there. And yet, Traefik starts up successfully (because it's always trying hard to do its best).

Simplifying the configuration even further doesn't really give us any automatic guarantee. As there's no way to reliably tell if we got a 404 because we haven't added a Marathon application yet, I removed the test. It doesn't add any value to me. WDYT?

ldez · 2017-08-02T20:27:24Z

@timoreimann I agree TestSimpleConfiguration is useless. I think you can remove this test (which tests nothing)

timoreimann · 2017-08-04T08:03:25Z

@ldez @juliens PTAL again. All comments should be addressed.

ldez

LGTM

juliens

LGTM

juliens · 2017-08-04T08:39:19Z

integration/marathon_test.go

+	// enjoy DNS-discoverable container host names.
+	mesosSlaveIPAddr := s.composeProject.Container(c, containerNameMesosSlave).NetworkSettings.IPAddress
+	c.Assert(mesosSlaveIPAddr, checker.Not(checker.HasLen), 0)
+	c.Assert(s.extendDockerHostsFile(containerNameMesosSlave, mesosSlaveIPAddr), checker.IsNil)


I prefer when we split the call and the assert (more readable for me, but I can live with it)

err := s.extendDockerHostsFile(containerNameMesosSlave, mesosSlaveIPAddr) c.Assert(err, checker.IsNil)

Separating the call and the check would actually be more consistent with our existing style.

Updated.

emilevauge

Thanks a lot @timoreimann !
LGTM

Update github.com/docker/libcompose in glide.* files. Vendor github.com/docker/libcompose update.

- Update compose file. - Add integration test for Marathon application deployment.

timoreimann mentioned this pull request Apr 10, 2017

Fix default timeouts for Marathon provider. #1398

Merged

timoreimann force-pushed the improve-marathon-integration-tests branch from 1b788dc to 4dabfe1 Compare April 10, 2017 07:34

timoreimann changed the title ~~[WIP] Improve marathon integration tests.~~ [WIP] Improve Marathon integration tests. Apr 10, 2017

ldez added area/provider/marathon WIP labels Apr 14, 2017

timoreimann mentioned this pull request May 3, 2017

Bump go-marathon dep #1524

Merged

emilevauge mentioned this pull request Jul 7, 2017

Use single API call to fetch Marathon resources. #1815

Merged

timoreimann force-pushed the improve-marathon-integration-tests branch 3 times, most recently from f5194c7 to 4f0ab7c Compare July 8, 2017 20:00

timoreimann force-pushed the improve-marathon-integration-tests branch from 4f0ab7c to 9f0c14f Compare July 10, 2017 22:56

ldez added the kind/enhancement a new or improved feature. label Jul 11, 2017

emilevauge mentioned this pull request Jul 24, 2017

Add support for readiness checks. #1883

Merged

ldez added the size/L label Jul 25, 2017

timoreimann force-pushed the improve-marathon-integration-tests branch 6 times, most recently from 39fa372 to ffb862a Compare July 30, 2017 10:10

timoreimann requested a review from a team as a code owner July 30, 2017 10:10

timoreimann force-pushed the improve-marathon-integration-tests branch from 3ba68dd to 924f071 Compare July 30, 2017 23:56

ldez suggested changes Aug 2, 2017

View reviewed changes

ldez added contributor/waiting-for-corrections status/2-needs-review labels Aug 2, 2017

ldez added contributor/waiting-for-corrections and removed contributor/waiting-for-corrections labels Aug 2, 2017

timoreimann force-pushed the improve-marathon-integration-tests branch from 863dfe7 to 3d96921 Compare August 3, 2017 22:07

ldez removed the contributor/waiting-for-corrections label Aug 3, 2017

timoreimann force-pushed the improve-marathon-integration-tests branch from 3d96921 to 54ca70e Compare August 3, 2017 22:30

ldez approved these changes Aug 4, 2017

View reviewed changes

juliens approved these changes Aug 4, 2017

View reviewed changes

ldez added size/M and removed size/M labels Aug 4, 2017

ldez added this to the 1.4 milestone Aug 6, 2017

emilevauge approved these changes Aug 10, 2017

View reviewed changes

emilevauge added status/3-needs-merge and removed status/2-needs-review labels Aug 10, 2017

timoreimann force-pushed the improve-marathon-integration-tests branch from 5c194f6 to c5dcd56 Compare August 10, 2017 14:54

ldez added bot/merge-method-rebase bot/need-human-merge labels Aug 10, 2017

timoreimann added 2 commits August 10, 2017 21:05

Update github.com/docker/libcompose

758bd34

Update github.com/docker/libcompose in glide.* files. Vendor github.com/docker/libcompose update.

[marathon] Fix and extend integration tests.

95a49ff

- Update compose file. - Add integration test for Marathon application deployment.

ldez force-pushed the improve-marathon-integration-tests branch from c5dcd56 to 95a49ff Compare August 10, 2017 19:05

ldez removed the bot/need-human-merge label Aug 10, 2017

ldez merged commit 64b8fc5 into master Aug 10, 2017

ldez removed the status/3-needs-merge label Aug 10, 2017

ldez deleted the improve-marathon-integration-tests branch August 10, 2017 19:58

ldez removed the bot/merge-method-rebase label Aug 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Marathon integration tests. #1406

Improve Marathon integration tests. #1406

timoreimann commented Apr 10, 2017

timoreimann commented Apr 10, 2017

timoreimann commented May 3, 2017 •

edited

Loading

timoreimann commented May 3, 2017

timoreimann commented Jul 10, 2017

timoreimann commented Jul 11, 2017

ldez commented Jul 24, 2017

timoreimann commented Jul 24, 2017

timoreimann commented Jul 28, 2017

ldez Aug 1, 2017

timoreimann Aug 2, 2017

ldez Aug 2, 2017

timoreimann Aug 2, 2017

ldez Aug 2, 2017

ldez Aug 2, 2017

timoreimann Aug 2, 2017

ldez Aug 2, 2017

timoreimann Aug 2, 2017

ldez Aug 2, 2017

timoreimann commented Aug 2, 2017

ldez commented Aug 2, 2017

timoreimann commented Aug 4, 2017

ldez left a comment

juliens left a comment

juliens Aug 4, 2017

timoreimann Aug 4, 2017

emilevauge left a comment


		// Query application via Traefik.
		err = try.GetRequest("http://127.0.0.1:8000/service", 1*time.Minute, try.StatusCodeIs(http.StatusOK))

Improve Marathon integration tests. #1406

Improve Marathon integration tests. #1406

Conversation

timoreimann commented Apr 10, 2017

timoreimann commented Apr 10, 2017

timoreimann commented May 3, 2017 • edited Loading

timoreimann commented May 3, 2017

timoreimann commented Jul 10, 2017

timoreimann commented Jul 11, 2017

ldez commented Jul 24, 2017

timoreimann commented Jul 24, 2017

timoreimann commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Aug 2, 2017

ldez commented Aug 2, 2017

timoreimann commented Aug 4, 2017

ldez left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

timoreimann commented May 3, 2017 •

edited

Loading