Alon Bar-Lev has posted comments on this change.
Change subject: [WIP] packaging: added DB validation functions on upgrade
......................................................................
Patch Set 4: (30 inline comments)
....................................................
File packaging/setup/plugins/ovirt-engine-setup/upgrade/asynctasks.py
Line 36: @util.export
Line 37: class Plugin(plugin.PluginBase):
Line 38: """ DB Async tasks handling plugin."""
Line 39:
Line 40: _CLEANUP_WAIT_SECONDS = 180
put in environment with default? so we can customize?
Line 41:
Line 42: class _engineInMaintenance(base.Base):
Line 43: (
Line 44: _OP_NORMAL,
Line 42: class _engineInMaintenance(base.Base):
Line 43: (
Line 44: _OP_NORMAL,
Line 45: _OP_MAINTENANCE,
Line 46: ) = range(2)
If you end up with only two, you can as well store it in boolean.
Line 47:
Line 48: def __init__(
Line 49: self,
Line 50: parent,
Line 49: self,
Line 50: parent,
Line 51: dbstatement,
Line 52: ):
Line 53: self.services = parent.services
I would have holded the parent...
Line 54: self._origTimeout = 0
Line 55: self._dbstatement = dbstatement
Line 56:
Line 57: def _setEngineMode(self, mode=self._OP_NORMAL):
Line 63: try:
Line 64: self._dbstatement.updateVDCOption(
Line 65: {
Line 66: 'name': 'EngineMode',
Line 67: 'value': op,
If you go boolean:
'value': (
'MAINTENANCE' if self._maintenance
else 'ACTIVE'
),
Line 68: }
Line 69: )
Line 70: except:
Line 71: raise RuntimeError(
Line 66: 'name': 'EngineMode',
Line 67: 'value': op,
Line 68: }
Line 69: )
Line 70: except:
Do not swallow exceptions.
You can let these raise... but if you insist of altering text, you should at
least log these, and add the text to the exception you raise.
except Exception as e:
self.logger.debug('Cannot set mode', exc_info=True)
raise RuntimeError(_('bla bla bla. Error: {error}').format(error=e))
Line 71: raise RuntimeError(
Line 72: _(
Line 73: 'Failed to set engine to {op} mode. '
Line 74: 'Please check that engine is in correct state '
Line 77: op=op,
Line 78: )
Line 79: )
Line 80:
Line 81: def _configureTasksTimeout(self, timeout):
Not sure there is a need for one function to do get/set, as when you want to
restore you only set.
Line 82:
Line 83: # First, get the originalTimeout value
Line 84: originalTimeout = self._dbstatement.getVDCOption(
Line 85: name='AsyncTaskZombieTaskLifeInMinutes'
Line 82:
Line 83: # First, get the originalTimeout value
Line 84: originalTimeout = self._dbstatement.getVDCOption(
Line 85: name='AsyncTaskZombieTaskLifeInMinutes'
Line 86: )
comma
Line 87:
Line 88: # Now, set the value to timeout
Line 89: self._dbstatement.updateVDCOption(
Line 90: {
Line 89: self._dbstatement.updateVDCOption(
Line 90: {
Line 91: 'name': 'AsyncTaskZombieTaskLifeInMinutes',
Line 92: 'value': timeout,
Line 93: }
comma
Line 94: )
Line 95:
Line 96: # If everything went fine, return the original value
Line 97: return originalTimeout
Line 99: def __enter__(self):
Line 100: self._origTimeout = self._configureTasksTimeout(
Line 101: timeout=0
Line 102: )
Line 103: self.logger.debug(
this message can be moved to _setEngineMode, just print the action...
Line 104: 'Setting engine into maintenance mode'
Line 105: )
Line 106: self._setEngineMode(self._OP_MAINTENANCE)
Line 107:
Line 114: )
Line 115:
Line 116: def __exit__(self, exc_type, exc_value, traceback):
Line 117: # Restore previous engine configuration
Line 118: self.logger.debug(
if you move the debug message into function you don't need this one.
Line 119: 'Restoring engine to normal mode'
Line 120: )
Line 121: # Stop the engine first
Line 122: self.services.state(
Line 124: osetupcons.Const.ENGINE_SERVICE_NAME
Line 125: ],
Line 126: state=False,
Line 127: )
Line 128: # Restore normal mode
Comments that are obvious from code are not needed.
Line 129: self._setEngineMode(self._OP_NORMAL)
Line 130: # Restore previous zombie timeout
Line 131: self._configureTasksTimeout(
Line 132: timeout=self._origTimeout
Line 138:
Line 139: def _clearZombieTasks(self):
Line 140: args = [
Line 141: osetupcons.FileLocations.OVIRT_ENGINE_TASKCLEANER,
Line 142: '-u', self.environment[osetupcons.DBEnv.USER],
hmm... I thought we can select the long notation... much easier to read.
Line 143: '-s', self.environment[osetupcons.DBEnv.HOST],
Line 144: '-p', self.environment[osetupcons.DBEnv.PORT],
Line 145: '-d', self.environment[osetupcons.DBEnv.DATABASE],
Line 146: '-R',
Line 160: 'Failed to clear zombie tasks. Please try running
the '
Line 161: 'following command manually and repeat the
upgrade: '
Line 162: '\t{command}'
Line 163: ).format(
Line 164: command=' '.join(args)
Running the same command will likely to fail... so this is not a solution.
I think in this case we should instruct people to access support.
Line 165: )
Line 166: )
Line 167:
Line 168:
Line 173: select
Line 174: a.action_type,
Line 175: a.task_id,
Line 176: a.started_at,
Line 177: b.name
should be:
select
async_tasks.action_type,
async_tasks.task_id,
....
from
async_tasks,
storage_pool
where
...
Line 178: from async_tasks a, storage_pool b
Line 179: where a.storage_pool_id = b.id
Line 180: """,
Line 181: ownConnection=True,
Line 181: ownConnection=True,
Line 182: transaction=False,
Line 183: )[0]
Line 184:
Line 185: from async_tasks_map import async_tasks_map
what is this module?
Line 186: return '\n'.join(
Line 187: [
Line 188: '\n---- Task ID: {task_id:30} ------- \n'
Line 189: 'Task Name: {task_name:30}\n'
Line 184:
Line 185: from async_tasks_map import async_tasks_map
Line 186: return '\n'.join(
Line 187: [
Line 188: '\n---- Task ID: {task_id:30} ------- \n'
If you join by '\n' do not use it within... I think you are expecting
'\n\n'.join(...)
Line 189: 'Task Name: {task_name:30}\n'
Line 190: 'Task Description: {task_desc:30}\n'
Line 191: 'Started at: {started_at:30}\n'
Line 192: 'DC Name: {name:30}\n'.format(
Line 204:
Line 205: compensations = dbstatement.execute(
Line 206: statement="""
Line 207: select
Line 208: command_type, entity_type
either:
select command_type, entity_type
or:
select
command_type,
entity_type
Line 209: from business_entity_snapshot
Line 210: """,
Line 211: ownConnection=True,
Line 212: transaction=False,
Line 218: for entry in compensations
Line 219: ]
Line 220: )
Line 221:
Line 222: def _stopTasks(self, runningTasks, compensations):
Please rename: _askUserTo...
Line 223: now = datetime.datetime.now()
Line 224: timestamp = now.strftime('%b %d %H:%M:%S')
Line 225: return self.dialog.queryBoolean(
Line 226: dialog=self.dialog,
Line 225: return self.dialog.queryBoolean(
Line 226: dialog=self.dialog,
Line 227: name='OVESETUP_SHOW_RUNNING_TASKS',
Line 228: note=_(
Line 229: '[ {timestamp} ] The following system tasks have been
'
response to you... the user sees the progress as he keep see new messages, not
sure why timestamp is needed.
Line 230: 'found running in the system:\n'
Line 231: '{tasks}\n'
Line 232: '{compensations}\n'
Line 233: 'Would you like to proceed '
Line 264: if runningTasks or compensations:
Line 265:
Line 266: if not self._stopTasks(runningTasks, compensations):
Line 267: raise RuntimeError(
Line 268: _('User decided not to stop running tasks.
Exiting.')
Aborted by user?
Line 269: )
Line 270:
Line 271: self.dialog.note(
Line 272: text=self._infoStoppingTasks()
Line 276: dbstatement=dbstatement,
Line 277: parent=self,
Line 278: ):
Line 279: # Pull tasks in a loop for some time
Line 280: # _CLEANUP_WAIT_SECONDS = 180 (seconds, between
trials)
please drop such comments, as in next cycle these will be incorrect.
Line 281: waited = 0
Line 282: while runningTasks or compensations:
Line 283: time.sleep(1)
Line 284: waited += 1
Line 291: )
Line 292:
Line 293: # If not waited complete cycle, go for next
round
Line 294: if waited < self._CLEANUP_WAIT_SECONDS:
Line 295: continue
come on! we optimize 3 seconds wait?!?! just wait 3 seconds, don't loop.
anyhow, I stll don't understand this loop, please split it into two (or more),
something like:
complete = False
while not complete:
wait = max
while not complete and wait > 0:
runningTasks =
compensations =
complete = (runningTasks or compensations):
if not complete:
sleep(1)
wait--
if not complete:
self._stopTasks(runningTasks, compensations)
note
Line 296:
Line 297: # Should we continue?
Line 298: if not self._stopTasks(runningTasks,
compensations):
Line 299: # If not, break the loop
Line 294: if waited < self._CLEANUP_WAIT_SECONDS:
Line 295: continue
Line 296:
Line 297: # Should we continue?
Line 298: if not self._stopTasks(runningTasks,
compensations):
Why not have the stopTasks raise exception? both usages throw exception if
false.
Line 299: # If not, break the loop
Line 300: # There are still tasks running, so exit
and tell
Line 301: # to resolve before user continues.
Line 302: raise RuntimeError(
Line 326: stage=plugin.Stages.STAGE_INIT,
Line 327: )
Line 328: def _init(self):
Line 329: self.environment.setdefault(
Line 330: osetupcons.DBEnv.IGNORE_TASKS,
Use application terms: CLEAN_TASKS = True
Line 331: False
Line 332: )
Line 333:
Line 334: @plugin.event(
Line 339: osetupcons.DBEnv.IGNORE_TASKS
Line 340: ] == False
Line 341:
Line 342: @plugin.event(
Line 343: stage=plugin.Stages.STAGE_VALIDATION,
shouldn't this be at early misc?
Line 344: after=[
Line 345: osetupcons.Stages.FIX_DB_VIOLATIONS
Line 346: ],
Line 347: #before yum update
Line 341:
Line 342: @plugin.event(
Line 343: stage=plugin.Stages.STAGE_VALIDATION,
Line 344: after=[
Line 345: osetupcons.Stages.FIX_DB_VIOLATIONS
do not use after if not at the same phase.
Line 346: ],
Line 347: #before yum update
Line 348: condition=lambda self: self._enabled,
Line 349: )
Line 343: stage=plugin.Stages.STAGE_VALIDATION,
Line 344: after=[
Line 345: osetupcons.Stages.FIX_DB_VIOLATIONS
Line 346: ],
Line 347: #before yum update
do not comment here...
def __validation(self):
# NOTE:
# this must run before package update
# as the implementation use java serialization
# which cannot be used by different implementations.
Line 348: condition=lambda self: self._enabled,
Line 349: )
Line 350: def _validation(self):
Line 351:
....................................................
File packaging/setup/plugins/ovirt-engine-setup/upgrade/dbvalidations.py
Line 74:
Line 75: rc, stdout, stderr = self._dbUtil()
Line 76: if rc != 0:
Line 77: raise Exception(
Line 78: 'Error: failed checking DB:\n{output}\n'.format(
please split by \n so it will be readable.
please add gettext
Line 79: output=stdout,
Line 80: )
Line 81: )
Line 82:
Line 79: output=stdout,
Line 80: )
Line 81: )
Line 82:
Line 83: result = (
?
result = '{content}'.format(
...,
...,
)
But how does it work? why do you need the dbname?
Line 84: '{content}'
Line 85: ).format(
Line 86: dbname=dbname,
Line 87: content=stdout,
Line 123: if issues_found:
Line 124: if self.environment[
Line 125: osetupcons.DBEnv.FIX_DB_VIOLATIONS
Line 126: ] is None:
Line 127: self.dialog.note(
please set this as warning... so it will be eye catcher.
Line 128: text=_(
Line 129: 'The following inconsistencies were found '
Line 130: 'in the DB: {violations}. '
Line 131: ).format(
--
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: 4
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
