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

Added logic to read NetworkInfo from /state.json. #266

Merged
merged 6 commits into from
Sep 17, 2015
Merged

Conversation

ConnorDoyle
Copy link
Contributor

Overview

Mesos 0.25.0 will include a new way for network isolation modules to communicate assigned IP addresses assigned to containers at executor launch time. Where previously Mesos-DNS picked up the values of some special labels appended to a task status, Mesos 0.25.0 introduces a new structure called NetworkInfo.

Change summary

This change preserves the previous behavior of reading special labels to ease transition to users who relied on that method of IP address discovery. However, the information in the new NetworkInfo structure should take precedence if present.

Additional references

// TODO(CD): handle multiple NetworkInfo objects
// TODO(CD): only create A records if the address is IPv4
// TODO(CD): create AAAA records if the address is IPv6
latestContainerIP = status.ContainerStatus.NetworkInfos[0].IPAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

may be interesting to filter on label here at some point in the future when there are multiple IPs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not even sure what Mesos-DNS should do when there are multiple addreses assigned. I think we'd need conventions for handling additional metadata to give us clues (e.g. labels on NetworkInfo, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it's not clear to me either how mesos-dns should respond in
situations like that. It also just occurred to me that it might be nice to
include mention of this new behavior somewhere in the mesos-dns docs pages.

On Wed, Sep 16, 2015 at 7:15 PM, Connor Doyle [email protected]
wrote:

In records/state/state.go
#266 (comment):

@@ -103,7 +116,18 @@ func (t *Task) containerIP(src string) string {
continue
}

  •   // find the latest docker-inspect label
    
  •   // first try to extract the address from the NetworkInfo structure
    
  •   if len(status.ContainerStatus.NetworkInfos) > 0 {
    
  •       // TODO(CD): handle multiple NetworkInfo objects
    
  •       // TODO(CD): only create A records if the address is IPv4
    
  •       // TODO(CD): create AAAA records if the address is IPv6
    
  •       latestContainerIP = status.ContainerStatus.NetworkInfos[0].IPAddress
    

I'm not even sure what Mesos-DNS should do when there are multiple
addreses assigned. I think we'd need conventions for handling additional
metadata to give us clues (e.g. labels on NetworkInfo, etc.)


Reply to this email directly or view it on GitHub
https://github.com/mesosphere/mesos-dns/pull/266/files#r39696900.

@jdef
Copy link
Contributor

jdef commented Sep 16, 2015

Great writeup! Thanks for all the context. Couple of minor comments but overall looking good. LGTM after comments addressed.

@ConnorDoyle ConnorDoyle assigned tsenart and unassigned jdef Sep 17, 2015
@ConnorDoyle
Copy link
Contributor Author

Hey @tsenart, could you PTAL and merge if you don't find anything objectionable?

// NetworkInfo holds a network address resolution as defined in the
// /state.json Mesos HTTP endpoint.
type NetworkInfo struct {
IPAddress string `json:"ip_address,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know if the IPAddress is v4 or v6? Are all clients supposed to try parsing one and then fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. Using a string here was seen as a better alternative than trying to come up with a clever packed representation.

On Sep 17, 2015, at 01:39, Tomás Senart [email protected] wrote:

In records/state/state.go:

  • Timestamp float64 json:"timestamp"
  • State string json:"state"
  • Labels []Label json:"labels,omitempty"
  • ContainerStatus ContainerStatus json:"container_status,omitempty"
    +}

+// ContainerStatus holds container metadata as defined in the /state.json
+// Mesos HTTP endpoint.
+type ContainerStatus struct {

  • NetworkInfos []NetworkInfo json:"network_infos,omitempty"
    +}

+// NetworkInfo holds a network address resolution as defined in the
+// /state.json Mesos HTTP endpoint.
+type NetworkInfo struct {

  • IPAddress string json:"ip_address,omitempty"
    How do we know if the IPAddress is v4 or v6? Are all clients supposed to try parsing one and then fallback?


Reply to this email directly or view it on GitHub.

@tsenart
Copy link
Contributor

tsenart commented Sep 17, 2015

@ConnorDoyle: I'm briefly working on suggested change set that would set in place the constructs required to address your TODOs.

@tsenart
Copy link
Contributor

tsenart commented Sep 17, 2015

PTAL: #267

Tomás Senart and others added 3 commits September 17, 2015 20:29
This commit refactors the Task IP sourcing code to gracefully account
for the new `NetworkInfo` source with the same structure and be easily
changed to distinguish between IPv4 and IPv6.
jdef added a commit that referenced this pull request Sep 17, 2015
Added logic to read NetworkInfo from /state.json.
@jdef jdef merged commit 92897a1 into master Sep 17, 2015
@jdef jdef deleted the read-networkinfo branch September 17, 2015 21:50
@tsenart tsenart mentioned this pull request Sep 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants