vitalif
/
vitastor
5
4
Fork 9
Code Issues 1 Pull Requests 2 Actions Releases 69 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
huynp911 commented 2021-12-22 07:07:57 +03:00

  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

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
huynp911 added 1 commit 2022-02-08 12:37:11 +03:00
huynp911 added 1 commit 2022-02-08 12:44:42 +03:00
huynp911 added 1 commit 2022-02-08 12:46:08 +03:00
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
Owner

Doesn't it give the same result?

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

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
Owner

Ok, I see

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
Owner

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
Poster

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
Owner

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

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
Poster

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
Owner

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...
vitalif commented 2022-06-05 00:40:50 +03:00
Owner

Hi, sorry, I missed your PR. :-)

Hi, sorry, I missed your PR. :-)
vitalif commented 2022-06-06 15:47:06 +03:00
Owner

Ok, I merged only the first change with volume from snapshot creation fix.

Ok, I merged only the first change with volume from snapshot creation fix.
vitalif closed this pull request 2022-06-06 15:47:06 +03:00
vitalif commented 2022-06-06 16:35:04 +03:00
Owner

The case with missing /index/maxid was already handled in vitastor-cli create, but it had a bug which I just fixed in master

The case with missing /index/maxid was already handled in `vitastor-cli create`, but it had a bug which I just fixed in master

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
vitalif
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vitalif/vitastor#8
There is no content yet.
Delete Branch "huynp911/vitastor:master"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?