Marathon 1.5 support #327

kamsz · 2017-10-24T09:17:25Z

Tested very briefly so far, mostly with terraform-provider-marathon.

…ew structure

gambol99 · 2017-11-02T20:29:33Z

hi @kamsz ... apologizes for the delay, i'm gonna try look at this one day ..

kamsz · 2017-11-03T07:10:14Z

@gambol99 no prob, I'm using it heavily with terraform-provider-marathon and so far it works without issues.

timoreimann

AFAICS, the PR breaks backwards compatibility for clients that are not on Marathon 1.5 and haven't adjusted to the API changes yet. I think we should try hard to make the new features additive without removing the old functionality right away.

WDYT?

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

application.go

@@ -34,14 +34,6 @@ type Applications struct {
 	Apps []Application `json:"apps"`
 }

-// IPAddressPerTask is used by IP-per-task functionality https://mesosphere.github.io/marathon/docs/ip-per-task.html
-type IPAddressPerTask struct {


IP address per task seems to be removed. Is it because Marathon 1.5 does not support it anymore?

We should try hard not to break existing clients if somehow possible.

timoreimann · 2017-11-10T08:02:29Z

application.go

@@ -54,6 +46,12 @@ type Port struct {
 	Protocol string `json:"protocol,omitempty"`
 }

+// Network providers info about application networking


Typo: provides.

timoreimann · 2017-11-10T08:03:29Z

application.go

@@ -72,7 +72,6 @@ type Application struct {
 	Instances                  *int                `json:"instances,omitempty"`
 	Mem                        *float64            `json:"mem,omitempty"`
 	Tasks                      []*Task             `json:"tasks,omitempty"`
-	Ports                      []int               `json:"ports"`


Will removing this field break backwards compatibility?

timoreimann · 2017-11-10T08:03:54Z

application.go

@@ -92,14 +91,12 @@ type Application struct {
 	UpgradeStrategy       *UpgradeStrategy        `json:"upgradeStrategy,omitempty"`
 	UnreachableStrategy   *UnreachableStrategy    `json:"unreachableStrategy,omitempty"`
 	KillSelection         string                  `json:"killSelection,omitempty"`
-	Uris                  *[]string               `json:"uris,omitempty"`


Will removing this field break backwards compatibility?

timoreimann · 2017-11-10T08:04:45Z

application.go

@@ -493,7 +479,7 @@ func (r *Application) CheckHTTP(path string, port, interval int) (*Application,
 		return nil, ErrNoApplicationContainer
 	}
 	// step: get the port index
-	portIndex, err := r.Container.Docker.ServicePortIndex(port)
+	portIndex, err := r.Container.ServicePortIndex(port)


Backwards compatibility?

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

application.go

+}
+
+// HostNetwork sets the networking mode to host
+func (r *Application) HostNetwork(name string) *Application {


Instead of having three different methods to set the network mode, let's have a single one that takes a custom string type denoting the possible modes (which we can provide from a set of constants).

timoreimann · 2017-11-15T17:54:05Z

Hey there @kamsz, do you possibly have any news?

kamsz · 2017-11-16T22:05:45Z

@timoreimann Hi, not yet. I didn't have time to work on that this week. I'll try to look into this next week.

kamsz · 2017-11-23T15:22:39Z

@timoreimann I've tried my best to keep backward compatibility. I'll try to fix tests tomorrow. Could you please look at the changes and let me know what you think?

kamsz · 2017-11-24T11:10:33Z

I've fixed some tests. Not sure how to fix a group test, since methods from methods.yml would collide. Any ideas? @timoreimann @gambol99

timoreimann

I haven't been able to complete the review yet. Nevertheless, I wanted to leave notes on two major points that caught my attention: Using table-driven tests and dropping the Container part from new method names. Let me know what you think.

I'll also look into the group testing question you had. Maybe our Marathon fake framework can help with that, but I yet have to dig deeper to say for sure.

Thanks for all your work on getting go-marathon ready for 1.5! 👏 I'll get back to you as soon as I can.

timoreimann · 2017-11-24T22:32:34Z

application_test.go

@@ -42,6 +42,32 @@ func TestApplicationMemory(t *testing.T) {
 	assert.Equal(t, 50.0, *app.Mem)
 }

+func TestNewApplicationString(t *testing.T) {


Could you please try to merge this test with TestApplicationString in a table-driven fashion like we also do it here?

I think the test struct should come with a func(app *Application) that does the version-specific modifications, and another string specifying the golden test data to compare against.

Please also try to do so for the other newly created tests that are similar enough to existing tests.

timoreimann · 2017-11-24T22:38:21Z

docker.go

+//		port:			the port you are looking for
+func (container *Container) ContainerServicePortIndex(port int) (int, error) {
+	if container.PortMappings == nil || len(*container.PortMappings) == 0 {
+		return 0, errors.New("The docker does not contain any port mappings to search")


The docker sounds wrong -- should probably be The container.

timoreimann · 2017-11-24T22:41:34Z

docker.go

+	}
+
+	// step: we didn't find the port in the mappings
+	return 0, fmt.Errorf("The container port required was not found in the container port mappings")


Let's mention the container port we intend to find.

timoreimann · 2017-11-24T22:45:56Z

docker.go

@@ -255,6 +257,19 @@ func (docker *Docker) Host() *Docker {
 	return docker
 }

+// ExposeContainer sets the container to expose the following TCP ports
+//		ports:			the TCP ports the container is exposing
+func (container *Container) ExposeContainer(ports ...int) *Container {


I don't think we should have the Container part in the ExposeContainer method name: The method is already defined on a Container, so users should be able to infer the type easily (and we can't conflict).

I'm also thinking it could make it easier for people to transition from Marathon pre-v1.5 to v1.5+ if most method names stay the same. We might even reuse part of the logic. WDYT?

(Similar argumentation for other new methods also coming with a Container name part.)

kamsz · 2017-11-27T14:56:37Z

@timoreimann I've fixed most of the comments you had. Still not sure what to do with this group test.

timoreimann

I left a note on the first test in the PR and would love to see my suggestions be applied there and in the other tests. Basically, I'd like us to use table-driven tests where the setup is part of each test case (i.e., defined in the fields of the associated anonymous struct).

Regarding group tests: Do I understand correctly that you'd like the tests to return a different result from a particular endpoint than what's already specified in the YAML file today? If so, then I think scopes could solve your problem. Have you taken a look at that? Would it help?

I actually have a question on the group matter: In your PR I see group-related tests being amended but no production code getting added. Does that mean we have some kind of breaking change on the groups endpoint? Or did I miss something?

timoreimann · 2017-11-30T21:54:02Z

application_test.go

+	// Marathon >= 1.5
+	tests[1].app.
+		Container.Expose(80).Expose(443).
+		Docker.Container("quay.io/gambol99/apache-php:latest")


I think it'd be better to extend each test struct by a func(*Application) function which encapsulates the version-specific setup logic. That way, we are able to maintain all setup functionality inside tests.

Same for the other tests you have adapted.

timoreimann · 2017-11-30T21:55:22Z

application_test.go

+		Container.Expose(80).Expose(443).
+		Docker.Container("quay.io/gambol99/apache-php:latest")
+
+	for i := range tests {


I'd reference the value in the loop and access the necessary fields directly.

timoreimann · 2017-11-30T21:56:27Z

application_test.go

+
+	for i := range tests {
+		_, err := tests[i].app.CheckHTTP("/health", 80, 5)
+		assert.Nil(t, err)


We need a name/label here (set up like this) so that we can distinguish which test case actually fail.

Similar for the other assertions.

katallaxie · 2018-01-16T10:16:54Z

@kamsz are you still working on that? Or should I catch up? Because I would really love to see this in Traefik 1.5

timoreimann · 2018-01-19T15:26:09Z

@katallaxie from my perspective, I believe it's okay to carry the PR at this stage. Bringing the feature to completion would make a lot of folks quite happy. :)

Kamil Szczygieł added 8 commits October 20, 2017 09:50

move portmappings out of docker definition, network definition into n…

94d909a

…ew structure

remove ip per task

8bca4a2

add network name

6b06f58

fix docs

109dc08

addnetwork and emptynetworknames

1867d92

port mapping network names tests

494a09b

networking tests

093bf3d

remove uris/ports

d973696

gambol99 self-assigned this Nov 2, 2017

gambol99 added the enhancement label Nov 2, 2017

timoreimann requested changes Nov 10, 2017

View reviewed changes

deric mentioned this pull request Nov 23, 2017

Traefik on DC/OS (DCOS_OSS-1807). d2iq-archive/universe#1538

Merged

Kamil Szczygieł added 3 commits November 23, 2017 16:13

restore compatibility with marathon < 1.5

dd67f7c

fix service port index function

13b9fbd

fix retrieving service port in tasks

c50aabe

fix tests

f9dea27

timoreimann requested changes Nov 24, 2017

View reviewed changes

Kamil Szczygieł added 2 commits November 27, 2017 15:53

fix tests, rename new methods to match old ones

9004fe0

mention port number in error message

5b880b0

timoreimann requested changes Dec 2, 2017

View reviewed changes

timoreimann mentioned this pull request Dec 15, 2017

Marathon 1.5.0 breaks Traefik Marathon integration traefik/traefik#2115

Closed

luisdavim mentioned this pull request Jan 17, 2018

Marathon 1.5 API Support #324

Open

katallaxie mentioned this pull request Jan 28, 2018

Marathon 1.5 #335

Open

kamsz closed this Feb 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marathon 1.5 support #327

Marathon 1.5 support #327

kamsz commented Oct 24, 2017 •

edited

Loading

gambol99 commented Nov 2, 2017

kamsz commented Nov 3, 2017

timoreimann left a comment

timoreimann Nov 10, 2017

timoreimann Nov 10, 2017

timoreimann Nov 10, 2017

timoreimann Nov 10, 2017

timoreimann Nov 10, 2017

timoreimann Nov 10, 2017

timoreimann commented Nov 15, 2017

kamsz commented Nov 16, 2017

kamsz commented Nov 23, 2017

kamsz commented Nov 24, 2017

timoreimann left a comment

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

kamsz commented Nov 27, 2017

timoreimann left a comment

timoreimann Nov 30, 2017

timoreimann Nov 30, 2017

timoreimann Nov 30, 2017

katallaxie commented Jan 16, 2018

timoreimann commented Jan 19, 2018

Marathon 1.5 support #327

Marathon 1.5 support #327

Conversation

kamsz commented Oct 24, 2017 • edited Loading

gambol99 commented Nov 2, 2017

kamsz commented Nov 3, 2017

timoreimann left a 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 Nov 15, 2017

kamsz commented Nov 16, 2017

kamsz commented Nov 23, 2017

kamsz commented Nov 24, 2017

timoreimann left a 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

kamsz commented Nov 27, 2017

timoreimann left a 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

katallaxie commented Jan 16, 2018

timoreimann commented Jan 19, 2018

kamsz commented Oct 24, 2017 •

edited

Loading