Change in vdsm[master]: Stop spm if refresh fail on bad parameters.

Eduardo has uploaded a new change for review.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................

Stop spm if refresh fail on bad parameters.

Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Signed-off-by: Eduardo <ewarszaw(a)redhat.com&gt;
---
M vdsm/storage/hsm.py
1 file changed, 6 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/30/13930/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index fd5d7d6..ba59348 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -788,12 +788,16 @@
             se.StoragePoolActionError(
                 "spUUID=%s, msdUUID=%s, masterVersion=%s" %
                 (spUUID, msdUUID, masterVersion)))
-        vars.task.getSharedLock(STORAGE, spUUID)
+        self.getPool(spUUID)  # Validate that is the correct pool.
+        vars.task.getExclusiveLock(STORAGE, spUUID)
         pool = self.getPool(spUUID)
         try:
             self.validateSdUUID(msdUUID)
             pool.refresh(msdUUID, masterVersion)
-        except:
+        except (se.StorageDomainAccessError, se.StorageDomainDoesNotExist,
+                se.StoragePoolWrongMaster):
+            self.log.error("refreshStoragePool failed", exc_info=True)
+            pool.stopSpm()
             self._disconnectPool(pool, pool.id, pool.scsiKey, False)
             raise
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;

Itamar Heim has abandoned this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Abandoned

no reply - abandoning - please restore if still relevant

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)gmail.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)gmail.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Patch Set 1:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2028/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1971/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Ayal Baron has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


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

(2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 788:             se.StoragePoolActionError(
Line 789:                 "spUUID=%s, msdUUID=%s, masterVersion=%s" %
Line 790:                 (spUUID, msdUUID, masterVersion)))
Line 791:         self.getPool(spUUID)  # Validate that is the correct pool.
Line 792:         vars.task.getExclusiveLock(STORAGE, spUUID)
this change is very problematic as it will serialize this op with others and fail on
timeout.
What cannot run concurrently and why?
Line 793:         pool = self.getPool(spUUID)
Line 794:         try:
Line 795:             self.validateSdUUID(msdUUID)
Line 796:             pool.refresh(msdUUID, masterVersion)


Line 796:             pool.refresh(msdUUID, masterVersion)
Line 797:         except (se.StorageDomainAccessError, se.StorageDomainDoesNotExist,
Line 798:                 se.StoragePoolWrongMaster):
Line 799:             self.log.error("refreshStoragePool failed",
exc_info=True)
Line 800:             pool.stopSpm()
who said this host is spm?
Line 801:             self._disconnectPool(pool, pool.id, pool.scsiKey, False)
Line 802:             raise
Line 803: 
Line 804:         if pool.hsmMailer:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Ayal Baron has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 796:             pool.refresh(msdUUID, masterVersion)
Line 797:         except (se.StorageDomainAccessError, se.StorageDomainDoesNotExist,
Line 798:                 se.StoragePoolWrongMaster):
Line 799:             self.log.error("refreshStoragePool failed",
exc_info=True)
Line 800:             pool.stopSpm()
this will throw a  not spm exception
Line 801:             self._disconnectPool(pool, pool.id, pool.scsiKey, False)
Line 802:             raise
Line 803: 
Line 804:         if pool.hsmMailer:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Ayal Baron has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Patch Set 1: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Eduardo has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 788:             se.StoragePoolActionError(
Line 789:                 "spUUID=%s, msdUUID=%s, masterVersion=%s" %
Line 790:                 (spUUID, msdUUID, masterVersion)))
Line 791:         self.getPool(spUUID)  # Validate that is the correct pool.
Line 792:         vars.task.getExclusiveLock(STORAGE, spUUID)
This change certainly opens the debate.
Is needed because the host pool view is changed during it.
Which pool ops can be started after this call and should operate concurrently in an
uncertain state of the pool?
If this is the spm should be no difference because the state should be already known but
in error that be lead to the pool disconnection.
I addition all the spm ops take the exclusive lock.

This verb can be starved sending concurrent pool shared lock operations:
updateVM, removeVM, uploadVM, getVmsList, getImageDomainsList. Should be considered if
these operations needs pool lock or lock in the MSD or lock-free.
In addition the shared lock is taken by
getStoragePoolInfo : No sense to run concurrently with refresh.
Line 793:         pool = self.getPool(spUUID)
Line 794:         try:
Line 795:             self.validateSdUUID(msdUUID)
Line 796:             pool.refresh(msdUUID, masterVersion)


Line 796:             pool.refresh(msdUUID, masterVersion)
Line 797:         except (se.StorageDomainAccessError, se.StorageDomainDoesNotExist,
Line 798:                 se.StoragePoolWrongMaster):
Line 799:             self.log.error("refreshStoragePool failed",
exc_info=True)
Line 800:             pool.stopSpm()
If not, calling stopSpm is innocuous.
Line 801:             self._disconnectPool(pool, pool.id, pool.scsiKey, False)
Line 802:             raise
Line 803: 
Line 804:         if pool.hsmMailer:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2028/ (2/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1971/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: Stop spm if refresh fail on bad parameters.
......................................................................


Patch Set 1:

ping - still relevant?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f85c3e731551b6974483cffe38c7ac37281370b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Paikov <paikov(a)gmail.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No