Marathon 1.5 API Support

[kshitizbakshi]

Hi,
DC/OS 1.10 Stable has been released. It brings along Marathon 1.5, which introduces breaking changes to the API. Specifically, the networking section has been revamped with new features. Marathon 1.5 changelog.

Marathon 1.5 will still accept 1.4-based app definitions, but will only generate 1.5-based outputs. This would mean that this library would stop working with Marathon 1.5.

Is there any plan already in place for supporting the new API?

[timoreimann]

@kshitizbakshi thanks for your issue.

go-marathon is driven by volunteers only -- I'm not aware of anyone that has committed adding support for 1.5 already. Feel free to take a stab yourself if you like to drive this forward.

[mikekatica]

I am experiencing these problems in Traefik. So far, the only difference in the Marathon API I have found is that "ipAddress" no longer is part of the top level app api. So IP-Per-Task always returns nil. I will take a stab at fixing that.

[mikekatica]

So far, I have confirmed this for Docker runtime services. Marathon 1.5 does not add an empty "IPAddress" section like 1.4 does. Might be best to just handle that outside of the library.

[kshitizbakshi]

There are more differences, which also correspond to new features in this version of DC/OS. The whole networking API has been revamped, since the UCR and the Docker containerizer now have feature parity on networking. I'll try to summarize some of these changes here.

The new API moves networks to a top level field, and networks is no longer a part of the container.docker field.

The network type names have also changed, to "host", "container", and "container/bridge".

About IP-Per-Container, I think the way to go now is to use the top level "networks" field in the Marathon app definition, use the network type "container" and choose the network "name" of the IP-Per-Container network to use. In a DC/OS Mesosphere templated AWS installation, the network name is "dcos". Not very sure on this yet, having not used the feature myself.

The new API also supports Port mappings for both UCR and Docker, which was not possible before. Port mappings has moved to container.portMappings.

More details are in the networking documentation for Marathon 1.5.

[kshitizbakshi]

One small issue I see here is that even if someone patched support for Marathon 1.5, how do we maintain support for the 1.4 API as well alongside? It will take time for a lot of people to move from 1.4 to 1.5.

[timoreimann]

@kshitizbakshi thanks for the summary.

You raise a very good question about the compatibility point. I can see at least two ways to deal with this:

1. add convenience getters for API-broken fields that try retrieving values from the new fields and in the case of absence try the old one. Of course, this only works if we can tell new and old field versions apart non-ambiguously.
2. just add support for the new fields but leave it up to the users to know which field version they need to request and do a necessary switch themselves.

We might not be able to make a general decision here -- some cases may be harder to cover than others. I suggest we look at things on a case by case basis and decide individually.

Thoughts?

[mikekatica]

Might be best to do a combination of the two. I am currently starting work in the marathon1.5 branch, and already have the "Networks" piece done, but when doing portDefinitions vs portMappings, it might be possible to roll them together. I want to get some feedback before I commit.
Relevant Marathon API

[timoreimann]

@mikekatica what's your current plan with regards to the port specs?

go-marathon already supports both. My gut feeling is that we shouldn't try to conflate the two. What we could do is to provide convenience setters specific to mode structs we'd define (host vs. container). Those would come on-top of the basic primitives you could use to hook up the right combination of mode and ports on your own.

It's probably easiest to discuss on the basis of some code -- feel free to push early and often so we can better see where we are heading and iterate on it continuously.

[mikekatica]

Sure. I wasn't sure where the handling of the portMappings came from, but that seems like something I can just dig for. Yes, I agree it is best to handle setting them where applicable based on the network definition.

[mikekatica]

@timoreimann Can you point me in the right direction here? I have run some test code against my working marathon instance, and the portMappings do not show from app.PortDefinitions. I must be missing something.

[mikekatica]

Ahh, the "Ports" part of the application API is deprecated. So looks like I'll add PortMappings, and modify the setter to pick the right one based on the container network type.

[mikekatica]

Looks like portMappings in Container.Docker is not being populated properly for some reason. Otherwise, should be correct.

[kshitizbakshi]

I think the portMappings in Container.Docker is removed now, and hence the API response from Marathon wouldn't have it. The new place for portmappings is Container.PortMappings.

Marathon 1.5 would still accept the app definitions in the old style, but when it gives you responses it'll use the new style.

[kshitizbakshi]

Ah, I see now that you've already made that change in your feature branch.

Also a general question here - we don't do any validation in Go-Marathon, right? For example, port mappings is only applicable for container/bridge networks (also container, maybe), and portDefinitions only for host networks. We leave that intelligence to the user?

I was thinking of adding a UCR container type option (that I did preliminarily add in my fork), but in that I also added forced setting of false to some parameters which weren't supported in UCR. Is that something we should be doing?

[timoreimann]

@kshitizbakshi I think a custom UCR container could fit well as we already have a small amount of container structs and helper methods producing values guaranteed to be consistent with Marathon semantics.

We should just make sure that users can still achieve the same by constructing everything manually to ensure that arbitrary configurations can be put together even in the absence of a particular factory method in go-marathon.

[kshitizbakshi]

What exactly do you mean by custom UCR container? Like a application.Container.UCR construct in go-marathon?
Because although there is a field "container.Docker" in the Marathon API, there isn't any for container.UCR; the API is weird in that sense. The way to set the UCR is to set the "type" to "MESOS" instead of "DOCKER". the container.Docker section still remains, which can define the Docker image.

[timoreimann]

@kshitizbakshi on second thought, a custom UCR type as I imagined it might not make so much sense since we'd supposedly have to duplicate many of the methods currently implemented on the Docker type.

So your idea to add a type option might be a better approach. Where and how would you fit it in exactly? I guess seeing some of your code would answer that question most easily.

There's also the option to implement the builder pattern where we would validate the input on the final call to a build() method.

[kshitizbakshi]

@timoreimann The way I did it in my fork to quickly test things out, is in this commit. I'm new to Go, so some things might not be idiomatic. The type option already exists, I just created a convenience function on Container to set the type to "MESOS". In addition, I had also forced some variables to the only-supported-values-with-UCR. This is the part I was especially unsure of, considering the scope of the library.

Some of those options have changed in 1.5 (example, forcePullImage is now supported in UCR I believe).

The builder pattern sounds cool, but the bigger question to answer is if input validation in terms of cross-matching parameters in scope of the library? The correct combination of options keeps changing rapidly.

[timoreimann]

@kshitizbakshi I'm okay with the approach you have taken. Technically, I wouldn't call it forcing parameters since nothing would keep users from trying to set the wrong port kind. That might not be necessary though, so let's keep it simple until we know it's not good enough and we can do better.

One thing though: we already have NewDockerContainer, so how about naming your function NewUCRContainer and let it return a properly initialized container to the caller?

[kshitizbakshi]

Yeah that sounds much better, thanks! I guess we could also have a NewUCRApplication function corresponding to the NewDockerApplication one.
But anyway, sorry for causing digression; this issue is about the 1.5 Networking API changes. Lets continue this when I create a PR for UCR?

[timoreimann]

@kshitizbakshi I'm fine with using this issue as a tracker for everything related to getting go-marathon ready for Marathon 1.5. For more specific discussions, however, it's probably good to spawn off separate issues and/or PRs.

So yeah, please go ahead and submit a PR. Happy to review (though I'm somewhat short on time right now and still have to finish the Pod support PR first).

[kamsz]

@timoreimann @kshitizbakshi I've started working on migrating to 1.5 API here: https://github.com/ContainerLabs/go-marathon/tree/marathon1.5.

[katallaxie]

When do we see this in master? There is a related issue traefik/traefik#2115 which would then be fixed.

[luisdavim]

It would be great if this lib would have some support from mesosphere.

[luisdavim]

this PR #327

[kevinschoon]

FWIW I am running the code in #335 against Marathon 1.5 and it has been working mostly well. An ideal situation would be Mesosphere releasing protobuf definitions along side Marathon but their code doesn't seem setup to do that.