Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

task/install: Fix AssertionError #1942

Merged
merged 1 commit into from
May 28, 2024
Merged

task/install: Fix AssertionError #1942

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions teuthology/task/install/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from teuthology import contextutil, packaging
from teuthology.parallel import parallel
from teuthology.task import ansible
from teuthology.exceptions import ConfigError

from distutils.version import LooseVersion
from teuthology.task.install.util import (
Expand Down Expand Up @@ -565,16 +566,24 @@ def task(ctx, config):
log.debug('project %s' % project)
overrides = ctx.config.get('overrides')
repos = None
extra_system_packages = config.get('extra_system_packages', [])

if overrides:
install_overrides = overrides.get('install', {})
log.debug('INSTALL overrides: %s' % install_overrides)
teuthology.deep_merge(config, install_overrides.get(project, {}))
overrides_extra_system_packages = install_overrides.get('extra_system_packages')
if overrides_extra_system_packages:
teuthology.deep_merge(extra_system_packages, overrides_extra_system_packages)
repos = install_overrides.get('repos', None)
try:
install_overrides = overrides.get('install', {})
log.debug('INSTALL overrides: %s' % install_overrides)
teuthology.deep_merge(config, install_overrides.get(project, {}))
overrides_extra_system_packages = install_overrides.get('extra_system_packages')
if overrides_extra_system_packages:
extra_system_packages = config.get('extra_system_packages')
config['extra_system_packages'] = teuthology.deep_merge(extra_system_packages, overrides_extra_system_packages)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config['extra_system_packages'] = teuthology.deep_merge(extra_system_packages, overrides_extra_system_packages)
teuthology.deep_merge(extra_system_packages, overrides_extra_system_packages)

? (This one confused me.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of the deep merge should be stored in config because later we do this extra_system_packages=config.get('extra_system_packages', []),

Using config.get('extra_system_packages') is better than directly using variable extra_system_packages because when we log whole config there would be no doubt what the actual config would look like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of the deep merge should be stored in config because later we do this extra_system_packages=config.get('extra_system_packages', []),

The dictionary referenced by extra_system_packages should be the same as config['extra_system_packages'], no?

Copy link
Member Author

@VallariAg VallariAg May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll only be the same if config.get('extra_system_packages') is a list or a dictionary. If it's None (case:extra_system_packages not mentioned in install task definition and only in overrides), then extra_system_packages wouldn't reference it.

I think the regression (https://tracker.ceph.com/issues/66133) happened because of the above case:
"python3-pytest" package was defined in overrides.install.ceph.extra_system_packages and there was no "extra_system_packages" in install task definition config. So the extra_system_packages variable was an empty list which didn't reference the config value.
Then teuthology.deep_merge(config, install_overrides.get(project, {})) added the package to config["extra_system_packages"] (but not in extra_system_packages variable). We could see the package when config was logged but since we were using extra_system_package variable to makeup the final config, the package never got added to final config and so it was not installed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll only be the same if config.get('extra_system_packages') is a list or a dictionary. If it's None (case:extra_system_packages not mentioned in install task definition and only in overrides), then extra_system_packages wouldn't reference it.

Ah, of course.

repos = install_overrides.get('repos', None)
except AssertionError:
raise ConfigError(
"'install' task config and its overrides contain" \
"conflicting types for the same config key. Ensure that " \
"the configuration is of the same type (dict, list, etc.) " \
"in both the task definition and its overrides."
)

log.debug('config %s' % config)

Expand Down Expand Up @@ -609,7 +618,7 @@ def task(ctx, config):
downgrade_packages=config.get('downgrade_packages', []),
exclude_packages=config.get('exclude_packages', []),
extra_packages=config.get('extra_packages', []),
extra_system_packages=extra_system_packages,
extra_system_packages=config.get('extra_system_packages', []),
extras=config.get('extras', None),
enable_coprs=config.get('enable_coprs', []),
flavor=flavor,
Expand Down
Loading