Change in vdsm[master]: BZ#773210 - Avoid infinity loop when delete volume failed du...

[Igor Lvovsky]

Igor Lvovsky has uploaded a new change for review.

Change subject: BZ#773210 - Avoid infinity loop when delete volume failed during the merge
......................................................................

BZ#773210 - Avoid infinity loop when delete volume failed during the merge

Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
---
M vdsm/storage/image.py
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/87/987/1
--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#773210 - Avoid infinity loop when delete volume failed during the merge
......................................................................

Patch Set 1: Verified

verified by knesenko

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#773210 - Avoid infinity loop when delete volume failed during the merge
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

....................................................
Commit Message
Line 7: BZ#773210 - Avoid infinity loop when delete volume failed during the merge
an infinite loop is still possible (though in different occasions), isn't it?

In any case, a little sleep in the except: clause will protect us from tight-looping.

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Ayal Baron has posted comments on this change.

Change subject: BZ#773210 - Avoid infinity loop when delete volume failed during the merge
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 925:             try:
you should move this entire try..except clause to a separate (internal) function and run it with retry.  If it fails you should call:
chain.remove(srcVol.volUUID)

This would prevent endlessly looping in case of error and would take care of the sleep Dan requested.

Alternatively, just move the 'srcVol = vol' line into finally

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#773210 - Avoid infinity loop when delete volume failed during the merge
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 925:             try:
Yes I know, I already did it (moved 'srcVol = vol' line into finally).
I just didn't send it yet, because I am not sure about leave such ILLEGAL volumes
in the chain, wheter it will break some chain validation case

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

Patch Set 2: Verified; Looks good to me, approved

Boy, this is so ugly that I could cry. We must remember to add a timeout there.

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

BZ#773210 - Avoid infinite loop when delete volume failed during the merge

Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
---
M vdsm/storage/image.py
1 file changed, 5 insertions(+), 1 deletion(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

Eduardo has posted comments on this change.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

Patch Set 2: (2 inline comments)

....................................................
File vdsm/storage/image.py
Line 964:             except Exception:
Please catch the proper Exception, StorageException at least.
If catched, mark the volume as failed and FF to the next volume
Later retry a _finite_ number of times the failed volumes.

Line 969:                 time.sleep(10)
Nu, VeEmet!

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

Eduardo has posted comments on this change.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

Ayal Baron has posted comments on this change.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 969:                 time.sleep(10)
Sleep? really?
Why didn't you use retry as I asked on the previous review?
An infinite loop here is wrong, if we failed 3 times in a row there is no reason to believe number 4 would succeed...

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 969:                 time.sleep(10)
The infinite loop will occur if we have problem with access storage (not like in original code).
We spoke with Dan about it and we decided that preferable way is  keep the task running (but not in tight loop). We can't raise exception here because it will fail the whole merge task that this function only  last cleanup part of it

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

Change in vdsm[master]: BZ#773210 - Avoid infinite loop when delete volume failed du...

Ayal Baron has posted comments on this change.

Change subject: BZ#773210 - Avoid infinite loop when delete volume failed during the merge
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/image.py
Line 969:                 time.sleep(10)
So instead of failing the merge you'd keep the VM locked forever because the image is locked ?!?
Instead of raising you could log the problem and return without throwing an exception.
If you want to try many times then use retry and put 60 as the number of retries and 10s between each try:
try:
    misc.retry(func, expectedException=Exception, tries=60,
            timeout=None, sleep=10)
except Exception, e:
    self.log.warning(...)

--
To view, visit http://gerrit.ovirt.org/987
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1368141fb240c9ca4e3fb3cfad04f88312cc46b5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky