Ofer Schreiber has posted comments on this change.
Change subject: packaging: [DO NOT MERGE] Handling active tasks during upgrade
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(19 inline comments)
....................................................
File packaging/fedora/setup/common_utils.py
Line 1355: originalTimeout = 0
Line 1356: cmd = [
Line 1357: basedefs.FILE_RHEVM_CONFIG_BIN,
Line 1358: "-g", "AsyncTaskZombieTaskLifeInMinutes",
Line 1359: ]
Don't we have a facility for executing the rhevm_config?
Line 1360:
Line 1361: out, rc = execCmd(cmdList=cmd, failOnError=True, msg="Failed to
retrieve the value of AsyncTaskZombieTaskLifeInMinutes parameter.")
Line 1362: if out and not rc:
Line 1363: # The value is the second field in the output
Line 1357: basedefs.FILE_RHEVM_CONFIG_BIN,
Line 1358: "-g", "AsyncTaskZombieTaskLifeInMinutes",
Line 1359: ]
Line 1360:
Line 1361: out, rc = execCmd(cmdList=cmd, failOnError=True, msg="Failed to
retrieve the value of AsyncTaskZombieTaskLifeInMinutes parameter.")
why the msg is here and not in output_msgs?
Line 1362: if out and not rc:
Line 1363: # The value is the second field in the output
Line 1364: originalTimeout = out.split()[1]
Line 1365:
Line 1358: "-g", "AsyncTaskZombieTaskLifeInMinutes",
Line 1359: ]
Line 1360:
Line 1361: out, rc = execCmd(cmdList=cmd, failOnError=True, msg="Failed to
retrieve the value of AsyncTaskZombieTaskLifeInMinutes parameter.")
Line 1362: if out and not rc:
what should happen if the answer is false?
Line 1363: # The value is the second field in the output
Line 1364: originalTimeout = out.split()[1]
Line 1365:
Line 1366: # Now, set the value to timeout
Line 1360:
Line 1361: out, rc = execCmd(cmdList=cmd, failOnError=True, msg="Failed to
retrieve the value of AsyncTaskZombieTaskLifeInMinutes parameter.")
Line 1362: if out and not rc:
Line 1363: # The value is the second field in the output
Line 1364: originalTimeout = out.split()[1]
defining a var inside an if is a bad idea, it will cause runtime error if the
if was false (during the return)
Line 1365:
Line 1366: # Now, set the value to timeout
Line 1367: updateVDCOption(key="AsyncTaskZombieTaskLifeInMinutes",
Line 1368: value=timeout,
Line 1370:
Line 1371: # Return the original value
Line 1372: return originalTimeout
Line 1373:
Line 1374: def restoreEngineNormalMode(timeout):
Bad name, what is normal mode?
Line 1375: """
Line 1376: Restores the engine to normal working mode.
Line 1377:
Line 1378: """
Line 1391:
Line 1392: # First check that the file that we are trying to modify does
exist:
Line 1393: originalPath = basedefs.FILE_ENGINE_SYSCONFIG
Line 1394: if not os.path.exists(originalPath):
Line 1395: raise Exception("The file \"%s\" doesn't exist.",
originalPath)
output msgs
Line 1396:
Line 1397: # Make a backup copy that we will later restore when going out of
Line 1398: # the maintenance mode, but only if it doesn't already exist, as
Line 1399: # otherwise we lose the original content:
Line 1402: logging.debug("Making a backup copy \"%s\" to \"%s\".",
originalPath, backupPath)
Line 1403: copyFile(originalPath, backupPath)
Line 1404:
Line 1405: # All edits will be performed in a temporary file, and only if
Line 1406: # everything works correctly will the original be replaced:
+10!
Line 1407: temporaryPath = originalPath + ".tmp"
Line 1408: copyFile(originalPath, temporaryPath)
Line 1409:
Line 1410: # Edit the engine local configuration file as required, then
replace
....................................................
File packaging/fedora/setup/engine-upgrade.py
Line 63: UNSUPPORTED_VERSION = "2.2"
Line 64:
Line 65: #MSGS
Line 66: MSG_TASKS_COMPENSATIONS = "\n\nTasks:\n%s\n\nCompensations:\n%s\n\n"
Line 67: MSG_STOP_RUNNING_TASKS = "\nInfo: There are following running tasks
and compensations \
I think the grammer here is problematic
Line 68: found in the manager: %s\
Line 69: Would you like to proceed with the upgrade and try to stop tasks
automatically?\n(Answering \
Line 70: 'no' will stop the upgrade.)" % MSG_TASKS_COMPENSATIONS
Line 71: MSG_RUNNING_TASKS = "Checking active tasks and compensations"
Line 760: basedefs.EXEC_PSQL,
Line 761: "-U", SERVER_ADMIN,
Line 762: "-f", basedefs.FILE_DB_ASYNC_TASKS,
Line 763: "-d", dbName,
Line 764: ]
any reason not to use "execSqlCmd"?
or add it the ability to use file?
Line 765:
Line 766: out, rc = utils.execCmd(cmdList=cmd, failOnError=True, msg="Error
updating DB for getting async_tasks", envDict=utils.getPgPassEnv())
Line 767:
Line 768: def getRunningTasks(dbName=basedefs.DB_NAME):
Line 766: out, rc = utils.execCmd(cmdList=cmd, failOnError=True, msg="Error
updating DB for getting async_tasks", envDict=utils.getPgPassEnv())
Line 767:
Line 768: def getRunningTasks(dbName=basedefs.DB_NAME):
Line 769: # Get async tasks:
Line 770: query = "select * from fn_db_get_async_tasks();"
better be CONST IMO
Line 771: runningTasks, rc = utils.execRemoteSqlCommand(
Line 772: userName=SERVER_ADMIN,
Line 773: dbHost=SERVER_NAME,
Line 774: dbPort=SERVER_PORT,
Line 777: failOnError=True,
Line 778: errMsg="Can't get async tasks
list",
Line 779: )
Line 780:
Line 781: if "RECORD" in runningTasks:
What does it mean?
I'm missing explanation
Line 782: return runningTasks
Line 783: else:
Line 784: return None
Line 785:
Line 798:
Line 799:
Line 800: return compensations
Line 801:
Line 802: def checkRunningTasks(dbName=basedefs.DB_NAME,
service=basedefs.ENGINE_SERVICE_NAME):
won't we need all these functions sometime in the future outside of upgrade?
Line 803: # Find running tasks first
Line 804: deployDbAsyncTasks(dbName)
Line 805: runningTasks = getRunningTasks(dbName)
Line 806: compensations = getCompensations(dbName)
Line 814: answerYes = utils.askYesNo(MSG_STOP_RUNNING_TASKS %
Line 815: (runningTasks,
Line 816: compensations))
Line 817: if not answerYes:
Line 818: raise Exception("User decided not to stop running
tasks. Stopping upgrade.\n")
output msgs/MSG_BLA?
Line 819:
Line 820: # restart jboss/engine in maintenace mode (i.e different
port):
Line 821: utils.configureEngineForMaintenance()
Line 822: origTimeout = utils.configureTasksTimeout(timeout=0)
Line 818: raise Exception("User decided not to stop running
tasks. Stopping upgrade.\n")
Line 819:
Line 820: # restart jboss/engine in maintenace mode (i.e different
port):
Line 821: utils.configureEngineForMaintenance()
Line 822: origTimeout = utils.configureTasksTimeout(timeout=0)
why do you need origTimeout?
Line 823: startEngine(service)
Line 824:
Line 825: # Pull tasks in a loop for some time
Line 826: # MAINTENANCE_TASKS_WAIT_PERIOD = 60 (seconds, between
trials)
Line 841: raise Exception(MSG_TASKS_STILL_RUNNING %
RUNNING_TASKS_MSG)
Line 842:
Line 843:
Line 844: except:
Line 845: raise
This can be removed, the exception will be raised eventually.
Line 846:
Line 847: finally:
Line 848: # Restore normal engine configuration
Line 849: if origTimeout != 0:
Line 954: runFunc(stopEngineService, MSG_INFO_STOP_ENGINE)
Line 955: if updateRelatedToDB:
Line 956: runFunc([[stopDbRelatedServices, etlService,
notificationService]], MSG_INFO_STOP_DB)
Line 957:
Line 958: try:
WHY are you doing this?
I'm missing some comments here
Line 959: runFunc([checkRunningTasksFunc],
MSG_RUNNING_TASKS)
Line 960: except Exception as e:
Line 961: runFunc([[startDbRelatedServices, etlService,
notificationService]], MSG_INFO_START_DB)
Line 962: runFunc(startEngineService, MSG_INFO_START_ENGINE)
Line 956: runFunc([[stopDbRelatedServices, etlService,
notificationService]], MSG_INFO_STOP_DB)
Line 957:
Line 958: try:
Line 959: runFunc([checkRunningTasksFunc],
MSG_RUNNING_TASKS)
Line 960: except Exception as e:
Why do you need e?
even more - You "swallow" the exception and not logging it....
Line 961: runFunc([[startDbRelatedServices, etlService,
notificationService]], MSG_INFO_START_DB)
Line 962: runFunc(startEngineService, MSG_INFO_START_ENGINE)
Line 963: raise
Line 964: else:
Line 958: try:
Line 959: runFunc([checkRunningTasksFunc],
MSG_RUNNING_TASKS)
Line 960: except Exception as e:
Line 961: runFunc([[startDbRelatedServices, etlService,
notificationService]], MSG_INFO_START_DB)
Line 962: runFunc(startEngineService, MSG_INFO_START_ENGINE)
what will happen if one of these functions will raise an exception?
Line 963: raise
Line 964: else:
Line 965: # This means that user chose not to stop ovirt-engine
Line 966: logging.debug("exiting gracefully")
....................................................
File packaging/fedora/spec/ovirt-engine.spec.in
Line 753: %{engine_data}/scripts/engine-setup.py*
Line 754: %{engine_data}/scripts/engine-cleanup.py*
Line 755: %{engine_data}/scripts/engine-upgrade.py*
Line 756: %{engine_data}/scripts/post_upgrade.py*
Line 757: %{engine_data}/scripts/add_fn_db_get_async_tasks_function.sql*
No need for the * in the end - it's only for python files (creating the .pyc
and pyo)
Line 758:
Line 759: # Man pages
Line 760: %{_mandir}/man8/engine-setup.*
Line 761: %{_mandir}/man8/engine-upgrade.*
--
To view, visit http://gerrit.ovirt.org/8740
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I90abb3d1e6ad0ac5557485e40064e811fc87f491
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Kiril Nesenko <knese...@redhat.com>
Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
