task/install: Fix AssertionError #1942
Conversation
teuthology/task/install/__init__.py
Outdated
| @@ -571,7 +571,7 @@ def task(ctx, config): | |||
| install_overrides = overrides.get('install', {}) | |||
| log.debug('INSTALL overrides: %s' % install_overrides) | |||
| teuthology.deep_merge(config, install_overrides.get(project, {})) | |||
| teuthology.deep_merge(extra_system_packages, install_overrides.get('extra_system_packages', {})) | |||
| teuthology.deep_merge(extra_system_packages, install_overrides.get('extra_system_packages', [])) | |||
There was a problem hiding this comment.
I'm not sure this is going to work in all cases.
extra_system_packages is weird -- it can be either a list or a dictionary (with deb and rpm lists in it). So, if a dictionary is supplied in the config (i.e. extra_system_packages variable at this point contains a dictionary instead of an empty list), deep_merge() would refuse to merge it with an empty list throwing a similar assert.
There was a problem hiding this comment.
hmm, this stinks. So if it's a list then the packages should be appended to both deb and rpm? I suppose that's what teuthology should do?
There was a problem hiding this comment.
If it is a list, install task just passes the list verbatim to the package installation command. If it is a dictionary, install task fetches one of the lists from the dictionary and passes that. The list is never preprocessed/expanded into a dictionary, if that is what you are asking -- install task is prepared to deal with both a list and a dictionary:
teuthology/teuthology/task/install/deb.py
Lines 71 to 74 in a4b16c5
teuthology/teuthology/task/install/rpm.py
Lines 185 to 190 in a4b16c5
There was a problem hiding this comment.
@idryomov, do you think this would cover all cases?
| teuthology.deep_merge(extra_system_packages, install_overrides.get('extra_system_packages', [])) | |
| teuthology.deep_merge(extra_system_packages, install_overrides.get('extra_system_packages', type(extra_system_packages)())) |
Even if extra_system_packages config is None, deep_merge() function handles the case if b is None.
There was a problem hiding this comment.
As to what the override behavior should be, it's a choice that we need to make. I see at least three options:
- Require that the form is the same for the original and the override and otherwise treat the job as malformed (i.e. check that both are lists or both are dictionaries before merging).
- Make the override win, both in form and content (i.e. don't attempt to merge anything).
- Introduce a preprocessing step and transform both the original and the override into a canonical dictionary first, then merge and hand over the resulting dictionary to
installtask.
There was a problem hiding this comment.
@idryomov, do you think this would cover all cases?
No, I think it would blow up in case the job specifies e.g. a list in the original and a dictionary in the override (i.e. when the defaults don't kick in at all).
This is actually option 1 from a previous comment. If folks think it's good enough, I'm all for it ;)
There was a problem hiding this comment.
Yes, I think that works but perhaps even better would be:
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)
It is more verbose but the type(...) solution would look baffling to someone unfamiliar with this problem.
There was a problem hiding this comment.
I also think a try/except with a helpful error message would be good. An error from deep_merge does not necessarily indicate an error in the job config to a novice.
There was a problem hiding this comment.
Oh right, those are 3 great option! I think option 3 adds some additional flexibility whereas option 1 would throw an error and ask the person editing the config to do the same thing teuthology could (make both config into same type format). What do you think?
|
Intermediate fix #1943 is done to get QA runs green again. |
Fixes: https://tracker.ceph.com/issues/66093 https://tracker.ceph.com/issues/66133 Signed-off-by: Vallari Agrawal <[email protected]>
VallariAg
force-pushed
the
fix-install-task
branch
from
bbbf9e9
to
89d7af0
Compare
|
Went with option 1 (with proper try/except message) because existing code reads Also fixes: https://tracker.ceph.com/issues/66133 Tested with diff config:
|
| 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) |
There was a problem hiding this comment.
| 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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The result of the deep merge should be stored in
configbecause later we do thisextra_system_packages=config.get('extra_system_packages', []),
The dictionary referenced by extra_system_packages should be the same as config['extra_system_packages'], no?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it'll only be the same if
config.get('extra_system_packages')is a list or a dictionary. If it'sNone(case:extra_system_packagesnot mentioned in install task definition and only in overrides), thenextra_system_packageswouldn't reference it.
Ah, of course.
[1] and [2] added support for applying extra_system_packages overrides directly on install task, but at the same time broke our long standing workaround where we sneaked extra_system_packages directive in through an override on ceph task. This is likely getting addressed in [3], but it's better to not rely on this odd feature in the first place. [1] ceph/teuthology#1941 [2] ceph/teuthology#1943 [3] ceph/teuthology#1942 Fixes: https://tracker.ceph.com/issues/66232 Signed-off-by: Ilya Dryomov <[email protected]>
| 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) |
There was a problem hiding this comment.
I think it'll only be the same if
config.get('extra_system_packages')is a list or a dictionary. If it'sNone(case:extra_system_packagesnot mentioned in install task definition and only in overrides), thenextra_system_packageswouldn't reference it.
Ah, of course.
[1] and [2] added support for applying extra_system_packages overrides directly on install task, but at the same time broke our long standing workaround where we sneaked extra_system_packages directive in through an override on ceph task. This is likely getting addressed in [3], but it's better to not rely on this odd feature in the first place. [1] ceph/teuthology#1941 [2] ceph/teuthology#1943 [3] ceph/teuthology#1942 Fixes: https://tracker.ceph.com/issues/66232 Signed-off-by: Ilya Dryomov <[email protected]> (cherry picked from commit c61cb16)
[1] and [2] added support for applying extra_system_packages overrides directly on install task, but at the same time broke our long standing workaround where we sneaked extra_system_packages directive in through an override on ceph task. This is likely getting addressed in [3], but it's better to not rely on this odd feature in the first place. [1] ceph/teuthology#1941 [2] ceph/teuthology#1943 [3] ceph/teuthology#1942 Fixes: https://tracker.ceph.com/issues/66232 Signed-off-by: Ilya Dryomov <[email protected]> (cherry picked from commit c61cb16)
[1] and [2] added support for applying extra_system_packages overrides directly on install task, but at the same time broke our long standing workaround where we sneaked extra_system_packages directive in through an override on ceph task. This is likely getting addressed in [3], but it's better to not rely on this odd feature in the first place. [1] ceph/teuthology#1941 [2] ceph/teuthology#1943 [3] ceph/teuthology#1942 Fixes: https://tracker.ceph.com/issues/66232 Signed-off-by: Ilya Dryomov <[email protected]> (cherry picked from commit c61cb16)
Fixes: https://tracker.ceph.com/issues/66093
Before: https://pulpito.ceph.com/vallariag-2024-05-17_08:56:03-rbd:nvmeof-main-distro-default-smithi/
After: https://pulpito.ceph.com/vallariag-2024-05-17_10:54:43-rbd:nvmeof-main-distro-default-smithi/