Tools » teuthology

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.

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.

If master is not right, we should probably provide better error messages here at the least before letting the exception get thrown.

Did I say syntax error in the initial description? I meant to say NameError.

This has been submitted for review:

Pull request: https://github.com/ceph/teuthology/pull/224
Wip branch: wip-7707-wusui

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.

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.

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).

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.