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

marathon-sd - use Tasks.Ports instead of PortDefinitions.Ports if RequirePorts is false #5026

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
4 participants
@tommarute
Copy link
Contributor

tommarute commented Dec 21, 2018

Fixes #5022

@tommarute tommarute force-pushed the tommarute:fix-marathon-sd branch 2 times, most recently Dec 21, 2018

@simonpasquier
Copy link
Member

simonpasquier left a comment

I'll delegate to @rjanovski @adrien-f @ti-mo to evaluate the correctness of the fix.

Show resolved Hide resolved .gitignore Outdated
@ti-mo
Copy link
Contributor

ti-mo left a comment

Feels like this should have broken some tests, please also take a look. With this patch, portDefinition labels should not be gathered when requirePorts is false.

discovery/marathon/marathon_test.go Outdated
@@ -747,6 +822,7 @@ func marathonTestAppListWithContainerNetworkAndPortDefinition(labels map[string]
Container: container,
Networks: networks,
PortDefinitions: portDefinitions,
RequirePorts: true,

This comment has been minimized.

Copy link
@ti-mo

ti-mo Dec 21, 2018

Contributor

Apps in container networks should never have requirePorts set to true. (they don't bind onto the host network namespace) Not sure if Marathon accepts requirePorts with container networking, but if it does, it won't have any effect. Feel free to remove this line and drop the marathonTestAppListWithContainerNetworkAndPorts test below.

This comment has been minimized.

Copy link
@tommarute

tommarute Dec 26, 2018

Author Contributor

I get it. Thanks.

By the way, Should I delete this and TestMarathonSDSendGroupWithContainerNetworkAndPortDefinition? It seems that apps in container networks doesn't have portDefinitions as far as I confirmed my configuration of marathon.

This comment has been minimized.

Copy link
@ti-mo

ti-mo Jan 8, 2019

Contributor

Good catch, that's not a useful test case, but I think I included it for completeness. Feel free to remove.

discovery/marathon/marathon_test.go Outdated
@@ -358,6 +358,7 @@ func marathonTestAppListWithPortDefinitions(labels map[string]string, runningTas
{Labels: make(map[string]string), Port: 31000},
{Labels: labels, Port: 32000},
},
RequirePorts: true,

This comment has been minimized.

Copy link
@ti-mo

ti-mo Dec 21, 2018

Contributor

I would not change the behaviour of this test.

This comment has been minimized.

Copy link
@tommarute

tommarute Dec 27, 2018

Author Contributor

I removed it. But test failed. I think that I need to add Ports: []uint32{31000, 32000}, to a task.

This comment has been minimized.

Copy link
@ti-mo

ti-mo Jan 8, 2019

Contributor

Correct, but I wouldn't use those ports, since they already correspond to the expected test results. I've pushed a commit here: 4a4a442

This comment has been minimized.

Copy link
@tommarute

tommarute Jan 8, 2019

Author Contributor

Thanks! I'll pick it up.

discovery/marathon/marathon_test.go Outdated
@@ -410,6 +411,80 @@ func TestMarathonSDSendGroupWithPortDefinitions(t *testing.T) {
}
}

func marathonTestAppListWithPortDefinitionsAndPorts(labels map[string]string, runningTasks int) *AppList {

This comment has been minimized.

Copy link
@ti-mo

ti-mo Dec 21, 2018

Contributor

I would call this (and the exported Test* function below) marathonTestAppListWithPortDefinitionsRequirePorts and set RequirePorts: true in this one instead of modifying an existing test. This clearly highlights the nature of the test itself, not the code path it's intended to take.

discovery/marathon/marathon.go Outdated
@@ -427,7 +428,7 @@ func targetsForApp(app *App) []model.LabelSet {
ports, labels = extractPortMapping(app.Container.Docker.PortMappings, app.isContainerNet())
prefix = portMappingLabelPrefix

} else if len(app.PortDefinitions) != 0 {
} else if app.RequirePorts && len(app.PortDefinitions) != 0 {

This comment has been minimized.

Copy link
@ti-mo

ti-mo Dec 21, 2018

Contributor

This will effectively disable label gathering when requirePorts is false, which is the default. This should have broken at least a test or two, please take a look at this, I will dive in as well. This might be missing from coverage.

In any case, I would wrap ports[i] = app.PortDefinitions[i].Port below in an if app.RequirePorts instead of this. This will make sure ports is still initialized to an array of zeroes of the correct length in both cases, making your change on line 462 unnecessary. (a zero will cause a port lookup in the task)
Make sure to add a comment, eg.

// When requirePorts is false, this port becomes the 'servicePort', not the listen port.
// In this case, the port needs to be taken from the task instead of the app.
discovery/marathon/marathon.go Outdated
@@ -458,7 +459,7 @@ func targetsForApp(app *App) []model.LabelSet {

// A zero port can appear in a portMapping, which means it is auto-generated.
// The allocated port appears at the corresponding index in the task's 'ports' array.
if port == 0 && len(t.Ports) == len(ports) {
if (!app.RequirePorts || port == 0) && len(t.Ports) == len(ports) {

This comment has been minimized.

Copy link
@ti-mo

ti-mo Dec 21, 2018

Contributor

Undo this change, an upstream zero port will cause this code to do The Right Thing. (tm)

This comment has been minimized.

Copy link
@tommarute

tommarute Dec 27, 2018

Author Contributor

Awthanks!

discovery/marathon/marathon.go Outdated
@@ -458,7 +459,7 @@ func targetsForApp(app *App) []model.LabelSet {

// A zero port can appear in a portMapping, which means it is auto-generated.

This comment has been minimized.

Copy link
@ti-mo

ti-mo Dec 21, 2018

Contributor

Let's update this comment to:

// A zero port here means that either the portMapping has a zero port defined,
// or there is a portDefinition with requirePorts set to false. This means the port
// is auto-generated by Mesos and needs to be looked up in the task.

@tommarute tommarute force-pushed the tommarute:fix-marathon-sd branch to e3c13b3 Dec 27, 2018

@tommarute

This comment has been minimized.

Copy link
Contributor Author

tommarute commented Jan 7, 2019

@ti-mo Could you confirm my fixes and two remaining consideration?

  1. #5026 (comment)
  2. #5026 (comment)
@ti-mo

This comment has been minimized.

Copy link
Contributor

ti-mo commented Jan 7, 2019

@tommarute Will take a look tomorrow, been away from computers for a couple of weeks.

@ti-mo

This comment has been minimized.

Copy link
Contributor

ti-mo commented Jan 8, 2019

@tommarute I went over your changes, and to the best of my knowledge, this implementation should be correct. Please include #5026 (comment).

marathon-sd - use Tasks.Ports instead of PortDefinitions.Ports if Req…
…uirePorts is false (#5022)

Signed-off-by: tommarute <tommarute@gmail.com>

@tommarute tommarute force-pushed the tommarute:fix-marathon-sd branch to bda4848 Jan 8, 2019

@tommarute

This comment has been minimized.

Copy link
Contributor Author

tommarute commented Jan 8, 2019

@ti-mo I have included your commit. I really appreciate your help in resolving the issue.

@ti-mo

This comment has been minimized.

Copy link
Contributor

ti-mo commented Jan 8, 2019

@tommarute Thanks for the patience, sorry for the breakage.

@simonpasquier This looks good to me. Needs to be fixed on master as well. What does the 2.7 release window look like?

ports[i] = app.PortDefinitions[i].Port
// When requirePorts is false, this port becomes the 'servicePort', not the listen port.
// In this case, the port needs to be taken from the task instead of the app.
if app.RequirePorts {

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Jan 8, 2019

Member

You should prefer relabelling to adding business logic to SD. If there's metadata missing you should add it.

This comment has been minimized.

Copy link
@ti-mo

ti-mo Jan 8, 2019

Contributor

@brian-brazil This is the label/port gathering stage. The 'port' field means either 'data' port or 'service' port based on the state of this bool.

While I agree with your stance, I can't think of a way to handle this in relabeling without complicating the user's relabeling rule set. Additionally, the user will need to be aware of this idiosyncracy, one that we, the people working on this feature, were not even aware of until this issue popped up.

From the point of view of reducing maintenance cost and amount of incidents/issues, you are completely right, but this way we can at least fix it once, for all users. Unfortunately, consuming this API is hard to get right..

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Jan 8, 2019

Member

The fact that you need to add a boolean configuration option that the user has to care about indicates that this belongs in relabelling. It's better to have everything in relabelling, than each SD slowly reinventing it.

This comment has been minimized.

Copy link
@simonpasquier

simonpasquier Jan 8, 2019

Member

Hmm, RequirePorts is returned by the Marathon API, not exposed in the SD configuration if this is what you meant.

This comment has been minimized.

Copy link
@ti-mo

ti-mo Jan 8, 2019

Contributor

@brian-brazil This is not a configuration option, it's a variable set in the Marathon app definition, obtained by querying the Marathon API.

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Jan 8, 2019

Member

Ah, I'd missed that.

This comment has been minimized.

Copy link
@ti-mo

ti-mo Jan 8, 2019

Contributor

No worries, thanks for taking the time to review. 👍

@ti-mo

ti-mo approved these changes Jan 8, 2019

@simonpasquier
Copy link
Member

simonpasquier left a comment

👍

@simonpasquier

This comment has been minimized.

Copy link
Member

simonpasquier commented Jan 14, 2019

@brian-brazil brian-brazil merged commit 9922c35 into prometheus:release-2.6 Jan 14, 2019

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jan 14, 2019

Thanks!

@simonpasquier simonpasquier added this to the v2.6.1 milestone Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.