vitalif
/
vitastor
5
4
Fork 9
Code Issues 1 Pull Requests 3 Actions Releases 77 Wiki Activity

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

Closed
huynp911 wants to merge 4 commits from huynp911/vitastor:master into master
pull from: huynp911/vitastor:master
merge into: vitalif:master
Conversation 3 Commits 4 Files Changed 1
1 changed files with 9 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
patches/cinder-vitastor.py
View File

@ -1,3 +1,8 @@
# Workaround:
# 461: allow to create volume from snapshot
# 527: adjust iodepth for faster deletion
# 591: add failure case when creating a volume
# Vitastor Driver for OpenStack Cinder
#
# --------------------------------------------
@ -455,7 +460,7 @@ class VitastorDriver(driver.CloneableImageVD,
vol_name = utils.convert_str(volume.name)
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
Review

Doesn't it give the same result?

Doesn't it give the same result?
huynp911 commented 2022-06-05 13:06:33 +03:00
Review

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
Review

Ok, I see

Ok, I see
if not snap:
raise exception.SnapshotNotFound(snapshot_id = snap_name)
snap_inode_id = int(resp['responses'][0]['kvs'][0]['value']['id'])
@ -524,7 +529,7 @@ class VitastorDriver(driver.CloneableImageVD,
for kv in layers:
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
Review

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

4 is the default :-), why did you want to change it?
huynp911 commented 2022-06-05 13:08:54 +03:00
Review

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
Review

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...

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...
*(self._vitastor_args())
]
try:
@ -584,6 +589,8 @@ class VitastorDriver(driver.CloneableImageVD,
{ 'request_put': { 'key': 'config/inode/'+pool_s+'/'+str(image_id), 'value': json.dumps({
**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
Review

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
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
Review

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
Review

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...
] })
if not resp.get('succeeded'):
# repeat