Patch cinder driver: create snapshot from volume & create image #8

huynp911 · 2021-12-22T07:07:57+03:00

huynp911 commented

2021-12-22 07:07:57 +03:00

First-time contributor

Fix create snapshot from volume
vol_name is the new ID of the snapshot, not the parent ID of snapshot volume. That's why _get_name function can't find any valid volume
Add failure case of creating image to avoid infinite loop

1. Fix create snapshot from volume `vol_name` is the new ID of the snapshot, not the parent ID of snapshot volume. That's why `_get_name` function can't find any valid volume 2. Add failure case of creating image to avoid infinite loop

huynp911 added 1 commit 2021-12-22 07:07:57 +03:00

patch cinder driver 25fccefe6d

huynp911 added 1 commit 2022-02-08 12:37:11 +03:00

Update cinder driver workaround ec44cb09e7

huynp911 added 1 commit 2022-02-08 12:44:42 +03:00

Update cinder driver workaround fa95a26109

huynp911 added 1 commit 2022-02-08 12:46:08 +03:00

Update cinder driver workaround b57a40a67e

huynp911 closed this pull request

2022-05-23 12:21:52 +03:00

huynp911 reopened this pull request

2022-05-23 12:30:18 +03:00

vitalif requested changes 2022-06-05 00:39:55 +03:00

patches/cinder-vitastor.py

						
				@@ -456,3 +461,3 @@

				        snap_name = utils.convert_str(snapshot.name)

				        snap = self._get_image(vol_name+'@'+snap_name)

				        snap = self._get_image("volume-"+snapshot.volume_id+'@'+snap_name)

vitalif commented

2022-06-05 00:39:50 +03:00

Doesn't it give the same result?

huynp911 commented

2022-06-05 13:06:33 +03:00

First-time contributor

No it not the same, for example:
volume layer 1 (ID1) --> snapshot layer 1 (ID2) --> volume layer 2 (ID3)

I want to create a volume from the snapshot whose name is volume-ID1@snapshot-ID2.
But the problem is, vol_name has value volume-ID3, not volume-ID1. And cinder will try to find volume-ID3@snapshot-ID2, which not exist.
So I use snapshot.volume_id which has value ID1, and add a prefix volume-, and cinder will able to find the snapshot.

No it not the same, for example: volume layer 1 (ID1) --> snapshot layer 1 (ID2) --> volume layer 2 (ID3) I want to create a volume from the snapshot whose name is `volume-ID1@snapshot-ID2`. But the problem is, `vol_name` has value `volume-ID3`, not `volume-ID1`. And cinder will try to find `volume-ID3@snapshot-ID2`, which not exist. So I use `snapshot.volume_id` which has value ID1, and add a prefix `volume-`, and cinder will able to find the snapshot.

vitalif commented

2022-06-06 15:44:18 +03:00

Ok, I see

patches/cinder-vitastor.py

						
				@@ -525,3 +530,3 @@

				            args = [

				                'vitastor-cli', 'rm-data', '--pool', str(kv['value']['pool_id']),

				                '--inode', str(kv['value']['id']), '--progress', '0',

				                '--inode', str(kv['value']['id']), '--iodepth', '4', '--progress', '0',

vitalif commented

2022-06-05 00:36:27 +03:00

4 is the default :-), why did you want to change it?

huynp911 commented

2022-06-05 13:08:54 +03:00

First-time contributor

My centos cluster can't delete a large volume with higher iodepth. (it causes OSDs restart constantly, as I mentioned before)

After switching to debian, this issue is gone. So you can ignore this :v

My centos cluster can't delete a large volume with higher iodepth. (it causes OSDs restart constantly, as I mentioned before) After switching to debian, this issue is gone. So you can ignore this :v

vitalif commented

2022-06-06 01:55:53 +03:00

There was a bug in inode removal which I finally fixed in 0.7.1 - it was listing too many PGs at once regardless of iodepth and parallel_osds setting :-) so maybe now your problem won't reproduce anyway...

patches/cinder-vitastor.py

						
				@@ -585,2 +590,4 @@

				                    **cfg, 'name': vol_name,

				                }) } },

				            ], 'failure': [

				                { 'request_put': { 'key': 'index/maxid/'+pool_s, 'value': image_id } },

vitalif commented

2022-06-05 00:37:53 +03:00

It's definitely not good, the idea is:

check current maxid counter
create an image with the new ID only if maxid didn't change between 1 and 2
restart from 1 if maxid changed

It's definitely not good, the idea is: 1. check current maxid counter 2. create an image with the new ID only if maxid didn't change between 1 and 2 3. restart from 1 if maxid changed

huynp911 commented

2022-06-05 13:14:42 +03:00

First-time contributor

What if I put an inode using etcdctl: id = 1, maxid at that time was still 0.

You can see the conflict here, maxid = 0 but inode with ID 1 already existed.
-> openstack cinder can't create a new volume

What if I put an inode using etcdctl: id = 1, maxid at that time was still 0. You can see the conflict here, maxid = 0 but inode with ID 1 already existed. -> openstack cinder can't create a new volume

vitalif commented

2022-06-06 01:53:46 +03:00

Ok, so you want to fix the case where some inodes are created by hand and index/maxid is absent or incorrect?

I think in this case you should do a request_range on failure, check if the inode that you tried to create actually exists, and then scan available inode numbers if it does...

Or maybe I'll implement it in vitastor-cli create and then I'll just rework cinder driver to use vitastor-cli instead of direct etcd communication...

Ok, so you want to fix the case where some inodes are created by hand and index/maxid is absent or incorrect? I think in this case you should do a request_range on failure, check if the inode that you tried to create actually exists, and then scan available inode numbers if it does... Or maybe I'll implement it in `vitastor-cli create` and then I'll just rework cinder driver to use vitastor-cli instead of direct etcd communication...