canonical / operator-libs-linux

Linux helper libraries for the operator framework
Apache License 2.0
11 stars 39 forks source link

Request: Option for apt lib to install not backports version package #113

Open jneo8 opened 10 months ago

jneo8 commented 10 months ago

freeipmi-tools failed to install on focal

Relate: https://github.com/canonical/hardware-observer-operator/issues/128

Reproduce

install.py

apt.add_package("freeipmi-tools", update_cache=False)
$ python install.py
Traceback (most recent call last):
  File "/home/ubuntu/operator-libs-linux/./install.py", line 3, in <module>
    apt.add_package("freeipmi-tools", update_cache=False)
  File "/home/ubuntu/operator-libs-linux/lib/charms/operator_libs_linux/v0/apt.py", line 761, in add_package
    pkg, success = _add(p, version, arch)
                   ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/operator-libs-linux/lib/charms/operator_libs_linux/v0/apt.py", line 802, in _add
    pkg.ensure(state=PackageState.Present)
  File "/home/ubuntu/operator-libs-linux/lib/charms/operator_libs_linux/v0/apt.py", line 289, in ensure
    self._add()
  File "/home/ubuntu/operator-libs-linux/lib/charms/operator_libs_linux/v0/apt.py", line 261, in _add
    self._apt(
  File "/home/ubuntu/operator-libs-linux/lib/charms/operator_libs_linux/v0/apt.py", line 255, in _apt
    raise PackageError(
lib.charms.operator_libs_linux.v0.apt.PackageError: Could not install package(s) [['freeipmi-tools=1.6.9-2~bpo20.04.1']]: None

The apt lib choose backports version to install instead of focal-updates

$ apt-cache policy freeipmi-tools
freeipmi-tools:
  Installed: (none)
  Candidate: 1.6.4-3ubuntu1.1
  Version table:
     1.6.9-2~bpo20.04.1 100
        100 http://archive.ubuntu.com/ubuntu focal-backports/main amd64 Packages
     1.6.4-3ubuntu1.1 500
        500 http://archive.ubuntu.com/ubuntu focal-updates/main amd64 Packages
     1.6.4-3ubuntu1 500
        500 http://archive.ubuntu.com/ubuntu focal/main amd64 Packages

One way for us to resolve this issue is try to catch the target version from the command, but I feel more nice if apt lib can support it.

benhoyt commented 10 months ago

I don't know enough about apt to know why it's choosing the backports version. I'm on Jammy (22.04), but I don't have the backports one showing up, so I can't reproduce this.

$ apt-cache policy freeipmi-tools
freeipmi-tools:
  Installed: (none)
  Candidate: 1.6.9-2ubuntu0.22.04.2
  Version table:
     1.6.9-2ubuntu0.22.04.2 500
        500 http://gb.archive.ubuntu.com/ubuntu jammy-updates/main amd64 Packages
     1.6.9-2 500
        500 http://gb.archive.ubuntu.com/ubuntu jammy/main amd64 Packages

From this comment, it looks like when you run regular apt-get install it doesn't choose the backports one? Any idea why?

I notice from the apt.py code that it adds the argument --option=Dpkg::Options::=--force-confold to the apt-get install command. What does that do, and does that change this behaviour in manual tests?

Additionally, we're not actually capturing the error message here, I think because we're including e.output in the exception message instead of e.stderr, and of course stderr is where the error actually lives. See also.

jneo8 commented 10 months ago

I don't know enough about apt to know why it's choosing the backports version. I'm on Jammy (22.04), but I don't have the backports one showing up, so I can't reproduce this.

You should able to reproduce this on focal.(maybe using multipass)

As I know the version came from the function from_apt_cache, because the backport version is the first one show in command apt-cache show freeipmi-tools output and this function just return the first version it get.

Pjack commented 10 months ago

The behavior is different than command line apt install freeipmi-tools If we don't intend to install the backport package, it looks like a bug in this library.

tonyandrewmeyer commented 10 months ago

I notice from the apt.py code that it adds the argument --option=Dpkg::Options::=--force-confold to the apt-get install command. What does that do, and does that change this behaviour in manual tests?

--force-confold will automatically keep an old conf file when it's been modified, without prompting. It shouldn't ever impact package choice.

tonyandrewmeyer commented 10 months ago

You can do this by changing your install.py to:

import apt

repositories = apt.RepositoryMapping()
for repo in repositories:
    if repo.release.endswith("-backports"):
        repositories.disable(repo)

apt.add_package("freeipmi-tools", update_cache=False)

I reproduced the original issue on focal and after adjusting as above, it successfully installed 1.6.4-3ubuntu1.1.

@jneo8 does this cover what you need?

By the way, the default behaviour is that it'll run through the output of apt-policy show and pick the first one that matches the arch, and the version, if specified (if not specified, then only the arch needs to match).

jneo8 commented 10 months ago

You can do this by changing your install.py to:

import apt

repositories = apt.RepositoryMapping()
for repo in repositories:
    if repo.release.endswith("-backports"):
        repositories.disable(repo)

apt.add_package("freeipmi-tools", update_cache=False)

I reproduced the original issue on focal and after adjusting as above, it successfully installed 1.6.4-3ubuntu1.1.

@jneo8 does this cover what you need?

By the way, the default behaviour is that it'll run through the output of apt-policy show and pick the first one that matches the arch, and the version, if specified (if not specified, then only the arch needs to match).

This change the /etc/apt/sources.list and I don't want to cause another apt related bug because of this on production. Instead of disable backport repository, is that possible we just let apt have a chance to install Candidate version?

# It's the apt-cache policy output on focal now
$ sudo apt-cache policy freeipmi-tools
freeipmi-tools:
  Installed: (none)
  Candidate: 1.6.4-3ubuntu1.1
  Version table:
     1.6.9-2~bpo20.04.1 100
        100 http://archive.ubuntu.com/ubuntu focal-backports/main amd64 Packages
     1.6.4-3ubuntu1.1 500
        500 http://archive.ubuntu.com/ubuntu focal-updates/main amd64 Packages
     1.6.4-3ubuntu1 500
        500 http://archive.ubuntu.com/ubuntu focal/main amd64 Packages

Below is my workaround to get the Candidate version first and pass to apt.add_package

"""Apt helper module for missing features in operator_libs_linux."""
import re
from subprocess import PIPE, CalledProcessError, check_output
from typing import Optional

from charms.operator_libs_linux.v0 import apt

def get_candidate_version(package: str) -> Optional[str]:
    """Get candiate version of package from apt-cache."""
    try:
        output = check_output(
            ["apt-cache", "policy", package], stderr=PIPE, universal_newlines=True
        )
    except CalledProcessError as e:
        raise apt.PackageError(f"Could not list packages in apt-cache: {e.output}") from None

    lines = [line.strip() for line in output.strip().split("\n")]
    for line in lines:
        candidate_matcher = re.compile(r"^Candidate:\s(?P<version>(.*))")
        matches = candidate_matcher.search(line)
        if matches:
            return matches.groupdict().get("version")
    raise apt.PackageError(f"Could not find candidate version package in apt-cache: {output}")

def add_pkg_with_candidate_version(pkg: str) -> None:
    """Install package with apt-cache candidate version."""
    version = get_candidate_version(pkg)
    apt.add_package(pkg, version=version, update_cache=False)

I would like to contribute to apt lib to provide some similar feature like this.

jnsgruk commented 10 months ago

I think I would rather not - this looks quite convoluted.

I'd suggest instead just specifying the exact version that you want to install, which I think the lib already supports? That way a given revision of the charm will be tied to a known good version in the archive.

It does mean the charm will need bumping when the version changes in the archive, but that could be automated quite trivially 😀

jneo8 commented 10 months ago

Hi @jnsgruk ,

I believe there are two issues here:

jnsgruk commented 10 months ago

I'm suggesting the latter, that you hard code the versions.

This is relatively common in our product charms like MySQL/ PostgreSQL where we pin a given revision of a charm to a revision of the underlying snap to ensure compatibility.

In reality if you have some automation to bump versions and release to charmhub, this is quite low overhead and will give you more confidence that the things going into production have been tested together.

jneo8 commented 10 months ago

Shouldn't the apt lib install_package has the same behavior as apt install command?

Make sure they have the same behavior will decrease the difficulty for user to using this library.

Without this then the install_package will suddenly give you different version once backport repository released.

nishant-dash commented 10 months ago

While pinning a revision to a snap makes sense, pinning to a deb package not so much.

benhoyt commented 10 months ago

Yes, I agree that apt.add_package should install the same version as apt-get install / apt install, and it seems like a bug that it doesn't. Looking closer at this library I think it's trying to be way too clever, and don't understand a number of the design choices. For example, why doesn't apt.add_package() simply call apt-get install directly and use its logic?

In any case, I think an immediate workaround for @jneo8 is to do what @jnsgruk suggested and pin the version. However, @nishant-dash has some good points about why that's probably not a good idea in general.

I'd say we should make add_package follow the priority logic of the actual apt-get install command and choose the right package. I don't know how much work that is, but our team (@tonyandrewmeyer and I) probably can't spend time on this right now.

An alternative would be to rewrite this package with a v1 version, vastly simplifying it to basically just calling the apt commands and relying on their logic.

jnsgruk commented 10 months ago

An alternative would be to rewrite this package with a v1 version, vastly simplifying it to basically just calling the apt commands and relying on their logic.

This is a good idea. I was never a fan of the complexity this library introduced. I don't know how much work it'd be, but could be a possible "blue" or a job for next cycle if too much.