Alon Bar-Lev has posted comments on this change.
Change subject: [WIP] packaging: added DB validation functions
......................................................................
Patch Set 6: (23 inline comments)
....................................................
File packaging/setup/ovirt_engine_setup/constants.py
Line 906: )
Line 907: def CLEAR_TASKS_WAIT_PERIOD(self):
Line 908: return 'OVESETUP_ASYNC/clearTasksWait'
Line 909:
Line 910: ASYNC_TASKS_MAP = {
this does not belong here, please move it to the plugin.
Line 911: '1': [
Line 912: 'AddVmCommand',
Line 913: 'Adding a virtual machine',
Line 914: ],
Line 1020: 'MoveDisk',
Line 1021: 'Move Disk',
Line 1022: ],
Line 1023: }
Line 1024:
please always keep two spaces.
....................................................
File packaging/setup/plugins/ovirt-engine-setup/upgrade/asynctasks.py
Line 47: self._parent = parent
Line 48: self._origTimeout = 0
Line 49: self._dbstatement = dbstatement
Line 50:
Line 51: def _setEngineMode(self, active=True):
better use maintenance=True less confusing within the context of the code.
Line 52: mode = (
Line 53: 'ACTIVE' if active
Line 54: else 'MAINTENANCE'
Line 55: )
Line 85: def __enter__(self):
Line 86: self._origTimeout = self._dbstatement.getVDCOption(
Line 87: name='AsyncTaskZombieTaskLifeInMinutes',
Line 88: )
Line 89: self._dbstatement.updateVDCOption(
please collapse this into the setEngineMode with timeout= parameter.
Line 90: {
Line 91: 'name': 'AsyncTaskZombieTaskLifeInMinutes',
Line 92: 'value': 0,
Line 93: },
Line 123: def __init__(self, context):
Line 124: super(Plugin, self).__init__(context=context)
Line 125: self._enabled = False
Line 126:
Line 127: def _clearZombieTasks(self):
please move private methods before __init__
Line 128: rc, tasks, stderr = self.execute(
Line 129: [
Line 130: osetupcons.FileLocations.OVIRT_ENGINE_TASKCLEANER,
Line 131: '-u', self.environment[osetupcons.DBEnv.USER],
Line 139: ],
Line 140: raiseOnError=False,
Line 141: )
Line 142:
Line 143: if rc > 1:
and what if rc == 1? shouldn't we do something?
Line 144: raise RuntimeError(
Line 145: _(
Line 146: 'Failed to clear zombie tasks. Please access
support '
Line 147: 'in attempt to resolve problem '
Line 165: )[0]
Line 166:
Line 167: return ''.join(
Line 168: [
Line 169: '\n---- Task ID: {task_id:30} ------- \n'
why space before \n? Why leading \n? can't you join by '\n' to have same effect?
Line 170: 'Task Name: {task_name:30}\n'
Line 171: 'Task Description: {task_desc:30}\n'
Line 172: 'Started at: {started_at:30}\n'
Line 173: 'DC Name: {name:30}\n'.format(
Line 167: return ''.join(
Line 168: [
Line 169: '\n---- Task ID: {task_id:30} ------- \n'
Line 170: 'Task Name: {task_name:30}\n'
Line 171: 'Task Description: {task_desc:30}\n'
better to indent so:
Task Name: XXXXXXXXXXXXXXXxx
Description: YYYYYYYYYYYYYYY
Started at: ZZZZZZZZZZZZZZZZ
Line 172: 'Started at: {started_at:30}\n'
Line 173: 'DC Name: {name:30}\n'.format(
Line 174: task_id=entry['task_id'],
Line 175: task_name=self.environment[
Line 169: '\n---- Task ID: {task_id:30} ------- \n'
Line 170: 'Task Name: {task_name:30}\n'
Line 171: 'Task Description: {task_desc:30}\n'
Line 172: 'Started at: {started_at:30}\n'
Line 173: 'DC Name: {name:30}\n'.format(
please localize
Line 174: task_id=entry['task_id'],
Line 175: task_name=self.environment[
Line 176: osetupcons.AsyncTasksEnv.ASYNC_TASKS_MAP
Line 177: ][entry['action_type']][0],
Line 193: from business_entity_snapshot
Line 194: """,
Line 195: ownConnection=True,
Line 196: transaction=False,
Line 197: )[0]
how can it be more than one if you get only 1st line?
Line 198:
Line 199: return '\n'.join(
Line 200: [
Line 201: '{command_type:30} {entity_type:30}'.format(**entry)
Line 209: name='OVESETUP_SHOW_RUNNING_TASKS',
Line 210: note=_(
Line 211: 'The following system tasks have been '
Line 212: 'found running in the system:\n'
Line 213: '{tasks}\n'
please separate tasks note from compensations note.
Line 214: '{compensations}\n'
Line 215: 'Would you like to proceed '
Line 216: 'and try to stop these tasks automatically?\n'
Line 217: '(Answering "no" will stop the upgrade (@VALUES@) '
Line 225: _(
Line 226: 'There are still following tasks running'
Line 227: 'in the system:\n'
Line 228: '{tasks}\n'
Line 229: '{compensations}\n'
there is no need to repeat the message, you can just throw exception: 'Upgrade
cannot be completed, tasks or compensation still running'.
Line 230: 'Please make sure that there are no '
Line 231: 'running system tasks before you '
Line 232: 'continue. Stopping upgrade.'
Line 233: ).format(
Line 244: 'Press Ctrl+C to interrupt. '
Line 245: ).format(
Line 246: cleanup_wait=self.environment[
Line 247: osetupcons.AsyncTasksEnv.CLEAR_TASKS_WAIT_PERIOD
Line 248: ]
can we print how many running?
Line 249: )
Line 250: )
Line 251: time.sleep(
Line 252: self.environment[
Line 260: self._getRunningTasks(dbstatement),
Line 261: self._getCompensations(dbstatement),
Line 262: )
Line 263:
Line 264: def _clearRunningTasks(self):
this function does not clear... please collapse with _infoStoppingTasks.
Line 265: dbstatement = database.Statement(self.environment)
Line 266: with self._engineInMaintenance(
Line 267: dbstatement=dbstatement,
Line 268: parent=self,
Line 266: with self._engineInMaintenance(
Line 267: dbstatement=dbstatement,
Line 268: parent=self,
Line 269: ):
Line 270: while True:
this loop should be within _infoStoppingTasks renamed to waitFor...
Line 271: try:
Line 272: runningTasks = self._getRunningTasks(dbstatement)
Line 273: compensations =
self._getCompensations(dbstatement)
Line 274: if runningTasks or compensations:
Line 293:
Line 294: @plugin.event(
Line 295: stage=plugin.Stages.STAGE_SETUP,
Line 296: )
Line 297: def _setup(self):
better do this at validation, so you have this set on customization cli
Line 298: self._enabled = self.environment[
Line 299: osetupcons.AsyncTasksEnv.CLEAR_TASKS
Line 300: ] is True
Line 301:
Line 302: @plugin.event(
Line 303: # NOTE:
Line 304: # this must run before the package update
Line 305: # to prevent entering an upgrade flow when DB is in
Line 306: # an inconsistent state.
comment is useless validation is always before package update.
Line 307: stage=plugin.Stages.STAGE_VALIDATION,
Line 308: condition=lambda self: self._enabled,
Line 309: )
Line 310: def _validation(self):
Line 309: )
Line 310: def _validation(self):
Line 311: runningTasks, compensations = self._checkRunningTasks()
Line 312: if runningTasks or compensations:
Line 313: self._askUserToStopTasks(runningTasks, compensations)
I think we can wait here for tasks before continue.
Line 314:
Line 315: @plugin.event(
Line 316: # NOTE:
Line 317: # this must run before the package update
Line 320: stage=plugin.Stages.STAGE_EARLY_MISC,
Line 321: condition=lambda self: self._enabled,
Line 322: )
Line 323: def _misc(self):
Line 324:
no need for space line.
please add log info before each stage
Line 325: self._clearZombieTasks()
Line 326: self._clearRunningTasks()
Line 327:
Line 328:
....................................................
File packaging/setup/plugins/ovirt-engine-setup/upgrade/dbvalidations.py
Line 74: rc, stdout, stderr = self._dbUtil()
Line 75: if rc != 0:
Line 76: raise RuntimeError(
Line 77: _(
Line 78: 'Error: failed checking DB:\n'
no need for Error.
Line 79: '{output}\n'.format(
Line 80: output=stdout,
Line 81: )
Line 82: )
Line 105: ],
Line 106: condition=lambda self: self._enabled,
Line 107: )
Line 108: def _customization(self):
Line 109: violations, issues_found = self._checkDb()
please add logger.info before.
Line 110: if issues_found:
Line 111: if self.environment[
Line 112: osetupcons.DBEnv.FIX_DB_VIOLATIONS
Line 113: ] is None:
Line 135: if not self.environment[
Line 136: osetupcons.DBEnv.FIX_DB_VIOLATIONS
Line 137: ]:
Line 138: raise RuntimeError(
Line 139: _('User decided to skip db fix')
Aborted, database integrity cannot be established
Line 140: )
Line 141:
Line 142: @plugin.event(
Line 143: stage=plugin.Stages.STAGE_EARLY_MISC,
Line 144: name=osetupcons.Stages.FIX_DB_VIOLATIONS,
Line 145: condition=lambda self: self._enabled,
Line 146: )
Line 147: def _misc(self):
Line 148: self._dbUtil(fix=True)
please add logger.info before
Line 149:
Line 150:
--
To view, visit http://gerrit.ovirt.org/15970
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I25979acbf54d980168be929638ff4aed800bf6d3
Gerrit-PatchSet: 6
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: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
