Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-20 Thread fsimonce 
Federico Simoncelli has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1:

(2 comments)


File vdsm/API.py
Line 962: def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963: domainsMap):
Line 964: return self._irs.connectStoragePool(
Line 965: self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966: domainsMap)
This was indeed a list in my first POC but then I realized that we can't change 
all at once. As we want incremental changes I wanted connectStoragePool to work 
as it is now without having to deal with missing attached (only) domains. I 
am sure there is some code on the engine side expecting the domain to be 
reported as attached. So for now it's a map relying on the reconstructMaster 
code (both on vdsm and engine side) for creating and parsing the domains map 
(really simple and a noop for me).
Line 967: 
Line 968: def connectStorageServer(self, domainType, connectionParams):
Line 969: return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:   connectionParams)



File vdsm/storage/sp.py
Line 1980: 
Line 1981: ### Deprecated Methods Section End ###
Line 1982: 
Line 1983: 
Line 1984: class StoragePoolLegacy(StoragePoolBase):
You're right. I couldn't find a good name though. But as a general class design 
is this ok with you?
Line 1985: 
Line 1986: def __init__(self, spUUID, domainMonitor, taskManager):
Line 1987: super(StoragePool, self) \
Line 1988: .__init__(spUUID, domainMonitor, taskManager)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-20 Thread abaron 
Ayal Baron has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1:

(2 comments)


File vdsm/API.py
Line 962: def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963: domainsMap):
Line 964: return self._irs.connectStoragePool(
Line 965: self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966: domainsMap)
hmmm.
Engine updates status of domains in case of conflict by running 
getStoragePoolInfo.
This is mostly relevant in cases where engine calls activateStorageDomain or 
deactivateStorageDomain and for some reason misses the reply.
However, passing the list of domains makes that flow irrelevant since there are 
2 meanings for active domain:
1. engine determines health according to it
2. vdsm monitors it
Both of these only require passing a list.

Attached domain has only 1 meaning: it is marked as belonging to the pool (on 
the domain itself and in the pool md).

Since the pool MD is now actually determined by the engine, getting it back in 
getStoragePoolInfo and updating according to it is a bit silly.
This raises 2 questions:
1. what happens if engine calls detachStorageDomain and looses connection with 
vdsm - engine doesn't know whether the command succeeded or failed (today I 
believe it would determine that it failed and then update using 
getStoragePoolInfo.  Using a map this behaviour would remain the same, but if 
the reason of the disconnect is that vdsm restarted right before it notified 
engine of the result, either success or failure, then engine has no way of 
determining what current state is.
2. same thing with attachStorageDomain
Sounds like we'd need to use getStorageDomainInfo to see what is persisted on 
the domain to be able to determine that (check end state).

So unless I'm missing something, we need to update engine logic (simplify 
mostly but use getStorageDomainInfo to figure out what happened).

Also sounds like these should really be async operations in engine (so that we 
don't finish until we determine whether sd is attached or not)
Line 967: 
Line 968: def connectStorageServer(self, domainType, connectionParams):
Line 969: return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:   connectionParams)



File vdsm/storage/sp.py
Line 1980: 
Line 1981: ### Deprecated Methods Section End ###
Line 1982: 
Line 1983: 
Line 1984: class StoragePoolLegacy(StoragePoolBase):
well, what you actually separated out here is the pool md methods.
you could use composition instead of inheritance here.
Then you'd have a single pool class, and 2 md classes and when you come to make 
additional changes (since you want to change verbs as well) then the changes 
would be orthogonal
Line 1985: 
Line 1986: def __init__(self, spUUID, domainMonitor, taskManager):
Line 1987: super(StoragePool, self) \
Line 1988: .__init__(spUUID, domainMonitor, taskManager)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-20 Thread fsimonce 
Federico Simoncelli has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1:

(1 comment)


File vdsm/API.py
Line 962: def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963: domainsMap):
Line 964: return self._irs.connectStoragePool(
Line 965: self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966: domainsMap)
What I thought was that in the end each action attach/detach (if they'll 
survive) activate/deactivate they'll all result in the same call 
(connectStoragePool) on all the hosts.

Now some hosts might not receive this call but it's not a reason to make the 
action async (hard to deal with it). When the hosts are available and they 
report a vds domain status that is different from what is expected then we 
should trigger a proper connectStoragePool to fix that.

Is this answering all your questions or is there a scenario that I didn't cover?
Line 967: 
Line 968: def connectStorageServer(self, domainType, connectionParams):
Line 969: return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:   connectionParams)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-20 Thread abaron 
Ayal Baron has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1:

(1 comment)


File vdsm/API.py
Line 962: def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963: domainsMap):
Line 964: return self._irs.connectStoragePool(
Line 965: self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966: domainsMap)
Is this answering all your questions or is there a scenario that I didn't 
cover?

No it does not.
It is fine for activate/deactivate, but attach/detach do more than that (spm 
side).
Note that HSMs don't really care about pool, they can run VMs regardless of 
being connected to the pool and they don't require the domain lock even, just 
sanlock on images (once utilised), so the connectSP on the other hosts is not 
really interesting in this case (and engine should only pass domains that the 
host should access in order to run VMs.
Going forward, once we are able to run spm ops from any host on any domain 
then the host will acquire the domain lock so no issue there either.

Currently however, detach and attach are special.  Because we want to prevent a 
case where 2 different SPMs can make changes to a domain then as long as we 
don't lock the domain for the duration of the operation we need to 'own' it.  
This is done by actually writing something on it.

So, engine has to somehow guarantee whether this 'owning' process or 'deowning' 
process succeeded.

This is relevant on the spm only.

activateStorageDomain and deactivateStorageDomain can be decommissioned the 
moment you start passing the active domain list in connectSP.

attachStorageDomain and detachStorageDomain can remain the same on vdsm side, 
but have to undergo some changes in engine

Did this answer the question? am I missing something obvious which could 
simplify things?
Line 967: 
Line 968: def connectStorageServer(self, domainType, connectionParams):
Line 969: return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:   connectionParams)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-20 Thread fsimonce 
Federico Simoncelli has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1:

(1 comment)


File vdsm/API.py
Line 962: def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963: domainsMap):
Line 964: return self._irs.connectStoragePool(
Line 965: self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966: domainsMap)
 No it does not. It is fine for activate/deactivate, but attach/detach do more 
 than that (spm side). 

Ok this is an entirely different matter, it seems to me that it's not related 
to comment 1 and 2 where we were discussing the domain map (vs a list) and 
when/how to sync it (e.g. async vs on a mismatch detected in the result of 
getVdsStats).

If this is a new discussion, then:

 Note that HSMs don't really care about pool

And therefore also the SPM *host* (as it's an HSM to begin with). Now, about 
attach/detach we have two ways to go:

* force connectStoragePool to validate that all the domains passed in the 
list/map are marked with the proper pool uuid (more similar to what we do now)
* on connectStoragePool we allow any domain to be part of the list/map (which 
is what I thought, as it gives us more flexibility in terms of what domains we 
can see, monitor and eventually consume) and if the host is the SPM validate 
that the SPM actions will run only on the domains that are marked with the 
proper pool uuid.

 activateStorageDomain and deactivateStorageDomain can be decommissioned the 
 moment you start passing the active domain list in connectSP.

 attachStorageDomain and detachStorageDomain can remain the same on vdsm side, 
 but have to undergo some changes in engine

 Did this answer the question? am I missing something obvious which could 
 simplify things?

Looks fine.
Line 967: 
Line 968: def connectStorageServer(self, domainType, connectionParams):
Line 969: return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:   connectionParams)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-19 Thread fsimonce 
Federico Simoncelli has uploaded a new change for review.

Change subject: sp: [wip] receive domains map in connectStoragePool
..

sp: [wip] receive domains map in connectStoragePool

Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Signed-off-by: Federico Simoncelli fsimo...@redhat.com
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
4 files changed, 243 insertions(+), 135 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/27/21427/1

diff --git a/vdsm/API.py b/vdsm/API.py
index f91910c..90ba002 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -958,9 +958,12 @@
 APIBase.__init__(self)
 self._UUID = UUID
 
-def connect(self, hostID, scsiKey, masterSdUUID, masterVersion):
-return self._irs.connectStoragePool(self._UUID, hostID, scsiKey,
-masterSdUUID, masterVersion)
+# FIXME: WIP patch, requires backward compatibility considerations
+def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
+domainsMap):
+return self._irs.connectStoragePool(
+self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
+domainsMap)
 
 def connectStorageServer(self, domainType, connectionParams):
 return self._irs.connectStorageServer(domainType, self._UUID,
diff --git a/vdsm/BindingXMLRPC.py b/vdsm/BindingXMLRPC.py
index dd82b05..a1c8817 100644
--- a/vdsm/BindingXMLRPC.py
+++ b/vdsm/BindingXMLRPC.py
@@ -542,10 +542,12 @@
 image = API.Image(imgUUID, spUUID, sdUUID)
 return image.download(methodArgs, volUUID)
 
+# FIXME: WIP patch, requires backward compatibility considerations
 def poolConnect(self, spUUID, hostID, scsiKey, msdUUID, masterVersion,
-options=None):
+domainsMap=None, options=None):
 pool = API.StoragePool(spUUID)
-return pool.connect(hostID, scsiKey, msdUUID, masterVersion)
+return pool.connect(hostID, scsiKey, msdUUID, masterVersion,
+domainsMap)
 
 def poolConnectStorageServer(self, domType, spUUID, conList, options=None):
 pool = API.StoragePool(spUUID)
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 3707528..56b24b4 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -981,8 +981,8 @@
 poolName, masterDom, domList, masterVersion, leaseParams)
 
 @public
-def connectStoragePool(self, spUUID, hostID, scsiKey,
-   msdUUID, masterVersion, options=None):
+def connectStoragePool(self, spUUID, hostID, scsiKey, msdUUID,
+   masterVersion, domainsMap=None, options=None):
 
 Connect a Host to a specific storage pool.
 
@@ -1010,11 +1010,12 @@
 hostID, scsiKey)))
 with rmanager.acquireResource(STORAGE, HSM_DOM_MON_LOCK,
   rm.LockType.exclusive):
-return self._connectStoragePool(spUUID, hostID, scsiKey, msdUUID,
-masterVersion, options)
+return self._connectStoragePool(
+spUUID, hostID, scsiKey, msdUUID, masterVersion,
+domainsMap, options)
 
 def _connectStoragePool(self, spUUID, hostID, scsiKey, msdUUID,
-masterVersion, options=None):
+masterVersion, domainsMap=None, options=None):
 misc.validateUUID(spUUID, 'spUUID')
 
 # TBD: To support multiple pool connection on single host,
@@ -1055,7 +1056,13 @@
  masterVersion=masterVersion)
 return
 
-pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng)
+if domainsMap is not None:
+pool = sp.StoragePool(spUUID, self.domainMonitor,
+  self.taskMng, domainsMap, masterVersion)
+else:
+pool = sp.StoragePoolLegacy(
+spUUID, self.domainMonitor, self.taskMng)
+
 res = pool.connect(hostID, scsiKey, msdUUID, masterVersion)
 
 if res:
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 46e8224..638891f 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -38,6 +38,7 @@
 import fileSD
 import sd
 import misc
+from misc import deprecated
 import fileUtils
 from vdsm.config import config
 from sdc import sdCache
@@ -101,7 +102,7 @@
 MAX_DOMAINS /= 48
 
 
-class StoragePool(Securable):
+class StoragePoolBase(Securable):
 '''
 StoragePool object should be relatively cheap to construct. It should defer
 any heavy lifting activities until the time it is really needed.
@@ -113,7 +114,7 @@
 lvExtendPolicy = config.get('irs', 'vol_extend_policy')
 
 def __init__(self, spUUID, domainMonitor, taskManager):
-

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-19 Thread oVirt Jenkins CI Server 
oVirt Jenkins CI Server has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1: Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4723/ : FAILURE

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5603/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

2013-11-19 Thread abaron 
Ayal Baron has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
..


Patch Set 1:

(3 comments)


Commit Message
Line 3: AuthorDate: 2013-11-18 10:54:41 -0500
Line 4: Commit: Federico Simoncelli fsimo...@redhat.com
Line 5: CommitDate: 2013-11-19 10:40:26 -0500
Line 6: 
Line 7: sp: [wip] receive domains map in connectStoragePool
why?
Line 8: 
Line 9: Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d



File vdsm/API.py
Line 962: def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963: domainsMap):
Line 964: return self._irs.connectStoragePool(
Line 965: self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966: domainsMap)
why is this a map and not a list?
I can't think of any reason for engine to pass anything but active domains. In 
that case, passing 'domUUID:active' is redundant, just pass the list of domains.
Internally in the pool class, if for bc reasons and avoiding changing too much 
you want to have the map format then you can convert the list to map
Line 967: 
Line 968: def connectStorageServer(self, domainType, connectionParams):
Line 969: return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:   connectionParams)



File vdsm/storage/sp.py
Line 1980: 
Line 1981: ### Deprecated Methods Section End ###
Line 1982: 
Line 1983: 
Line 1984: class StoragePoolLegacy(StoragePoolBase):
We might end up having several of these since we're making these changes 
iteratively. So I'm not sure StoragePool and StoragePoolLegacy are the proper 
terms. what do you think?
Line 1985: 
Line 1986: def __init__(self, spUUID, domainMonitor, taskManager):
Line 1987: super(StoragePool, self) \
Line 1988: .__init__(spUUID, domainMonitor, taskManager)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches