Skip to content
This repository has been archived by the owner on Aug 26, 2024. It is now read-only.

Fix the container to contain predictable non-overlapping dependencies from different ecosystems #18

Closed
webknjaz opened this issue Jan 19, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@webknjaz
Copy link
Member

requirements.txt in this repository requests cryptography==36.0.1 managed by pip but requirements.yml pulls in openstack.cloud which in turn has python38-cryptography package managed by dnf. Both installers fight for the control over the system-wide site-packages folder.
If we look into the log at https://github.com/ansible/creator-ee/pull/12/checks#step:6:1013 and earlier, we'll notice this pattern:

  1. dnf installs cryptography v2.8 (as needed by the openstack.cloud collection)

      Installing       : python38-cryptography-2.8-3.module_el8.5.0+742+dba   38/45 
  2. pip installs cryptography v36.0.1 as requested by this project's direct requirements in the exact same location on disk:

      Attempting uninstall: cryptography
        Found existing installation: cryptography 2.8
        Uninstalling cryptography-2.8:
          Successfully uninstalled cryptography-2.8
    WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: pip.pypa.io/warnings/venv

    (dnf keeps thinking that v2.8 is still installed and pip rightfully warns us that something sketchy is happening)

  3. The build process goes on and attempts to install some more packages via dnf. Some of them are installed already (at least that's what dnf thinks):

    Package python3-rpm-4.14.3-19.el8.x86_64 is already installed.
    Package python38-cryptography-2.8-3.module_el8.5.0+742+dbad1979.x86_64 is already installed.
    Package python38-pytz-2019.3-3.module_el8.5.0+742+dbad1979.noarch is already installed.
    Package python38-pyyaml-5.4.1-1.module_el8.5.0+758+421c7348.x86_64 is already installed.
    Package python38-requests-2.22.0-9.module_el8.5.0+742+dbad1979.noarch is already installed.

    The truth is that the statement about python38-cryptography-2.8 is a lie. A package with this name exists on disk but its version is different.

While the software that we invoke may hot be crashing, there are some things in the system that may depend on those system-provided versions that are no longer in place. For example, if some Ansible collection requires the use of an older cryptography API, then we would only hit that case (potentially) in runtime. But there is no guarantee that it would crash the program somehow. That API may have different defaults which would only influence some of the protocol-level behavior — everything will appear to work, except for having a security issue, for example.
Another problem with this is that the OS-packaged software is usually expected to be dynamically linked against system (often cryptographically significant) libraries that can be updated separately when mitigating vulnerabilities. While the pip-installed wheels bundle those libraries by linking them statically. This means that there is another expectation that is broken.

It should be possible to fix this by using a virtualenv which is the best practice when it comes to isolating the application dependencies that are managed by pip from the interpreter-global installation.

@webknjaz webknjaz added the bug Something isn't working label Jan 19, 2022
@ssbarnea
Copy link
Member

How about installing using --user?

Also, I am not convinced that breakage will happen until I will see a real bug report, preferably for one crated just to demonstrate that it is possible to happen.

If we automate the rpm packaging we could switch to use of rpms only BUT where is the latest ansible-core rpm? How often is that updated? Keep in mind that creator-ee is upstream, so it is expected to be less stable than an official AAP execution environment, which will likely be build on RHEL and use only rpms.

So, even if it breaks, you could say that is "by design". I would prefer to do something about this when the need is real.

@webknjaz
Copy link
Member Author

How about installing using --user?

This would be better than it is now. But it'd still leave the room for breaking stuff. Especially as people start inheriting the image, requesting things of certain versions to be installed by dnf (via extra collection requirements, for example) and having dnf responding with "skipping, that's already installed". The more collections+other transitive dependencies get added, the more misleading metadata this generates. At some point, there will be dependencies that differ from what some of the collections expect. Sometimes those differences will be slight and undetectable (while still being important), other times they will cause problems at runtime.

Thinking about this more, I'm pretty sure that ansible-builder should make use of something like resolvelib to coordinate the requirements for pip+dnf+ansible-galaxy.

Also, I am not convinced that breakage will happen until I will see a real bug report, preferably for one crated just to demonstrate that it is possible to happen.

The original post demonstrates exactly that.

If we automate the rpm packaging we could switch to use of rpms only BUT where is the latest ansible-core rpm? How often is that updated? Keep in mind that creator-ee is upstream, so it is expected to be less stable than an official AAP execution environment, which will likely be build on RHEL and use only rpms.

So, even if it breaks, you could say that is "by design". I would prefer to do something about this when the need is real.

That's beside the point. We shouldn't produce broken environments, period.

@mattclay
Copy link
Member

There are at least two issues here:

  1. Using multiple non-cooperating package managers to install or upgrade arbitrary packages in the same location.
  2. Installing or upgrading packages from multiple package lists independently.

The first case deals with pip and dnf. PEP 668 provides insights on this issue. The safest approach is to install Python packages exclusively with one package manager. Use dnf or a virtual environment with pip. Avoid combining both if possible, as that requires the --system-site-packages option, which can be problematic.

The second case deals with conflicts that occur when requirements are defined by multiple independent projects in the same Python environment. Use separate virtual environments for each project when feasible. Otherwise, make sure all Python requirements in a single environment are evaluated together to detect conflicts.

@ssbarnea
Copy link
Member

@mattclay In the absence of an automatic process for producing rpms for all our dependencies for the current base distribution using pip will continue to prefered. I am personally against using virtualenvs inside containers, for multiple reasons and one of them being that we want to use at least some system packages. Slowly we could change the way we use pip to force it to skip installing dependencies and having us manually install them using dnf. At the end we can check for presence of conflicts and fail the build if we find any.

If done right the only packages installed with pip will be out own ones and in the longer term, we could also start installing these as RPMs once we have an rpm repository for our ansible tools. Still considering that even ansible-core does not publish an rpm for centos-stream automatically on new releases I am inclined to believe we will not see this soon and continue to install using pip.

AFAIK, the purpose of creator-ee is to ensure we test the bleeding edge and fail-fast when needed. That project should not be seen a a production level execution environment, which would have a different update and testing policy, where I fully support a pure rpm approach.

Another reason for keeping use of pip is the fact that dependabot does not support updating system packages. So, we cannot really produce a pinned list of dependencies and automate testing of updates when newer versions appear.

With current setup, [](https://github.com/ansible/creator-ee/blob/main/_build/requirements.txt) acts as built manifest and when a new dependency makes a new release we get a pull-request to update it, one that can assert if the change has a negative impact or not.

The reality is that I expect that lost of users of creator-ee will wait for inclusion of very receiving bugfixes in those deps so they can test the impact on their work. That means that we need to be sure we can update this container as fast as possible, likely even automating its release (one we have some extra tests).

@ssbarnea
Copy link
Member

@webknjaz Being back at your original problem related to cryptography. We can change the way we install with pip to something like pip install --no-deps -r requirements.in -c requirements.txt && pip check.

That should install only the top level deps from pip and fail if we miss real dependencies from system or if the system ones are outdated.

We will need to include a short list of deps that are not packaged as rpms into requirements.in, at least until we get them as rpms.

@webknjaz
Copy link
Member Author

@ssbarnea I think that having a venv with system site-packages is a good start but pip check will still not take into account a lot of requested installs. It could be a good start, though.

FWIW I don't mean that we should be solving all of these problems in this repository. It's probably best to solve this on the ansible-builder level, as it seems. I just wanted it documented and exposed here first. cryptography is only one example of this problem. I think @cidrblock knows an all-in-one container trying to put all of the collections in a EE that struggles with installing all of the deps properly.

For now, let's do your suggestion here but also track this problem with ansible-builder, okay? But let's keep this issue open since I don't think there's a solution until there's a more concrete dependency resolution coordination suggestion.

@ziegenberg
Copy link
Contributor

I think with the latest changes, this looks like it's solved now. Or am I wrong?

@ssbarnea
Copy link
Member

Sadly the situation is only improved and not fixed. We will need some changes in ansible-builder in order to fix it, like an option to pass --no-deps to all pip install commands, so we avoid installing or upgrading dependencies that come from rpms.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants