Change in vdsm[master]: [WIP] Add new repository management code

Saggi Mizrahi has uploaded a new change for review.

Change subject: [WIP] Add new repository management code
......................................................................

[WIP] Add new repository management code

Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Signed-off-by: Saggi Mizrahi 
---
M tests/Makefile.am
A tests/imageMainuplatorTests.py
A tests/repositoryEngineTests.py
A vdsm/storage/imageRepository/engines/__init__.py
A vdsm/storage/imageRepository/engines/localfs/__init__.py
A vdsm/storage/imageRepository/imageManipulator.py
6 files changed, 1,339 insertions(+), 5 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 16:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/418/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Deepak C Shetty has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 16: (4 inline comments)

few Qs.

....................................................
File vdsm/storage/imageRepository/imageManipulator.py
Line 132:     code = 400
Line 133:     message = "Base has wrong mutablility state for requested operation"
Line 134: 
Line 135: 
Line 136: class WrokerLostError(ImageManipulatorError):
s/WrokerLostError/WorkerLostError
Line 137:     """Means we lost communication with the worker. The end state of the
Line 138:     operation is unknown. The only way to know is to inspect the repo and see
Line 139:     what is there and what isn't"""
Line 140:     code = 410

Line 150:     code = 400
Line 151:     message = "Internal communication with the worker failed"
Line 152: 
Line 153: 
Line 154: class UnhandeledExceptionError(ImageManipulatorError):
s/UnhandeledExceptionError/UnhandledExceptionError
Line 155:     code = 500
Line 156: 
Line 157:     def __init__(self, e):
Line 158:         self.message = str(e)

....................................................
File vdsm/storage/storageRepositoryManager.py
Line 36:     def createStorageRepository(self, repoFormat, creationParameters={}):
Line 37:         engine = self._getEngine(repoFormat)
Line 38: 
Line 39:         try:
Line 40:             engine.create(**creationParameters)
Why use engine.create here ? IIUC ImageRepository is supposed to encapsulate the engine and its low level details for create\delete StorageRepository, we should go via ImageRepository itself.

In other words, why expose the engine to SRM ? Curently engine is used by ImageRepo as well SRM.. 2 places!
Line 41:         except Exception as e:
Line 42:             self.log.error("Storage repository creation failed", exc_info=True)
Line 43:             raise se.StorageDomainCreationError(str(e))
Line 44:

Line 45:     def deleteStorageRepository(self, repoFormat, deletionParameters={}):
Line 46:         engine = self._getEngine(repoFormat)
Line 47: 
Line 48:         try:
Line 49:             engine.delete(**deletionParameters)
The same comment as applicable to create above, applies here.
Line 50:         except Exception as e:
Line 51:             self.log.error("Storage repository deletion failed", exc_info=True)
Line 52:             raise se.StorageDomainFormatError(str(e))
Line 53:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 17:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/215/ (1/3)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 17:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 17:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1104/ (3/3)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 17: Fails

Build Failed

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Deepak C Shetty has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 16: (1 inline comment)

1 more Q, as i understand more of this :)

....................................................
File vdsm/storage/imageRepository/imageManipulator.py
Line 475:             orphanVolumes.discard(tag.volumeId)
Line 476: 
Line 477:             if tag.isWeak():
Line 478:                 # Only weak tags have orphan potential
Line 479:                 orphanTags.add(tag.id)
Isn't there a possibility of this clashing with createVirtualDisk, where too  initially the tag is weak and then made strong.. if this clashed with that.. a potential new virtual disk can be tagged as orphan here and subsequently deleted/removed as part of fix op.. won't that be a issue ?
Line 480:                 continue
Line 481: 
Line 482:             safe.append(tag.id)
Line 483:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Deepak C Shetty has posted comments on this change.

Change subject: [WIP] Add new repository management code
......................................................................

Patch Set 17: (3 inline comments)

Few Qs and suggestions.

....................................................
File vdsm/storage/imageRepository/engines/engineUtils.py
Line 76:     _cls_lock = threading.Lock()
Line 77: 
Line 78:     @property
Line 79:     def sanlockFD(self):
Line 80:         if SanlockLockingBackend._sanlock_fd is not None:
s/_sanlock_fd/_sanlockFD ?
Line 81:             return SanlockLockingBackend._sanlock_fd
Line 82: 
Line 83:         with self._cls_lock:
Line 84:             if SanlockLockingBackend._sanlock_fd is not None:

Line 159:     VOL_MD_SUFFIX = ".md.json"
Line 160:     TMP_SUFFIX = ".tmp"
Line 161:     LOCK_SUFFIX = ".lock"
Line 162: 
Line 163:     def getCapabilites(self):
Having getCapabilities @classmethod will be useful, so that clients don't necessarily have to instantiate to know the capabilities.
Line 164:         caps = [CAP_SPARSE_FILES,
Line 165:                 CAP_NATIVE_THIN_PROVISIONING,
Line 166:                 CAP_VOLUME_PREPARE_REQUIRED]
Line 167:

Line 169:             caps.append(CAP_MULTI_HOST)
Line 170: 
Line 171:         return caps
Line 172: 
Line 173:     def __init__(self, path, formatStr, multiHost=False):
IIUC, The signature doesn't match with localfs/__init__.py where u have
return FsEngine(path, FORMAT, LocalFsLockBackend())

Do you intend to take the lockImpl as part of init or is it ok to set the lockImpl via a classmethod function ? Doing the latter will allow getCapabilities to be a classmethod as well
Line 174:         self._formatStr = formatStr
Line 175:         self._root = os.path.abspath(path)
Line 176:         self.sanityCheck()
Line 177:         self._multiHost = multiHost

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib09c89cf982b475f45d26b2428fe05e2f4565dab
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Saggi Mizrahi has abandoned this change.

Change subject: [WIP] Add new repository management code
......................................................................

Abandoned