Skip to content
135 changes: 134 additions & 1 deletion command/agent/command.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package agent

import (
"errors"
"flag"
"fmt"
"io"
Expand Down Expand Up @@ -76,6 +77,10 @@ func (c *Command) readConfig() *Config {
cmdFlags.Var((*AppendSliceValue)(&tags), "tag",
"tag pair, specified as key=value")
cmdFlags.StringVar(&cmdConfig.Discover, "discover", "", "mDNS discovery name")
cmdFlags.Var((*AppendSliceValue)(&cmdConfig.JoinSRV), "join-srv",
"SRV record to join on startup")
cmdFlags.Var((*AppendSliceValue)(&cmdConfig.RetryJoinSRV), "retry-join-srv",
"SRV record to join on startup with retry")
cmdFlags.StringVar(&cmdConfig.Interface, "iface", "", "interface to bind to")
cmdFlags.StringVar(&cmdConfig.TagsFile, "tags-file", "", "tag persistence file")
cmdFlags.BoolVar(&cmdConfig.EnableSyslog, "syslog", false,
Expand Down Expand Up @@ -427,9 +432,129 @@ func (c *Command) startAgent(config *Config, agent *Agent,
if config.Discover != "" {
c.Ui.Info(fmt.Sprintf(" mDNS cluster: %s", config.Discover))
}

return ipc
}

// startupJoin is invoked to handle any joins specified to take place at start time

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment is stale.

func (c *Command) startupJoinSRV(config *Config, agent *Agent) error {
if len(config.JoinSRV) == 0 {
return nil
}

c.Ui.Output(fmt.Sprintf("Joining cluster via SRV...(replay: %v)", config.ReplayOnJoin))
n, err := c.joinSRV(agent, config.ReplayOnJoin, config.JoinSRV)
if err != nil {
return err
}

if n == 0 {
return errors.New("Failed to join any hosts via SRV")
}

c.Ui.Info(fmt.Sprintf("Join completed. Synced with %d initial agents", n))
return nil

}

// retryJoinSRV is invoked to handle joins with retries. This runs until at least a
// single successful join or RetryMaxAttempts is reached
func (c *Command) retryJoinSRV(config *Config, agent *Agent, errCh chan struct{}) {
// Quit fast if there is no nodes to join
if len(config.RetryJoinSRV) == 0 {
return
}

// Track the number of join attempts
attempt := 0
for {
// Try to perform the join
n, err := c.joinSRV(agent, config.ReplayOnJoin, config.RetryJoinSRV)

if err != nil {
c.logger.Printf("[ERR] agent: Failed to join via SRV: %v", err)
}

if err == nil && n > 0 {
c.logger.Printf("[INFO] agent: Join completed. Synced with %d initial agents", n)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need a return here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if it would make sense to make the return conditional, to provide something like a 'max trys = -1' to just keep polling, even after the cluster has been joined.

We have a use case for continuous polling.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the use case for continuous polling? Once you join, the cluster takes care of membership and a new guy can bootstrap with one of the agents in the srv record.

On Sep 14, 2015, at 9:11 PM, Dale Hamel notifications@github.com wrote:

In command/agent/command.go:

  • if len(config.RetryJoinSRV) == 0 {
  •   return
    
  • }
  • // Track the number of join attempts
  • attempt := 0
  • for {
  •   // Try to perform the join
    
  •   n, err := c.joinSRV(agent, config.ReplayOnJoin, config.RetryJoinSRV)
    
  •   if err != nil {
    
  •       c.logger.Printf("[ERR] agent: Failed to join via SRV: %v", err)
    
  •   }
    
  •   if err == nil && n > 0 {
    
  •       c.logger.Printf("[INFO] agent: Join completed. Synced with %d initial agents", n)
    
    I'm wondering if it would make sense to make the return conditional, to provide something like a 'max trys = -1' to just keep polling, even after the cluster has been joined.

We have a use case for continuous polling.


Reply to this email directly or view it on GitHub.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That assumes that the SRV record points to the hosts in the existing cluster. If that changes, you could have a split where you have two clusters who will never know about each other.

I think I mentioned this concern in another comment.

Let's say you have 4 nodes, A,B,C,D. Only A and B are in the SRV record. C and D join them.

A and B get torn down, and a new SRV record is built with E and F. You now have two clusters, DC, EF. If DC keep checking the SRV record ,they will find EF. If they don't, you'll have a partition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see. Sorry for my confusion I just want to understand how this will be used and how it differs from existing options.

In your example, what kind of process decides who gets added to the SRV record? If you always populated the SRV record with all the Consul servers, for example, it would always be sufficient for new nodes to join.

I hope I haven't yagni-d you with the join options but that's where I thought it was going :-) For the polling case I'd had a -discover_srv option that takes the list and calls your function forever in a separate go routine vs. -1 retry count. If you can use the new join options without adding the polling support I still think that's ideal, though.

On Sep 15, 2015, at 6:35 AM, Dale Hamel notifications@github.com wrote:

In command/agent/command.go:

  • if len(config.RetryJoinSRV) == 0 {
  •   return
    
  • }
  • // Track the number of join attempts
  • attempt := 0
  • for {
  •   // Try to perform the join
    
  •   n, err := c.joinSRV(agent, config.ReplayOnJoin, config.RetryJoinSRV)
    
  •   if err != nil {
    
  •       c.logger.Printf("[ERR] agent: Failed to join via SRV: %v", err)
    
  •   }
    
  •   if err == nil && n > 0 {
    
  •       c.logger.Printf("[INFO] agent: Join completed. Synced with %d initial agents", n)
    
    That assumes that the SRV record points to the hosts in the existing cluster. If that changes, you could have a split where you have two clusters who will never know about each other.

I think I mentioned this concern in another comment.

Let's say you have 4 nodes, A,B,C,D. Only A and B are in the SRV record. C and D join them.

A and B get torn down, and a new SRV record is built with E and F. You now have two clusters, DC, EF. If DC keep checking the SRV record ,they will find EF. If they don't, you'll have a partition.


Reply to this email directly or view it on GitHub.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd also second that the continuous polling is ideally not Serf's responsibility. I think the common use case will be the one-shot join, and leaning on the gossip protocol to keep the list of members converged. I'm not totally opposed but I think we should have a very solid and general use case in mind before adding the additional complexity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Our specific use case, which I think is quite applicable:

  • We manage a VPC in terraform
  • The VPC has jump hosts (bastion hosts) in the public subnet, which are required to bootstrap anything in the private subnet
  • We use terraform to create SRV records for the VPC that all serf nodes use to find th e cluster
  • All other nodes in the VPC discover the cluster using this SRV record pointing to these bastion hosts

All this is done as a workaround for the fact that mDNS can't work in ec2, so SRV records to these bastion hosts are created.

If the bastion hosts disappear, which might well happen, then we will have a cluster partition since we are only registering the bastion nodes in SRV records.

Our workaround right now is we have a crontab that looks up the SRV record and just sends a serf join on every host, but that feels like a huge hack when serf could support this natively.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For this case, how do you bootstrap a new VPC + bastion initially (if there are no servers in the private subnet yet)? If I understand you correctly, the continuous polling is only useful when there are existing servers in the private subnet to find a new bastion that was added and join it to the cluster, right? (And the init time SRV join is useful when the bastion is up and new private subnet servers come up and need to join).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@slackpad the bastions find each other through the SRV record once it's registered, then all other nodes find the bastions.

The continuous polling is useful for any other nodes (private or public doesn't really matter) to discover the cluster. The only case where I think it's totally necessary is when both bastions go away, which would result in the original cluster becoming 'orphaned'.

A workaround would be to have the nodes register themselves as part of the SRV record, but I'd prefer to not have the nodes updating the records and to just keep them in terraform as that's a lot more predictable.

Or we could just stick with the crontab approach, but I think that the continuous polling to prevent orphaned clusters is the simplest, most reliable approach. Under the current implementation, it never has to try a join if the cluster is already fully joined.

}

// Check if the maximum attempts has been exceeded
attempt++
if config.RetryMaxAttempts > 0 && attempt > config.RetryMaxAttempts {
c.logger.Printf("[ERR] agent: maximum retry SRV join attempts made, exiting")
close(errCh)
return
}

c.logger.Printf("[INFO] agent: Will check SRV again in %v", config.RetryInterval)
time.Sleep(config.RetryInterval)
}
}

func (c *Command) joinSRV(agent *Agent, replay bool, srvrecords []string) (int, error) {
records := c.findSRV(agent, srvrecords)
// Attempt the join only if there are new records
if len(records) > 0 {
n, err := agent.Join(records, replay)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is totally what I was thinking. The one remaining WAN-related detail (if you think SRV records would be useful for WAN joining in addition to LAN joining) is to add WAN versions of these. Instead of copy-paste the LAN ones, if we could pass a function down to do the join ( then I think you could re-use all of this stuff. Basically you'd pass in a function that calls agent.Join() for the LAN version, and agent.JoinWAN() for the LAN version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@slackpad it might be useful, but I think it could probably make it into a different PR as for now it's basically YAGNI, at least for us.

Aside from that, are we good to merge here?


if err != nil {
return n, err
}

if n > 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can probably delete this since the calling code will print the same number.

c.logger.Printf("[INFO] agent: Joined %d hosts via SRV", n)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It doesn't look like n ever gets returned. It might simplify this to check if len(records) == 0 and bail, de-nest, and then return n, nil at the end (you can't do that now because it's scoped inside the if).

}
return 0, nil
}

// findSRV looks up the SRV records and returns a slice of all SRV records
// that are not currently cluster members
func (c *Command) findSRV(agent *Agent, srvrecords []string) []string {
var hosts []string

// map the members so that we only do a single O(n) search through members
known_members := make(map[string]bool)
for _, v := range agent.Serf().Members() {
if v.Status.String() == serf.SerfAlive.String() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need for String() -> if v.Status == serf.SerfAlive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are two different types, MemberStatus and SerfState, as far as I can tell the integers have no guarantee of matching, but the strings do map to the same thing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops I picked the wrong constant - there's a MemberStatus StatusAlive that should work instead of the string (if we end up adding this back).

member := fmt.Sprintf("%s:%d", v.Addr.String(), v.Port)
known_members[member] = true
}
}

// Look up each SRV record and check if it's already in the cluster
for _, record := range srvrecords {
_, srvhosts, err := net.LookupSRV("", "", record)

if err != nil {
c.logger.Printf("[ERR] agent: Failed to poll for new SRV hosts: %v", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd probably add a continue here, and maybe include the offending srv record in the error message.

}

// Filter each hosts in the SRV record
for _, host := range srvhosts {
// Find its addresses, as it's the only unique ID we can rely on
ipaddrs, err := net.LookupIP(host.Target)

if err != nil {
c.logger.Printf("[ERR] agent: resolve SRV record %s to IP %v", host.Target, err)
}

// For each address the host has, check if it's already in the cluster
for _, ipaddr := range ipaddrs {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that we are just doing this at start time, you can simplify this by getting rid of the host checking. There's no ongoing polling, so this will only be called when this guy doesn't know about anyone else. Might as well try to join them all, since that's what will happen anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Leaving this one untouched until we decide what to do about polling in above comment thread.

addr := fmt.Sprintf("%s:%d", ipaddr.String(), host.Port)
if _, known := known_members[addr]; !known {
// If the host is not already in the cluster,
// Add it to the list of hosts to try to join
hosts = append(hosts, addr)
}
}
}
}
return hosts
}

// startupJoin is invoked to handle any joins specified to take place at start time
func (c *Command) startupJoin(config *Config, agent *Agent) error {
if len(config.StartJoin) == 0 {
Expand Down Expand Up @@ -559,6 +684,11 @@ func (c *Command) Run(args []string) int {
return 1
}

if err := c.startupJoinSRV(config, agent); err != nil {
c.Ui.Error(err.Error())
return 1
}

// Enable log streaming
c.Ui.Info("")
c.Ui.Output("Log data will now stream in as it occurs:\n")
Expand All @@ -567,6 +697,7 @@ func (c *Command) Run(args []string) int {
// Start the retry joins
retryJoinCh := make(chan struct{})
go c.retryJoin(config, agent, retryJoinCh)
go c.retryJoinSRV(config, agent, retryJoinCh)

// Wait for exit
return c.handleSignals(config, agent, retryJoinCh)
Expand Down Expand Up @@ -720,7 +851,9 @@ Options:
-retry-join=addr An agent to join with. This flag be specified multiple times.
Does not exit on failure like -join, used to retry until success.
-retry-interval=30s Sets the interval on which a node will attempt to retry joining
nodes provided by -retry-join. Defaults to 30s.
nodes provided by -retry-join or -retry-join-srv. Defaults to 30s.
-join-srv=record SRV record to discover peers. Can be specified multiple times.
-retry-join-srv=record Like join-srv, but retry on failure.
-retry-max=0 Limits the number of retry events. Defaults to 0 for unlimited.
-role=foo The role of this node, if any. This can be used
by event scripts to differentiate different types
Expand Down
15 changes: 15 additions & 0 deletions command/agent/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,15 @@ type Config struct {
// allows Serf agents to join each other with zero configuration.
Discover string `mapstructure:"discover"`

// SRVRecords is used look for other agents using DNS SRV records.
// When this is set, the agent will look up the SRV record
// and attempt to add any hosts it finds. You can specify multiple times
// to look up multiple SRV records.
JoinSRV []string `mapstructure:"join-srv"`

// Exactly like join-srv, only keep trying until a successful join happens.
RetryJoinSRV []string `mapstructure:"retry-join-srv"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should probably be underscores to follow the config file convention of all of the other options.

// Interface is used to provide a binding interface to use. It can be
// used instead of providing a bind address, as Serf will discover the
// address of the provided interface. It is also used to set the multicast
Expand Down Expand Up @@ -380,6 +389,12 @@ func MergeConfig(a, b *Config) *Config {
if b.Discover != "" {
result.Discover = b.Discover
}
if len(b.JoinSRV) > 0 {
result.JoinSRV = b.JoinSRV
}
if len(b.RetryJoinSRV) > 0 {
result.RetryJoinSRV = b.RetryJoinSRV
}
if b.Interface != "" {
result.Interface = b.Interface
}
Expand Down