Support multi-port service routing for containers running on Marathon #1742

aantono · 2017-06-13T13:55:36Z

Implementation of #1621

aantono · 2017-06-14T16:31:08Z

Rebased against master, should be good to go. Any other corrections that are required?

timoreimann

Sorry for the long delay -- real life has been super busy.

I'm half-way through the PR, so I'm sending my first batch of comments out.

I also want to take the opportunity and discuss a design question. What I dislike about the PR is that it adds a whole bunch of new methods and complicates the template significantly. Do you think it might be possible to model everything in terms of services and assign a default service when no explicit services exist? My thinking is that this would allow us to have a single implementation of the various template functions. (We might need to extend/break the signature for each and every, but personally I'm okay with that as I don't think we consider the template to be part of a stable API.)

Would also be curious to hear @emilevauge's thoughts on the overall design.

timoreimann · 2017-06-14T07:07:07Z

provider/marathon/marathon.go

@@ -279,6 +298,14 @@ func (p *Provider) applicationFilter(app marathon.Application, filteredTasks []m
 	}, filteredTasks)
 }

+func (p *Provider) hasApplication(task marathon.Task, apps []marathon.Application) bool {
+	_, err := getApplication(task, apps)
+	if err != nil {


Simplify to return err == nil.

timoreimann · 2017-06-19T15:12:34Z

provider/marathon/marathon.go

@@ -420,6 +447,187 @@ func (p *Provider) hasCircuitBreakerLabels(application marathon.Application) boo
 	return ok
 }

+// Map of services properties
+// we can get it with label[serviceName][propertyName] and we got the propertyValue
+type labelServiceProperties map[string]map[string]string


I suggest we add another type for the inner map just for readability reasons.

timoreimann · 2017-06-19T15:14:13Z

provider/marathon/marathon.go

+		log.Errorf("Unable to get marathon application from task %s", task.AppID)
+		return false
+	}
+	return len(extractServicesLabels(application.Labels)) > 0


You could also just do return hasServices(application).

timoreimann · 2017-06-27T20:08:04Z

provider/marathon/marathon.go

+	return len(extractServicesLabels(application.Labels)) > 0
+}
+
+// Extract the service labels from application labels of dockerData struct


dockerData does not seem to exist?

(if it's supposed to exist: please choose a name that doesn't contain the word data.)

timoreimann · 2017-06-27T20:08:43Z

provider/marathon/marathon.go

+
+	for index, serviceProperty := range *labels {
+		matches := servicesPropertiesRegexp.FindStringSubmatch(index)
+		if matches != nil {


Let's check for == nil and continue the loop in that case to remove one level of indentation.

timoreimann · 2017-06-27T20:37:45Z

provider/marathon/marathon.go

+		}
+	}
+
+	return v


Proposal: Instead of the regex-based approach, we could write a little parser. That is, have a piece of code that strips off the components making up our valid syntax and bail out as soon as something doesn't fit.

IMHO, that could make the implementation easier to understand and not require us to maintain a non-trivial regex at all. WDYT?

The parser route is definitely possible, though I personally prefer the regex as it will be more performant and less code to maintain. After the parser logic is all coded up, it will effectively do the same thing as Regex, scan the label text, tokenize it by dot-delimited groups, build the same result map with the rest of the logic to follow.

The regex logic is actually not very complex (IMHO).
^traefik\.(?P<service_name>.*?)\.(?P<property_name>port|portIndex|weight|protocol|frontend\.(.*))$ breaks down as following:

The label MUST start with traefik. (use of ^ indicates start of line)

The next section, defined by (?P<service_name>.*?) group is a named & numbered capturing group (submatch) that will basically extract a piece of text that sits in between . separators after traefik. segment.

The next section, defined by (?P<property_name>port|portIndex|weight|protocol|frontend\.(.*)) group is another named & numbered submatch, but conditional on property_name text segment be only one of the listed options: port, portIndex, weight, protocol or frontend.*

The rest of the label till end of line/entry, defined by $

The reason this regex looks complex is due to using named groups and having to use \ in many places to escape the . :)

The issues with the parser are:

instead of 3 lines of code used in regex (regex definition, servicesPropertiesRegexp.FindStringSubmatch(index) and servicesPropertiesRegexp.SubexpNames()) we would have much more code for the parser logic

parser logic would be custom-written, needs to be extensively tested and add complexity that has to be maintained long term

regex performance is already very good (Google guys have done their part. :) ), the parser might need optimizations to tune it to make sure performance is not suffering.

Thoughts?

Thanks for the explanation. My main concern is about how the regular expression is applied in the code, and how the regexp package API is used to extract the kind of information we are looking for. To be honest, I think the API isn't the most intuitive, which may stem from the fact that regular expressions aren't the most trivial topic.

Let me share my thoughts on your issues with the parser approach:

code size. Yes, we'd have somewhat more code. IMO though, I think it'd be easier to understand. When I tried to understand your regex, I basically had to jump back and forth between the regex and the algorithm in extractServicesLabels to convince myself that the code is really covering all cases. Personally, I tend to prefer a bit less dense code at the benefit of better comprehensibility.

parser logic/testing. For sure we'd need a custom parser. I disagree on the testing argument though: The regex and the parser approaches should have roughly the same test cases. In fact, I think the more explicit nature of the parser approach make it much clearer to see which cases need to be tested. On the contrary, it's harder to figure out the circumstances under which a regular expression matches or not (which is what took me so long to understand).

performance. Regexes require full-blown state machines whereas parsing is mostly a matter of simple string comparisons and manipulations. So I have my doubts whether the regex will outperform the parser. More importantly though, I wouldn't worry about this matter at all: It probably takes a lot of regex'ing/parsing for any performance impact to be observable, and even if we think it was, we'd need benchmarks to prove it. For now, let's focus on a solution that's easiest to read, comprehend, and test.

I'll add another point in favor of parsers: they are often simpler to extend. When the structure of what your regular expression has to match changes, chances are you'll have to rebuild the expression or start introducing hard-to-understand features like look-ahead/look-behind. That's usually where things get ugly.

FWIW, the stdlib is full of parsers, so at least the Go team thinks it's a good and fast-enough thing. :)

One implementation speaks louder than 100 discussions, so I went ahead and implemented the parser for comparison reasons. I admit it's a bit larger (especially because your implementation can be further simplified now that we say the service name is mandatory -- I'll address that in a separate comment). OTOH, it's super clear to see what to expect from the label pattern when being parsed, which makes writing down all test cases a rather mechanical matter.

Overall, I'm leaning towards the parser for clarity reasons, but I can see the advantage of a shorter implementation when using a regular expression. I guess I can convince myself that staying with what we have right now in the PR is acceptable, though I wouldn't want to leave this comment thread without a rather complete discussion. :-)

I'll also try to get some additional feedback from the other maintainers on this point once we've covered the remainders of the PR.

Those are fair points. Do you think we can go ahead with this PR as is, and have a separate PR for creating a "generic" parser to handle the label service tokenization and extraction? I can see how that (Parser) can be a generic/common facility used by pretty much any provider that has a "service" concept. (like various Docker derivatives [docker, kubernetes, mesos, marathon, etc.])

Good idea. Okay with me. 👌

timoreimann · 2017-06-27T20:41:24Z

provider/marathon/marathon.go

+
+// Gets the entry for a service label searching in all labels of the given application
+func getApplicationServiceLabel(application marathon.Application, serviceName string, entry string) (string, bool) {
+	value, ok := extractServicesLabels(application.Labels)[serviceName][entry]


AFAICS, this would panic if serviceName didn't exist in the map. We should cover that case and log a warning.

It would not panic (just tested the usecase on play.golang.org) The ok would equal false and value would have default string value of ""

We do check for ok in all the places we call getApplicationServiceLabel, like getServicePriority for example, so that covers the use-case (IMHO)

timoreimann · 2017-06-27T20:43:29Z

provider/marathon/marathon.go

+	application, err := getApplication(task, applications)
+	if err != nil {
+		log.Errorf("Unable to get marathon application from task %s", task.AppID)
+		return make([]string, 0, 0)


Let's just return []string{} or []string{""}.

timoreimann · 2017-06-27T20:50:04Z

provider/marathon/marathon.go

+	}
+	// Returning an empty name, as an indication that there are no sub-services, but only main Application
+	return []string{""}
+}


I'd change the implementation like this:

Start off with a var names []string declaration.

Iterate over labelServiceProperties (without checking the size), appending each key to names as we go.

Returnnames.

Seeding the slice with the known length shouldn't be required performance-wise for the number of services we'll usually we dealing with.

timoreimann · 2017-06-27T20:57:03Z

provider/marathon/marathon.go

@@ -502,7 +710,7 @@ func processPorts(application marathon.Application, task marathon.Task) (int, er
 		return 0, errors.New("no port found")
 	}

-	portIndex := 0
+	//portIndex := 0


Remove the line.

aantono · 2017-06-28T14:50:12Z

Hello @timoreimann, thanks for the detailed feedback. The current implementation was modeled (with some code copying) from the existing Docker service labels support. I was trying to keep the same design, etc. Let me know if you would like to deviate from how it is currently done in the Docker provider, would be happy to do so. (especially if we are not required to keep the template "API" backwards-compatible).

timoreimann · 2017-07-01T21:26:31Z

Let me know if you would like to deviate from how it is currently done in the Docker provider, would be happy to do so. (especially if we are not required to keep the template "API" backwards-compatible).

I'm currently checking back with the other maintainers as to what our understanding is towards any API guarantees. My personal opinion is that compatibility should only be maintained on the higher template rendering level (meaning that the same template input parameters should ideally yield the same output configuration across different Traefik versions) but should not apply to the specific template functions.

Once we have clarity on this point, we can decide how to proceed. I'd feel much more comfortable if we had the desirable freedom so that we don't have to add too much complexity to the Marathon provider.

Will get back to you on this one ASAP!

timoreimann

Two other comments.

I'll continue with the review as soon as we have clarity on the template API point, as this is crucial in deciding whether we'll stick to the current implementation or have the freedom to refactor more aggressively.

timoreimann · 2017-07-01T21:32:13Z

provider/marathon/marathon.go

+				continue
+			}
+
+			result := make(map[string]string)


Here's the promised comment I announced in another comment:

I don't think we need the result map. Our regular expression pattern basically guarantees us that we'll have service_name and property_name sub-matches. So we can just extract the names/values at the known index positions and use them to populate the labelServiceProperties map.

timoreimann · 2017-07-01T21:33:11Z

provider/marathon/marathon.go

+			}
+			serviceName := result["service_name"]
+			if _, ok := v[serviceName]; !ok {
+				v[serviceName] = make(map[string]string)


make can now use serviceProperties.

timoreimann · 2017-07-01T21:34:23Z

provider/marathon/marathon.go

@@ -420,6 +444,193 @@ func (p *Provider) hasCircuitBreakerLabels(application marathon.Application) boo
 	return ok
 }

+// Map of services properties
+// we can get it with label[serviceName][propertyName] and we got the propertyValue
+type serviceProperties map[string]string


Since this is a map from property (names) to values, how about calling it servicePropertyValues?

timoreimann · 2017-07-06T08:04:00Z

provider/marathon/marathon.go

+				continue
+			}
+
+			// Accordig to the regex, match index 1 is "service_name" and match index 2 is the "property_name"


Typo: Accordig -> According

timoreimann · 2017-07-06T08:05:52Z

provider/marathon/marathon.go

+			// Accordig to the regex, match index 1 is "service_name" and match index 2 is the "property_name"
+			serviceName := matches[1]
+			propertyName := matches[2]
+			v[serviceName] = make(servicePropertyValues)


You dropped the check if the servicePropertyValues map exists. Aren't you now overwriting any pre-existing value with a brand new map on every loop iteration?

Good catch!

timoreimann · 2017-07-06T08:10:38Z

@aantono could you please only push commits (as opposed to rebasing) during the review? Otherwise, it's very hard to see which changes you added. Thanks!

timoreimann · 2017-07-06T08:11:08Z

Tests are failing. :-)

timoreimann · 2017-07-06T11:40:28Z

@aantono looks like we're okay to consider covering the extended service-based functionality by the existing template functions, even if it means adjusting the signatures.

I haven't deeply explored the idea but think it should be feasible. Would you mind taking a stab and see how far we can get with this? Thanks!

aantono · 2017-07-06T13:59:09Z

@timoreimann, sounds good. I will try to model everything in terms of services and assign a _default_ service when no explicit services exist. Am I correct to understand that we are OK with removing the non-service-based methods from the Template?

timoreimann · 2017-07-06T15:16:38Z

@aantono:

Am I correct to understand that we are OK with removing the non-service-based methods from the Template?

Yes, I think we're free to go wild. :-)

timoreimann

Reflecting a bit, I believe we're finally good on this PR. A quick test with our Marathon cluster also showed no problems.

LGTM -- thanks a lot @aantono, excellent work. 👏

timoreimann · 2017-08-18T22:37:58Z

@containous/docker could someone please review the tiny improvement to the Docker regex?

timoreimann · 2017-08-18T22:41:13Z

@aantono can I leave the resolution of the conflicts to you? I probably won't have too much time over the next couple of days.

aantono · 2017-08-19T00:38:39Z

Sounds good. I'll rebase as soon as I get home.

…ional parameter to the template methods.

…y required. Reverted accidental Metrics PR changes and general code cleanup and docs.

… methods to make them more separate.

Additional changes: - Filter tasks with multiple services if at least one service is missing its port. - Extend tests for previous case. - Use %q for logging.

… being invalid value, service does not get filtered

We cannot filter services while the server rendering will eventually drop broken services anyway.

aantono · 2017-08-19T01:59:55Z

Rebased, should be good to merge now...

timoreimann · 2017-08-19T05:23:44Z

@aantono the build is red now. Something must have gone wrong during your rebase. Could you check please?

…ices that have some invalid ports.

vdemeester

LGTM 🐸

timoreimann · 2017-08-21T08:14:25Z

I'm going to have a final look at the diff. Please don't merge yet.

ldez · 2017-08-21T08:50:11Z

@timoreimann Sorry I did not see your comment 😞
Next time add bot/need-human-merge

timoreimann · 2017-08-21T08:53:01Z

@ldez no worries, will do next time.

Everything looks fine anyway, so no harm done. :)

ldez added area/provider/marathon kind/enhancement a new or improved feature. labels Jun 13, 2017

timoreimann self-requested a review June 13, 2017 14:12

ldez added the contributor/waiting-for-corrections label Jun 13, 2017

aantono force-pushed the Issue1621 branch 3 times, most recently from 2eb5931 to bc13b74 Compare June 14, 2017 16:30

ldez added status/1-needs-design-review and removed contributor/waiting-for-corrections labels Jun 14, 2017

timoreimann suggested changes Jun 27, 2017

View reviewed changes

emilevauge added status/2-needs-review and removed status/1-needs-design-review labels Jun 27, 2017

aantono force-pushed the Issue1621 branch 3 times, most recently from 8c50caf to 459ec75 Compare June 30, 2017 18:42

timoreimann suggested changes Jul 1, 2017

View reviewed changes

aantono force-pushed the Issue1621 branch from 459ec75 to 57c7415 Compare July 5, 2017 15:04

timoreimann suggested changes Jul 6, 2017

View reviewed changes

ldez added the contributor/waiting-for-corrections label Jul 6, 2017

ldez modified the milestone: 1.4 Jul 7, 2017

juliens added the contributor/needs-resolve-conflicts label Jul 10, 2017

ldez added the contributor/needs-resolve-conflicts label Aug 18, 2017

timoreimann approved these changes Aug 18, 2017

View reviewed changes

aantono and others added 12 commits August 18, 2017 20:46

Doc fix

d3aeaa0

Added support for multi-port Marathon applications

0d202d9

Fixed typo, added map check

28105ab

Reworked Marathon template to collapse serviceName argument as an opt…

c799a37

…ional parameter to the template methods.

Removed optional serviceName from template methods, made it explicitl…

ab10c7e

…y required. Reverted accidental Metrics PR changes and general code cleanup and docs.

GoDoc cleanup, unit-test changes, refactored getPort and processPorts…

ad5438e

… methods to make them more separate.

Added integration test for application with service tags, added docs.

fc6b8d8

Code and test cases cleanup

e1269d7

Refactored unit test cases

d6f5132

Return to original port processing structure.

6f87f7c

Additional changes: - Filter tasks with multiple services if at least one service is missing its port. - Extend tests for previous case. - Use %q for logging.

Added test case to verify that if service port is missing, instead of…

8df777b

… being invalid value, service does not get filtered

Log illegal service configurations only.

30cf659

We cannot filter services while the server rendering will eventually drop broken services anyway.

aantono force-pushed the Issue1621 branch from b391b53 to 30cf659 Compare August 19, 2017 01:55

Fixed tests after rebase to reflect that we no longer filter out serv…

def9bbb

…ices that have some invalid ports.

ldez removed the contributor/needs-resolve-conflicts label Aug 19, 2017

vdemeester approved these changes Aug 21, 2017

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Aug 21, 2017

traefiker merged commit ec3e2c0 into traefik:master Aug 21, 2017

traefiker removed the status/3-needs-merge label Aug 21, 2017

Support multi-port service routing for containers running on Marathon #1742

Support multi-port service routing for containers running on Marathon #1742

Conversation

aantono commented Jun 13, 2017

aantono commented Jun 14, 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

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

aantono commented Jun 28, 2017

timoreimann commented Jul 1, 2017

timoreimann left a comment • edited

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 Jul 6, 2017

timoreimann commented Jul 6, 2017

timoreimann commented Jul 6, 2017

aantono commented Jul 6, 2017

timoreimann commented Jul 6, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann commented Aug 18, 2017

timoreimann commented Aug 18, 2017

aantono commented Aug 19, 2017

aantono commented Aug 19, 2017

timoreimann commented Aug 19, 2017

vdemeester left a comment

Choose a reason for hiding this comment

timoreimann commented Aug 21, 2017

ldez commented Aug 21, 2017

timoreimann commented Aug 21, 2017

timoreimann left a comment •

edited