Fixes and improvements for package installation during image builds #77
Conversation
|
Wow, that's a lot of patches thank you. I'll let PR CI run and then review it if there is no major issue. |
|
Hmm, PR CI errors are not related to this pull request. It seems that some update started to cause issues when commiting to containers in the github runners. I need to look into it first. |
|
Hopefully, I fixed all PR CI issues, can you please rebase this pull request? |
|
Also, I am curious, what is your motivation behind this pull request? How are you using this project? Thank you. |
The task for passkey testing was intended to go into a different block instead (see Fedora).
Use separate tasks to install the IPA client, SSSD, and test dependencies. Ensure packages are listed under the correct task.
Remove duplicate entries in the list of packages to install. Use the same package name for pycodestyle in Debian and Ubuntu. Correct the task name.
The bullseye-backports repository is only for Debian 11. Use Ansible's apt_repository module to configure it.
The cache ideally needs to be updated just once; otherwise the same metadata is downloaded repeatedly during the build.
While the contents of a buildroot may not be signed, any packages that are installed from the distribution's base repositories should always be.
extended_packageset already has default values in the inventory, and in the playbook for VMs.
The core DNF plugins are not used by any CentOS-specific tasks. The tasks for Fedora already install these plugins before they are needed.
…RHEL Only configure additional repositories in the ground base image. The EPEL repository should only be added if extended_packageset is set to "yes". Adjust the task names.
The base-ground container does not run systemd and will not respond to a signal. Closing stdin will cause it to stop instead.
This is in preparation for the next change, which needs to identify the package manager used before the containers are created. A single command is now used to find Python 3 in the running container or to install it otherwise. On Debian/Ubuntu, the package manager will no longer attempt to display interactive prompts when installing it.
This avoids the need to individually re-download or clean up the cached repository metadata for each image that is built.
These containers are useful in helping others reproduce specific issues we observe in SSSD, in a controlled manner. This pull request fixes inconsistencies that make it difficult to rebuild the images as necessary, in order to accomplish that. We are concerned with multiple versions of RHEL and Ubuntu primarily. The comments here may provide an example of why this is useful: |
There was a problem hiding this comment.
Removal of the groups without base_ prefix from when clauses will break downstream automation that depends on the simple names.
We are not using two inventories for installing packages and configuring services but a single one that has only the names like nfs, client...
We are installing and configuring VMs so this split to layers does not make any sense there and makes the configuration needlessly complex and prone to issues.
| @@ -149,15 +134,18 @@ | |||
| name: | |||
| - acl | |||
| - 389-ds-base | |||
| when: "'base_ldap' in group_names or 'ldap' in group_names or 'ipa' in group_names" | |||
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just ldap or ipa.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
There was a problem hiding this comment.
I am probably missing something, why would it be run twice and slow down the build?
When containers are built the phases for base with packages base_xxx are separate from the services so packages role is not executed again in services phase.
The inventory for VMs is already huge and complicated and we do not want it to be inflated by the extra groups like base_XXX.
I would rather see the whole base_XXX deal dropped and used just roles like ipa, ldap ... instead.
| name: | ||
| - nfs-kernel-server | ||
| when: "'base_nfs' in group_names or 'nfs' in group_names" |
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just nfs.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| name: | ||
| - krb5-admin-server | ||
| - krb5-config | ||
| - krb5-kdc | ||
| when: "'base_kdc' in group_names or 'kdc' in group_names" |
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just kdc.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| - tcpdump | ||
| - wireshark-cli | ||
| disable_gpg_check: yes | ||
| when: "'base_client' in group_names or 'client' in group_names" |
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just client.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| - augeas-tools | ||
| when: "'base_client' in group_names or 'client' in group_names" |
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just client.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| @@ -167,7 +155,7 @@ | |||
| name: | |||
| - samba-dc | |||
| - samba-winbind-clients | |||
| when: "'base_samba' in group_names or 'samba' in group_names" | |||
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just samba.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| @@ -176,7 +164,7 @@ | |||
| state: present | |||
| name: | |||
| - nfs-utils | |||
| when: "'base_nfs' in group_names or 'nfs' in group_names" | |||
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just nfs.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| @@ -187,7 +175,7 @@ | |||
| - krb5-libs | |||
| - krb5-server | |||
| - krb5-workstation | |||
| when: "'base_kdc' in group_names or 'kdc' in group_names" | |||
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just kdc.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
| @@ -198,7 +186,7 @@ | |||
| - ci-sssd-random | |||
| - umockdev | |||
| when: passkey_support | |||
| when: "'base_client' in group_names or 'client' in group_names or 'base_ipa' in group_names or 'ipa' in group_names" | |||
There was a problem hiding this comment.
This and similar ones are unwanted change, this is there specifically for use in other ci where this is applied to VMs instead of containers and mixing in the base_xxx and xxx.
we got inventory that has group just xxx.
There was a problem hiding this comment.
Let's change the other CI for the VMs, please, so that the VM host is in both the base_xxx and xxx groups. We do not need this task to run twice for the containers; it slows down the build.
|
|
||
| - name: Select packageset |
There was a problem hiding this comment.
For testing we try to reduce the set of installed packages this is needed for
"Install extended set of packages"
There was a problem hiding this comment.
No, it is explained in the commit message 551db92 that this line actually has no effect.
| loop_var: include_file | ||
| with_first_found: | ||
| - files: '{{ ansible_distribution | distro_includes(ansible_distribution_major_version) }}' | ||
| - name: 'Include distribution specific tasks [{{ ansible_distribution }}]' |
There was a problem hiding this comment.
Please, keep the plugin. It may not be needed now, but it most certainly will be needed in the future.
| @@ -156,3 +172,5 @@ compose down | |||
| # Create development images with additional packages | |||
| build_base_image "ci-client:${TAG}" client-devel | |||
| build_base_image "ci-ipa:${TAG}" ipa-devel | |||
|
|
|||
| rm --recursive --force "$SHARED_CACHE_ROOT" | |||
|
Hi @dpward It has been a while since we left a few comments in this PR, will you be able to review those? If there is anything we can do to help, please let us know. Kind regards |
These changes make image building more consistent between different distributions, by fixing discrepancies and re-using task lists where appropriate. (The task lists for RHEL now include those for CentOS; and the task lists for Ubuntu now include those for Debian.) This is intended to simply maintenance and allow for easier testing with other base images.
This also improves the build speed. Temporary volumes are now created and mounted at the paths where repository metadata is stored, so that it does not need to be re-downloaded for each image that is built (ipa, ldap, etc.) For Debian/Ubuntu, this metadata is now re-used across different package management operations for the same image.
Builds were performed successfully from these base images: