Marathon 1.5 #335

katallaxie · 2018-01-28T03:08:29Z

Adds to PR #327

Adds suggestions by @gambol99 in Marathon 1.5 support #327
Fixes linting errors
Improved https://goreportcard.com/report/github.com/katallaxie/go-marathon

…ew structure

timoreimann

Thanks for picking up the work!

Could you please move the dep-related changes into a separate PR? It requires a bit more before I'd consider ourselves to be "dep ready" (namely, extension of the Makefile and improvements on the README). While I definitely like to use dep to improve test stability, I'd rather not have the effort block progress on the Marathon 1.5 support.

katallaxie · 2018-01-28T09:44:18Z

👍 I have removed it. I can readly add it after this PR.

timoreimann · 2018-01-28T10:39:14Z

.gitignore

@@ -20,6 +20,7 @@ coverage
 # Folders
 _obj
 _test
+vendor


For the sake of completeness, this should still be undone.

timoreimann

Some more selected notes. More importantly to me, however, are the remarks I left at #327 (review).

I am also not clear if the changes we make are 100% backwards compatible with Marathon 1.4. Ideally, we can maintain API compatibility so users coming from 1.4 don't need to change anything. But I'm not sure if that's realistic to achieve. (I haven't digged into the concrete version delta yet.)

timoreimann · 2018-01-28T10:48:38Z

queue.go

@@ -32,7 +32,7 @@ type Item struct {
 	Application Application `json:"app"`
 }

-// Delay cotains the application postpone infomation
+// Delay cotains the application postpone information


The second word also comes with a typo. :-)

timoreimann · 2018-01-28T10:50:26Z

group_test.go

 			t.Fail()
 		}
 	}
 }
+
+// TODO @kamsz: How to work with old and new endpoints from methods.yml?


This TODO still needs to be resolved.

We could just leave them, because in 1.5 they are moved to a canonical representation when they are used.

Not perfectly sure I understand. What do you mean by canonical representation?

Ok, I just meant we could leave the fields in the fakeendpoints and test for both. Because in 1.5 they are moved to a canonical representation anyway as they are used. Or, one would not have to test the old ones anyway.

So, how do you suggest to proceed?

IIUC, with Marathon 1.5 we will continue to be able to write in the old format, while reading only returns the canonical representation (with deprecated fields being dropped).

In that case, I think we should have tests for reading both the old format (for go-marathon clients that haven't upgrade to Marathon 1.5+ yet) and the new format. At some point in the future, we can deprecate the old format.

WDYT?

timoreimann · 2018-01-28T10:52:23Z

application_test.go

-	expectedAppJSONBytes, err := ioutil.ReadFile("tests/app-definitions/TestApplicationString-output.json")
-	if err != nil {
-		panic(err)
+	type test struct {


We can use an anonymous struct here as the type will never be reused.

ok, it was just for sake of clarity.

timoreimann · 2018-01-28T11:08:31Z

README.md

@@ -1,5 +1,6 @@
 [![Build Status](https://travis-ci.org/gambol99/go-marathon.svg?branch=master)](https://travis-ci.org/gambol99/go-marathon)
 [![GoDoc](http://godoc.org/github.com/gambol99/go-marathon?status.png)](http://godoc.org/github.com/gambol99/go-marathon)
+[![Go Report Card](https://goreportcard.com/badge/github.com/katallaxie/go-marathon)](https://goreportcard.com/report/github.com/katallaxie/go-marathon)


This points at the wrong repo. But really, I'd rather have this added by a separate PR as well.

Remember that if we have to revert the final commit that will end up in master for whatever functional reason, all drive-by changes will be gone as well.

Lets keep it for now :)

timoreimann · 2018-02-07T00:14:20Z

Hey @katallaxie, any updates available?

Apart from the question on how we should test functionality against pre-Marathon 1.5 and 1.5+, respectively, my other request to transform tests into table-driven forms should be more easily feasible.

Let me know your thoughts and/or questions.

alenkacz · 2018-03-03T20:32:03Z

is there a way I can help to move this forward?

timoreimann · 2018-03-03T22:27:07Z

From my point of view, when trying to finish this PR we should strive for backwards compatibility: Consumers upgrading to a Marathon 1.5-compatible version of go-marathon should ideally not need to make any code changes, regardless of whether they use Marathon 1.4 or 1.5.

If backwards compatibility is very hard to achieve in one aspect or another, we can discuss whether we can introduce a new API, break an existing API to force consumers to adjust, or come up with another resolution. All things equal, an approach maintaining backwards compatibility should be preferred.

alenkacz · 2018-03-04T20:18:36Z

@timoreimann After looking at the code, these are my thoughts...

keep old and new set and get methods for networks and ports
by using either of them, set both variants of fields (1.4 and 1.5) - make sure 1.4 can accept jsons with new fields added in 1.4
— so e.g. Application.SetNetwork would set network in both old and new way
potentially we can read Marathon version out of info endpoint and use it to fill one or the other field based on that version (which is probably a cleaner solution)

Sounds good?

timoreimann · 2018-03-05T02:14:13Z

@alenkacz sounds good to me.

Reading the version and applying the right version-specific functionality would indeed enable us to minimize code complexity. I'm trying to think of ways where this may not work though... supposedly, the approach can't handle the case where a Marathon 1.4 cluster is rolling upgraded to 1.5, and consumers expect their (already upgraded) go-marathon clients to continue to work without a restart? For high availability scenarios, this could be too much of a problem, and we might need to stick to the idea of making all API calls dual-supportive. WDYT?

alenkacz · 2018-03-05T07:18:54Z

@timoreimann good point, but I would not be concerned about that. With minor updates between versions there are usually zookeeper migrations involved so you would not want to have both old and new versions running at the same time... Also the suggested update guide in the docs https://mesosphere.github.io/marathon/docs/upgrade/index.html confirms that

timoreimann · 2018-03-06T08:21:02Z

@alenkacz the Upgrading an HA installation section from that document describes that you should run both old and new Marathon instances at one point in time. This makes a lot of sense to me as certain kinds of environments may not be able to tolerate downtime. (FWIW, that also happens to be how my organization has been doing the upgrade procedure for the past ~1.5 years.)

Coupling go-marathon's runtime behavior to the Marathon version also means that you possibly cannot switch back and forth between versions that easily. This is important when you find your new Marathon version to be broken for you, and you want to roll back to a previous version without having to restart a potential high number of go-marathon clients.

In summary, I believe it's worth trying to populate and read structs for both versions statically. If we find out that it's not possible for one reason or another, we can look for alternatives; my preference then would go towards pushing the problem back to the consumer so he/she can deal with it.

alenkacz · 2018-03-06T08:27:04Z

@timoreimann well the version would have to be negotiated for a single connection and there is no way that you can switch versions without losing a connection to that node...

But OK, let's try to go the other way, I think it is good approach as well...

alenkacz · 2018-03-18T09:42:12Z

@timoreimann on a second look I unfortunately don't think there is a good way to do this in a backward compatible manner :( Currently this client API is very tightly coupled with the JSON so there are methods like ExposePort(docker *Docker) called on a Docker object, while 1.5 requires similar thing but on an object one level higher ExposePort(container *Container). From Container we could set both docker and container, but from Docker we cannot set anything on container (so we cannot set new 1.5 property from current client method).

Because of that, there will never be a smooth update from 1.4 to 1.5 in terms of this client (if your client code uses the API features that were changed, you will have to change your code).

Few things come to my mind:

keep both new and old portmappings in the application structure (basically how this PR looks right now) - which means that when you update from 1.4 to 1.5 your code will still compile but you won't be getting the data you are expecting from the methods you are using (this is the easiest way, but from a user perspective not very nice way)
drop old methods so e.g. drop ExposePort(docker *Docker) and have only ExposePort(container *Container) and then based on marathon version set old or new way or both as we discussed (I will have to check how Marathon behaves when both old and new are set in terms of normalization - I would hope that old will get ignored) - this way when users update, their code won't compile BUT will work smoothly when they update their cluster (I like this way more)
more options... ? I'll keep thinking :)

In any way it would be great if there was something in between client and the actual json serialization. If that was the case, this migration would be easier...

I'll try to work on idea n.2 since n.1 is what we have right now :)

timoreimann · 2018-03-18T13:54:19Z

@alenkacz thanks for the update. I'm still somewhat worried about the approach to automatically adjust API calls based on the returned Marathon version. HTTP doesn't know sessions, so technically we won't lose a connection when hosts are swapped out behind the scene unless IP addresses are used and changed, right? (In my organization, we have been pointing go-marathon at DNS records only that we map to new hosts when we upgrade.)

If automatic conversion is not feasible, then I'm inclined to say we go with what this PR provides already. It's been a while since I have looked at it, but I'd be okay if it either populated both 1.4 and 1.5 fields from the new 1.5 API calls; or if it only populated the 1.5 fields from the 1.5 calls, requiring the user to populate the 1.4 fields instead. We can then first try to get this version out of the door and always iterate on it if we feel like it's worthwhile. I'd only like to make sure we don't break the API unnecessarily, unless there's a strong reason to. IMHO, users running on pre-1.5 shouldn't be "punished" by API changes they may not be affected by for potentially a long time.

WDYT?

alenkacz · 2018-03-18T15:38:53Z

@timoreimann yeah I got to the same conclusion looking at the code - since getting version before the request means doing two separate request, we are not guaranteed that it will be reliable (not talking about the network overhead).

What I don't like about the current solution is that

I have client against 1.4 setting network via old way and reading it from the old fields
then i switch to 1.5
I can still submit the apps through the old way (with 1.4 fields) but when I read from the API, the network information is accessible through a different fields and my old code suddenly does not work and I am informed about this in the runtime...

The whole problem is that the 1.5 networking changes were kind of messy and not backward compatible at all... so if we could hide this mess at least from the clients of this library, that would be nice :) I'll try something and we'll see. My 3 week vacation starts on Tuesday so we'll see how far I get :)

timoreimann · 2018-03-18T16:09:55Z

@alenkacz I guess you could still maintain read compatibility by making your client code check if a Marathon 1.5 field is populated, and if it is not fall back to the 1.4 field. It takes some extra logic outside of the library, but would be a good first step for users willing to pay a little price in order to use the library against Marathon 1.5 rather sooner than later.

alenkacz · 2018-03-18T17:25:39Z

@timoreimann yeah sure, if there is a big push to releasing that I guess what's in this branch is better than nothing - even though it is not the most pleasant client-side experience :)

Kamil Szczygieł added 14 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

restore compatibility with marathon < 1.5

dd67f7c

fix service port index function

13b9fbd

fix retrieving service port in tasks

c50aabe

fix tests

f9dea27

fix tests, rename new methods to match old ones

9004fe0

mention port number in error message

5b880b0

timoreimann requested changes Jan 28, 2018

View reviewed changes

Döll, Sebastian and others added 3 commits January 28, 2018 11:44

~ adding fixing tests and linting for marathon 1.5

63b9934

Merge branch 'master' into marathon1.5

c064ab4

~ adding go report card

e2b51e3

timoreimann requested changes Jan 28, 2018

View reviewed changes

kevinschoon mentioned this pull request Feb 19, 2018

Marathon 1.5 API Support #324

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marathon 1.5 #335

Marathon 1.5 #335

katallaxie commented Jan 28, 2018 •

edited

Loading

timoreimann left a comment

katallaxie commented Jan 28, 2018

timoreimann Jan 28, 2018 •

edited

Loading

katallaxie Jan 28, 2018 •

edited

Loading

timoreimann left a comment

timoreimann Jan 28, 2018

katallaxie Jan 28, 2018

timoreimann Jan 28, 2018

katallaxie Jan 28, 2018

timoreimann Jan 28, 2018

katallaxie Jan 28, 2018 •

edited

Loading

katallaxie Jan 30, 2018

timoreimann Jan 30, 2018

timoreimann Jan 28, 2018

katallaxie Jan 28, 2018

timoreimann Jan 28, 2018

katallaxie Jan 28, 2018

timoreimann commented Feb 7, 2018

alenkacz commented Mar 3, 2018

timoreimann commented Mar 3, 2018

alenkacz commented Mar 4, 2018

timoreimann commented Mar 5, 2018

alenkacz commented Mar 5, 2018

timoreimann commented Mar 6, 2018 •

edited

Loading

alenkacz commented Mar 6, 2018

alenkacz commented Mar 18, 2018 •

edited

Loading

timoreimann commented Mar 18, 2018

alenkacz commented Mar 18, 2018

timoreimann commented Mar 18, 2018

alenkacz commented Mar 18, 2018

Marathon 1.5 #335

Are you sure you want to change the base?

Marathon 1.5 #335

Conversation

katallaxie commented Jan 28, 2018 • edited Loading

timoreimann left a comment

Choose a reason for hiding this comment

katallaxie commented Jan 28, 2018

timoreimann Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

katallaxie Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

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

katallaxie Jan 28, 2018 • edited Loading

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 Feb 7, 2018

alenkacz commented Mar 3, 2018

timoreimann commented Mar 3, 2018

alenkacz commented Mar 4, 2018

timoreimann commented Mar 5, 2018

alenkacz commented Mar 5, 2018

timoreimann commented Mar 6, 2018 • edited Loading

alenkacz commented Mar 6, 2018

alenkacz commented Mar 18, 2018 • edited Loading

timoreimann commented Mar 18, 2018

alenkacz commented Mar 18, 2018

timoreimann commented Mar 18, 2018

alenkacz commented Mar 18, 2018

katallaxie commented Jan 28, 2018 •

edited

Loading

timoreimann Jan 28, 2018 •

edited

Loading

katallaxie Jan 28, 2018 •

edited

Loading

katallaxie Jan 28, 2018 •

edited

Loading

timoreimann commented Mar 6, 2018 •

edited

Loading

alenkacz commented Mar 18, 2018 •

edited

Loading