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

Improve Marathon integration tests. #1406

Merged
merged 2 commits into from Aug 10, 2017
Merged

Conversation

timoreimann
Copy link
Contributor

This is a continuation of #1398.

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

@timoreimann
Copy link
Contributor Author

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

@timoreimann timoreimann force-pushed the improve-marathon-integration-tests branch from 1b788dc to 4dabfe1 Compare April 10, 2017 07:34
@timoreimann timoreimann changed the title [WIP] Improve marathon integration tests. [WIP] Improve Marathon integration tests. Apr 10, 2017
@timoreimann
Copy link
Contributor Author

timoreimann commented May 3, 2017

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 timoreimann mentioned this pull request May 3, 2017
@timoreimann
Copy link
Contributor Author

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

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

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 timoreimann force-pushed the improve-marathon-integration-tests branch from 4f0ab7c to 9f0c14f Compare July 10, 2017 22:56
@timoreimann
Copy link
Contributor Author

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

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

ldez commented Jul 24, 2017

How you have retry since docker/libcompose#483?

@timoreimann
Copy link
Contributor Author

@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.

@ldez ldez added the size/L label Jul 25, 2017
@timoreimann
Copy link
Contributor Author

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

@timoreimann timoreimann force-pushed the improve-marathon-integration-tests branch 6 times, most recently from 39fa372 to ffb862a Compare July 30, 2017 10:10
@timoreimann timoreimann requested a review from a team as a code owner July 30, 2017 10:10
@timoreimann timoreimann force-pushed the improve-marathon-integration-tests branch from 3ba68dd to 924f071 Compare July 30, 2017 23:56
// 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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea (on both). Done.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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)
Copy link
Member

Choose a reason for hiding this comment

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

2 minutes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)


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

Choose a reason for hiding this comment

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

1 minutes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cut down to 30 seconds.

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

showTraefikLog := true
Copy link
Member

Choose a reason for hiding this comment

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

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

@timoreimann
Copy link
Contributor Author

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
Copy link
Member

ldez commented Aug 2, 2017

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

@timoreimann
Copy link
Contributor Author

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

Copy link
Member

@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

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.

LGTM

// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Updated.

@ldez ldez added size/M and removed size/M labels Aug 4, 2017
@ldez ldez added this to the 1.4 milestone Aug 6, 2017
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 @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.
@ldez ldez force-pushed the improve-marathon-integration-tests branch from c5dcd56 to 95a49ff Compare August 10, 2017 19:05
@ldez ldez merged commit 64b8fc5 into master Aug 10, 2017
@ldez ldez deleted the improve-marathon-integration-tests branch August 10, 2017 19:58
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.

None yet

4 participants