Ceph Monitor Charm

"Remove chrony in a container" patch doesn't work

Bug #1929054 reported by Alex Kavanagh
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ceph Monitor Charm
In Progress
Undecided
Alex Kavanagh
charms.ceph
Fix Released
Undecided
Alex Kavanagh

Bug Description

The "remove chrony in a container" patch doesn't actually work as it has a bug. The bug is that "chrony" is passed as a string to filter_missing_packages() which is expecting an list of packages. As strings are iterable, this results in filter_missing_packages looking a ['c', 'h', ...].

Fix: pass an an array.

Note, this got merged as there is no related test to verify it. This may be because there are no tests associated with the feature currently.

Revision history for this message
Alex Kavanagh (ajkavanagh) wrote :

Note the actual bug in in charms.ceph, but it affects the ceph-mon charm.

Alex Kavanagh (ajkavanagh)
Changed in charms.ceph:
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in charm-ceph-mon:
assignee: nobody → Alex Kavanagh (ajkavanagh)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to charms.ceph (master)

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/charms.ceph/+/792720

Changed in charms.ceph:
status: New → In Progress
Alex Kavanagh (ajkavanagh)
Changed in charm-ceph-mon:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to charm-ceph-mon (master)

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/charm-ceph-mon/+/792721

Revision history for this message
Liam Young (gnuoy) wrote :

I think it still doesn't work even after https://review.opendev.org/c/openstack/charms.ceph/+/792720. The logic seems wrong. Specifically:

         install_list = filter_missing_packages([CHRONY_PACKAGE])
         if not install_list:
             rm_packages.append(CHRONY_PACKAGE)

Since filter_missing_packages returns a list of packages which are installed this is only adding 'chrony' to the list if it is not present.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to charms.ceph (master)

Reviewed: https://review.opendev.org/c/openstack/charms.ceph/+/792720
Committed: https://opendev.org/openstack/charms.ceph/commit/626a5ce3ca868286d7341fd7e53e9e161f969ecc
Submitter: "Zuul (22348)"
Branch: master

commit 626a5ce3ca868286d7341fd7e53e9e161f969ecc
Author: Alex Kavanagh <email address hidden>
Date: Sat May 22 15:30:36 2021 +0100

    Fix "Remove chrony in a container" patch with test

    Unfortunately, the original patch didn't work [1]. Then a fix was
    applied also without a test [2]. Unfortunately, this wasn't correct
    either.

    This patch simplifies the function, which makes it a little easier to
    reason about. Also a test is added to stop regressions in the future.

    [1] Ie3c9c5899c1d46edd21c32868938d3290db321e7
    [2] Ifef76eacd8bd837d2181ec75e406aa35f88b8b5b
    Closes-Bug: #1929054

    Change-Id: I93f53274bc8c7e62ebb8cb7bf81ff816f4bfd98b

Changed in charms.ceph:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to charms.ceph (stable/21.04)

Fix proposed to branch: stable/21.04
Review: https://review.opendev.org/c/openstack/charms.ceph/+/792881

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to charms.ceph (stable/21.04)

Reviewed: https://review.opendev.org/c/openstack/charms.ceph/+/792881
Committed: https://opendev.org/openstack/charms.ceph/commit/d1515daf737ceaca8e544b15bdcee5c10f67888e
Submitter: "Zuul (22348)"
Branch: stable/21.04

commit d1515daf737ceaca8e544b15bdcee5c10f67888e
Author: Trent Lloyd <email address hidden>
Date: Thu May 20 11:47:43 2021 +0800

    filter_missing_packages expects a list, not string

    filter_missing_packages expects a list and not a single package name as
    a string. Pass the chrony package as a single item list to avoid it
    checking installation candiates for 'c', 'h', 'r', 'o', 'n' and 'y'

    This patch simplifies the function, which makes it a little easier to
    reason about. Also a test is added to stop regressions in the future.

    [1] Ie3c9c5899c1d46edd21c32868938d3290db321e7
    [2] Ifef76eacd8bd837d2181ec75e406aa35f88b8b5b
    Closes-Bug: #1929054

    Change-Id: I93f53274bc8c7e62ebb8cb7bf81ff816f4bfd98b

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on charm-ceph-mon (master)

Change abandoned by "Alex Kavanagh <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/charm-ceph-mon/+/792721
Reason: This is now fixed due to a charms.ceph sync in the last cycle.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.