Better default behaviour inside of unit tests

[pengwyn]

Occasionally there can be a race condition in tests which use the KV store. This is because the store does not need to be tolerant of concurrent writes in normal juju execution, but when tests are run in parallel they can write to the same (default) sqlite DB.

See https://bugs.launchpad.net/charm-helpers/+bug/1911919 for a collection of similar bugs.

The workaround in charms has been to manually create the KV store with ":memory:" so that each test will run with a different DB. This PR replicates this behaviour, by changing the default KV store location if zaza is imported. It can be overridden with the UNIT_STATE_DB as normal.

The actual fix is very small. The bulk of the PR is introducing a test to see that a) this behaviour can be forced and b) that the behaviour is fixed when a fake zaza is imported. I forced the problem by starting a subprocess that locks the DB.

My main worry with the PR is the crude checking of "zaza" in the imported modules. But I went with this approach to avoid any extra dependencies of charmhelpers on zaza or zaza on charmhelpers.

The tests can be checked by looking at commit ec50d90, which should give a failure.

[pengwyn]

After putting this together, I realise I have focused only on zaza. Does that capture most charm tests, or would a different detection of e.g. unittest be worthwhile?

[ajkavanagh]

I really like the intention of this change, and it's approach to having a default for 'testing mode'. However, I don't really like the 'linking' to zaza; i.e. creating it as a dependency. i.e. charms use charm-helpers; zaza tests charms (and, when incorrectly mocked), accidentally use charm-helpers, and in this case, expose that it's not multi-process/thread safe. This change kind of creates a reverse dependency of charm-helpers on zaza, (even if only tangentially).

My first thoughts on how to resolve this for consideration, are instead to add a charm-helpers specific environment variable that, if set to some value, will inform charmhelpers that it needs to use :memory: for the kv store, and then a concomitant change in zaza to use this as a default. Then the dependency change would be "zaza depends on charms depends on charm-helpers" and charm-helpers is unaware of what is saying "use :memory:". Thoughts?

[pengwyn]

Yes I see your point and I'm liking the module check less as I think about it more.

Writing out what might be good to have:

1. An external option for KV store to be concurrency safe.
2. A future proof way for the option to indicate "in testing mode"
3. No dependency on other packages
4. Not require the charm-writer (who doesn't know about the charm-helpers details) to explicitly set this option.
5. Don't cause surprise logic change with just an import.

It's 4 that is the tricky one, but I think your suggestion will handle that case too. The problem that people seem to be facing currently is 4, because you have to be bitten by the concurrency issue to know you should look for these changes. And we definitely don't want to break 5 as you mentioned. Finally, relying on UNIT_STATE_DB could break future store changes.

Here's a list of approaches I've thoought of, plus what you mentioned:

• A) The user must manually set UNIT_STATE_DB=:memory: and make sure it is passed to tests. No code changes. Ticks 1,3,5 but crucially misses 4.

• B) The user must manually set CHARM_HELPERS_TESTMODE=1. charm-helpers bases its default behaviour on this. Ticks 1,2,3,5 but still misses 4.

• C) As the original PR, autodetect zaza, but allow override with UNIT_STATE_DB. Solves 1,3,4, but fails 5.

• D) Detect if currently running in a test suite more rigorously. AFAIK this isn't possible with e.g. unittest. If so it'd solve 1-5

• E) As with B, but zaza also sets CHARM_HELPERS_TESTMODE on test run (not just import). Solves 1-5, although there could be problems if not using zaza.

• F) As with E, but zaza sets ZAZA_TESTS instead and charm-helpers then listens for that instead of an import.

Option E is my interpretation of what you wrote and it seems to tick the most boxes. I'm very new to charms, is the coverage of zaza across various charm packages close to 100%?

I added F too but that still suffers from the reverse dependency issue.

[pengwyn]

I make a small change to switch from checking for zaza to looking at CHARM_HELPERS_TESTMODE instead.

[ajkavanagh]

@pengwyn so I led you slightly astray as I was thrown a bit by seeing zaza in it and hadn't thought things through properly. zaza is a red-herring as it's not involved in unit tests at all. It's only a functional test framework and is never in the same process as any charm code. e.g. it runs outside of the juju model and thus would never interact with the charm code.

Having said that, the change to introduce a CHARM_HELPERS_TESTMODE seems quite reasonable, but does mean that the library still has no automatic mechanism of detecting whether it is running in unit tests of within a charm on a unit.

One option to detect whether charmhelpers is running in a charm is to check for one or more of these env variables: (e.g. from vault running in a test model, during update-status):

# env | grep JUJU
JUJU_METER_INFO=not set
JUJU_CHARM_HTTPS_PROXY=
JUJU_CONTEXT_ID=vault/0-update-status-7919217765836578507
JUJU_MODEL_UUID=5fbe3e08-8eb6-4f1c-820d-0a68d362f428
JUJU_VERSION=2.9.37
JUJU_CHARM_HTTP_PROXY=
JUJU_UNIT_NAME=vault/0
JUJU_HOOK_NAME=update-status
JUJU_AGENT_SOCKET_ADDRESS=@/var/lib/juju/agents/unit-vault-0/agent.socket
JUJU_CHARM_FTP_PROXY=
JUJU_AVAILABILITY_ZONE=nova
JUJU_METER_STATUS=AMBER
JUJU_CHARM_DIR=/var/lib/juju/agents/unit-vault-0/charm
JUJU_DEBUG=/tmp/juju-debug-hooks-1193241599
JUJU_API_ADDRESSES=172.20.0.182:17070 252.182.0.1:17070
JUJU_PRINCIPAL_UNIT=
JUJU_MODEL_NAME=zaza-e79c0a05dd9d
JUJU_MACHINE_ID=3
JUJU_SLA=unsupported
JUJU_DISPATCH_PATH=hooks/update-status
JUJU_AGENT_SOCKET_NETWORK=unix
JUJU_CHARM_NO_PROXY=127.0.0.1,localhost,::1

Then a test where UNIT_STATE_DB is not present, but, say, at least JUJU_CHARM_DIR is also not present might be enough to say "running in test mode."

[ajkavanagh]

I think this is a better overall solution; it drops the dependency on zaza.

[pengwyn]

My bad! I think I got confused originally and didn't realise the test failures were all unit tests.

I like that idea, so I made an update to still keep CHARM_HELPERS_TESTMODE but the default is now "auto" which looks for one of those juju env vars.

It's a bit over-engineered, but this should give an option to anyone doing odd tests with faking JUJU_* envs to still use the in-memory version. I also added a test for this case.

[ajkavanagh]

LGTM