Whirr
  1. Whirr
  2. WHIRR-3

Add support for EBS storage on EC2

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.1.0
    • Component/s: contrib/python
    • Labels:
      None
    1. HADOOP-6108.patch
      124 kB
      Tom White
    2. HADOOP-6108.patch
      124 kB
      Tom White
    3. HADOOP-6108.patch
      123 kB
      Tom White
    4. HADOOP-6108.patch
      122 kB
      Tom White

      Issue Links

        Activity

        Hide
        Tom White added a comment -

        The scripts to be added are available at http://www.cloudera.com/hadoop-ec2-ebs-beta.

        Show
        Tom White added a comment - The scripts to be added are available at http://www.cloudera.com/hadoop-ec2-ebs-beta .
        Hide
        Tom White added a comment -

        Here is a new patch which provides EBS support, amongst other things.

        The scripts have been refactored to make it easy to support other providers. The current patch does not include anything other than EC2, but the plan is to add other providers, perhaps by using libcloud (http://libcloud.org/). Since the scripts are no longer EC2 specific they are under src/contrib/cloud.

        These new scripts are a superset of the the bash scripts (in src/contrib/ec2) and are intended to replace them. They are also more amenable to testing (there are some unit tests included).

        There are also integration (functional) tests for testing running a MapReduce job on a transient and a persistent (EBS) cluster.

        Show
        Tom White added a comment - Here is a new patch which provides EBS support, amongst other things. The scripts have been refactored to make it easy to support other providers. The current patch does not include anything other than EC2, but the plan is to add other providers, perhaps by using libcloud ( http://libcloud.org/ ). Since the scripts are no longer EC2 specific they are under src/contrib/cloud. These new scripts are a superset of the the bash scripts (in src/contrib/ec2) and are intended to replace them. They are also more amenable to testing (there are some unit tests included). There are also integration (functional) tests for testing running a MapReduce job on a transient and a persistent (EBS) cluster.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422222/HADOOP-6108.patch
        against trunk revision 824942.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 21 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        -1 release audit. The applied patch generated 3 release audit warnings (more than the trunk's current 0 warnings).

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/testReport/
        Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12422222/HADOOP-6108.patch against trunk revision 824942. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 3 release audit warnings (more than the trunk's current 0 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/87/console This message is automatically generated.
        Hide
        Tom White added a comment -

        One release audit warning is for a JSON file. Anyone know if it's possible to add a license header to JSON files? If not, I can just exclude it.

        Show
        Tom White added a comment - One release audit warning is for a JSON file. Anyone know if it's possible to add a license header to JSON files? If not, I can just exclude it.
        Hide
        Doug Cutting added a comment -

        > Anyone know if it's possible to add a license header to JSON files?

        The JSON spec does not include a comment syntax. Some parsers (e.g., Jackson) will strip C-style comments, but it looks like this is read by simplejson, which I don't think supports comments.

        Show
        Doug Cutting added a comment - > Anyone know if it's possible to add a license header to JSON files? The JSON spec does not include a comment syntax. Some parsers (e.g., Jackson) will strip C-style comments, but it looks like this is read by simplejson, which I don't think supports comments.
        Hide
        Tom White added a comment -

        I verified that simplejson doesn't support C-style comments, so I have excluded *.json files from RAT. Here's a new patch.

        Show
        Tom White added a comment - I verified that simplejson doesn't support C-style comments, so I have excluded *.json files from RAT. Here's a new patch.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422889/HADOOP-6108.patch
        against trunk revision 828181.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 21 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12422889/HADOOP-6108.patch against trunk revision 828181. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/100/console This message is automatically generated.
        Hide
        Aaron Kimball added a comment -

        README.txt
        34] you seem to pick an arbitrary AMI. How's this chosen? Is there a list of appropriate AMIS somewhere?

        58] "with with"

        integration-test/transient-cluster.sh]
        1) /bin/bash -> /usr/bin/env bash
        25) Is it really necessary to source ~/.bashrc? The /bin/bash on the shebang line should take care of this.

        persistent-cluster.sh]
        same two concerns as above.

        34) the `sleep 5` can be pushed above the if and used only once.

        teststorage.py]
        TestJsonVolumeSpecManager really needs some comments; there's a number of multiply-nested arrays being dereferenced and it's not obvious what these prove. Is this just testing that the "spec" record earlier in the file was parsed correctly?

        hadoop-ec2-init-remote.sh ]
        96) consider putting this in /etc/profile too, so that it affects all users, not just root.
        171) given EBS replication, is it ok to go to dfs rep 2 here?

        VERSION ] should this file match the version associated with the hadoop-common project at large?

        cli.py]

        instead of a million 'from.... import' commands, consider 'import hadoop.cloud.commands as commands', then use commands.MASTER, commands.attach_storage(), etc.

        254) _prompt consider being case-insensitive here.
        291) in the event of a timeout, should you still print the master url? Is there definitely a master? Please add a comment saying why further error handling is not needed. A message suggesting follow-up action items for the user (e.g. "check hadoop-ec2 list to make sure it really booted") would be helpful too.

        329) ditto.

        346) since we're already parsing a config file, can the proxy port be configurable? What if the user's got two clusters running simultaneously?

        461) Add a message saying how to get usage/help text out of the app?

        cluster.py]

        get_cluster() 33) unpythonic key check; this should throw KeyError if provider is not found.

        Cluster stub functions ) For mandatory methods (e.g. get_provider_code), how about raise Exception("Unimplemented") instead of pass, or maybe add UnimplementedException / NotYetImplemented / Something similar?

        commands.py]

        36) Maybe add a comment describing high-level purpose of ROLES, and examples of potential future use (e.g., "HBase support might add new roles here, as would divorced JT/NN, etc.")

        launch_master() 83) better error msg / comment explaining how this timeout is handled in the rest of the method's logic?

        launch_slaves() 190) ditto.

        231) Since we support both Hadoop 0.18 and 0.20 in the documentation, is there additional logic required to gracefully support both cluster versions in this code here?

        storage.py]

        JsonVolumeManager._load() ) why not just percolate the IOError? Do we want file errors to silently initialize an empty vol manager? (Is this file something the user specifies, or an optional file? If the former, this should fail hard. If the latter, silent continue seems ok.)

        class Storage ) same comment as above re. NotYetImplemented vs. pass

        util.py ]

        bash_quote() ) do you need to double-escape backslashes? The unit tests also don't cover internal backslash characters.

        ec2.py ]
        authorize_role() 150) Why not just catch InvalidPermission.Duplicate?

        Show
        Aaron Kimball added a comment - README.txt 34] you seem to pick an arbitrary AMI. How's this chosen? Is there a list of appropriate AMIS somewhere? 58] "with with" integration-test/transient-cluster.sh] 1) /bin/bash -> /usr/bin/env bash 25) Is it really necessary to source ~/.bashrc? The /bin/bash on the shebang line should take care of this. persistent-cluster.sh] same two concerns as above. 34) the `sleep 5` can be pushed above the if and used only once. teststorage.py] TestJsonVolumeSpecManager really needs some comments; there's a number of multiply-nested arrays being dereferenced and it's not obvious what these prove. Is this just testing that the "spec" record earlier in the file was parsed correctly? hadoop-ec2-init-remote.sh ] 96) consider putting this in /etc/profile too, so that it affects all users, not just root. 171) given EBS replication, is it ok to go to dfs rep 2 here? VERSION ] should this file match the version associated with the hadoop-common project at large? cli.py] instead of a million 'from.... import' commands, consider 'import hadoop.cloud.commands as commands', then use commands.MASTER, commands.attach_storage(), etc. 254) _prompt consider being case-insensitive here. 291) in the event of a timeout, should you still print the master url? Is there definitely a master? Please add a comment saying why further error handling is not needed. A message suggesting follow-up action items for the user (e.g. "check hadoop-ec2 list to make sure it really booted") would be helpful too. 329) ditto. 346) since we're already parsing a config file, can the proxy port be configurable? What if the user's got two clusters running simultaneously? 461) Add a message saying how to get usage/help text out of the app? cluster.py] get_cluster() 33) unpythonic key check; this should throw KeyError if provider is not found. Cluster stub functions ) For mandatory methods (e.g. get_provider_code), how about raise Exception("Unimplemented") instead of pass , or maybe add UnimplementedException / NotYetImplemented / Something similar? commands.py] 36) Maybe add a comment describing high-level purpose of ROLES, and examples of potential future use (e.g., "HBase support might add new roles here, as would divorced JT/NN, etc.") launch_master() 83) better error msg / comment explaining how this timeout is handled in the rest of the method's logic? launch_slaves() 190) ditto. 231) Since we support both Hadoop 0.18 and 0.20 in the documentation, is there additional logic required to gracefully support both cluster versions in this code here? storage.py] JsonVolumeManager._load() ) why not just percolate the IOError? Do we want file errors to silently initialize an empty vol manager? (Is this file something the user specifies, or an optional file? If the former, this should fail hard. If the latter, silent continue seems ok.) class Storage ) same comment as above re. NotYetImplemented vs. pass util.py ] bash_quote() ) do you need to double-escape backslashes? The unit tests also don't cover internal backslash characters. ec2.py ] authorize_role() 150) Why not just catch InvalidPermission.Duplicate?
        Hide
        Tom White added a comment -

        Thanks for the really detailed review, Aaron! Responses inline below.

        > README.txt
        > 34] you seem to pick an arbitrary AMI. How's this chosen? Is there a list of appropriate AMIS somewhere?

        I think listing the AMIs on a wiki page is probably best, since these may be updated from time to time. AMIs simply need to run scripts passed as (compressed) user data and have Java installed. It would be a good follow up issue to include scripts to create these AMIs.

        > 58] "with with"

        Fixed.

        > integration-test/transient-cluster.sh]
        > 1) /bin/bash -> /usr/bin/env bash

        Fixed.

        > 25) Is it really necessary to source ~/.bashrc? The /bin/bash on the shebang line should take care of this.

        Only for interactive shells. I've run these scripts using Hudson, and this is then needed.

        > persistent-cluster.sh]
        > same two concerns as above.

        > 34) the `sleep 5` can be pushed above the if and used only once.

        Fixed.

        > teststorage.py]
        > TestJsonVolumeSpecManager really needs some comments; there's a number of multiply-nested arrays being dereferenced and it's not obvious what these prove. Is this just testing that the "spec" record earlier in the file was parsed correctly?

        Yes. I've addressed this by improving the naming in the test.

        > 171) given EBS replication, is it ok to go to dfs rep 2 here?

        Possibly, although I would like to be conservative, and have users pro-actively reduce it.

        > VERSION ] should this file match the version associated with the hadoop-common project at large?

        Fixed.

        > cli.py]

        > instead of a million 'from.... import' commands, consider 'import hadoop.cloud.commands as commands', then use commands.MASTER, commands.attach_storage(), etc.

        Done.

        > 254) _prompt consider being case-insensitive here.

        Done.

        > 291) in the event of a timeout, should you still print the master url? Is there definitely a master? Please add a comment saying why further error handling is not needed. A message suggesting follow-up action items for the user (e.g. "check hadoop-ec2 list to make sure it really booted") would be helpful too.

        I've improved the message. Printing the master address is useful for diagnosing the problem.

        > 329) ditto.

        Ditto.

        > 346) since we're already parsing a config file, can the proxy port be configurable? What if the user's got two clusters running simultaneously?

        I agree that this could be a limitation for some users. I'd like to tackle this in a follow-up issue.

        > 461) Add a message saying how to get usage/help text out of the app?

        Changed to print usage.

        > cluster.py]

        > get_cluster() 33) unpythonic key check; this should throw KeyError if provider is not found.

        Fixed.

        > Cluster stub functions ) For mandatory methods (e.g. get_provider_code), how about raise Exception("Unimplemented") instead of pass, or maybe add UnimplementedException / NotYetImplemented / Something similar?

        Done.

        > commands.py]

        > 36) Maybe add a comment describing high-level purpose of ROLES, and examples of potential future use (e.g., "HBase support might add new roles here, as would divorced JT/NN, etc.")

        Done.

        > launch_master() 83) better error msg / comment explaining how this timeout is handled in the rest of the method's logic?

        > launch_slaves() 190) ditto.

        Improved message, and changed to exit on timeout.

        > 231) Since we support both Hadoop 0.18 and 0.20 in the documentation, is there additional logic required to gracefully support both cluster versions in this code here?

        This code supports both Hadoop 0.18 and 0.20, so no further changes should be needed.

        storage.py]

        > JsonVolumeManager._load() ) why not just percolate the IOError? Do we want file errors to silently initialize an empty vol manager? (Is this file something the user specifies, or an optional file? If the former, this should fail hard. If the latter, silent continue seems ok.)

        We want the latter case.

        > class Storage ) same comment as above re. NotYetImplemented vs. pass

        Done

        > util.py ]

        > bash_quote() ) do you need to double-escape backslashes? The unit tests also don't cover internal backslash characters.

        The double backslash is a python-escaped backslash. I've added a test for an internal backslash.

        > ec2.py ]
        > authorize_role() 150) Why not just catch InvalidPermission.Duplicate?

        I couldn't see a clean way of catching this error and ignoring other exceptions.

        Show
        Tom White added a comment - Thanks for the really detailed review, Aaron! Responses inline below. > README.txt > 34] you seem to pick an arbitrary AMI. How's this chosen? Is there a list of appropriate AMIS somewhere? I think listing the AMIs on a wiki page is probably best, since these may be updated from time to time. AMIs simply need to run scripts passed as (compressed) user data and have Java installed. It would be a good follow up issue to include scripts to create these AMIs. > 58] "with with" Fixed. > integration-test/transient-cluster.sh] > 1) /bin/bash -> /usr/bin/env bash Fixed. > 25) Is it really necessary to source ~/.bashrc? The /bin/bash on the shebang line should take care of this. Only for interactive shells. I've run these scripts using Hudson, and this is then needed. > persistent-cluster.sh] > same two concerns as above. > 34) the `sleep 5` can be pushed above the if and used only once. Fixed. > teststorage.py] > TestJsonVolumeSpecManager really needs some comments; there's a number of multiply-nested arrays being dereferenced and it's not obvious what these prove. Is this just testing that the "spec" record earlier in the file was parsed correctly? Yes. I've addressed this by improving the naming in the test. > 171) given EBS replication, is it ok to go to dfs rep 2 here? Possibly, although I would like to be conservative, and have users pro-actively reduce it. > VERSION ] should this file match the version associated with the hadoop-common project at large? Fixed. > cli.py] > instead of a million 'from.... import' commands, consider 'import hadoop.cloud.commands as commands', then use commands.MASTER, commands.attach_storage(), etc. Done. > 254) _prompt consider being case-insensitive here. Done. > 291) in the event of a timeout, should you still print the master url? Is there definitely a master? Please add a comment saying why further error handling is not needed. A message suggesting follow-up action items for the user (e.g. "check hadoop-ec2 list to make sure it really booted") would be helpful too. I've improved the message. Printing the master address is useful for diagnosing the problem. > 329) ditto. Ditto. > 346) since we're already parsing a config file, can the proxy port be configurable? What if the user's got two clusters running simultaneously? I agree that this could be a limitation for some users. I'd like to tackle this in a follow-up issue. > 461) Add a message saying how to get usage/help text out of the app? Changed to print usage. > cluster.py] > get_cluster() 33) unpythonic key check; this should throw KeyError if provider is not found. Fixed. > Cluster stub functions ) For mandatory methods (e.g. get_provider_code), how about raise Exception("Unimplemented") instead of pass, or maybe add UnimplementedException / NotYetImplemented / Something similar? Done. > commands.py] > 36) Maybe add a comment describing high-level purpose of ROLES, and examples of potential future use (e.g., "HBase support might add new roles here, as would divorced JT/NN, etc.") Done. > launch_master() 83) better error msg / comment explaining how this timeout is handled in the rest of the method's logic? > launch_slaves() 190) ditto. Improved message, and changed to exit on timeout. > 231) Since we support both Hadoop 0.18 and 0.20 in the documentation, is there additional logic required to gracefully support both cluster versions in this code here? This code supports both Hadoop 0.18 and 0.20, so no further changes should be needed. storage.py] > JsonVolumeManager._load() ) why not just percolate the IOError? Do we want file errors to silently initialize an empty vol manager? (Is this file something the user specifies, or an optional file? If the former, this should fail hard. If the latter, silent continue seems ok.) We want the latter case. > class Storage ) same comment as above re. NotYetImplemented vs. pass Done > util.py ] > bash_quote() ) do you need to double-escape backslashes? The unit tests also don't cover internal backslash characters. The double backslash is a python-escaped backslash. I've added a test for an internal backslash. > ec2.py ] > authorize_role() 150) Why not just catch InvalidPermission.Duplicate? I couldn't see a clean way of catching this error and ignoring other exceptions.
        Hide
        Aaron Kimball added a comment -

        > README.txt
        > 34] you seem to pick an arbitrary AMI. How's this chosen? Is there a list of appropriate AMIS somewhere?

        I think listing the AMIs on a wiki page is probably best, since these may be updated from time to time. AMIs simply need to run scripts passed as (compressed) user data and have Java installed. It would be a good follow up issue to include scripts to create these AMIs.

        Agree. Maybe create this wiki page and reference that in the README?

        > 25) Is it really necessary to source ~/.bashrc? The /bin/bash on the shebang line should take care of this.

        Only for interactive shells. I've run these scripts using Hudson, and this is then needed.

        Hm. I'm under the impression that .bashrc is for interactive, non-login shells only. (From reading the INVOCATION section of the bash manpage.) Whereas .bash_profile is for interactive login shells. If .bash_profile specifies AWS_ACCESS_KEY_ID, etc, and .bashrc specifies a different one, then users who have already configured their AWS credentials in .bash_profile will see their environment shadowed. (I admit this case is rare, but the two files are specified as different for a reason.) A more plausible purpose is that users specify a default set of AWS credentials in .bashrc, but sometimes override them on the command-line before invoking this script. So I'm not convinced this is a good general-purpose solution. Hudson is likely using a non-interactive non-login shell to invoke its scripts, in which case it should either specify AWS credentials in the environment it explicitly passes to subshells, or should specify the config script via the $BASH_ENV variable. (e.g.: BASH_ENV=/home/hudson/.bashrc /path/to/run-your-test-script-here)

        > 346) since we're already parsing a config file, can the proxy port be configurable? What if the user's got two clusters running simultaneously?

        I agree that this could be a limitation for some users. I'd like to tackle this in a follow-up issue.

        Ok.

        .. Everything else looks fine. Thanks for adding more comments. I'm +.9 as-is; would prefer to see the bashrc hack addressed though.

        Show
        Aaron Kimball added a comment - > README.txt > 34] you seem to pick an arbitrary AMI. How's this chosen? Is there a list of appropriate AMIS somewhere? I think listing the AMIs on a wiki page is probably best, since these may be updated from time to time. AMIs simply need to run scripts passed as (compressed) user data and have Java installed. It would be a good follow up issue to include scripts to create these AMIs. Agree. Maybe create this wiki page and reference that in the README? > 25) Is it really necessary to source ~/.bashrc? The /bin/bash on the shebang line should take care of this. Only for interactive shells. I've run these scripts using Hudson, and this is then needed. Hm. I'm under the impression that .bashrc is for interactive, non-login shells only. (From reading the INVOCATION section of the bash manpage.) Whereas .bash_profile is for interactive login shells. If .bash_profile specifies AWS_ACCESS_KEY_ID, etc, and .bashrc specifies a different one, then users who have already configured their AWS credentials in .bash_profile will see their environment shadowed. (I admit this case is rare, but the two files are specified as different for a reason.) A more plausible purpose is that users specify a default set of AWS credentials in .bashrc, but sometimes override them on the command-line before invoking this script. So I'm not convinced this is a good general-purpose solution. Hudson is likely using a non-interactive non-login shell to invoke its scripts, in which case it should either specify AWS credentials in the environment it explicitly passes to subshells, or should specify the config script via the $BASH_ENV variable. (e.g.: BASH_ENV=/home/hudson/.bashrc /path/to/run-your-test-script-here ) > 346) since we're already parsing a config file, can the proxy port be configurable? What if the user's got two clusters running simultaneously? I agree that this could be a limitation for some users. I'd like to tackle this in a follow-up issue. Ok. .. Everything else looks fine. Thanks for adding more comments. I'm +.9 as-is; would prefer to see the bashrc hack addressed though.
        Hide
        Tom White added a comment -

        Hudson is likely using a non-interactive non-login shell to invoke its scripts, in which case it should either specify AWS credentials in the environment it explicitly passes to subshells, or should specify the config script via the $BASH_ENV variable. (e.g.: BASH_ENV=/home/hudson/.bashrc /path/to/run-your-test-script-here)

        Good point. I have removed the lines that source ~/.bashrc.

        I've also referenced the the EC2 wiki page from the README.

        Show
        Tom White added a comment - Hudson is likely using a non-interactive non-login shell to invoke its scripts, in which case it should either specify AWS credentials in the environment it explicitly passes to subshells, or should specify the config script via the $BASH_ENV variable. (e.g.: BASH_ENV=/home/hudson/.bashrc /path/to/run-your-test-script-here) Good point. I have removed the lines that source ~/.bashrc. I've also referenced the the EC2 wiki page from the README.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12426143/HADOOP-6108.patch
        against trunk revision 883206.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 21 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12426143/HADOOP-6108.patch against trunk revision 883206. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 21 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/148/console This message is automatically generated.
        Hide
        Aaron Kimball added a comment -

        Sounds good; thanks. This gets a +1 from Hudson, so +1 from me as well.

        Show
        Aaron Kimball added a comment - Sounds good; thanks. This gets a +1 from Hudson, so +1 from me as well.
        Hide
        Doug Cutting added a comment -

        +1 This looks like a big improvement, and, as a new contrib module, this has no compatibility concerns.

        Should we deprecate the old ec2 stuff after this has been committed?

        Show
        Doug Cutting added a comment - +1 This looks like a big improvement, and, as a new contrib module, this has no compatibility concerns. Should we deprecate the old ec2 stuff after this has been committed?
        Hide
        Tom White added a comment -

        I've just committed this.

        > Should we deprecate the old ec2 stuff after this has been committed?

        Yes. I've opened HADOOP-6403 for this.

        Show
        Tom White added a comment - I've just committed this. > Should we deprecate the old ec2 stuff after this has been committed? Yes. I've opened HADOOP-6403 for this.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #94 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/94/)
        . Add support for EBS storage on EC2.

        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #94 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/94/ ) . Add support for EBS storage on EC2.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk #175 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/175/)
        . Add support for EBS storage on EC2.

        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk #175 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/175/ ) . Add support for EBS storage on EC2.

          People

          • Assignee:
            Tom White
            Reporter:
            Tom White
          • Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development