Change in vdsm[master]: sdcache: add refresh to connectStoragePool

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

sdcache: add refresh to connectStoragePool

Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Signed-off-by: Federico Simoncelli 
---
M vdsm/storage/sp.py
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/22/9422/1

diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index ac5e1db..2577db7 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -640,6 +640,7 @@
         self.scsiKey = scsiKey
         # Make sure SDCache doesn't have stale data (it can be in case of FC)
         sdCache.invalidateStorage()
+        sdCache.refresh()
         # Rebuild whole Pool
         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
         self.__createMailboxMonitor()

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/146/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 1:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/146/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

Ayal Baron has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/sp.py
Line 638:         self._saveReconnectInformation(hostID, scsiKey, msdUUID, masterVersion)
Line 639:         self.id = hostID
Line 640:         self.scsiKey = scsiKey
Line 641:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 642:         sdCache.invalidateStorage()
why is this not needed in refresh storage pool? (i.e. sp.py - refresh method) which is the only other method which calls sdCache.refresh(0
Line 643:         sdCache.refresh()
Line 644:         # Rebuild whole Pool
Line 645:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 646:         self.__createMailboxMonitor()

Line 639:         self.id = hostID
Line 640:         self.scsiKey = scsiKey
Line 641:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 642:         sdCache.invalidateStorage()
Line 643:         sdCache.refresh()
why isn't this needed in connectStorageServer? (the only other place which calls invalidateStorage)?
Line 644:         # Rebuild whole Pool
Line 645:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 646:         self.__createMailboxMonitor()
Line 647:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/sp.py
Line 638:         self._saveReconnectInformation(hostID, scsiKey, msdUUID, masterVersion)
Line 639:         self.id = hostID
Line 640:         self.scsiKey = scsiKey
Line 641:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 642:         sdCache.invalidateStorage()
Yes it might be useful to have it there too to trigger a rescan.
Line 643:         sdCache.refresh()
Line 644:         # Rebuild whole Pool
Line 645:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 646:         self.__createMailboxMonitor()

Line 639:         self.id = hostID
Line 640:         self.scsiKey = scsiKey
Line 641:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 642:         sdCache.invalidateStorage()
Line 643:         sdCache.refresh()
This is not needed in connectStorageServer because you would delete the entire domain cache when you connect to a new storage server. E.g.: you want to add a new storage => you would clean the cache of all the know domains.

I think you're looking forward to unify the methods, and I actually agree, I thought about it a couple of times. We might try to do so and add a parameter to skip the domain cache clean up in the few cases we don't want it.
Line 644:         # Rebuild whole Pool
Line 645:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 646:         self.__createMailboxMonitor()
Line 647:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/208/ (1/2)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/208/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 1239:         """
Line 1240:         Refresh storage pool.
Line 1241:          'msdUUID' - master storage domain UUID
Line 1242:         """
Line 1243:         sdCache.invalidateStorage()
Now I'm actually worried about this change. We're injecting a "random" delay (iscsi-rescan time) somewhere in any following domain call/property-evaluation. Having it in connectStoragePool is safe because we're not connected to the pool yet but here we might have tasks going on (and the domain monitors).
Line 1244:         sdCache.refresh()
Line 1245:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 1246: 
Line 1247:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

Ayal Baron has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2: Looks good to me, approved

It looks good to me, but it needs thorough testing with many storage domains to make sure it's behaving properly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

Daniel Paikov has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Daniel Paikov 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 639:         self.id = hostID
Line 640:         self.scsiKey = scsiKey
Line 641:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 642:         sdCache.invalidateStorage()
Line 643:         sdCache.refresh()
Sorry, I find it very hard to follow. was http://gerrit.ovirt.org/9275 a mistake? is bug https://bugzilla.redhat.com/show_bug.cgi?id=870768 still solved?

Besides that, the code here looks very similar to self.refresh().

I know this patch has all the acks, but it seems that we're running in circles.
Line 644:         # Rebuild whole Pool
Line 645:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 646:         self.__createMailboxMonitor()
Line 647:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Daniel Paikov 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 639:         self.id = hostID
Line 640:         self.scsiKey = scsiKey
Line 641:         # Make sure SDCache doesn't have stale data (it can be in case of FC)
Line 642:         sdCache.invalidateStorage()
Line 643:         sdCache.refresh()
At least for me this doesn't look like running in circles. You probably didn't notice that in 9275 refresh was substituted by invalidateStorage (rescan multipath). That would have been correct if it wasn't for a special case (when you start the spm on a different node where you created the pool). In such case you also need to clean up (refresh) the domain cache. That is why we now need both. I agree that this looks very similar to self.refresh(). If you want I can resend the patch calling self.refresh() here, I think it's cleaner.
Line 644:         # Rebuild whole Pool
Line 645:         self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
Line 646:         self.__createMailboxMonitor()
Line 647:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Daniel Paikov 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

Patch Set 2:

I cannot say that I understand this, but at least I'm convinced that Federico does.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Daniel Paikov 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: sdcache: add refresh to connectStoragePool
......................................................................

sdcache: add refresh to connectStoragePool

When connecting to a storage pool we should clear the domain cache to
pick up all the changes that might have been introduced by other hosts.
Additionally since StoragePool.refresh has a similar behavior to
connectStoragePool it should also contain the invalidateStorage call to
force an iscsi rescan.

In this patch:
* reinstate the sdCache.refresh call in connectStoragePool
* add invalidateStorage to StoragePool.refresh

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=879253
Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Signed-off-by: Federico Simoncelli 
---
M vdsm/storage/sp.py
1 file changed, 2 insertions(+), 0 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Daniel Paikov: Verified
  Dan Kenigsberg:

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2d3adcff7bb0e97be5c797cd720c6353920d9db0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Daniel Paikov 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server