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

Simplified public ip allocation/release and added port forwarding rule

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Compute
    • Labels:
      None

      Description

      It started as a small patch and ended a little bigger than expected.
      Simplified some IP allocation/release
      Fixed the CloudStackNetwork class
      Modified CloudStackAddress class
      Added CloudStackPortForwarding class
      Added attributed to node for port_forwarding_rules
      Added list_public_ips and test...

      It's lacking a few tests but I would appreciate if you could commit to trunk so I can build on this.

      thanks

      1. libcloud.patch
        26 kB
        sebastien goasguen

        Activity

        Hide
        kami Tomaz Muraus added a comment -

        Ah, good point. I didn't even notice it when applying the patch.

        It shouldn't be a big deal though because this ticket and the CHANGES file still correctly attribute the changes.

        Show
        kami Tomaz Muraus added a comment - Ah, good point. I didn't even notice it when applying the patch. It shouldn't be a big deal though because this ticket and the CHANGES file still correctly attribute the changes.
        Hide
        sebgoa sebastien goasguen added a comment -

        Ah shoot, in the rebase/squash it messed up the authorship

        Show
        sebgoa sebastien goasguen added a comment - Ah shoot, in the rebase/squash it messed up the authorship
        Hide
        kami Tomaz Muraus added a comment -

        Merged, thanks!

        Show
        kami Tomaz Muraus added a comment - Merged, thanks!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3700a9a5c6338d5c96543aa35eefef1e578806c9 in branch refs/heads/trunk from Tomaz Muraus
        [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=3700a9a ]

        LIBCLOUD-381: CloudStack driver: Fixed port forwarding handling, added tests
        for volumes, improved docstrings

        Signed-off-by: Tomaz Muraus <tomaz@apache.org>

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3700a9a5c6338d5c96543aa35eefef1e578806c9 in branch refs/heads/trunk from Tomaz Muraus [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=3700a9a ] LIBCLOUD-381 : CloudStack driver: Fixed port forwarding handling, added tests for volumes, improved docstrings Signed-off-by: Tomaz Muraus <tomaz@apache.org>
        Hide
        sebgoa sebastien goasguen added a comment -

        Ok, I rebased..it was just the update of CHANGES.
        I am not sure yet that it's properly squashed, but if you can try that would be great

        thanks

        Show
        sebgoa sebastien goasguen added a comment - Ok, I rebased..it was just the update of CHANGES. I am not sure yet that it's properly squashed, but if you can try that would be great thanks
        Hide
        kami Tomaz Muraus added a comment -

        sebastien goasguen Thanks.

        Last patch is not generated against the latest trunk and I can't cleanly apply it. Can you please re-generate it against the latest trunk (git pull trunk ; squash ; git format-patch)?

        Show
        kami Tomaz Muraus added a comment - sebastien goasguen Thanks. Last patch is not generated against the latest trunk and I can't cleanly apply it. Can you please re-generate it against the latest trunk (git pull trunk ; squash ; git format-patch)?
        Hide
        sebgoa sebastien goasguen added a comment -

        Fixed in the attached patch libcloud381.4.patch
        It was missing a listPortForwarding fixture.

        Show
        sebgoa sebastien goasguen added a comment - Fixed in the attached patch libcloud381.4.patch It was missing a listPortForwarding fixture.
        Hide
        kami Tomaz Muraus added a comment -

        Thanks.

        I've ran the tests and noticed some failures in the KTUCloud driver which inherits from CloudStack.

        Need to make sure the KTUCloud also supports this new functionality and update the tests accordingly.

        Show
        kami Tomaz Muraus added a comment - Thanks. I've ran the tests and noticed some failures in the KTUCloud driver which inherits from CloudStack. Need to make sure the KTUCloud also supports this new functionality and update the tests accordingly.
        Hide
        sebgoa sebastien goasguen added a comment -

        Ok, check the new patch. it's a squash of the last two plus one from today.

        I rebased on trunk from yesterday other patches.

        I corrected docstrings, added a few tests including list_volumes

        I corrected a lot of functionality in the networking

        Still missing tests for the port forwarding rules but they are a bit more tedious, working on it.

        Show
        sebgoa sebastien goasguen added a comment - Ok, check the new patch. it's a squash of the last two plus one from today. I rebased on trunk from yesterday other patches. I corrected docstrings, added a few tests including list_volumes I corrected a lot of functionality in the networking Still missing tests for the port forwarding rules but they are a bit more tedious, working on it.
        Hide
        kami Tomaz Muraus added a comment -

        Alright, I'll wait for a new patch.

        Show
        kami Tomaz Muraus added a comment - Alright, I'll wait for a new patch.
        Hide
        sebgoa sebastien goasguen added a comment -

        Don't merge this, I will send you something better

        Show
        sebgoa sebastien goasguen added a comment - Don't merge this, I will send you something better
        Hide
        sebgoa sebastien goasguen added a comment -

        a squash commit that addresses your comments

        Show
        sebgoa sebastien goasguen added a comment - a squash commit that addresses your comments
        Hide
        kami Tomaz Muraus added a comment -

        Thanks, here are a couple of comments:

        1. Please underscore separated variable names (publicport -> public port, privateport -> private_port, ...).

        I know some of the existing variable in the driver, but we should do it right and be consistent with other drivers going forward.

        2. There is a typo in the docstring - virtualmachine -> node

        3. There are a couple of missing docstrings (ex_allocate_public_ip doesn't include a docstring for location argument, etc.)

        4. ex_release_public_ip method is backward incompatible so we should clearly document this in the change log and upgrading page (this is more of a note to myself)

        Show
        kami Tomaz Muraus added a comment - Thanks, here are a couple of comments: 1. Please underscore separated variable names (publicport -> public port, privateport -> private_port, ...). I know some of the existing variable in the driver, but we should do it right and be consistent with other drivers going forward. 2. There is a typo in the docstring - virtualmachine -> node 3. There are a couple of missing docstrings (ex_allocate_public_ip doesn't include a docstring for location argument, etc.) 4. ex_release_public_ip method is backward incompatible so we should clearly document this in the change log and upgrading page (this is more of a note to myself)

          People

          • Assignee:
            kami Tomaz Muraus
            Reporter:
            sebgoa sebastien goasguen
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development