apt: include arch in check for installed packages on multi-arch systems #24846
Conversation
ansibot
added
affects_2.4
ansibot
added
needs_revision
| @@ -140,6 +140,40 @@ | |||
| - name: uninstall hello with apt | |||
| apt: pkg=hello state=absent purge=yes | |||
|
|
|||
| # verify that apt is handling multi-arch systems properly | |||
| - name: add second architecture | |||
There was a problem hiding this comment.
You need to check that {{ apt_foreign_arch }} isn't the default architecture.
| apt: deb="/var/cache/apt/archives/hello_{{ hello_version.stdout }}_{{ apt_foreign_arch }}.deb" | ||
| register: apt_multi_secondary | ||
|
|
||
| - name: verify installation of hello:{{ apt_foreign_arch }} |
There was a problem hiding this comment.
You could check results of previous tasks install hello and uninstall hello too.
| apt: pkg={{ item }} state=absent purge=yes | ||
| with_lines: dpkg-query --showformat='${Package}:${Architecture}\n' --show '*:{{ apt_foreign_arch }}' | ||
|
|
||
| - name: remove second architecture |
There was a problem hiding this comment.
The cleaning tasks need to be executed on an always block.
There was a problem hiding this comment.
why? the tests are marked as destructive, and having the second arch enabled does not really hurt. on the other side blocks make playbooks less readable.
| @@ -140,6 +140,40 @@ | |||
| - name: uninstall hello with apt | |||
| apt: pkg=hello state=absent purge=yes | |||
|
|
|||
| # verify that apt is handling multi-arch systems properly | |||
There was a problem hiding this comment.
In order to improve readability, test tasks related to multi-arch could be moved in a dedicated YAML file and used here using an include task.
gundalow
added
the
ci_verified
|
Number of failures in the integration tests |
|
|
||
| - name: uninstall all other {{ apt_foreign_arch }} packages that were installed as dependencies | ||
| apt: pkg={{ item }} state=absent purge=yes | ||
| with_lines: dpkg-query --showformat='${Package}:${Architecture}\n' --show '*:{{ apt_foreign_arch }}' |
There was a problem hiding this comment.
This task fails due to #23601 when the control machine isn't using Debian (or a derivatives).
Besides, when Debian is used on the control machine, following error appears:
TASK [apt : remove second architecture] *********************************************************************************************************************
fatal: [testhost]: FAILED! => {"changed": true, "cmd": ["dpkg", "--remove-architecture", "i386"], "delta": "0:00:00.015897", "end": "2017-05-21 21:55:06.487061", "failed": true, "rc": 2, "start": "2017-05-21 21:55:06.471164", "stderr": "dpkg: error: cannot remove architecture 'i386' currently in use by the database", "stderr_lines": ["dpkg: error: cannot remove architecture 'i386' currently in use by the database"], "stdout": "", "stdout_lines": []}
You could use instead something like:
- name: Remove packages with foreign architecture
apt:
name: "*:{{ apt_foreign_arch }}"
state: absent
purge: yes
There was a problem hiding this comment.
ack, did that (not pushed yet), works fine, thanks
Thanks: Stefan Löwen <stefan.loewen@gmail.com> Fixes: ansible#24673
ansibot
removed
the
ci_verified
|
ready_for_review |
There was a problem hiding this comment.
Hi @evgeni ; I think this is fine as a small fix and reasonable to go forward. You could close this as is.
Talking generally (not just this specific patch) the module should have a bit of work done on it;
- the module currently calls dpkg three times for every single .deb it queries - this could be replaced with one call querying
- the dpg call should return a simple data structure which can be queried (like the cache??) which could then be mocked and tested easily.
- it would be good to have more coverage in the test cases for package upgrades [which are currently missing from the tests (https://codecov.io/gh/ansible/ansible/src/e38c29e71907e273b14b55c60e2899254949a2b2/lib/ansible/modules/packaging/os/apt.py#L660)
If I was refactoring, I think I would pull out the following functions which would be easy to unit test:
def read_debs(list_of_files):
# blah blah
available_packages = (# equivalent of
for i in *;
do dpkg-deb --showformat '${PACKAGE} ${ARCHITECTURE} ${VERSION}\n' --show $i ;
done )
return
def identify_changes(installed_packages, available_packages):
# blah blah
return [list of packages where available is better]
And I'd probably pull out a Package object for both installed and available packages which would have
- package.version()
- package.architecture()
- package.name()
which you can do a comparison on but would could run package.path() for the installable debs so you could easily build the list of packages to install
I met with @gundalow yesterday and we were discussing this. If you want to take it these you could do it together with the rest of the patch. If not then one of us will bring this up at the core team meeting and hopefully someone else would look into it.
| try: | ||
| installed_pkg = apt.Cache()[pkg_name] | ||
| installed_pkg = apt.Cache()[pkg_key] |
There was a problem hiding this comment.
Elsewhere in this module the cache which is returned from get_cache(module) is always carefully used. In this case there is a direct call to apt.Cache(). Do you know why there is this discrepancy and is it safe? If possible it would be preferable to use the cache object which is already passed into this function as an argument.
| @@ -551,8 +551,13 @@ def install_deb(m, debs, cache, force, install_recommends, allow_unauthenticated | |||
| pkg = apt.debfile.DebPackage(deb_file) | |||
There was a problem hiding this comment.
This is a huge try/except block which makes debugging what's going on quite difficult and can mask nasty errors. This and other prolbems with this code are from before the current PR, however it would be really good if this situation could be improved.
|
HI @mikedlr, I totally agree that the module could need some good love, refactoring and tests. I tried to be as non-invasive as possible in my patch, as I did not want to take the refactoring route just now and I have no clue why some parts (like the Cache you mention) are written like they are today. I would favor we would fix that bug (which it is) now, and then decide on a separate plan how to get the module in a nicer shape. Happy to help with the further tasks, but probably not as the main driver (time constraints etc). |
|
Okay, then please don't merge the unit test I made right now. It will be brittle under the refactoring. Whoever does the refactoring should either merge it or add new separate unit tests. |
|
+1 |
|
OK, backed out your commit. |
bcoca
removed
the
needs_triage
ansibot
added
bug
SUMMARY
See #24673 and #24703.
On systems that have multi-arch enabled, you need to lookup packages via
package:archfromapt.Cache(). Newer versions ofpython-aptalso accept this notation when multi-arch is not enabled, but the version shipped in Debian Jessie and Ubuntu 14.04 does not.Thanks: Stefan Löwen stefan.loewen@gmail.com
Fixes: #24673
ISSUE TYPE
COMPONENT NAME
apt
ANSIBLE VERSION