Douglas Schilling Landgraf has uploaded a new change for review.
Change subject: utils: replace order of import for persist/unpersist ......................................................................
utils: replace order of import for persist/unpersist
Preferable use the new ovirt-node Config class for persist/unpersist commands. The previous persist/unpersist provider uses /tmp/ovirt.log and might affect the vdsm start as missing ownership of file.
Change-Id: I51571693d2ff145c1b72b47e28c5c2f09b15acb7 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1274063 Signed-off-by: Douglas Schilling Landgraf dougsland@redhat.com --- M lib/vdsm/utils.py 1 file changed, 7 insertions(+), 7 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/47829/1
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py index 31d8529..bcd7cff 100644 --- a/lib/vdsm/utils.py +++ b/lib/vdsm/utils.py @@ -59,15 +59,15 @@ from . import constants
try: - # If failing to import old code, then try importing the legacy code - from ovirtnode import ovirtfunctions - persist = ovirtfunctions.ovirt_store_config - unpersist = ovirtfunctions.remove_config + from ovirt.node.utils.fs import Config + persist = Config().persist + unpersist = Config().unpersist except ImportError: try: - from ovirt.node.utils.fs import Config - persist = Config().persist - unpersist = Config().unpersist + # If failing to import old code, then try importing the legacy code + from ovirtnode import ovirtfunctions + persist = ovirtfunctions.ovirt_store_config + unpersist = ovirtfunctions.remove_config except ImportError: persist = lambda name: None unpersist = lambda name: None
automation@ovirt.org has posted comments on this change.
Change subject: utils: replace order of import for persist/unpersist ......................................................................
Patch Set 1:
* Update tracker::#1274063::OK * Check Bug-Url::OK * Check Public Bug::#1274063::OK, public bug * Check Product::#1274063::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Ryan Barry has posted comments on this change.
Change subject: utils: replace order of import for persist/unpersist ......................................................................
Patch Set 1: Code-Review+1
Douglas Schilling Landgraf has posted comments on this change.
Change subject: utils: replace order of import for persist/unpersist ......................................................................
Patch Set 1: Verified+1
I couldn't reproduce the report but the stactktrace is clear. I have tested the patch starting and stopping vdsm and also persisting and unpersiting files using from vdsm utils, everything working.
Dan Kenigsberg has posted comments on this change.
Change subject: utils: replace order of import for persist/unpersist ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/47829/1/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 63: persist = Config().persist Line 64: unpersist = Config().unpersist Line 65: except ImportError: Line 66: try: Line 67: # If failing to import old code, then try importing the legacy code Please remind me: why do we still need this import at all?
http://gerrit.ovirt.org/33468 does not explain, so I don't know if its reasons are still relevant.
Please explain in the commit message why this double-import is important, or drop it. Line 68: from ovirtnode import ovirtfunctions Line 69: persist = ovirtfunctions.ovirt_store_config Line 70: unpersist = ovirtfunctions.remove_config Line 71: except ImportError:
automation@ovirt.org has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2:
* Update tracker::#1274063::OK * Check Bug-Url::OK * Check Public Bug::#1274063::OK, public bug * Check Product::#1274063::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Douglas Schilling Landgraf has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2: Verified+1
Yeela Kaplan has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/47829/1/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 63: persist = Config().persist Line 64: unpersist = Config().unpersist Line 65: except ImportError: Line 66: try: Line 67: # If failing to import old code, then try importing the legacy code
Please remind me: why do we still need this import at all?
There is the comment that explains it '# If failing to import old code, then try importing the legacy code'.
At the time we had to support legacy code of ovirt node. Fabian should know if we can remove this section now. Line 68: from ovirtnode import ovirtfunctions Line 69: persist = ovirtfunctions.ovirt_store_config Line 70: unpersist = ovirtfunctions.remove_config Line 71: except ImportError:
Yeela Kaplan has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/47829/2/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 58 Line 59 Line 60 Line 61 Line 62 Douglas, What you're saying is that we did the import wrong?
We tried to import the legacy code first instead of the new code?
I think Fabian knows best if we still need to try and import legacy code...
Dan Kenigsberg has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2: Code-Review+1
Waiting for Fabian to approve (or even better, a commit-message explanation why the old-style import is no longer needed)
Douglas Schilling Landgraf has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/47829/2/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 58 Line 59 Line 60 Line 61 Line 62
Douglas,
The best is always try to use the new code instead of the old one which brings the issue that vdsm cannot be started related in the description of patch or at least the import order should be: "Try first the new code and IF FAIL use the old". I believe at this time, it's safe enough to deprecate and remove the old code but Fabian can give the final ack.
Fabian Deutsch has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2: Code-Review+1
Dan Kenigsberg has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 2: Code-Review+2
Trusting Fabian.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: utils: replace import for persist/unpersist ......................................................................
utils: replace import for persist/unpersist
Use the new ovirt-node Config class for persist/unpersist commands. The previous persist/unpersist provider uses /tmp/ovirt.log and might affect the vdsm start as missing ownership of file.
Change-Id: I51571693d2ff145c1b72b47e28c5c2f09b15acb7 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1274063 Signed-off-by: Douglas Schilling Landgraf dougsland@redhat.com Reviewed-on: https://gerrit.ovirt.org/47829 Continuous-Integration: Jenkins CI Reviewed-by: Fabian Deutsch fabiand@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/utils.py 1 file changed, 5 insertions(+), 11 deletions(-)
Approvals: Fabian Deutsch: Looks good to me, but someone else must approve Douglas Schilling Landgraf: Verified Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved
automation@ovirt.org has posted comments on this change.
Change subject: utils: replace import for persist/unpersist ......................................................................
Patch Set 3:
* Update tracker::#1274063::OK * Set MODIFIED::bug 1274063::::#1274063::::IGNORE, not oVirt prod but ovirt-node
vdsm-patches@lists.fedorahosted.org