Ceph S3 Outscale storage driver fixes #792

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@jmunhoz
Contributor

jmunhoz commented May 20, 2016

  • Simplify the Outscale driver
  • Remove unused Outscale connection classes
  • Fix default signature v2 issue with the Outscale driver

Signed-off-by: Javier M. Mellid jmunhoz@igalia.com

Ceph S3 Outscale storage driver fixes
- Simplify the Outscale driver
- Remove unused Outscale connection classes
- Fix default signature v2 issue with the Outscale driver

Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>

@jmunhoz jmunhoz referenced this pull request May 20, 2016

Closed

Add S3_RGW storage driver #786

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 20, 2016

Member

@jmunhoz Thanks!

Can you please also add some tests - at least the one which instantiate both drivers and verify that host, etc. on the connection class is set correctly (this should be easy to do and be a bare minimum).

Member

Kami commented May 20, 2016

@jmunhoz Thanks!

Can you please also add some tests - at least the one which instantiate both drivers and verify that host, etc. on the connection class is set correctly (this should be easy to do and be a bare minimum).

Ceph S3 RGW storage drivers tests
- Basic testing support covering RGW/Outscale storage drivers

Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>
@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz May 20, 2016

Contributor

@Kami I pushed some test cases covering both drivers.

Contributor

jmunhoz commented May 20, 2016

@Kami I pushed some test cases covering both drivers.

@asfgit asfgit closed this in 0062f8b May 22, 2016

asfgit pushed a commit that referenced this pull request May 22, 2016

Ceph S3 RGW storage drivers tests
- Basic testing support covering RGW/Outscale storage drivers

Closes #792

Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>
Signed-off-by: Tomaz Muraus <tomaz@tomaz.me>
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 22, 2016

Member

Thanks.

I've added some additional asserts (084ce3c) and merged changes into trunk. I believe there is still a bug with they way the host is handled though:

       ....
        self.driver_v2 = self.driver_type(*self.driver_args,
                                          signature_version='2',
                                          host='host1')
        self.driver_v4 = self.driver_type(*self.driver_args,
                                          signature_version='4',
                                          host='host2')

        print self.driver_v2.connectionCls.host
        print self.driver_v4.connectionCls.host

And when running it:

osu.eu-west-2.outscale.com
osu.eu-west-2.outscale.com

As you can see, the host argument is ignored. That's probably because when connectionCls is instantiated, the host argument you set in the driver class constructor is overridden - as mentioned before, I recommend you to use _ex_connection_class_kwargs method. Overriding class variables is usually not a good idea, because it results in bugs and issues like that.

It would be great if you can look into it and also add corresponding tests (I didn't include those tests in the commit so it would cause the build to fail). When working on a fix, you can start with a failing test case :)

Member

Kami commented May 22, 2016

Thanks.

I've added some additional asserts (084ce3c) and merged changes into trunk. I believe there is still a bug with they way the host is handled though:

       ....
        self.driver_v2 = self.driver_type(*self.driver_args,
                                          signature_version='2',
                                          host='host1')
        self.driver_v4 = self.driver_type(*self.driver_args,
                                          signature_version='4',
                                          host='host2')

        print self.driver_v2.connectionCls.host
        print self.driver_v4.connectionCls.host

And when running it:

osu.eu-west-2.outscale.com
osu.eu-west-2.outscale.com

As you can see, the host argument is ignored. That's probably because when connectionCls is instantiated, the host argument you set in the driver class constructor is overridden - as mentioned before, I recommend you to use _ex_connection_class_kwargs method. Overriding class variables is usually not a good idea, because it results in bugs and issues like that.

It would be great if you can look into it and also add corresponding tests (I didn't include those tests in the commit so it would cause the build to fail). When working on a fix, you can start with a failing test case :)

@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz May 23, 2016

Contributor

Thanks @Kami I reviewed the test case and I think it is working as expected. The Outscale driver maps valid Outscale regions to the Outscale hosts. It should not let arbitrary hosts.

In this case, if you don't add any explicit valid region the Outscale driver works with the default region and it assigns the default host 'osu.eu-west-2.outscale.com'. The default region/host is the same for AWS2 and AWS4. It is the reason to see the same default host twice.

You can have a look in this line https://github.com/apache/libcloud/blob/trunk/libcloud/storage/drivers/rgw.py#L116

Maybe it makes sense raising an expection when host is not none? Something similar to:

   if region not in S3_RGW_OUTSCALE_HOSTS_BY_REGION:
      raise LibcloudError('Unknown region (%s)' % (region), driver=self)
   if host is not None:
      raise LibcloudError('This driver does not accept arbitrary host', driver=self)
   host = S3_RGW_OUTSCALE_HOSTS_BY_REGION[region]
Contributor

jmunhoz commented May 23, 2016

Thanks @Kami I reviewed the test case and I think it is working as expected. The Outscale driver maps valid Outscale regions to the Outscale hosts. It should not let arbitrary hosts.

In this case, if you don't add any explicit valid region the Outscale driver works with the default region and it assigns the default host 'osu.eu-west-2.outscale.com'. The default region/host is the same for AWS2 and AWS4. It is the reason to see the same default host twice.

You can have a look in this line https://github.com/apache/libcloud/blob/trunk/libcloud/storage/drivers/rgw.py#L116

Maybe it makes sense raising an expection when host is not none? Something similar to:

   if region not in S3_RGW_OUTSCALE_HOSTS_BY_REGION:
      raise LibcloudError('Unknown region (%s)' % (region), driver=self)
   if host is not None:
      raise LibcloudError('This driver does not accept arbitrary host', driver=self)
   host = S3_RGW_OUTSCALE_HOSTS_BY_REGION[region]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment