Change in vdsm[master]: getStorageDomainInfo: SP keys when disconnected.

Eduardo has uploaded a new change for review.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

getStorageDomainInfo: SP keys when disconnected.

Before this patch, getStorageDomainInfo() returns the real pool
info part (master_ver, spm_id, lver) only if the host is connected
to the same pool which the domain is attached. This patch reads
directly the pool MD for any MSD.
This changes the previous behaviour.

TODO: pool related information should not be returned as part of
this API response.

Change-Id: I8b7323d4ccaaaec0f39d5590a06879faa7fc999e
Signed-off-by: Eduardo 
---
M vdsm/storage/hsm.py
1 file changed, 10 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/19555/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 2b91640..6262822 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -2736,8 +2736,11 @@
         # These keys are not likely to change (also because of
         # BC) so it's not that horrible. In any case please
         # remove this when we can stop supporting this API.
-        info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
         if info['role'] == sd.MASTER_DOMAIN:
+            pmd = sp.StoragePool._getPoolMD(dom)
+            info['lver'] = pmd[sp.PMDK_LVER]
+            info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
+            info['spm_id'] = -1
             try:
                 # Verify that the host is connected to the same pool which
                 # the SD is attached to.
@@ -2757,9 +2760,12 @@
                                   "master is %s",
                                   sdUUID, pool.masterDomain.sdUUID)
                 else:
-                    poolInfo = pool.getInfo()
-                    for key in ('lver', 'spm_id', 'master_ver'):
-                        info[key] = poolInfo[key]
+                    # spm_id is relevant only when the host is connected
+                    # to the pool and this SD is the actual master
+                    info['spm_id'] = pmd[sp.PMDK_SPM_ID]
+
+        else:  # sd.REGULAR_DOMAIN
+            info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})

return dict(info=info)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 1:

Build Successful

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

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3750/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2:

Build Successful

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

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3751/ : SUCCESS

Saggi Mizrahi has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

....................................................
File vdsm/storage/hsm.py
Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Just add a comment since you are accessing a private method
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1
Line 2746:             try:

Eduardo has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2: Verified+1

[root@camel-vdsb ~]# vdsClient -s 0 getStorageDomainInfo 0c3606f8-e509-4007-ae1e-aa023d7a0a94
        uuid = 0c3606f8-e509-4007-ae1e-aa023d7a0a94
        vguuid = lge2ds-GmcH-g3c6-sMP9-3c9q-7rf9-IIvd06
        lver = 4
        state = OK
        version = 3
        role = Master
        pool = ['6e6a4fb2-3ec2-44f1-b07c-2d4f5acaee66']
        spm_id = -1
        type = ISCSI
        class = Data
        master_ver = 2
        name = ddd

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2:

(1 comment)

....................................................
Commit Message
Line 12: directly the pool MD for any MSD.
Line 13: This changes the previous behaviour.
Line 14: 
Line 15: TODO: pool related information should not be returned as part of
Line 16: this API response.
so why are you returning pool-related information in more cases? The motivation for this patch eludes me.
Line 17: 
Line 18: Change-Id: I8b7323d4ccaaaec0f39d5590a06879faa7fc999e

Ayal Baron has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2: Code-Review-1

(6 comments)

1. No clear motivation for this patch.
2. It is breaking encapsulation

....................................................
Commit Message
Line 12: directly the pool MD for any MSD.
Line 13: This changes the previous behaviour.
Line 14: 
Line 15: TODO: pool related information should not be returned as part of
Line 16: this API response.
+1
Line 17: 
Line 18: Change-Id: I8b7323d4ccaaaec0f39d5590a06879faa7fc999e

....................................................
File vdsm/storage/hsm.py
Line 2735:         # This, of-course breaks backward compatibility.
Line 2736:         # These keys are not likely to change (also because of
Line 2737:         # BC) so it's not that horrible. In any case please
Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
remove this if
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]

Line 2737:         # BC) so it's not that horrible. In any case please
Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
change else to: if info['role'] == sd.MASTER_DOMAIN:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1

Line 2738:         # remove this when we can stop supporting this API.
Line 2739:         if info['role'] != sd.MASTER_DOMAIN:
Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
instead of accessing internals, you should be adding a method that returns the info you need or something
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1
Line 2746:             try:

Line 2740:             info.update({'lver': -1, 'spm_id': -1, 'master_ver': 0})
Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
I see no reason to return this info if getPool fails?
i.e. move both lines down to join the spm_id line
Line 2745:             info['spm_id'] = -1
Line 2746:             try:
Line 2747:                 # Verify that the host is connected to the same pool which
Line 2748:                 # the SD is attached to.

Line 2741:         else:
Line 2742:             pmd = sp.StoragePool._getPoolMD(dom)
Line 2743:             info['lver'] = pmd[sp.PMDK_LVER]
Line 2744:             info['master_ver'] = pmd[sp.PMDK_MASTER_VER]
Line 2745:             info['spm_id'] = -1
remove this line (spm_id = -1) as with above changes it is redundant
Line 2746:             try:
Line 2747:                 # Verify that the host is connected to the same pool which
Line 2748:                 # the SD is attached to.
Line 2749:                 pool = self.getPool(info['pool'][0])

Ayal Baron has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/storage/hsm.py
Line 2726:         # getSharedLock(connectionsResource...)
Line 2727: 
Line 2728:         vars.task.getSharedLock(STORAGE, sdUUID)
Line 2729:         info = dom.getInfo()
Line 2730:         # This only occurred because someone
As far as I can see engine never read this part of the info.
As you wrote in the commit message, getStorageDomainInfo should not return pool info. I could not agree more.
Pending a bit testing, I see no reason why not to just delete lines 2730 - 2767 here (everything regarding the pool).
Line 2731:         # thought it would be clever to return pool
Line 2732:         # information in the domain.getInfo() method
Line 2733:         # In a perfect world I would have just stopped
Line 2734:         # giving this information in the response.

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

....................................................
Commit Message
Line 12: directly the pool MD for any MSD.
Line 13: This changes the previous behaviour.
Line 14: 
Line 15: TODO: pool related information should not be returned as part of
Line 16: this API response.
I also prefer to return the pool-related info only on the connected pool master. It seems easier, it would respect the current api and it would prevent the use of StoragePool internals from hsm.py.
Line 17: 
Line 18: Change-Id: I8b7323d4ccaaaec0f39d5590a06879faa7fc999e

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/storage/hsm.py
Line 2726:         # getSharedLock(connectionsResource...)
Line 2727: 
Line 2728:         vars.task.getSharedLock(STORAGE, sdUUID)
Line 2729:         info = dom.getInfo()
Line 2730:         # This only occurred because someone
+1 to remove this all.
Line 2731:         # thought it would be clever to return pool
Line 2732:         # information in the domain.getInfo() method
Line 2733:         # In a perfect world I would have just stopped
Line 2734:         # giving this information in the response.

Itamar Heim has posted comments on this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Patch Set 2:

ping?

Itamar Heim has abandoned this change.

Change subject: getStorageDomainInfo: SP keys when disconnected.
......................................................................

Abandoned

no reply - abandoning - please restore if still relevant