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

Add Network.Name to IpAddress #3974

Closed
wants to merge 2 commits into from

Conversation

ozdanborne
Copy link

This PR adds the optional field ipAddress.Name, which is read and written as an optional field in ContainerInfo.NetworkInfo.Name.

This branch successfully builds but fails to pass the existing tests, presumably because I am incorrectly reading and storing the nullable string from the protobuf.

Not looking to merge this until the first commit (bump to Mesos 1.0.0 protobufs) is mainlined through the official mesos-utils build system. But am looking to get this working ASAP as early access to CNI plugins. Can anyone provide some scala-foo for what I may be doing wrong?

@ozdanborne
Copy link
Author

ozdanborne commented Jun 10, 2016

Some sample test failures:

- BuildIfMatchesWithIpAddress *** FAILED *** (6 milliseconds)
  0 was not equal to 1 (TaskBuilderTest.scala:588)
- BuildIfMatchesWithIpAddressAndCustomExecutor *** FAILED *** (6 milliseconds)
  0 was not equal to 1 (TaskBuilderTest.scala:639)
- BuildIfMatchesWithIpAddressAndDiscoveryInfo *** FAILED *** (4 milliseconds)
  0 was not equal to 1 (TaskBuilderTest.scala:694)

@ozdanborne
Copy link
Author

Cool, I was just missing one embarrassing line. This all seems to be working now. Is there any other work that needs to be done while we wait for 1.0.0 mesos-utils to drop? Presumably just:

  • tests
  • documentation

@jdef
Copy link
Contributor

jdef commented Jun 16, 2016

just merged this morning: 731b4d4

if this meets the need, can we close this out?

@vixns vixns mentioned this pull request Jun 19, 2016
@kensipe
Copy link
Contributor

kensipe commented Jun 20, 2016

@djosborne is the feature merge provided by jdef sufficient? can we close this?

@ozdanborne
Copy link
Author

@kensipe I built from master last week after it was merged and was unable to get a task to launch with CNI. I will retry with latest master and close if it works, or post debug info if it does not.

@ozdanborne
Copy link
Author

ozdanborne commented Jun 22, 2016

Just got this working. The network names that have been added are sufficient for CNI for Mesos tasks.

FYI I was running into an issue where Marathon was complaining that 1.0.0 protobufs were imcompatible with Mesos 0.28.2. Apparently the main Dockerfile installs Mesos 0.28.2, whereas Dockerfile.development correctly installs 1.0.0-rc1. I can raise an issue if this is not being tracked

@ozdanborne ozdanborne closed this Jun 22, 2016
@ozdanborne
Copy link
Author

@jdef Just launched a Docker task on a Docker UDN using the new Api. All worked as expected. Nice!

@jkidambi
Copy link

Is there a plan to support the labels field mentioned in http://mesos.apache.org/documentation/cni/?

@ozdanborne
Copy link
Author

@jkidambi That's currently supported. Marathon's ipAddress.labels field gets pushed into Networkinfo.labels. More info here: https://mesosphere.github.io/marathon/docs/ip-per-task.html

@jkidambi
Copy link

@djosborne Thanks. I did figure that out after I posted that question, and have been able to successfully test it end-to-end.

@jkidambi
Copy link

@djosborne Is there a way to specify multiple networks per container? I'm not seeing a way to do that. If there is a better place to post such questions, I'll do so.

@ozdanborne
Copy link
Author

ozdanborne commented Jul 28, 2016

@jkidambi I don't think there is currently. I think since configurable networking for Mesos is a relatively new concept, use cases for multi-network tasks hasn't been clearly ironed out yet, and there's not a lot of support for it yet.

@marcomonaco marcomonaco added the pr label Mar 6, 2017
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