lightos driver: add lightos volume driver #1

yogev-lb · 2022-01-12T12:50:34Z

initial commit to add our driver to the cinder fork

in order to run the unittests:

Test all lightos ut:

source .tox/py38/bin/activate
stestr run -n cinder.tests.unit.volume.drivers.lightos.test_lightos_storage.LightOSStorageVolumeDriverTest
stestr run -n cinder.tests.unit.volume.drivers.lightos.test_lightos_storage.LightOSStorageVolumeDriverTest.<test_name>

Test specific method by name:

source .tox/py38/bin/activate
stestr run -n cinder.tests.unit.volume.drivers.lightos.test_lightos_storage.LightOSStorageVolumeDriverTest.<test_name>

ronen-lb · 2022-01-12T18:33:21Z

cinder/volume/drivers/lightos/constants.py

@@ -0,0 +1,3 @@
+DEVICE_SCAN_ATTEMPTS_DEFAULT = 5
+LIGHTOS = "LIGHTOS"
+LIGHTOS_API_SERVICE_TIMEOUT = 30


ronen-lb · 2022-01-13T12:54:56Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+        self.driver.configuration.lightos_api_address = ""
+        self.assertRaises(exception.InvalidParameterValue, lightos.LightOSConnection, self.driver.configuration)
+
+    # Yogev


there are some local comments as well as commented out code that should be removed

ronen-lb · 2022-01-14T10:34:07Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+FAKE_LIGHTOS_CLUSTER_INFO: Dict[str, str] = {
+    'UUID': "926e6df8-73e1-11ec-a624-07ba3880f6cc",
+    'subsystemNQN': "nqn.2014-08.org.nvmexpress:NVMf:uuid:"\
+        "f4a89ce0-9fc2-4900-bfa3-00ad27995e7b"


possibly also init statistics, to enable future coverage of get_Stats

ronen-lb · 2022-01-14T10:37:41Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+                    "src_snapshot_name": kwargs["src_snapshot_name"],
+                    "acl": kwargs["acl"],
+                    "UUID": str(uuid.uuid4()),
+                    "state": "Available",


not critical but real lightOS response will return the as Creating.
we want it as avaialble in DB.
so can possibly init here to creating.
and clone object update cloned pbject to available before adding to db.

ronen-lb · 2022-01-14T10:40:56Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+                    "UUID": str(uuid.uuid4()),
+                    "state": "Available",
+                }
+                project = self.db["projects"].setdefault(project_name, {})


so create a volume will also set/create a project in db?

ronen-lb · 2022-01-14T10:44:53Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+                project_name = kwargs["project_name"]
+                volume_uuid = kwargs["volume_uuid"]
+                project = self.db["projects"].get(project_name, None)
+                if project:


if we dont find project we seem to fall down to raise runtime error.
i think close emulation of product is
return (httpstatus.INVALID_ARGUMENT, None)

ronen-lb · 2022-01-14T10:45:59Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+                if project:
+                    volume = next(
+                        iter(
+                            volume for volume in project["volumes"] \


why not use a dict under project["volumes"] with volume uuid as key?

ronen-lb · 2022-01-14T10:46:39Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+                project_name = kwargs["project_name"]
+                volume_uuid = kwargs["volume_uuid"]
+                project = self.db["projects"].get(project_name, None)
+                if project:


see comment above about project not found

ronen-lb · 2022-01-14T11:04:53Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+
+    def __init__(self):
+        self.data = {
+            "projects"


to initialist it as a map shouldnt it be "projects" : {} ?

ronen-lb · 2022-01-14T11:07:28Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+    def create_volume(self, volume):
+        assert volume.project_name and volume.name, "must be provided"
+        project = self.get_or_create_project(volume.project_name)
+        volumes = project.setdefault("volumes", {})


create volume in lightOS is not idempotent we will return error if already exist

ronen-lb · 2022-01-14T11:08:40Z

cinder/tests/unit/volume/drivers/lightos/test_lightos_storage.py

+        proj_vols = project.get("volumes", None)
+        if not proj_vols:
+            return
+        proj_vols.remove(volume)


lightos wil lreturn err if volume is not found (likely something we need to change)

ronen-lb · 2022-01-14T15:14:28Z

cinder/volume/drivers/lightos/lightos.py

-            states=states_to_wait_for,
-            vol_uuid=vol_uuid,
-            vol_name=vol_name)
+        states = ('Available', 'Deleting', 'Deleted', 'Failed', 'UNKNOWN')


note that lightOS will never return Deleted

not for now, but need to discuss what we want to do for states rollback and in particular migrating

ronen-lb · 2022-01-14T15:19:23Z

cinder/volume/drivers/lightos/lightos.py

+                                              vol_name=vol_name)
+            state = resp.get('state', 'UNKNOWN') if \
+                status_code == httpstatus.OK and resp else 'UNKNOWN'
+            if state in states and status_code != httpstatus.NOT_FOUND:


why do we break here on any err exepect for not_found?
i.e for internal error (etcd down) we want to retry.
possibly the only two cases we dont want to retry are if we fail authntication (shouldnt happen happy flow)
or invalide arg (no name and no uuid, should also not happen happy flow)

ronen-lb · 2022-01-14T15:22:46Z

cinder/volume/drivers/lightos/lightos.py

-            project_name, timeout, states=(
-                'Deleted', 'Deleting', 'UNKNOWN'), vol_uuid=vol_uuid)
+        assert vol_uuid, 'LightOS volume UUID must be specified'
+        states = ('Deleted', 'Deleting', 'UNKNOWN')


lightOS wont return state Deleted, either one of the active volume states (Aviaalble, updating, creating, migrating, rollback), failed state or deleting
deleted volume will return unkown

ronen-lb · 2022-01-14T15:31:26Z

cinder/volume/drivers/lightos/lightos.py

+                return 'Deleted'
+            state = resp.get('state', 'UNKNOWN') if \
+                status_code == httpstatus.OK and resp else 'UNKNOWN'
+            if state in states:


thnk this would be clearr if you just check state== deleting (deleted and unkown should never be returned)

ronen-lb · 2022-01-14T15:32:55Z

cinder/volume/drivers/lightos/lightos.py

@@ -738,7 +738,7 @@ def _create_volume(self, volume, src_snapshot_lightos_name):
                src_snapshot_lightos_name=src_snapshot_lightos_name)

        # when we get here, we definitely tried to create it in the past
-        if status_code == httpstatus.OK:
+        if status_code in (httpstatus.OK, httpstatus.CREATED):


this is for UT? lighttOS can return status created?

in order to mock these two pieces of code we must inject them from outside.

…nection add some validation where needed for input values from config.

wait for volume delete and wait for volume create behave differently. NOT_FOUND means different things between create and delete. for create it means that we didn't start yet. for delete it means that the volume is gone and nothing to do.

Wait for snapshot delete and wait for snapshot create behave differently. NOT_FOUND means different things between create and delete. for create it means that we didn't start yet. for delete it means that the snapshot is gone and nothing to do.

add unittests for the lightos cinder driver. try to mock as littel as possible by overriding the send_cmd method of the connector.

tomerg-lb

@yogev-lb , is this still relevant? If not, please close

yogev-lb requested a review from tomerg-lb January 12, 2022 12:50

yogev-lb force-pushed the yogev/add-unittests branch 3 times, most recently from 2653c0b to b9ab6b9 Compare January 12, 2022 15:56

yogev-lb requested review from amittar-lb, yant-lb and yuval-lb January 12, 2022 16:06

yogev-lb force-pushed the yogev/add-unittests branch from b9ab6b9 to 6f24010 Compare January 12, 2022 16:52

yogev-lb requested a review from sagi-lb January 12, 2022 16:52

ronen-lb reviewed Jan 12, 2022

View reviewed changes

yogev-lb force-pushed the yogev/add-unittests branch 2 times, most recently from a3b644a to 8d41f09 Compare January 13, 2022 10:19

ronen-lb reviewed Jan 13, 2022

View reviewed changes

yogev-lb force-pushed the yogev/add-unittests branch 3 times, most recently from 4a32ddd to 67a8f77 Compare January 14, 2022 07:14

ronen-lb reviewed Jan 14, 2022

View reviewed changes

yogev-lb force-pushed the yogev/add-unittests branch from 67a8f77 to d7ba8d7 Compare January 14, 2022 14:40

ronen-lb reviewed Jan 14, 2022

View reviewed changes

yogev-lb force-pushed the yogev/add-unittests branch from d7ba8d7 to 89d1c55 Compare January 14, 2022 15:27

ronen-lb reviewed Jan 14, 2022

View reviewed changes

yogev-lb force-pushed the yogev/add-unittests branch 4 times, most recently from fd084ae to 2192673 Compare January 16, 2022 09:37

opendev code

6965d75

yogev-lb force-pushed the yogev/add-unittests branch from 2192673 to 65e652d Compare January 16, 2022 10:09

yogev-lb added 8 commits January 16, 2022 18:16

lightos driver: add lightos volume driver

b8ca7aa

drivers/lightos: dependency injection for lightos_client and connector

4e63e6a

in order to mock these two pieces of code we must inject them from outside.

lightos/lightos.py: extract apiservers init from driver to LightOSCon…

dec4211

…nection add some validation where needed for input values from config.

lightos/lightos.py: create volume may return http.OK or http.CREATED

b18eec5

drivers/lightos: extend_volume quit retry if volume not found

019ef4f

lightos: unittests - initial commit

682c746

add unittests for the lightos cinder driver. try to mock as littel as possible by overriding the send_cmd method of the connector.

yogev-lb force-pushed the yogev/add-unittests branch from 65e652d to 682c746 Compare January 16, 2022 16:20

tomerg-lb reviewed Sep 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightos driver: add lightos volume driver #1

lightos driver: add lightos volume driver #1

yogev-lb commented Jan 12, 2022 •

edited

Loading

ronen-lb Jan 12, 2022

ronen-lb Jan 13, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022 •

edited

Loading

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022 •

edited

Loading

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

ronen-lb Jan 14, 2022

tomerg-lb left a comment

lightos driver: add lightos volume driver #1

Are you sure you want to change the base?

lightos driver: add lightos volume driver #1

Conversation

yogev-lb commented Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronen-lb Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronen-lb Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerg-lb left a comment

Choose a reason for hiding this comment

yogev-lb commented Jan 12, 2022 •

edited

Loading

ronen-lb Jan 14, 2022 •

edited

Loading

ronen-lb Jan 14, 2022 •

edited

Loading