Bug #7707
closeduri is undefined on upgrade tests
0%
Description
Occasionally, the upgrade tests would fail with a python syntax error with uri being undefined.
Updated by Anonymous about 10 years ago
The uri variable is sometimes undefined on upgrade tests. This may be due to things not being set perfectly in yaml files, but having an undefined value should probably be avoided.
In _get_baseurlinfo_and_dist in tasks/install.py, the uri setting code looks like:
if tag: uri = 'ref/' + tag elif branch: uri = 'ref/' + branch elif sha1: uri = 'sha1/' + sha1 else: # FIXME: Should master be the default? log.debug("defaulting to master branch") uri = 'ref/master'
In _upgrade_deb packages, code looks like:
if 'sha1' in config: uri = 'sha1/' + config.get('sha1') elif 'branch' in config: uri = 'ref/' + config.get('branch') elif 'tag' in config: uri = 'ref/' + config.get('tag') ...that's it...
So there are times when uri is not defined.
Even though the _get_baseurlinfo_and_dist indicates that the code may not be completely kosher, I suspect that we should probably do something similar to _upgrade_deb_packages as well. It's probably better unitialized variable errors.
Updated by Anonymous about 10 years ago
The uri variable is sometimes undefined on upgrade tests. This may be due to things not being set perfectly in yaml files, but having an undefined value should probably be avoided.
In _get_baseurlinfo_and_dist in tasks/install.py, the uri setting code looks like:
if tag: uri = 'ref/' + tag elif branch: uri = 'ref/' + branch elif sha1: uri = 'sha1/' + sha1 else: # FIXME: Should master be the default? log.debug("defaulting to master branch") uri = 'ref/master'
In _upgrade_deb packages, code looks like:
if 'sha1' in config: uri = 'sha1/' + config.get('sha1') elif 'branch' in config: uri = 'ref/' + config.get('branch') elif 'tag' in config: uri = 'ref/' + config.get('tag') ...that's it...
So there are times when uri is not defined.
Even though the _get_baseurlinfo_and_dist comment indicates that the code may not be completely kosher, I suspect that we should probably do something similar to _upgrade_deb_packages as well. It's probably better unitialized variable errors.
Updated by Anonymous about 10 years ago
If master is not right, we should probably provide better error messages here at the least before letting the exception get thrown.
Updated by Anonymous about 10 years ago
Did I say syntax error in the initial description? I meant to say NameError.
Updated by Anonymous about 10 years ago
This has been submitted for review:
Pull request: https://github.com/ceph/teuthology/pull/224
Wip branch: wip-7707-wusui
Updated by Anonymous about 10 years ago
Does anyone know why the section of code that initializes uri values in _get_baseurlinfo_and_dist in task/install.py looks like:
if tag: uri = 'ref/' + tag elif branch: uri = 'ref/' + branch elif sha1: uri = 'sha1/' + sha1
and later in install.py in _upgrade_deb_packages the code looks like:
if 'sha1' in config: uri = 'sha1/' + config.get('sha1') elif 'branch' in config: uri = 'ref/' + config.get('branch') elif 'tag' in config: uri = 'ref/' + config.get('tag')
The order of the if statements is what's important here: If tag and sha1 are both set, in the first example, uri would be set to tag. In the second example, it would be set to sha1.
Updated by Anonymous almost 10 years ago
uri being undefined on upgrade tests is induced by not specifying a sha1, branch, or tag when running upgrades. The following yaml snippet demonstrates the bug:
tasks: - install.upgrade: all: targets:
Note that this does not make sense. If an upgrade is performed, the version to be upgraded to should be included. So this is probably an error, but a more meaningful error message than 'uri referenced before assignment' should be returned.
The consolidation of code into a separate routine (discussed in
previous updates to this bug) probably should still be done.
Updated by Sage Weil almost 10 years ago
Warren Usui wrote:
uri being undefined on upgrade tests is induced by not specifying a sha1, branch, or tag when running upgrades. The following yaml snippet demonstrates the bug:
[...]
Note that this does not make sense. If an upgrade is performed, the version to be upgraded to should be included. So this is probably an error, but a more meaningful error message than 'uri referenced before assignment' should be returned.
The consolidation of code into a separate routine (discussed in
previous updates to this bug) probably should still be done.
I think this is intentional. That is, the target is to upgrade all: do whatever the default is that is specified in the overrides: section (or, I guess, master, although this should never happen when scheduled via schedule_suite.sh).
Updated by Anonymous almost 10 years ago
So in the example that I just gave, defaulting to master is what we want? If so, then the previously proposed fix that made the install.upgrade code behave the same as the install code is probably correct.
Updated by Anonymous almost 10 years ago
- Status changed from In Progress to Resolved
Change checked in to master:
SHA1 -- c0ba105453bde0b3fc5b6f55d5366cf473430dd8