Change in vdsm[master]: vdsm-tool: vdsm-id: add force option to force generate id

[Alon Bar-Lev]

Alon Bar-Lev has uploaded a new change for review.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

vdsm-tool: vdsm-id: add force option to force generate id

this is handy to hide the existence and usage of /etc/vdsm/vdsm.id from
users (other components).

Change-Id: I89f1e29c9cdad0cadb32545fa27c1702ad2e116a
Signed-off-by: Alon Bar-Lev 
---
M lib/vdsm/tool/vdsm-id.py
M lib/vdsm/utils.py
2 files changed, 35 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/08/20808/1

diff --git a/lib/vdsm/tool/vdsm-id.py b/lib/vdsm/tool/vdsm-id.py
index 109c382..01f94a9 100644
--- a/lib/vdsm/tool/vdsm-id.py
+++ b/lib/vdsm/tool/vdsm-id.py
@@ -17,9 +17,13 @@
 # Refer to the README and COPYING files for full details of the license
 #

+
+import sys
+import argparse
+
+
 from vdsm.utils import getHostUUID
 from vdsm.tool import expose
-import sys

@expose("vdsm-id")
@@ -27,7 +31,19 @@
     """
     Printing host uuid
     """
-    hostUUID = getHostUUID(False)
+    parser = argparse.ArgumentParser('vdsm-tool configure')
+    parser.add_argument(
+        '--force',
+        dest='force',
+        default=False,
+        action='store_true',
+        help='Generate vdsmid if not available',
+    )
+    args = parser.parse_args(sys.argv[2:])
+    hostUUID = getHostUUID(
+        legacy=False,
+        force=args.force,
+    )
     if hostUUID is None:
         raise RuntimeError('Cannot retrieve host UUID')
     sys.stdout.write(hostUUID)
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index 78d055e..d38a273 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -48,6 +48,7 @@
 import subprocess
 import threading
 import time
+import uuid

from cpopen import CPopen as BetterPopen
 import constants
@@ -599,7 +600,7 @@
 __hostUUID = None

-def getHostUUID(legacy=True):
+def getHostUUID(legacy=True, force=False):
     global __hostUUID
     if __hostUUID:
         return __hostUUID
@@ -624,6 +625,14 @@
             if p.returncode == 0 and 'Not' not in out:
                 #Avoid error string - 'Not Settable' or 'Not Present'
                 __hostUUID = out.strip()
+            elif force:
+                hostid = str(uuid.uuid4())
+                with open(constants.P_VDSM_NODE_ID, 'w') as f:
+                    f.write("%s\n", hostid)
+                if isOvirtNode():
+                    from ovirtnode import ovirtfunctions
+                    ovirtfunctions.ovirt_store_config(constants.P_VDSM_NODE_ID)
+                __hostUUID = hostid
             else:
                 logging.warning('Could not find host UUID.')

@@ -877,3 +886,10 @@
     logging.error("Panic: %s", msg, exc_info=True)
     os.killpg(0, 9)
     sys.exit(-3)
+
+
+def isOvirtNode():
+    return (
+        os.path.exists('/etc/rhev-hypervisor-release') or
+        glob.glob('/etc/ovirt-node-*-release')
+    )

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 1:

Build Successful

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

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

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 1:

(2 comments)

....................................................
File lib/vdsm/tool/vdsm-id.py
Line 18: #
Line 19: 
Line 20: 
Line 21: import sys
Line 22: import argparse
if you're into reordering, alphabetizing would be nice.
Line 23: 
Line 24: 
Line 25: from vdsm.utils import getHostUUID
Line 26: from vdsm.tool import expose

....................................................
File lib/vdsm/utils.py
Line 631:                     f.write("%s\n", hostid)
Line 632:                 if isOvirtNode():
Line 633:                     from ovirtnode import ovirtfunctions
Line 634:                     ovirtfunctions.ovirt_store_config(constants.P_VDSM_NODE_ID)
Line 635:                 __hostUUID = hostid
why not use __hostUUID to begin with?
Line 636:             else:
Line 637:                 logging.warning('Could not find host UUID.')
Line 638: 
Line 639:             if legacy:

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 1:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 886:     logging.error("Panic: %s", msg, exc_info=True)
Line 887:     os.killpg(0, 9)
Line 888:     sys.exit(-3)
Line 889: 
Line 890: 
this function is not expensive, but still, please use @memoize.
Line 891: def isOvirtNode():
Line 892:     return (
Line 893:         os.path.exists('/etc/rhev-hypervisor-release') or
Line 894:         glob.glob('/etc/ovirt-node-*-release')

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 1:

(3 comments)

....................................................
File lib/vdsm/tool/vdsm-id.py
Line 18: #
Line 19: 
Line 20: 
Line 21: import sys
Line 22: import argparse
Done
Line 23: 
Line 24: 
Line 25: from vdsm.utils import getHostUUID
Line 26: from vdsm.tool import expose

....................................................
File lib/vdsm/utils.py
Line 631:                     f.write("%s\n", hostid)
Line 632:                 if isOvirtNode():
Line 633:                     from ovirtnode import ovirtfunctions
Line 634:                     ovirtfunctions.ovirt_store_config(constants.P_VDSM_NODE_ID)
Line 635:                 __hostUUID = hostid
because this is singleton variable... I want to avoid set it if not all success.
Line 636:             else:
Line 637:                 logging.warning('Could not find host UUID.')
Line 638: 
Line 639:             if legacy:

Line 886:     logging.error("Panic: %s", msg, exc_info=True)
Line 887:     os.killpg(0, 9)
Line 888:     sys.exit(-3)
Line 889: 
Line 890: 
Done
Line 891: def isOvirtNode():
Line 892:     return (
Line 893:         os.path.exists('/etc/rhev-hypervisor-release') or
Line 894:         glob.glob('/etc/ovirt-node-*-release')

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 2: Verified-1

Build Failed

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

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

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 3:

Build Successful

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

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

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 4:

Build Successful

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

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

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

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 4:

(2 comments)

....................................................
File lib/vdsm/tool/vdsm-id.py
Line 30: def getUUID():
Line 31:     """
Line 32:     Printing host uuid
Line 33:     """
Line 34:     parser = argparse.ArgumentParser('vdsm-tool configure')
vdsm-tool vdsm-id
Line 35:     parser.add_argument(
Line 36:         '--force',
Line 37:         dest='force',
Line 38:         default=False,

....................................................
File lib/vdsm/utils.py
Line 612:             with open(constants.P_VDSM_NODE_ID) as f:
Line 613:                 __hostUUID = f.readline().replace("\n", "")
Line 614:         else:
Line 615:             p = subprocess.Popen([constants.EXT_SUDO,
Line 616:                                   constants.EXT_DMIDECODE, "-s",
guess the reason for it raised for ppc usage that doesn't contain dmidecode outputs, so why not providing proper specific solution for each arch instead of adding such hack? otherwise why wouldn't it fail?
Line 617:                                   "system-uuid"],
Line 618:                                  close_fds=True, stdin=subprocess.PIPE,
Line 619:                                  stdout=subprocess.PIPE,
Line 620:                                  stderr=subprocess.PIPE)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 5:

Build Successful

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

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

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
it might be nice to document that unlike the rest of the function, this branch requires root permission.
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 5:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
Done
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

Build Successful

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

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

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

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

can't find your response to my question "why not having appropriate solution for cases where dmidecode is not the way to get unique uuid, instead of making such hack" ..? isn't that relevant only for new archs? if yes, lets have specific solution for each as we do for gathering hardware information

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

what hack? maybe I do not follow.

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
why do you need the force option unless dmidecode doesn't work properly ?
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
this is a question for the entire patch...

1. there is no machine uuid at all.

2. it is nested virtualization and similar.

3. it is other arch

4. it is vdsm-reg that needs to get id (which is what I am doing now... remove vdsm-reg)
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
first, please explain this in the commit message

second, for other archs as ppc you should have specific code base (already done in http://gerrit.ovirt.org/#/c/19395/ so use that), the use of dmidecode should use the dmidecode_util.py and not explicit as now.

third, also appropriate solution for nested visualization instead of relying on uuid package

fourth - you have to have something depends of the machine that should be much more unique than using uuid package

and last, when you have specific solution for archs and nested virt, why would you need force in any flow?
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
I do not understand... I am not trying to fix the world now...

all I need is to ask vdsm what is its id without having to deal with handling the /etc/vdsm/vdsm.id

are the above valid to this task?

the new code of vdsm-reg will call vdsm-tool vdsm-id --force and get id no matter if it is uuid or generated from anywhere else, this id is required for registration.

if you then want to improve how it is 'generated' you are welcome to do so, it will not break the interface.
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
when did it fail to gather HostUUID anyway? did you bump in case where getHostUUID was failed? .. I haven't . the only case I can think of is ppc.. so I'm just trying to understand why will we ever reach to the force part. anyhow, I don't mind to have such ability to get the uuid, but still .. why\when
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
there are bioses without uuid.
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
:/ What can I say against that.. if there is no way to get better uuid for a server and we must use uuid package so let it be. for now.

Although I'm quite convinced that we can find better uuid to use based on the hardware. Don't know how urgent this workaround is, but I would prefer to see hardware based solution..
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
the /etc/vdsm/vdsm.id feature is existing feature, we are not discussing extending this, but make it more easier to use.

if you have idea of hardware based solution it should be in separate patch... and separate idea... and even if you find a solution, you should always allow software based uuid, to allow workaround cases you cannot think of.
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 6:

(1 comment)

....................................................
File lib/vdsm/utils.py
Line 624: 
Line 625:             if p.returncode == 0 and 'Not' not in out:
Line 626:                 #Avoid error string - 'Not Settable' or 'Not Present'
Line 627:                 __hostUUID = out.strip()
Line 628:             elif force:
I will think about it . for now I accept this solution to enforce response from that function. the commit message should emphasize that this is to avoid cases where hardware uuid is not "easy" to gather imo.. it doesn't really relate to the existence of /etc/vdsm/vdsm.id file
Line 629:                 hostid = str(uuid.uuid4())
Line 630:                 with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 631:                     f.write("%s\n", hostid)
Line 632:                 ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7:

modified commit message

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7:

Build Successful

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

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

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

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7: Code-Review+1

Itamar Heim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7:

ping

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7:
> ping

Waiting for me to have time to perform the registration rewrite thing.

Itamar Heim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7:

ping

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7: Code-Review-1

Alon, when do you plan to start working on the registration ? what are the plans in that area?

other than that, please add your reply on Patch Set 6 to the commit message and ill put back my -1.

I would continue with the patch to save time in future issues that might raise regarding to the id

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 7:

these pings are useless.

it is waiting for resources.

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 9: Code-Review+1

modified a bit the commit. any rejections to take it ?

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 8:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9012/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9153/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8224/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 9:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9013/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9154/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8225/ : SUCCESS

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 10:

rebase

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 10:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9762/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8824/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9609/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : There was an infra issue, please contact [e-mail]

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9764/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8826/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9611/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_storage_functional_tests_localfs_ge... : There was an infra issue, please contact [e-mail]

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(1 comment)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
i would handle IOError here, return None and add exception message. no need for full backtrace
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])
Line 834:                     __hostUUID = hostid
Line 835:                 else:

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(1 comment)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
> i would handle IOError here, return None and add exception message. no need

the entire idea of force is to fail if not succeeding... no?
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])
Line 834:                     __hostUUID = hostid
Line 835:                 else:

Saggi Mizrahi has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(2 comments)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/tool/vdsm-id.py
File lib/vdsm/tool/vdsm-id.py:

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: 
Line 21: import argparse
argparse is a 2.7 feature.
Iirc it's not available in el6.

Should this code be supported on el6?
Line 22: import sys
Line 23: 
Line 24: 
Line 25: from ..utils import getHostUUID

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 825: 
Line 826:                 if ret == 0 and 'Not' not in out:
Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
I think that a getter shouldn't really create the things it's supposed to retrieve.

The whole concept of force getter is a bit odd. You are trying so hard to get the UUID that you end up making one up?
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(2 comments)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/tool/vdsm-id.py
File lib/vdsm/tool/vdsm-id.py:

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: 
Line 21: import argparse
> argparse is a 2.7 feature.

we have:

%if 0%{?rhel} == 6
 BuildRequires: python-argparse
 BuildRequires: python-ordereddict
 %endif
Line 22: import sys
Line 23: 
Line 24: 
Line 25: from ..utils import getHostUUID

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 825: 
Line 826:                 if ret == 0 and 'Not' not in out:
Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
> I think that a getter shouldn't really create the things it's supposed to r

yes. if there is one in bios, we generate random.

force is introduce so we have id no matter what.

IMO it was a mistake to use BIOS ones anyway, we should have generated random always, but...
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])

Saggi Mizrahi has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(1 comment)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 825: 
Line 826:                 if ret == 0 and 'Not' not in out:
Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
> yes. if there is one in bios, we generate random.

Isn't there a stage where we can call
ensureHostUUID() that does this?
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(1 comment)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 825: 
Line 826:                 if ret == 0 and 'Not' not in out:
Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
> Isn't there a stage where we can call

and expose the structure of the id as uuid? instead of having this at one place?
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(2 comments)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 825: 
Line 826:                 if ret == 0 and 'Not' not in out:
Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
> and expose the structure of the id as uuid? instead of having this at one p

it is valid way to sign UUID to host for now. later we'll find better unique UUID on ppc or other arcs that don't retreive uuid by dmidecode. so force it is.. its not simple getter, its a getter with a bit of logic
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])

Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
> the entire idea of force is to fail if not succeeding... no?

return None it is count as failure in that part, so i prefer it cleaner
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])
Line 834:                     __hostUUID = hostid
Line 835:                 else:

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(1 comment)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/tool/vdsm-id.py
File lib/vdsm/tool/vdsm-id.py:

Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: 
Line 21: import argparse
> we have:

we use all over already. don't worry about it :)
Line 22: import sys
Line 23: 
Line 24: 
Line 25: from ..utils import getHostUUID

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

(2 comments)

http://gerrit.ovirt.org/#/c/20808/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 825: 
Line 826:                 if ret == 0 and 'Not' not in out:
Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
> it is valid way to sign UUID to host for now. later we'll find better uniqu

yes, the firmware of ppc have different nature/format, it is serial number based.
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])

Line 827:                     # Avoid error string - 'Not Settable' or 'Not Present'
Line 828:                     __hostUUID = out.strip()
Line 829:                 elif force:
Line 830:                     hostid = str(uuid.uuid4())
Line 831:                     with open(constants.P_VDSM_NODE_ID, 'w') as f:
> return None it is count as failure in that part, so i prefer it cleaner

no... None is not failure == no uuid, it is supported by legacy and should be continue to be so.
Line 832:                         f.write("%s\n", hostid)
Line 833:                     ovirtNodePersist([constants.P_VDSM_NODE_ID])
Line 834:                     __hostUUID = hostid
Line 835:                 else:

Yaniv Bronhaim has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11: Code-Review+1

[Alon Bar-Lev]

Alon Bar-Lev has abandoned this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Abandoned

vdsm old stuff, still required but someone from vdsm should take

[e-mail] has posted comments on this change.

Change subject: vdsm-tool: vdsm-id: add force option to force generate id
......................................................................

Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found