Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-2458

Add option to disable IP hostname checking for DataNode registration

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.1.0
    • Fix Version/s: 1.2.0
    • Component/s: deployment
    • Labels:
      None
    • Flags:
      Patch

      Description

      Add a puppet config option to control ip_hostname_check. If deploying on a cloud that does not provide reverse DNS service, the DataNode registration to the NameNode will fail. Exposing this option allows deployments to work around this.

      1. BIGTOP-2458.patch
        3 kB
        Cory Johns
      2. BIGTOP-2458.patch
        2 kB
        Cory Johns
      3. BIGTOP-2458.patch
        2 kB
        Cory Johns
      4. BIGTOP-2458.patch
        2 kB
        Cory Johns
      5. BIGTOP-2458.patch
        2 kB
        Cory Johns
      6. BIGTOP-2458.patch
        2 kB
        Cory Johns

        Activity

        Hide
        kwmonroe Kevin W Monroe added a comment -

        In the cluster.yaml block of your patch, I think you should use the hiera value instead of 'false'. So instead of this:

        #hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check: false

        You would do something like this:

        #hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check: %

        {hiera('hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check')}

        I'm not 100% on this (still new to the relationship between hiera, cluster.yaml, and init.pp), so hopefully a more seasoned committer will verify the best practice here.

        Show
        kwmonroe Kevin W Monroe added a comment - In the cluster.yaml block of your patch, I think you should use the hiera value instead of 'false'. So instead of this: #hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check: false You would do something like this: #hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check: % {hiera('hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check')} I'm not 100% on this (still new to the relationship between hiera, cluster.yaml, and init.pp), so hopefully a more seasoned committer will verify the best practice here.
        Hide
        johnsca Cory Johns added a comment -

        My understanding (from https://github.com/apache/bigtop/blob/master/bigtop-deploy/puppet/hiera.yaml#L7) was that cluster.yaml was used to build the Hiera data, so your suggestion seems self-referential to me.

        I could, however, see the argument that the init.pp line should instead be:

        $namenode_datanode_registration_ip_hostname_check = hiera('hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check', true),

        But as it works the way it is now, I'm not at all clear what the "more correct" way to do it is. I'm still pretty new to Puppet.

        Show
        johnsca Cory Johns added a comment - My understanding (from https://github.com/apache/bigtop/blob/master/bigtop-deploy/puppet/hiera.yaml#L7 ) was that cluster.yaml was used to build the Hiera data, so your suggestion seems self-referential to me. I could, however, see the argument that the init.pp line should instead be: $namenode_datanode_registration_ip_hostname_check = hiera('hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check', true), But as it works the way it is now, I'm not at all clear what the "more correct" way to do it is. I'm still pretty new to Puppet.
        Hide
        cos Konstantin Boudnik added a comment -

        I believe Kevin suggests that we need to be able to parametrize this setting, so a user can control it from site.yaml file. Hard-coding the value in the cluster.yaml won't help, as this file isn't supposed to be modified on every deployment. Instead, you can something similar to

        hadoop::common_hdfs::hadoop_namenode_host: "%{hiera('bigtop::hadoop_head_node')}"
        

        In other words, in the cluster.yaml do something like

        hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check: "%{hiera('bigtop::hadoop_ip_hostname_check')}"
        

        then {[bigtop::hadoop_ip_hostname_check}} could be set in the site.yaml during the deployment, if needed. Providing the default value in the init.pp file makes it always on, unless specified otherwise in the site.yaml. Does it make sense?

        Also, do you think it would be a bit easier to read, if the variable name is somewhat shorter? Thanks!

        Show
        cos Konstantin Boudnik added a comment - I believe Kevin suggests that we need to be able to parametrize this setting, so a user can control it from site.yaml file. Hard-coding the value in the cluster.yaml won't help, as this file isn't supposed to be modified on every deployment. Instead, you can something similar to hadoop::common_hdfs::hadoop_namenode_host: "%{hiera('bigtop::hadoop_head_node')}" In other words, in the cluster.yaml do something like hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check: "%{hiera('bigtop::hadoop_ip_hostname_check')}" then {[bigtop::hadoop_ip_hostname_check}} could be set in the site.yaml during the deployment, if needed. Providing the default value in the init.pp file makes it always on, unless specified otherwise in the site.yaml. Does it make sense? Also, do you think it would be a bit easier to read, if the variable name is somewhat shorter? Thanks!
        Hide
        johnsca Cory Johns added a comment -

        We were overriding it in site.yaml by specifying the full hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check property name. It seemed to work, but that doesn't mean it's right.

        Show
        johnsca Cory Johns added a comment - We were overriding it in site.yaml by specifying the full hadoop::common_hdfs::namenode_datanode_registration_ip_hostname_check property name. It seemed to work, but that doesn't mean it's right.
        Hide
        cos Konstantin Boudnik added a comment -

        Ah, if it works - it is fine then I haven't tested it myself, but I trust you have validated both scenarios (with site.yaml mods and without it), so we can go ahead with it.

        Show
        cos Konstantin Boudnik added a comment - Ah, if it works - it is fine then I haven't tested it myself, but I trust you have validated both scenarios (with site.yaml mods and without it), so we can go ahead with it.
        Hide
        johnsca Cory Johns added a comment -

        I'm more than happy to change it; I do think the bigtop:: param namespace and explicit hiera() call would be cleaner.

        Show
        johnsca Cory Johns added a comment - I'm more than happy to change it; I do think the bigtop:: param namespace and explicit hiera() call would be cleaner.
        Hide
        johnsca Cory Johns added a comment -

        New patch with explicit hiera() call.

        Show
        johnsca Cory Johns added a comment - New patch with explicit hiera() call.
        Hide
        cos Konstantin Boudnik added a comment -

        Cory Johns, I believe you lost the change to the cluster.yaml. Or have I misread the patch?

        Show
        cos Konstantin Boudnik added a comment - Cory Johns , I believe you lost the change to the cluster.yaml . Or have I misread the patch?
        Hide
        johnsca Cory Johns added a comment -

        I thought it wasn't needed with the hiera() call pulling from the bigtop:: namespace. Is that incorrect?

        Show
        johnsca Cory Johns added a comment - I thought it wasn't needed with the hiera() call pulling from the bigtop:: namespace. Is that incorrect?
        Hide
        johnsca Cory Johns added a comment -

        Ok, hopefully I have understood things correctly this time and the latest patch follows the proper convention.

        Show
        johnsca Cory Johns added a comment - Ok, hopefully I have understood things correctly this time and the latest patch follows the proper convention.
        Hide
        johnsca Cory Johns added a comment -

        New patch based on Olaf Flebbe's feedback in BIGTOP-2459. No bigtop:: namespace, implicit use of Hiera to populate the param, and default value given in cluster.yaml.

        Show
        johnsca Cory Johns added a comment - New patch based on Olaf Flebbe 's feedback in BIGTOP-2459 . No bigtop:: namespace, implicit use of Hiera to populate the param, and default value given in cluster.yaml .
        Hide
        cos Konstantin Boudnik added a comment -

        Same here - I don't see how + $namenode_datanode_registration_ip_hostname_check gets its value from the cluster.pp without a call to hiera

        Show
        cos Konstantin Boudnik added a comment - Same here - I don't see how + $namenode_datanode_registration_ip_hostname_check gets its value from the cluster.pp without a call to hiera
        Hide
        oflebbe Olaf Flebbe added a comment - - edited

        The value is supplied automatically by puppet

        Show
        oflebbe Olaf Flebbe added a comment - - edited The value is supplied automatically by puppet
        Hide
        oflebbe Olaf Flebbe added a comment - - edited

        LGTM +1

        No handling of default value os not ok.

        in init.pp in the argument list the value should have a default value = "true", Please do not set default values in cluster.yaml.

        if hdfs-site.xml the value should be tested to be different from undef before inserting the sniplet.

        Show
        oflebbe Olaf Flebbe added a comment - - edited LGTM +1 No handling of default value os not ok. in init.pp in the argument list the value should have a default value = "true", Please do not set default values in cluster.yaml. if hdfs-site.xml the value should be tested to be different from undef before inserting the sniplet.
        Hide
        johnsca Cory Johns added a comment -

        Updated patch per feedback.

        Show
        johnsca Cory Johns added a comment - Updated patch per feedback.
        Hide
        johnsca Cory Johns added a comment -

        Removed the if from the patch because it's incorrect.

        It doesn't seem like we need an explicit check for undefined there since we're providing a default value. Is that not accurate? Are we wanting to allow explicitly setting it to undef to not render that property, and if so, what is the value to that? Other boolean params (e.g., hdfs_webhdfs_enabled) all seem to skip this check.

        I'm also not familiar enough with Puppet templates to the right syntax for testing for undefined.

        Show
        johnsca Cory Johns added a comment - Removed the if from the patch because it's incorrect. It doesn't seem like we need an explicit check for undefined there since we're providing a default value. Is that not accurate? Are we wanting to allow explicitly setting it to undef to not render that property, and if so, what is the value to that? Other boolean params (e.g., hdfs_webhdfs_enabled ) all seem to skip this check. I'm also not familiar enough with Puppet templates to the right syntax for testing for undefined.
        Hide
        oflebbe Olaf Flebbe added a comment -

        Regarding the if: Congrats, you found a bug!

        Your assumption is correct:

        The intention of templating the hdfs-site.xml etc was only to override necessary parameters, leaving out settings which are default.

        So the design was to have puppet class parameters have the value undef, if the setting has not to be propagated to the output file, or set explicitly, where its value has to be set in the file.

        If one is using <% if @parameter %> the block is inserted in template if the value is not false. In most occasions that was technically identically to not undef, until you found a parameter defaulting to "false".

        The correct syntax is

        <% if @parameter.nil? -%>
        <% end -%>
        

        and the default value in init.pp should be undef.

        Would you mind issueing a new patch? We have to check other values as well.

        Thank you very much for your contribution!

        Show
        oflebbe Olaf Flebbe added a comment - Regarding the if: Congrats, you found a bug! Your assumption is correct: The intention of templating the hdfs-site.xml etc was only to override necessary parameters, leaving out settings which are default. So the design was to have puppet class parameters have the value undef, if the setting has not to be propagated to the output file, or set explicitly, where its value has to be set in the file. If one is using <% if @parameter %> the block is inserted in template if the value is not false. In most occasions that was technically identically to not undef, until you found a parameter defaulting to "false". The correct syntax is <% if @parameter.nil? -%> <% end -%> and the default value in init.pp should be undef. Would you mind issueing a new patch? We have to check other values as well. Thank you very much for your contribution!
        Hide
        oflebbe Olaf Flebbe added a comment -

        Oops I accidentily pushd the last patch to upstream as is.

        Cory Johns It would be nice if you can propose a new JIRA to implement the changes I suggested above. Thanks!

        Show
        oflebbe Olaf Flebbe added a comment - Oops I accidentily pushd the last patch to upstream as is. Cory Johns It would be nice if you can propose a new JIRA to implement the changes I suggested above. Thanks!
        Hide
        oflebbe Olaf Flebbe added a comment -

        Pushed as is (accidentely)

        Show
        oflebbe Olaf Flebbe added a comment - Pushed as is (accidentely)
        Hide
        kwmonroe Kevin W Monroe added a comment -

        FYI Olaf Flebbe, Cory adjusted this implementation as you suggested in https://issues.apache.org/jira/browse/BIGTOP-2547.

        Show
        kwmonroe Kevin W Monroe added a comment - FYI Olaf Flebbe , Cory adjusted this implementation as you suggested in https://issues.apache.org/jira/browse/BIGTOP-2547 .

          People

          • Assignee:
            johnsca Cory Johns
            Reporter:
            johnsca Cory Johns
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development