action add-disk failed with "cannot import name 'filter_missing_packages'"

[Joe Guo]

when using `add-disk` action I got this error:

    juju run-action ceph-osd/8 add-disk osd-devices=/dev/bcache6 --wait
    ...
    Traceback (most recent call last):
        File "/var/lib/juju/agents/unit-ceph-osd-8/charm/actions/add-disk", line 29, in <module>
        import ceph_hooks
        File "hooks/ceph_hooks.py", line 29, in <module>
        import charms_ceph.utils as ceph
        File "lib/charms_ceph/utils.py", line 57, in <module>
        from charmhelpers.fetch import (
    ImportError: cannot import name 'filter_missing_packages'
    ...

Error is raised in `charmhelpers` package.
The ceph-osd charm has included a local copy of it at `hooks/charmhelpers` which does have `filter_missing_packages`.

However, on this host, `charmhelpers` is also installed globally at:

    /usr/local/lib/python3.5/dist-packages/charmhelpers

In charm code, when we add local modules to python sys.path, we are using:

    cat actions/add_disks.py
    ...
    sys.path.append('lib')
    sys.path.append('hooks')
    ...

So the local modules are added at the end, which python interpreter will search last.
Thus, when a global copy of package such as `charmhelpers` exists, python will use that first, which is wrong, and caused above import error.

Suggestion:

When we add local module, we should insert it at beginning, so it's preferred over system/global ones:

    sys.path.insert(0, 'hooks')

This issue not only exists in action/add_disks.py, but also in many other files in this charm.
And other charms as well.

[Alex Kavanagh (ajkavanagh)]

Hi Joe

Your analysis is correct; the charm should not depend on the system packaged version of charm-helpers which is why it ships with a local vendored-in copy. So, this is a bug.

In terms of fixing it, if you are doing so, please could you follow the pattern established in other charms:

https://github.com/openstack/charm-nova-compute/blob/433796779e4f31320e6c57527f421d4e9aaacedc/actions/pause_resume.py#L20

I can't remember why we insert at the second position rather than the first, but it does seem to be a pattern we've established!

Many thanks
Alex.

[Joe Guo (guoqiao)]

Hi Alex,

Thanks for the feedback, code changed to the suggested pattern.

Re: why insert at 1 instead of 0, I think it's because 0 is normally current dir, which should always have highest priority. E.g:

python3 -c "import sys;print(sys.path)"
['', '/usr/lib/python38.zip', '/usr/lib/python3.8', '/usr/lib/python3.8/lib-dynload', '/usr/local/lib/python3.8/dist-packages', '/usr/lib/python3/dist-packages']

[Alex Kavanagh (ajkavanagh)]

Hi Joe

> Re: why insert at 1 instead of 0, I think it's because 0 is normally current dir, which should always have highest priority. E.g:

Yes, that's it. I vaguely remember this. (Disclaimer: I implemented the pattern you see in charms :) Thanks for the confirmation, and for implementing a fix.

Cheers
Alex.

[Billy Olsen (billy-olsen)]

PR raised at https://review.opendev.org/c/openstack/charm-ceph-osd/+/781221

[OpenStack Infra (hudson-openstack)]

Change abandoned by "James Page <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/charm-ceph-osd/+/781221
Reason: This review is > 12 weeks without comment, and failed testing the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[Felipe Reyes (freyes)]

Removing assignee since the proposed fix was abandoned and haven't seen recent updates. Setting the status back to 'new' since this should be triaged again to make sure this is still a problem y recent versions of the charm.