Akrog / cinderlib

Cinder Library allows using storage drivers outside of Cinder
Apache License 2.0
14 stars 5 forks source link

Can't do Backend for ibm svc v7000 #15

Closed tanshaolong closed 5 years ago

tanshaolong commented 5 years ago

I am try cinderlib for IBM svc v7000, but I get below error messages. Would you help how to resolve the issue or give some hint? Thank you very much.

Test enviroment: Cinderlib version: v0.3.9 Cinder release: Pike Storage: IBM SVC V7000 Versions: Unknown Connection type: FC

Test code: import cinderlib as cl lvm = cl.Backend(volume_driver='cinder.volume.drivers.ibm.storwize_svc.storwize_svc_fc.StorwizeSVCFCDriver', san_ip='...', san_login='superuser', san_password='****', storwize_svc_volpool_name='mdiskgrp1', volume_backend_name='svc1234')

Error messages: Traceback (most recent call last): File "mytest.py", line 11, in volume_backend_name='svc1234') File "/usr/lib/python2.7/site-packages/cinderlib/cinderlib.py", line 86, in init self.driver.do_setup(objects.CONTEXT) File "/usr/lib/python2.7/site-packages/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py", line 2262, in do_setup volumes = objects.VolumeList.get_all_by_host(admin_context, self.host) File "/usr/lib/python2.7/site-packages/cinder/objects/volume.py", line 613, in get_all_by_host volumes = db.volume_get_all_by_host(context, host, filters) File "/usr/lib/python2.7/site-packages/cinder/db/api.py", line 274, in volume_get_all_by_host return IMPL.volume_get_all_by_host(context, host, filters=filters) AttributeError: 'DB' object has no attribute 'volume_get_all_by_host'

Configuration in cinder.conf [svc1234] volume_driver=cinder.volume.drivers.ibm.storwize_svc.storwize_svc_fc.StorwizeSVCFCDriver san_ip=... san_login=superuser san_password=***** storwize_svc_volpool_name=mdiskgrp1 volume_backend_name=svc1234

Akrog commented 5 years ago

Thank you for the great bug report.

The issue is caused by the SVC driver accessing the database directly, which is not considered good practice in Cinder, though this code could come from a time when this was a common practice in Cinder drivers.

The problem with drivers using the DB directly is that cinderlib doesn't require a DB, it can work storing everything in memory. To facilitate storage of the resources' metadata cinderlib has its own persistence plugin mechanism, and has some ad-hoc "workarounds" for drivers that access the DB directly.

Since you are the first one to try the SVC driver, you were the one that discovered these DB dependencies on the driver which will require adding workarounds in cinderlib for the DB access that driver has.

As a temporary workaround, and to confirm that there are no additional issues with the driver, you can use one of the database persistence plugins: in-memory or using a database (it can be a sqlite database).

To use a different persistence plugin you must pass the persistence_config parameter to the global_setup method before creating any Backend as described in the initialization part of the docs. Database plugins are also explained in the metadata persistence plugin doc.

Example using a SQLite file called 'cl.sqlite` in the current directory:

import cinderlib as cl

persistence_config = {'storage': 'db', 'connection': 'sqlite:///cl.sqlite'}
cl.setup(persistence_config=persistence_config)

Using a memory SQLite database:

import cinderlib as cl

persistence_config = {'storage': 'memory_db'}
cl.setup(persistence_config=persistence_config)

This is a workaround because we are limited to using DB persistence plugins and we cannot use any other plugins, which means that projects like Ember-CSI will not work using their default persistence model, which in that case is storing data as CRDs in Kubernetes.


The following is only relevant for coding the fix. Upon looking at the driver I see that we have the following DB dependencies:

In cinderlib we don't currently support groups or consistency groups, so we don't need to implement _group_type_get_full and we have already implemented workaround for methods volume_get and volume_type_get, so we only need to implement workarounds for volume_get_all_by_host and volume_type_get_by_name.

PS: cinderlib is now an official OpenStack project hosted in https://github.com/openstack/cinderlib so I have created a bug in launchpad replicating this one to track it there.

Akrog commented 5 years ago

@tanshaolong I have proposed a patch to fix this, but I don't have the backend to confirm that the code actually resolves the issue, would you mind testing the code?

To test it you can just edit file /usr/lib/python2.7/site-packages/cinderlib/persistence/base.py and add at the end of the file in the DB class, right before def vol_type_to_dict(volume_type): the following code:

    def _volume_type_get_by_name(self, context, name, session=None):
        return self.volume_type_get(context, name)

    def volume_get_all_by_host(self, context, host, filters=None):
        backend_name = host.split('#')[0].split('@')[1]
        result = self.persistence.get_volumes(backend_name=backend_name)
        return [vol._ovo for vol in result]

Or just replace the file with https://git.openstack.org/cgit/openstack/cinderlib/tree/cinderlib/persistence/base.py?h=refs/changes/12/648212/1

tanshaolong commented 5 years ago

@tanshaolong I have proposed a patch to fix this, but I don't have the backend to confirm that the code actually resolves the issue, would you mind testing the code?

To test it you can just edit file /usr/lib/python2.7/site-packages/cinderlib/persistence/base.py and add at the end of the file in the DB class, right before def vol_type_to_dict(volume_type): the following code:

    def _volume_type_get_by_name(self, context, name, session=None):
        return self.volume_type_get(context, name)

    def volume_get_all_by_host(self, context, host, filters=None):
        backend_name = host.split('#')[0].split('@')[1]
        result = self.persistence.get_volumes(backend_name=backend_name)
        return [vol._ovo for vol in result]

Or just replace the file with https://git.openstack.org/cgit/openstack/cinderlib/tree/cinderlib/persistence/base.py?h=refs/changes/12/648212/1

Thanks for your quick response. I will try your solution and show the result at here.

tanshaolong commented 5 years ago

@tanshaolong I have proposed a patch to fix this, but I don't have the backend to confirm that the code actually resolves the issue, would you mind testing the code?

To test it you can just edit file /usr/lib/python2.7/site-packages/cinderlib/persistence/base.py and add at the end of the file in the DB class, right before def vol_type_to_dict(volume_type): the following code:

    def _volume_type_get_by_name(self, context, name, session=None):
        return self.volume_type_get(context, name)

    def volume_get_all_by_host(self, context, host, filters=None):
        backend_name = host.split('#')[0].split('@')[1]
        result = self.persistence.get_volumes(backend_name=backend_name)
        return [vol._ovo for vol in result]

Or just replace the file with https://git.openstack.org/cgit/openstack/cinderlib/tree/cinderlib/persistence/base.py?h=refs/changes/12/648212/1

I have tried your fix and I don't get any exception. I think your code woke. Thank you very much. I will test cinderlib other features in IBM SVC.

tanshaolong commented 5 years ago

Thank you for the great bug report.

The issue is caused by the SVC driver accessing the database directly, which is not considered good practice in Cinder, though this code could come from a time when this was a common practice in Cinder drivers.

The problem with drivers using the DB directly is that cinderlib doesn't require a DB, it can work storing everything in memory. To facilitate storage of the resources' metadata cinderlib has its own persistence plugin mechanism, and has some ad-hoc "workarounds" for drivers that access the DB directly.

Since you are the first one to try the SVC driver, you were the one that discovered these DB dependencies on the driver which will require adding workarounds in cinderlib for the DB access that driver has.

As a temporary workaround, and to confirm that there are no additional issues with the driver, you can use one of the database persistence plugins: in-memory or using a database (it can be a sqlite database).

To use a different persistence plugin you must pass the persistence_config parameter to the global_setup method before creating any Backend as described in the initialization part of the docs. Database plugins are also explained in the metadata persistence plugin doc.

Example using a SQLite file called 'cl.sqlite` in the current directory:

import cinderlib as cl

persistence_config = {'storage': 'db', 'connection': 'sqlite:///cl.sqlite'}
cl.setup(persistence_config=persistence_config)

Using a memory SQLite database:

import cinderlib as cl

persistence_config = {'storage': 'memory_db'}
cl.setup(persistence_config=persistence_config)

This is a workaround because we are limited to using DB persistence plugins and we cannot use any other plugins, which means that projects like Ember-CSI will not work using their default persistence model, which in that case is storing data as CRDs in Kubernetes.

The following is only relevant for coding the fix. Upon looking at the driver I see that we have the following DB dependencies:

  • volume_get_all_by_host when using objects.VolumeList.get_all_by_host
  • volume_type_get when using objects.VolumeType.get_by_name_or_id
  • volume_type_get_by_name when using objects.VolumeType.get_by_name_or_id
  • volume_get directly called by get_by_id when using objects.Volume.get_by_id
  • _group_type_get_full directly called by get_by_id when using objects.GroupType.get_by_id

In cinderlib we don't currently support groups or consistency groups, so we don't need to implement _group_type_get_full and we have already implemented workaround for methods volume_get and volume_type_get, so we only need to implement workarounds for volume_get_all_by_host and volume_type_get_by_name.

PS: cinderlib is now an official OpenStack project hosted in https://github.com/openstack/cinderlib so I have created a bug in launchpad replicating this one to track it there.

I also have tried your workaround with memory sqlite and sqlite file database. Don't your fixed code, they work yet. Thank you.

tanshaolong commented 5 years ago

Thank you for the great bug report.

The issue is caused by the SVC driver accessing the database directly, which is not considered good practice in Cinder, though this code could come from a time when this was a common practice in Cinder drivers.

The problem with drivers using the DB directly is that cinderlib doesn't require a DB, it can work storing everything in memory. To facilitate storage of the resources' metadata cinderlib has its own persistence plugin mechanism, and has some ad-hoc "workarounds" for drivers that access the DB directly.

Since you are the first one to try the SVC driver, you were the one that discovered these DB dependencies on the driver which will require adding workarounds in cinderlib for the DB access that driver has.

As a temporary workaround, and to confirm that there are no additional issues with the driver, you can use one of the database persistence plugins: in-memory or using a database (it can be a sqlite database).

To use a different persistence plugin you must pass the persistence_config parameter to the global_setup method before creating any Backend as described in the initialization part of the docs. Database plugins are also explained in the metadata persistence plugin doc.

Example using a SQLite file called 'cl.sqlite` in the current directory:

import cinderlib as cl

persistence_config = {'storage': 'db', 'connection': 'sqlite:///cl.sqlite'}
cl.setup(persistence_config=persistence_config)

Using a memory SQLite database:

import cinderlib as cl

persistence_config = {'storage': 'memory_db'}
cl.setup(persistence_config=persistence_config)

This is a workaround because we are limited to using DB persistence plugins and we cannot use any other plugins, which means that projects like Ember-CSI will not work using their default persistence model, which in that case is storing data as CRDs in Kubernetes.

The following is only relevant for coding the fix. Upon looking at the driver I see that we have the following DB dependencies:

  • volume_get_all_by_host when using objects.VolumeList.get_all_by_host
  • volume_type_get when using objects.VolumeType.get_by_name_or_id
  • volume_type_get_by_name when using objects.VolumeType.get_by_name_or_id
  • volume_get directly called by get_by_id when using objects.Volume.get_by_id
  • _group_type_get_full directly called by get_by_id when using objects.GroupType.get_by_id

In cinderlib we don't currently support groups or consistency groups, so we don't need to implement _group_type_get_full and we have already implemented workaround for methods volume_get and volume_type_get, so we only need to implement workarounds for volume_get_all_by_host and volume_type_get_by_name.

PS: cinderlib is now an official OpenStack project hosted in https://github.com/openstack/cinderlib so I have created a bug in launchpad replicating this one to track it there.

tanshaolong commented 5 years ago

@Akrog Hi Gorka,

I test create\clone\snapshot volume in IBM SVC today. Some issues I have created new threads to track. When I create a new volume, I find cinderlib can't give the volume of ourselves name in IBM SVC. I don't sure whether or not it is as design. Or how could we set ourselves volume name in IBM svc?

My method for creating volume is: vol = lvm.create_volume(size=1, name="mytest", display_name="aaaa")

The display name in IBM SVC is "volume-224690b0-a9c8-4a1c-baf8-eee48be1a1b8" yet.

Thanks Ray

Akrog commented 5 years ago

@tanshaolong Hi Ray,

The name and display_name parameters refer to the same thing. The name alias was added in cinderlib for convenience (it's shorter). They are intended as user metadata, so users can store a user friendly name if they want, but are not used to define the name in the backend.

In most cases the name used in the backend will be volume- and the ID of the volume, and you can easily know the volume name in the backend by doing volume_name = 'volume-' + vol.id.

There are multiple reasons for this:

The reason why it starts with the word volume is because the default volume_name_template configuration is 'volume-%s' which we then use to format with the id of the volume.

You can change the volume_name_template to not include the volume- prefix, but I wouldn't recommend. You can even hack the code so that the name of the volume is used to create the volume in the backend, but I definitely don't recommend this.

tanshaolong commented 5 years ago

Got it. Thank Gorka