Change in vdsm[master]: Define some constants in API.py

Adam Litke has uploaded a new change for review.

Change subject: Define some constants in API.py
......................................................................

Define some constants in API.py

Signed-off-by: Adam Litke 
Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
---
M vdsm/API.py
1 file changed, 16 insertions(+), 0 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke

Adam Litke has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 1:

Hi, this patch is a first step at exposing some constants at the API bridge level.  Do you agree with the way I have implemented it?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 1:

I must say that I do not understand the implementation. How is a prospective user of API.Volume.copy() is going to use these definition? Could you suggest a use case in the commit message?
Do we really need the 'STRINGS' at the API level? (I hope that) currently they are limited to the on-disk representation of metadata.

Note that I know too well how the current usage is confusing and needs a change...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

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

-1 for visibility

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

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

(1 inline comment)

....................................................
File vdsm/API.py
Line 607:         SHARED = 6
I understand why the Binding should not import vdsm internals. But I do not see how API.py can get away without having these imports.

Here, you assume that API.Volume.Roles.SHARED == storage.volume.SHARED_VOL but this is enforced only by hard-coding "6" for both enumerations, twice.

I do not fancy that. You can set

API.Volume.Roles.SHARED = storage.volume.SHARED_VOL

or have a completely different value there - but have an explicit conversion to the internal value when you pass it to the internal function.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Mark Wu has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File vdsm/API.py
Line 610:         INTERNAL = storage.volume.INTERNAL_VOL
Since currently the role "INTERNAL" is only used by the internal snapshot management, I think we needn't export it in API.  Does it make sense?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

(2 inline comments)

approach looks good to me, but 2 minor questions within.

....................................................
File vdsm/API.py
Line 760:     DEFAULT_VERSION = storage.sd.DOM_SAFELEASE_VERS[0]
why would we want DEFAULT_VERSION exposed in the API?

Line 945:     MAX_HOST_ID = storage.safelease.MAX_HOST_ID
pardon, but why this has to be exposed to Vdsm API users?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Adam Litke has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 4: (3 inline comments)

....................................................
File vdsm/API.py
Line 610:         INTERNAL = storage.volume.INTERNAL_VOL
Fair enough.  I will remove it.  We can always introduce it again if needed.

Line 760:     DEFAULT_VERSION = storage.sd.DOM_SAFELEASE_VERS[0]
It's exposed so that the xmlrpc bindings can set it as a default parameter in domainCreate.  API.py already handles version==None in this way so I guess we can remove it.

Line 945:     MAX_HOST_ID = storage.safelease.MAX_HOST_ID
It's a default parameter for poolSpmStart in the xmlrpc bindings.  Also handled in API.py so removable.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 5: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Mark Wu has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 5: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Saggi Mizrahi

Peter V. Saveliev has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 5: Verified; Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Define some constants in API.py
......................................................................

Patch Set 5: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Define some constants in API.py
......................................................................

Define some constants in API.py

The API bindings should be implemented in terms of the API bridge (API.py) and
not rely on vdsm internals.  Currently the xmlrpc binding imports some internal
modules in order to provide some default values (ie. BLANK_UUID and volume
constants).  Constants that are needed at the API level need to appear in
API.py.

This patch converts the obvious offenses.  If this approach is acceptible I will
follow up with additional patches.  In the future, APIs that take 'magic'
parameters must formally define the valid values in API.py.

Signed-off-by: Adam Litke 
Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
2 files changed, 43 insertions(+), 15 deletions(-)

Approvals:
  Mark Wu: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Peter V. Saveliev: Verified; Looks good to me, but someone else must approve

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iaef19f956ec1d6f83fd48e71a2f0a018505266be
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi