Uploaded image for project: 'Libcloud'
  1. Libcloud
  2. LIBCLOUD-278

Name a ScriptDeployment object without absolute path fails the execution

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.2
    • Fix Version/s: 0.12.1
    • Component/s: Core
    • Labels:
      None

      Description

      If you set a name to a ScriptDeployment this way:

      sd = ScriptDeployment('touch total_success', name='success.sh')

      it fails the execution when you call the conn.deploy_node() function without feedback to the user.

      The 'put' method of the 'ParamikoSSHClient' copies the file 'success.sh' in the relative path of the logged user (/root/success.sh or /home/ubuntu/success.sh in Ubuntu machines), but the 'run' method of the same class runs directly the 'success.sh' script and this file is not in the classpath.

      If you set the line previous to the return statement of the 'run' method:

      print ("Out: '%s', Error: '%s', Status: '%d'") % (so, se, status)

      You get:

      Out: '', Error: 'bash: success.sh: command not found', Status: '127'

      If the name of the 'ScriptDeployment' is an absolute path (sd = ScriptDeployment('touch total_success', name='/home/ubuntu/success.sh')), the execution runs fine.

      1. libcloud_278_full.patch
        7 kB
        Jaume Devesa
      2. libcloud_278.patch
        1 kB
        Jaume Devesa

        Activity

        Hide
        jdevesa Jaume Devesa added a comment -

        This is the patch that solves the issue. Since the execution is in the remote machine, I couldn't find a way to write a test. Please tell me if you have any idea.

        However, I've prepared a gist to test the functionality in Amazon: https://gist.github.com/4460584 You probably have hundreds of them and feel more comfortable with Rackspace provider... but this is the one I've used and it might be useful

        Show
        jdevesa Jaume Devesa added a comment - This is the patch that solves the issue. Since the execution is in the remote machine, I couldn't find a way to write a test. Please tell me if you have any idea. However, I've prepared a gist to test the functionality in Amazon: https://gist.github.com/4460584 You probably have hundreds of them and feel more comfortable with Rackspace provider... but this is the one I've used and it might be useful
        Hide
        kami Tomaz Muraus added a comment -

        Thanks, the change sounds good to me.

        As far as the tests go - I do think we need to have tests for this case and one approach you can use is to mock some methods called inside BaseSSHClient.run and test BaseSSHClient directly using a unit test.

        Show
        kami Tomaz Muraus added a comment - Thanks, the change sounds good to me. As far as the tests go - I do think we need to have tests for this case and one approach you can use is to mock some methods called inside BaseSSHClient.run and test BaseSSHClient directly using a unit test.
        Hide
        jdevesa Jaume Devesa added a comment - - edited

        Ok! I'll see what can I do.

        Anyway, IMHO the method 'run' of the ScriptDeployment should raise an Exception if the property 'self.stderr' is not empty after the script execution: that will be helpful to Mock behaviors and also will return feedback to the user about how the execution finished.

        Show
        jdevesa Jaume Devesa added a comment - - edited Ok! I'll see what can I do. Anyway, IMHO the method 'run' of the ScriptDeployment should raise an Exception if the property 'self.stderr' is not empty after the script execution: that will be helpful to Mock behaviors and also will return feedback to the user about how the execution finished.
        Hide
        kami Tomaz Muraus added a comment -

        ScriptDeployment is a generic utility class and you can write a script which intentionally writes something to stderr so this behavior would not be desirable in this scenario.

        We could maybe add an argument or something like that and default to a sane value.

        Show
        kami Tomaz Muraus added a comment - ScriptDeployment is a generic utility class and you can write a script which intentionally writes something to stderr so this behavior would not be desirable in this scenario. We could maybe add an argument or something like that and default to a sane value.
        Hide
        jdevesa Jaume Devesa added a comment - - edited

        Yes, you are right.

        About testing the BaseSSHClient class, I don't think that will give us to much value, because what I've modified is the ParamikoSSHClient. But this class is not covered by the tests: http://ci.apache.org/projects/libcloud/coverage/libcloud_compute_ssh.html#n114

        I will try to mock the 'socket' module this way: http://paperairoplane.net/?p=312 but using the 'mock' library instead the 'mox' one.

        Show
        jdevesa Jaume Devesa added a comment - - edited Yes, you are right. About testing the BaseSSHClient class, I don't think that will give us to much value, because what I've modified is the ParamikoSSHClient. But this class is not covered by the tests: http://ci.apache.org/projects/libcloud/coverage/libcloud_compute_ssh.html#n114 I will try to mock the 'socket' module this way: http://paperairoplane.net/?p=312 but using the 'mock' library instead the 'mox' one.
        Hide
        kami Tomaz Muraus added a comment -

        Yes, mock should work fine and we already use it to mock other things in the tests.

        Show
        kami Tomaz Muraus added a comment - Yes, mock should work fine and we already use it to mock other things in the tests.
        Hide
        jdevesa Jaume Devesa added a comment -

        To test the functionality, I've filled the ParamikoSSHClientTests, which was empty, (https://github.com/apache/libcloud/blob/trunk/libcloud/test/compute/test_ssh_client.py), using the 'patch' method of the 'mock' library to mock the object 'paramiko.SSHClient()'. Easier than mocking the socket module!

        It seems the paramiko module does not offer compatibility with Python 3, so I've skipped the tests in this environment.

        I've attached the full patch in the libcloud_278_full.patch file. Tell me if something is wrong.

        PD: mock library is awesome

        Show
        jdevesa Jaume Devesa added a comment - To test the functionality, I've filled the ParamikoSSHClientTests, which was empty, ( https://github.com/apache/libcloud/blob/trunk/libcloud/test/compute/test_ssh_client.py ), using the 'patch' method of the 'mock' library to mock the object 'paramiko.SSHClient()'. Easier than mocking the socket module! It seems the paramiko module does not offer compatibility with Python 3, so I've skipped the tests in this environment. I've attached the full patch in the libcloud_278_full.patch file. Tell me if something is wrong. PD: mock library is awesome
        Hide
        kami Tomaz Muraus added a comment -

        Good work with the tests.

        I've merged your patch into trunk and 0.12.x branch. Thanks.

        Show
        kami Tomaz Muraus added a comment - Good work with the tests. I've merged your patch into trunk and 0.12.x branch. Thanks.
        Hide
        kami Tomaz Muraus added a comment -

        I just noticed while working on some other code that this change introduced a pretty bad regression.

        run method allows you to run an arbitrary command on a remote server. With this change, users current working directory will be prep-pended to the command.

        For example, if you pass bin "ls -la ." to run it would result in "<users home directory>/ls -la" which is wrong.

        I will revert this change and add some tests so a regression like this won't be able to slip in next time.

        I think a better change is to modify "client.put" method to return a directory where a file has been saved. Then we can prep-pend this path to the command which is passed to "client.run" inside ScriptDeployment.

        Show
        kami Tomaz Muraus added a comment - I just noticed while working on some other code that this change introduced a pretty bad regression. run method allows you to run an arbitrary command on a remote server. With this change, users current working directory will be prep-pended to the command. For example, if you pass bin "ls -la ." to run it would result in "<users home directory>/ls -la" which is wrong. I will revert this change and add some tests so a regression like this won't be able to slip in next time. I think a better change is to modify "client.put" method to return a directory where a file has been saved. Then we can prep-pend this path to the command which is passed to "client.run" inside ScriptDeployment.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1454783 from Tomaz Muraus
        [ https://svn.apache.org/r1454783 ]

        Remove changes introduced as part of LIBCLOUD-278.

        Those changes introduces a regression in client.run method which would only make
        it work if you passed in a relative or absolute path to the shell script to this
        method.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1454783 from Tomaz Muraus [ https://svn.apache.org/r1454783 ] Remove changes introduced as part of LIBCLOUD-278 . Those changes introduces a regression in client.run method which would only make it work if you passed in a relative or absolute path to the shell script to this method.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1454786 from Tomaz Muraus
        [ https://svn.apache.org/r1454786 ]

        Modify ScriptDeployment to work correctly if "self.name" contains a relative
        path.

        Part of LIBCLOUD-278.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1454786 from Tomaz Muraus [ https://svn.apache.org/r1454786 ] Modify ScriptDeployment to work correctly if "self.name" contains a relative path. Part of LIBCLOUD-278 .
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1454788 from Tomaz Muraus
        [ https://svn.apache.org/r1454788 ]

        Fix a regression in ParamikoSSHClient.run method.

        Change SSHClient interface a bit and make "put" method return a full path to the
        location where a file has been saved.

        Remove old invalid test and add a new one.

        Part of LIBCLOUD-278.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1454788 from Tomaz Muraus [ https://svn.apache.org/r1454788 ] Fix a regression in ParamikoSSHClient.run method. Change SSHClient interface a bit and make "put" method return a full path to the location where a file has been saved. Remove old invalid test and add a new one. Part of LIBCLOUD-278 .
        Hide
        jdevesa Jaume Devesa added a comment -

        Wow. So sorry, I didn't think in that case.

        Show
        jdevesa Jaume Devesa added a comment - Wow. So sorry, I didn't think in that case.
        Hide
        kami Tomaz Muraus added a comment -

        Jaume Devesa no problem.

        We should have done a better job with the review and have tests to catch issues like this in the first place

        In any case, if you go over past couple of comments in this ticket you can see I've pushed some commits which revert the original change and fix it in the correct place (ScriptDeployment.run method).

        I would appreciate if you could go over those commits and verify that they look reasonable and fix the original issue you had.

        Show
        kami Tomaz Muraus added a comment - Jaume Devesa no problem. We should have done a better job with the review and have tests to catch issues like this in the first place In any case, if you go over past couple of comments in this ticket you can see I've pushed some commits which revert the original change and fix it in the correct place (ScriptDeployment.run method). I would appreciate if you could go over those commits and verify that they look reasonable and fix the original issue you had.
        Hide
        jdevesa Jaume Devesa added a comment -

        Tomaz,

        works as expected and definitely code is better placed.

        Show
        jdevesa Jaume Devesa added a comment - Tomaz, works as expected and definitely code is better placed.
        Hide
        kami Tomaz Muraus added a comment -

        Jaume Devesa Awesome. Thanks for testing it out.

        Show
        kami Tomaz Muraus added a comment - Jaume Devesa Awesome. Thanks for testing it out.

          People

          • Assignee:
            kami Tomaz Muraus
            Reporter:
            jdevesa Jaume Devesa
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development