Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Expose containerPort as $PORT_NAME env vars #5888

Merged
merged 3 commits into from
Jan 12, 2018
Merged

Conversation

zen-dog
Copy link
Contributor

@zen-dog zen-dog commented Jan 10, 2018

when using docker user networking (and not hostPorts exist). This part of code went missing from 1.3 to 1.4.

JIRA: COPS-2072

when using docker user networking (and not `hostPorts` exist). This part of code went missing from 1.3 to 1.4.

JIRA: COPS-2072
env += (s"PORT_${portName.toUpperCase}" -> effectivePort.toString)
// TODO(jdef) port name envvars for generated container ports
case (PortRequest(Some(portName), port), None) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments as for #5887

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building your change at jenkins-marathon-pipelines-PR-5888-1.

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔ Build of #5888 completed successfully.

See details at jenkins-marathon-pipelines-PR-5888-1.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/builds/marathon-1.6.0-pre-247-gb4ed634.tgz",
"sha1": "3654040793fe64972ac3763aea6ea9bea016a872"

You can run system integration test changes of this PR against Marathon
master by triggering this Jenkins job with the Pull_Request_id 5888.
The job then reports back to this PR.

\\ ٩( ᐛ )و //

- random container ports are now handled correctly
Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building your change at jenkins-marathon-pipelines-PR-5888-2.

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✗ Build of #5888 failed.

See the logs and test results for details.

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building your change at jenkins-marathon-pipelines-PR-5888-3.

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✗ Build of #5888 failed.

See the logs and test results for details.

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

assert("8080" == env("PORT_8080"))
assert("8080" == env("PORT_HTTP"))

val port1 = env("PORT1")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto re: sanity check that port1 ! = 0

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building your change at jenkins-marathon-pipelines-PR-5888-4.

Copy link
Contributor

@meln1k meln1k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor formatting question

import org.apache.mesos.Protos.TaskInfo
import org.apache.mesos.{ Protos => MesosProtos }
Copy link
Contributor

@meln1k meln1k Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like intelij and SBT have a different opinion on how to format imports :)

Copy link

@mesosphere-ci mesosphere-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔ Build of #5888 completed successfully.

See details at jenkins-marathon-pipelines-PR-5888-4.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

"url": "https://s3.amazonaws.com/downloads.mesosphere.io/marathon/builds/marathon-1.6.0-pre-256-gb737b0d.tgz",
"sha1": "8bde77dace968b2e82fc2c6e1f886df1a982d5f6"

You can run system integration test changes of this PR against Marathon
master by triggering this Jenkins job with the Pull_Request_id 5888.
The job then reports back to this PR.

\\ ٩( ᐛ )و //

@zen-dog zen-dog merged commit 9bd8810 into master Jan 12, 2018
@jdef
Copy link
Contributor

jdef commented Jan 12, 2018

lgtm

the port-by-name docs could use some clarification/editing as well: https://github.com/mesosphere/marathon/blob/master/docs/docs/networking.md#per-task-environment-variables

for example, PORT_name doesn't always refer to a host port. these variables are actually pretty confusing because their meaning changes depending upon how ports are allocated by an app. It might be worth a table that clearly shows how the value of the PORTxxx variables changes depending on the app configuration.

@zen-dog zen-dog deleted the ad/fix-port-names branch April 7, 2018 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants