Alon Bar-Lev has posted comments on this change.
Change subject: [WIP] packaging: added DB validation functions on upgrade
......................................................................
Patch Set 3: (32 inline comments)
....................................................
File packaging/setup/plugins/ovirt-engine-setup/upgrade/asynctasks.py
Line 36: class Plugin(plugin.PluginBase):
Line 37: """ DB Async tasks handling plugin."""
Line 38:
Line 39: _CLEANUP_WAIT_SECONDS = 180
Line 40: _CLEANUP_WAIT_MINUTES = self._CLEANUP_WAIT_SECONDS / 60
cannot be that self._ is working at this context.
Line 41: _NORMAL = 'normal'
Line 42: _MAINTENANCE = 'maintenance'
Line 43:
Line 44: class _engineInMaintenance(object):
Line 38:
Line 39: _CLEANUP_WAIT_SECONDS = 180
Line 40: _CLEANUP_WAIT_MINUTES = self._CLEANUP_WAIT_SECONDS / 60
Line 41: _NORMAL = 'normal'
Line 42: _MAINTENANCE = 'maintenance'
nomeric please... prefix please..
(
OP_NORMAL,
OP_MAINTENANCE,
) = range(2)
Line 43:
Line 44: class _engineInMaintenance(object):
Line 45: def __init__(
Line 46: self,
Line 44: class _engineInMaintenance(object):
Line 45: def __init__(
Line 46: self,
Line 47: configureTasksTimeout,
Line 48: setEngineMode,
I think that these functions can move into this class, they are not used
outside, just helpers of this one.
Line 49: services,
Line 50: logger,
Line 51: ):
Line 52: self._configureTasksTimeout = configureTasksTimeout
Line 45: def __init__(
Line 46: self,
Line 47: configureTasksTimeout,
Line 48: setEngineMode,
Line 49: services,
for this you can pass self of parent...
Line 50: logger,
Line 51: ):
Line 52: self._configureTasksTimeout = configureTasksTimeout
Line 53: self._setEngineMode = setEngineMode
Line 46: self,
Line 47: configureTasksTimeout,
Line 48: setEngineMode,
Line 49: services,
Line 50: logger,
for this you can inherit from otopi Base
Line 51: ):
Line 52: self._configureTasksTimeout = configureTasksTimeout
Line 53: self._setEngineMode = setEngineMode
Line 54: self.services = services
Line 52: self._configureTasksTimeout = configureTasksTimeout
Line 53: self._setEngineMode = setEngineMode
Line 54: self.services = services
Line 55: self.logger = logger
Line 56: self.timeout = 0
_ for private
Line 57:
Line 58: def __enter__(self):
Line 59: self.timeout = self._configureTasksTimeout(
Line 60: timeout=self.timeout
Line 55: self.logger = logger
Line 56: self.timeout = 0
Line 57:
Line 58: def __enter__(self):
Line 59: self.timeout = self._configureTasksTimeout(
do not use the same variable for different values
Line 60: timeout=self.timeout
Line 61: )
Line 62: self.logger.debug(
Line 63: 'Setting engine into maintenance mode'
Line 94: def __init__(self, context):
Line 95: super(Plugin, self).__init__(context=context)
Line 96: self._enabled = False
Line 97:
Line 98: def _zombieUtil(self, dbname, user, host, port, clear=None):
not sure there is a valid usage for one time called function called by already
well established function.
Line 99:
Line 100: args = [
Line 101: osetupcons.FileLocations.OVIRT_ENGINE_TASKCLEANER,
Line 102: '--user={user}'.format(
Line 117: '-R',
Line 118: '-A',
Line 119: '-J',
Line 120: '-q',
Line 121: ]
if you can avoid mixing long and short options it would be great.
Line 122:
Line 123: return self.execute(
Line 124: args,
Line 125: raiseOnError=False,
Line 143: clear=True,
Line 144: )
Line 145: if rc > 1:
Line 146: raise RuntimeError(
Line 147: _('Failed to clear zombie tasks!')
Please do not use '!'
Please explain more to the user what should he probably do.
Line 148: )
Line 149:
Line 150: def _setEngineMode(self, mode=self._NORMAL):
Line 151: if mode == self._MAINTENANCE:
Line 146: raise RuntimeError(
Line 147: _('Failed to clear zombie tasks!')
Line 148: )
Line 149:
Line 150: def _setEngineMode(self, mode=self._NORMAL):
if mode = maint:
op = 'MAINT..'
else:
op = 'ACTIVE'
self.dbstatement.update...
Line 151: if mode == self._MAINTENANCE:
Line 152: self.dbstatement.updateVDCOption(
Line 153: {
Line 154: 'name': 'EngineMode',
Line 164: }
Line 165: )
Line 166: self.in_maintenance = False
Line 167: else:
Line 168: self.logger.debug(
no need for these with the mode is not exposed out of interface... as you have
full control over that.. not that this is that important... but it reduces code
size.
Line 169: 'Usupported Engine mode requested, doing nothing.'
Line 170: )
Line 171:
Line 172:
Line 190:
Line 191:
Line 192: def _getRunningTasks(self, dbname, user, host, port, password):
Line 193:
Line 194: tasks = self.dbstatement.execute(
best in this case to do ownConnection=True, transaction=False
Line 195: statement="""
Line 196: select
Line 197: a.action_type,
Line 198: a.task_id,
Line 227: statement="""
Line 228: select
Line 229: command_type, entity_type
Line 230: from business_entity_snapshot
Line 231: """,
best in this case to do ownConnection=True, transaction=False
Line 232: )[0]
Line 233:
Line 234: return '\n'.join(
Line 235: [
Line 244: return self.dialog.queryBoolean(
Line 245: dialog=self.dialog,
Line 246: name='OVESETUP_SHOW_RUNNING_TASKS',
Line 247: note=_(
Line 248: '[ {timestamp} ] The following system tasks have been
'
why do you need the timestamp?
Line 249: 'found running in the system:\n'
Line 250: '{tasks}\n'
Line 251: '{compensations}\n'
Line 252: 'Would you like to proceed '
Line 257: tasks=runningTasks,
Line 258: compensations=compensations,
Line 259: ),
Line 260: prompt=True,
Line 261: true=_('Yes'),
these are the defaults
Line 262: false=_('No'),
Line 263: )
Line 264:
Line 265: def _infoStoppingTasks(self, retry=False):
Line 267: timestamp = now.strftime('%b %d %H:%M:%S')
Line 268: if retry:
Line 269: header = 'Retrying to clear system tasks '
Line 270: else:
Line 271: header = 'System will try to clear running tasks '
I don't think there is need for this complexity.
A simple message of:
[INFO] Waiting for completion of running tasks...
[INFO] Waiting for completion of running tasks...
[INFO] Waiting for completion of running tasks...
[INFO] Waiting for completion of running tasks...
Is OK... if you want to add timestamp, than ok... but should OK.
Better is to present the # of running tasks... this is useful information.
Line 272:
Line 273: return _(
Line 274: '[ {timestamp} ] {header}'
Line 275: 'during the next {cleanup_wait_minutes} minutes.\n'
Line 273: return _(
Line 274: '[ {timestamp} ] {header}'
Line 275: 'during the next {cleanup_wait_minutes} minutes.\n'
Line 276: ).format(
Line 277: timestamp = timestamp,
no spaces
Line 278: header=header,
Line 279: cleanup_wait_minutes=self._CLEANUP_WAIT_MINUTES
Line 280: )
Line 281:
Line 287:
Line 288: if runningTasks or compensations:
Line 289:
Line 290: if not self._stopTasks(runningTasks, compensations):
Line 291: # User decided not to stop tasks
no need for comments that state exactly what the one line bellow states.
Line 292: raise RuntimeError(
Line 293: _('User decided not to stop running tasks.
Exiting.')
Line 294: )
Line 295:
Line 317: 'Still waiting for system tasks to be
cleared.'
Line 318: )
Line 319: if waited < self._CLEANUP_WAIT_SECONDS:
Line 320: continue
Line 321:
Loop should stop here... and you need another one.... this is too complex
structure for me to understand.
Line 322: # Should we continue?
Line 323: if self._stopTasks(runningTasks,
compensations):
Line 324: #If yes, go for another iteration
Line 325: self.dialog.note(
Line 319: if waited < self._CLEANUP_WAIT_SECONDS:
Line 320: continue
Line 321:
Line 322: # Should we continue?
Line 323: if self._stopTasks(runningTasks,
compensations):
patten should be:
if failure:
raise exception
continnue as usual
Line 324: #If yes, go for another iteration
Line 325: self.dialog.note(
Line 326:
text=self._infoStoppingTasks(retry=True)
Line 327: )
Line 363: def _setup(self):
Line 364: self._enabled = self.environment[
Line 365: osetupcons.DBEnv.IGNORE_TASKS
Line 366: ] == False
Line 367: self.in_maintenance = False
underscore for private please.
initialize at constructor please.
Line 368: self.originalTimeout = 0
Line 369:
Line 370: @plugin.event(
Line 371: stage=plugin.Stages.STAGE_VALIDATION,
Line 376: condition=lambda self: self._enabled,
Line 377: )
Line 378: def _validation(self):
Line 379:
Line 380: self.dbstatement = database.Statement(self.environment)
please pass the statement as parameter, no need to hold it as member.
Line 381: self._clearZombieTasks()
Line 382: self._clearRunningTasks()
Line 383:
Line 384:
....................................................
File packaging/setup/plugins/ovirt-engine-setup/upgrade/dbvalidations.py
Line 34: @util.export
Line 35: class Plugin(plugin.PluginBase):
Line 36: """ DB validations plugin."""
Line 37:
Line 38: def _dbUtil(self, dbname, user, host, port, fix=None):
why isn't fix boolean?
Line 39:
Line 40: args = [
Line 41: osetupcons.FileLocations.OVIRT_ENGINE_DB_VALIDATOR,
Line 42: '--user={user}'.format(
Line 53: ),
Line 54: ]
Line 55: if fix:
Line 56: args.append = [
Line 57: '--fix=1'
--fix?
Line 58: ]
Line 59:
Line 60: return self.execute(
Line 61: args,
Line 76: output=stdout,
Line 77: )
Line 78: )
Line 79:
Line 80: result = (
please add this properly to dialog.py
Line 81: '\n\n ====== Validating database "{dbname}" ====== \n'
Line 82: '{content}'
Line 83: ).format(
Line 84: dbname=dbname,
Line 86: )
Line 87:
Line 88: return (result, rc)
Line 89:
Line 90: def _fixDb(self, dbname, user, host, port):
no need for this function.
Line 91:
Line 92: self._dbUtil(
Line 93: dbname=dbname,
Line 94: user=user,
Line 126: ],
Line 127: condition=lambda self: self._enabled,
Line 128: )
Line 129: def _validation(self):
Line 130: violations, issues_found = self._checkDb(
I would have loop over violations and present each one using its own note.
Line 131: dbname=self.environment[
Line 132: osetupcons.DBEnv.DATABASE
Line 133: ],
Line 134: user=self.environment[
Line 150: ] = self.dialog.queryBoolean(
Line 151: dialog=self.dialog,
Line 152: name='OVESETUP_FIX_DB_VALIDATIONS',
Line 153: note=_(
Line 154: 'The following inconsistencies were found '
please separate this to note (text) and dialog (question)
Line 155: 'in the DB: {violations}. Would you like '
Line 156: 'to automatically clear inconsistencies
before '
Line 157: 'upgraing?\n'
Line 158: '(Answering no will stop the upgrade)
(@VALUES@): '
Line 156: 'to automatically clear inconsistencies
before '
Line 157: 'upgraing?\n'
Line 158: '(Answering no will stop the upgrade)
(@VALUES@): '
Line 159: ).format(
Line 160: violations=violations
comma at end of lists
Line 161: ),
Line 162: prompt=True,
Line 163: true=_('Yes'),
Line 164: false=_('No'),
Line 159: ).format(
Line 160: violations=violations
Line 161: ),
Line 162: prompt=True,
Line 163: true=_('Yes'),
these are the default
Line 164: false=_('No'),
Line 165: )
Line 166:
Line 167: if self.environment[
Line 182: ],
Line 183: )
Line 184: else:
Line 185: raise Exception(
Line 186: _(
_('xxxx')
why not RuntimeError?
pattern should be:
if failure:
raise exception
continue
Line 187: "User decided to skip db fix"
Line 188: )
Line 189: )
Line 190:
--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
