Conditionally change the local DB file name

[lmlg]

This PR tests whether the ops library is present in the system to decide the final file name for the local key/value database in order to avoid conflicts between that library and charmhelpers.

[ajkavanagh]

So I'm generally against this change due to opportunities for regressions and 'papering' over the bug. Essentially charm-helpers 'owns' the file .unit-state.db as it predates the ops framework, and thus there are lots of charms already using that file. If the ops framework needs an SQLite file, then it should have its own filename and not clobber the charm-helper's one (used in classic and reactive charms). This is a hack, and it shouldn't be added to the library.

However, the Storage class initialise function has a path variable to allow multiple Storage objects to co-exist, so the ops framework could use. Or it could set the UNIT_STATE_DB file. Or the charm application code could do either of these actions, as the charm application code is bringing in charm-helpers. If the ops framework is bring in charm-helpers then it shouldn't clobber the file itself. I hope that makes sense.

[ajkavanagh]

It's not a 'suffix'; it's a hidden file. e.g. db_suffix isn't quite right; db_file would be more accurate.

[ajkavanagh]

This makes charm-helpers dependent on ops, which is entirely the wrong way around.

[lmlg]

I disagree that it creates a dependency. It merely checks that a library can be imported; charm-helpers doesn't require ops to be installed for it to work.

[lmlg]

So I'm generally against this change due to opportunities for regressions and 'papering' over the bug. Essentially charm-helpers 'owns' the file .unit-state.db as it predates the ops framework, and thus there are lots of charms already using that file. If the ops framework needs an SQLite file, then it should have its own filename and not clobber the charm-helper's one (used in classic and reactive charms). This is a hack, and it shouldn't be added to the library.

I'm mostly in agreement with your comment. However, I think that, although a hack, something like this is a necessary hack. Whether it's done in the ops framework or here is mostly immaterial to me.

However, the Storage class initialise function has a path variable to allow multiple Storage objects to co-exist, so the ops framework could use. Or it could set the UNIT_STATE_DB file. Or the charm application code could do either of these actions, as the charm application code is bringing in charm-helpers. If the ops framework is bring in charm-helpers then it shouldn't clobber the file itself. I hope that makes sense.

That's true (re: the path variable), but at the same time, the charm-helpers library instantiates Storage classes at several points, mostly when using local Key/Value databases, using the default path.

FWIW, we've taken the approach of setting the UNIT_STATE_DB env var to work around this issue in the ceph-mon charm, but a fix at the library level is needed (imo) to prevent inadvertent clashes in the future.

[ajkavanagh]

However, the Storage class initialise function has a path variable to allow multiple Storage objects to co-exist, so the ops framework could use. Or it could set the UNIT_STATE_DB file. Or the charm application code could do either of these actions, as the charm application code is bringing in charm-helpers. If the ops framework is bring in charm-helpers then it shouldn't clobber the file itself. I hope that makes sense.

That's true (re: the path variable), but at the same time, the charm-helpers library instantiates Storage classes at several points, mostly when using local Key/Value databases, using the default path.

This is true as it's a charm-helpers class being used in charm-helpers. It would naturally use the default.

FWIW, we've taken the approach of setting the UNIT_STATE_DB env var to work around this issue in the ceph-mon charm, but a fix at the library level is needed (imo) to prevent inadvertent clashes in the future.

The fix isn't valid in charm-helpers as it's the library at the bottom of the heap and provides mechanisms already to use different files. Also, the SQLite file is already called that in (almost?) every classic and reactive charm. This change would say "actually, the new default for existing charms, is this new file, because ops wants this one". That would create confusion.

If the ops framework imports charm-helpers directly, then the fix should be done there.

Otherwise, it's the responsibility of the charm code (which is importing both charm-helpers and the ops framework) to sort out what it wants (in this case using the UNIT_STATE_DB).

Realistically, the ops framework has created this problem, and if the fix isn't carried in charm code, then it should be changed in the ops framework.