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

proposal to add a new error type - ProviderError

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 0.12.3
    • Fix Version/s: 0.13.0
    • Component/s: Core
    • Labels:
      None

      Description

      libcloud.common.base.Response constructor is handling failures of provider operations, this way

      if not self.success():
      raise Exception(self.parse_error())

      for eg., in ec2 driver the parse_error method is overridden to handle specific cases like invalid credentials, malformed responses but all other error cases produced by ec2 will be raised as Exception. Its too generic and we need a classification (ProviderError) to capture all the errors generated by a cloud provider.

      • libcloud.common.base.Response can use it
        if not self.success():
        raise ProviderError(self.parse_error())
      • specific errors of providers can be derived from it
        class InvalidCredsError(ProviderError):

      Also drivers that are having provider specific error types (eg., SoftLayerException, LinodeException, HostVirtualException) can be extended from ProviderError. Currently its extending LibcloudError and since LibcloudError is inclusive of all internal stuffs, having a separate type for errors occured at provider side would simplify error handling for libcloud clients.

        Activity

        Hide
        kami Tomaz Muraus added a comment -

        I like the idea and agree that currently "LibcloudError" is used in too many places and having something like "ProviderError" for provider errors would be better.

        How did you envision the class hierarchy - would this class inherit from LibcloudError? This also implies how backward incompatible / breaking change this is.

        Also what would the existing "ContainerDoesntExist" and other errors inherit from? This new class?

        Show
        kami Tomaz Muraus added a comment - I like the idea and agree that currently "LibcloudError" is used in too many places and having something like "ProviderError" for provider errors would be better. How did you envision the class hierarchy - would this class inherit from LibcloudError? This also implies how backward incompatible / breaking change this is. Also what would the existing "ContainerDoesntExist" and other errors inherit from? This new class?
        Hide
        jayyy0v Jay Kumar added a comment -

        I see ProviderError can fit in between LibcloudError and its provider specific sub types.

        Proposed Hierarchy
        -------------------
        LibcloudError(Exception)

        ProviderError(LibcloudError)

        InvalidCredsError(ProviderError)

        Compatibility
        -------------
        Existing clients should be handling the types Exception, LibcloudError and other sub types of LibcloudError.

        The changes should seamlessly fit in as the ProviderError is still a LibcloudError and gets caught by existing clients as such except when errors are raised by libcloud.common.base.Response constructor

        if not self.success():
        raise Exception(self.parse_error())

        Changing this to raise ProviderError(self.parse_error()) might break a following client code:

        try:
        driver.operation()
        except LibcloudError:
        #handle
        except Exception:
        #handle

        Other Errors
        -------------
        I think errors like ContainerDoesNotExistError are of provider's (HTTP 404) like InvalidCredsError (HTTP 401).

        I noticed few other cases in drivers where LibcloudError is used to raise provider specific validation errors.

        • gogrid

        raise LibcloudError('No public unassigned IPs left'

        • joyent
          msg = 'Invalid location: "%s". Valid locations: %s'
          raise LibcloudError(msg % (kwargs['location'],
          ', '.join(LOCATIONS)), driver=self)

        Assume if the request is fired without this validation, provider might throw the same error. I suggest any validation that a driver does on behalf of provider should use ProviderError.

        What do you think?

        Show
        jayyy0v Jay Kumar added a comment - I see ProviderError can fit in between LibcloudError and its provider specific sub types. Proposed Hierarchy ------------------- LibcloudError(Exception) ProviderError(LibcloudError) InvalidCredsError(ProviderError) Compatibility ------------- Existing clients should be handling the types Exception, LibcloudError and other sub types of LibcloudError. The changes should seamlessly fit in as the ProviderError is still a LibcloudError and gets caught by existing clients as such except when errors are raised by libcloud.common.base.Response constructor if not self.success(): raise Exception(self.parse_error()) Changing this to raise ProviderError(self.parse_error()) might break a following client code: try: driver.operation() except LibcloudError: #handle except Exception: #handle Other Errors ------------- I think errors like ContainerDoesNotExistError are of provider's (HTTP 404) like InvalidCredsError (HTTP 401). I noticed few other cases in drivers where LibcloudError is used to raise provider specific validation errors. gogrid raise LibcloudError('No public unassigned IPs left' joyent msg = 'Invalid location: "%s". Valid locations: %s' raise LibcloudError(msg % (kwargs ['location'] , ', '.join(LOCATIONS)), driver=self) Assume if the request is fired without this validation, provider might throw the same error. I suggest any validation that a driver does on behalf of provider should use ProviderError. What do you think?
        Hide
        jc2k John Carr added a comment -

        I really like this idea. Right now my scripts just can't do reliable exception handling for a lot of cases.

        I think service based errors should be phased out. So no more SoftLayerException, LinodeException, etc. I don't think they add any meaningful information to a traceback.

        There should be more specific and generic exceptions:

        • LoadBalancerDoesNotExistError
        • NodeDoesNotExistError
        • VolumeDoesNotExistError
        • etc

        there should be *AlreadyExistsError variants too, as per the DNS interface.

        I don't like prefixing generally, I think "LoadBalancerError" is a much better name than "LibcloudLBError".

        A ValidationError would be good, which would be raised in the _init_ of Driver classes. It would be ideal if it could take the name of the parameter that caused validation to fail.

        For errors from API's that aren't generic we could have an OperationError (is this ProviderError?), with either flags or subclasses so that a Driver can signal to your code whether or not the issue is recoverable - whether the operation should be retried, or whether your script or background task should abort.

        Show
        jc2k John Carr added a comment - I really like this idea. Right now my scripts just can't do reliable exception handling for a lot of cases. I think service based errors should be phased out. So no more SoftLayerException, LinodeException, etc. I don't think they add any meaningful information to a traceback. There should be more specific and generic exceptions: LoadBalancerDoesNotExistError NodeDoesNotExistError VolumeDoesNotExistError etc there should be *AlreadyExistsError variants too, as per the DNS interface. I don't like prefixing generally, I think "LoadBalancerError" is a much better name than "LibcloudLBError". A ValidationError would be good, which would be raised in the _ init _ of Driver classes. It would be ideal if it could take the name of the parameter that caused validation to fail. For errors from API's that aren't generic we could have an OperationError (is this ProviderError?), with either flags or subclasses so that a Driver can signal to your code whether or not the issue is recoverable - whether the operation should be retried, or whether your script or background task should abort.
        Hide
        kami Tomaz Muraus added a comment -

        Jay Kumar I like this split and like you said, because it inherits from LibcloudError existing code will still work as expected.

        John Carr Yep, I agree. ProviderError is a good first (baby) step to get there. After that we can tackle other more specific errors in the compute and loadbalancer APIs.

        Show
        kami Tomaz Muraus added a comment - Jay Kumar I like this split and like you said, because it inherits from LibcloudError existing code will still work as expected. John Carr Yep, I agree. ProviderError is a good first (baby) step to get there. After that we can tackle other more specific errors in the compute and loadbalancer APIs.
        Hide
        kami Tomaz Muraus added a comment -

        Jay Kumar Just curious - are you going to work on ticket?

        Show
        kami Tomaz Muraus added a comment - Jay Kumar Just curious - are you going to work on ticket?
        Hide
        jayyy0v Jay Kumar added a comment -

        Tomaz Muraus Just waited for comments. I just started working on it and will upload the patch.

        Show
        jayyy0v Jay Kumar added a comment - Tomaz Muraus Just waited for comments. I just started working on it and will upload the patch.
        Hide
        jayyy0v Jay Kumar added a comment -

        Adds the new error type ProviderError and modifies InvalidCredsError to extend from it.

        Show
        jayyy0v Jay Kumar added a comment - Adds the new error type ProviderError and modifies InvalidCredsError to extend from it.
        Hide
        kami Tomaz Muraus added a comment - - edited

        Jay Kumar Patch looks good to me.

        Do you also plan to update all the affected code (provider drivers)?

        Show
        kami Tomaz Muraus added a comment - - edited Jay Kumar Patch looks good to me. Do you also plan to update all the affected code (provider drivers)?
        Hide
        jayyy0v Jay Kumar added a comment -

        Tomaz Muraus Yes, I am on it.

        First change is in common/base/Response constructor to raise ProviderError instead of Exception. It might break existing clients and should be mentioned in "change logs" of next version.

        Show
        jayyy0v Jay Kumar added a comment - Tomaz Muraus Yes, I am on it. First change is in common/base/Response constructor to raise ProviderError instead of Exception. It might break existing clients and should be mentioned in "change logs" of next version.
        Hide
        kami Tomaz Muraus added a comment -

        Jay Kumar Sounds good, thanks.

        Show
        kami Tomaz Muraus added a comment - Jay Kumar Sounds good, thanks.
        Hide
        kami Tomaz Muraus added a comment -

        Jay Kumar We have recently migrated from svn to git. If you want to preserve commit authorship you can now re-generate your patch using git-format patch.

        The patch looks good, so let me know once you have re-regenerated it and I'll merge it into master.

        Show
        kami Tomaz Muraus added a comment - Jay Kumar We have recently migrated from svn to git. If you want to preserve commit authorship you can now re-generate your patch using git-format patch. The patch looks good, so let me know once you have re-regenerated it and I'll merge it into master.
        Hide
        kami Tomaz Muraus added a comment -

        Jay Kumar Ping.

        Let me know if you plan to re-generate this patch or if you are fine with me applying it as it is without preserving the commit author.

        Show
        kami Tomaz Muraus added a comment - Jay Kumar Ping. Let me know if you plan to re-generate this patch or if you are fine with me applying it as it is without preserving the commit author.
        Hide
        jayyy0v Jay Kumar added a comment -

        Tomaz Muraus Sorry to make you wait. I'm bit late to the git party.

        https://github.com/apache/libcloud/pull/117

        Show
        jayyy0v Jay Kumar added a comment - Tomaz Muraus Sorry to make you wait. I'm bit late to the git party. https://github.com/apache/libcloud/pull/117
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 30dfdb4848cf7c0cfb42c331afd657b01fbf2be1 in branch refs/heads/0.12.x from Jay Kumar
        [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=30dfdb4 ]

        LIBCLOUD-331: Adds the new error type ProviderError. Modifies InvalidCredsError
        to extend from it.

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

        Show
        jira-bot ASF subversion and git services added a comment - Commit 30dfdb4848cf7c0cfb42c331afd657b01fbf2be1 in branch refs/heads/0.12.x from Jay Kumar [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=30dfdb4 ] LIBCLOUD-331 : Adds the new error type ProviderError. Modifies InvalidCredsError to extend from it. Signed-off-by: Tomaz Muraus <tomaz@apache.org>
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit cb9d374732fca01d488e117221baf1fbf6603032 in branch refs/heads/trunk from Jay Kumar
        [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=cb9d374 ]

        LIBCLOUD-331: Adds the new error type ProviderError. Modifies InvalidCredsError
        to extend from it.

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

        Show
        jira-bot ASF subversion and git services added a comment - Commit cb9d374732fca01d488e117221baf1fbf6603032 in branch refs/heads/trunk from Jay Kumar [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=cb9d374 ] LIBCLOUD-331 : Adds the new error type ProviderError. Modifies InvalidCredsError to extend from it. Signed-off-by: Tomaz Muraus <tomaz@apache.org>
        Hide
        kami Tomaz Muraus added a comment -

        I've changed your patch to use httplib.UNAUTHORIZED constant and merged it into trunk.

        Thanks

        Show
        kami Tomaz Muraus added a comment - I've changed your patch to use httplib.UNAUTHORIZED constant and merged it into trunk. Thanks

          People

          • Assignee:
            kami Tomaz Muraus
            Reporter:
            jayyy0v Jay Kumar
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development