Cassandra
  1. Cassandra
  2. CASSANDRA-43

Add configuration to choose which local IP-address to bind server on

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Need a configuration to choose which local IP-address to bind server on

      1. 0002-v2.patch
        6 kB
        Jonathan Ellis
      2. ASF.LICENSE.NOT.GRANTED--0001-CASSANDRA-43-fix-strange-line-endings-and-indentatio.txt
        10 kB
        Eric Evans
      3. ASF.LICENSE.NOT.GRANTED--0002-CASSANDRA-43-configurable-address-binding.txt
        6 kB
        Eric Evans
      4. diff.txt
        15 kB
        Per Mellqvist
      5. diff.txt
        16 kB
        Per Mellqvist
      6. diff.txt
        4 kB
        Per Mellqvist
      7. endpoint_diff.txt
        0.9 kB
        Per Mellqvist

        Activity

        Gavin made changes -
        Workflow patch-available, re-open possible [ 12751844 ] reopen-resolved, no closed status, patch-avail, testing [ 12755083 ]
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12460524 ] patch-available, re-open possible [ 12751844 ]
        Jonathan Ellis made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Jonathan Ellis added a comment -

        looks like I forgot to resolve this last week.

        Show
        Jonathan Ellis added a comment - looks like I forgot to resolve this last week.
        Jonathan Ellis made changes -
        Attachment 0002-v2.patch [ 12406049 ]
        Hide
        Jonathan Ellis added a comment -

        back to letting thrift set reuseaddr etc. centralize ListenAddress logic in getHostName.

        Show
        Jonathan Ellis added a comment - back to letting thrift set reuseaddr etc. centralize ListenAddress logic in getHostName.
        Jonathan Ellis made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Assignee Eric Evans [ urandom ]
        Hide
        Jonathan Ellis added a comment -

        Reverted 0002 since it prevents server from starting up. Haven't debugged more than that.

        Show
        Jonathan Ellis added a comment - Reverted 0002 since it prevents server from starting up. Haven't debugged more than that.
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Jonathan Ellis added a comment -

        applied Eric's tweaks to Per's first patch.

        Show
        Jonathan Ellis added a comment - applied Eric's tweaks to Per's first patch.
        Eric Evans made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Eric Evans added a comment -

        Attached are two new patches.

        The first fixes the crazy line endings and indentation in EndPoint.java that have been making patch generation such a pain. The second is basically Per's first patch, rebased against current trunk, and with the configuration directive renamed to "ListenAddress".

        Per, is this ok with you?

        Show
        Eric Evans added a comment - Attached are two new patches. The first fixes the crazy line endings and indentation in EndPoint.java that have been making patch generation such a pain. The second is basically Per's first patch, rebased against current trunk, and with the configuration directive renamed to "ListenAddress". Per, is this ok with you?
        Eric Evans made changes -
        Hide
        Eric Evans added a comment -

        Also, jbellis raised a valid point on IRC that DNS is a point of failure and shouldn't be mandatory.

        I don't see any reason why you couldn't retain the behavior of this patch with respect to returning a string representation of the selected IP address.

        Show
        Eric Evans added a comment - Also, jbellis raised a valid point on IRC that DNS is a point of failure and shouldn't be mandatory. I don't see any reason why you couldn't retain the behavior of this patch with respect to returning a string representation of the selected IP address.
        Hide
        Eric Evans added a comment -

        First, a few minor issues/styling nits:

        This patch makes the LocalIP config required, so test/conf/storage-conf.xml should be updated accordingly or the tests fail.

        Speaking of the directive, I can see where you got the name "LocalIP", but its purpose might be more obvious if it were named something like "ListenInterface" or "BindInterface". Or "ListenAddress" maybe?

        If FBUtilities.isHostLocalHost() is no longer needed, it probably makes sense to just remove it from the source altogether instead of commenting it out (it can always be retrieved from history later if needed).

        You should use a space between operators in your expressions, since that is the convention used throughout the project.

        Now, as to the functionality:

        The way the patch is setup, it's expecting that a) either the address has been explicitly configured, or b) that it is able to find at least one IPv4 interface with a non-loopback address (and it will run with the last one it finds). If neither condition is met, an exception is thrown and startup is prevented.

        I'm not sure this is the best behavior. Firstly, I'd hate to start down a path that led to no support for IPv6. Secondly, in the absence of a configuration, address selection is less deterministic than what is in trunk (depends on order of interface/address enumeration versus the host network configuration).

        I think this needs to take one of two routes:

        1. Everyone configures the interface to bind to. No exceptions. Throw an exception for missing or invalid configuration.

        2. The default (read: trunk) behavior is retained (i.e. InetAddress.getLocalHost()), which can be overridden by configuration.

        Personally, I favor #2 since it is the element of least surprise to those who already have working clusters.

        Show
        Eric Evans added a comment - First, a few minor issues/styling nits: This patch makes the LocalIP config required, so test/conf/storage-conf.xml should be updated accordingly or the tests fail. Speaking of the directive, I can see where you got the name "LocalIP", but its purpose might be more obvious if it were named something like "ListenInterface" or "BindInterface". Or "ListenAddress" maybe? If FBUtilities.isHostLocalHost() is no longer needed, it probably makes sense to just remove it from the source altogether instead of commenting it out (it can always be retrieved from history later if needed). You should use a space between operators in your expressions, since that is the convention used throughout the project. Now, as to the functionality: The way the patch is setup, it's expecting that a) either the address has been explicitly configured, or b) that it is able to find at least one IPv4 interface with a non-loopback address (and it will run with the last one it finds). If neither condition is met, an exception is thrown and startup is prevented. I'm not sure this is the best behavior. Firstly, I'd hate to start down a path that led to no support for IPv6. Secondly, in the absence of a configuration, address selection is less deterministic than what is in trunk (depends on order of interface/address enumeration versus the host network configuration). I think this needs to take one of two routes: 1. Everyone configures the interface to bind to. No exceptions. Throw an exception for missing or invalid configuration. 2. The default (read: trunk) behavior is retained (i.e. InetAddress.getLocalHost()), which can be overridden by configuration. Personally, I favor #2 since it is the element of least surprise to those who already have working clusters.
        Per Mellqvist made changes -
        Attachment endpoint_diff.txt [ 12405628 ]
        Hide
        Per Mellqvist added a comment -

        Separate diff file for EndPoint.java
        This change has to be applied manually due to some mismatch between svn diff and patch

        Show
        Per Mellqvist added a comment - Separate diff file for EndPoint.java This change has to be applied manually due to some mismatch between svn diff and patch
        Per Mellqvist made changes -
        Attachment diff.txt [ 12405627 ]
        Hide
        Per Mellqvist added a comment -

        After beating my head repeatedly on some weird mismatch between svn diff and patch this diff file now contains all changes EXCEPT the changes for EndPoint.java. Those changes are in a separate patch file that will have to be applied manually as patch complains about the diff file syntax.

        Show
        Per Mellqvist added a comment - After beating my head repeatedly on some weird mismatch between svn diff and patch this diff file now contains all changes EXCEPT the changes for EndPoint.java. Those changes are in a separate patch file that will have to be applied manually as patch complains about the diff file syntax.
        Hide
        Eric Evans added a comment -

        I'm having some trouble applying this patch. I think the error message, "patch: **** malformed patch at line 84: @@ -20,6 +20,7 @@" is a red herring. The actual problem seems to be with the hunk preceding it (for src/org/apache/cassandra/net/EndPoint.java).

        Per, can you try regenerating the patch?

        Show
        Eric Evans added a comment - I'm having some trouble applying this patch. I think the error message, "patch: **** malformed patch at line 84: @@ -20,6 +20,7 @@" is a red herring. The actual problem seems to be with the hunk preceding it (for src/org/apache/cassandra/net/EndPoint.java). Per, can you try regenerating the patch?
        Per Mellqvist made changes -
        Attachment diff.txt [ 12405240 ]
        Hide
        Per Mellqvist added a comment -

        New patch instead of the first one.

        This patch removes all the host-based stuff. If a <LocalIp> setting is present in storage-conf.xml that ip-address will be used. Otherwise cassandra will use the local ip-address of the server IF there is only one (not counting loopback). If the server has multiple ip-addresses cassandra must be told which one to use in storage-conf.xml.

        Show
        Per Mellqvist added a comment - New patch instead of the first one. This patch removes all the host-based stuff. If a <LocalIp> setting is present in storage-conf.xml that ip-address will be used. Otherwise cassandra will use the local ip-address of the server IF there is only one (not counting loopback). If the server has multiple ip-addresses cassandra must be told which one to use in storage-conf.xml.
        Matthieu Riou made changes -
        Workflow jira [ 12459716 ] no-reopen-closed, patch-avail [ 12460524 ]
        Hide
        Jonathan Ellis added a comment -

        IMO if you're going to start mixing IPs in you should clean out the old hostname-based stuff entirely. (If no IP is given then pick one.)

        Show
        Jonathan Ellis added a comment - IMO if you're going to start mixing IPs in you should clean out the old hostname-based stuff entirely. (If no IP is given then pick one.)
        Hide
        Jonathan Ellis added a comment -

        Per, you should join us in #cassandra on freenode irc. http://www.mibbit.com/?server=irc.freenode.net&channel=%23cassandra&nick=mibbit if you don't have an irc client.

        Show
        Jonathan Ellis added a comment - Per, you should join us in #cassandra on freenode irc. http://www.mibbit.com/?server=irc.freenode.net&channel=%23cassandra&nick=mibbit if you don't have an irc client.
        Hide
        Per Mellqvist added a comment -

        Note that one use of this is runnng several instances on the same machine for local testing.
        One config file to run on 127.0.0.1 another one to run on 127.0.0.2 etc

        Show
        Per Mellqvist added a comment - Note that one use of this is runnng several instances on the same machine for local testing. One config file to run on 127.0.0.1 another one to run on 127.0.0.2 etc
        Per Mellqvist made changes -
        Field Original Value New Value
        Attachment diff.txt [ 12404381 ]
        Hide
        Per Mellqvist added a comment -

        Patch (generated by 'svn diff' on revision 760988)

        The patch adds a new option to storage-conf.xml
        <LocalIp>127.0.0.1</LocalIp>

        The option is used to indicate on which local ip-address to bind.
        This can be used to run the server on internal vs external ip-addresses etc.

        Show
        Per Mellqvist added a comment - Patch (generated by 'svn diff' on revision 760988) The patch adds a new option to storage-conf.xml <LocalIp>127.0.0.1</LocalIp> The option is used to indicate on which local ip-address to bind. This can be used to run the server on internal vs external ip-addresses etc.
        Per Mellqvist created issue -

          People

          • Assignee:
            Eric Evans
            Reporter:
            Per Mellqvist
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development