Merge lp://staging/~cprov/uci-engine/charm-tests into lp://staging/uci-engine

PASSED: Continuous integration, rev:813
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1483/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1483/rebuild

I keep getting the following trace on with and without a deployment. (both with novarc file sourced.)

Traceback (most recent call last):
  File "./run-tests", line 32, in <module>
    retval = run_tests.main(sys.argv[1:], sys.stdout, sys.stderr)
  File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line 565, in main
    options.include_regexps, options.exclude_regexps)
  File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line 448, in load_charm_tests
    suite.setCharmRoots()
  File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line 356, in setCharmRoots
    s.charm_root = '/'.join(flat_tests[0].id().split('.')[:3])
IndexError: list index out of range

Marking the MP, needs fixing. Please feel free to ignore if i'm doing abs stupid :)

LGTM with one minor nit.

There are many issues with the MP, the main ones are:
- it breaks --list

- it uses imp.load_module() whose doc says: "This function does more than importing the module: if the module was already imported, it is equivalent to a reload()!" (The question mark is theirs). See https://docs.python.org/2/library/functions.html?highlight=reload#reload and the endless list of issues associated with that function.

There is no way we can trust reload(), it's meant to be used interactively with a full understanding of the fallouts.

The root issue that you're pointing out here is that we're using hooks.py to mean different things, python doesn't allow that. So that's what need to be changed.

I had a look at it and the second similar issue is that we *also* do 'import charmhelpers' with various definitions across charms and also try to convince python that they are all the same. This one is slightly simpler to fix: as a project, we should use the *same* version in all charms (we do use the same revno but build a different python package via different charm-helpers.yaml files).

I think it's time to stop using bigger hammers that do more harm than good, step back to the original decision to rely on 'import hooks' and fix that.

Sivaa,

Thanks for pointing this problem, it only happens when you run run-tests without filters (an empty test suite for charm tests shows up mysteriously). Fix added.

> I keep getting the following trace on with and without a deployment. (both
> with novarc file sourced.)
>
> Traceback (most recent call last):
> File "./run-tests", line 32, in <module>
> retval = run_tests.main(sys.argv[1:], sys.stdout, sys.stderr)
> File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line
> 565, in main
> options.include_regexps, options.exclude_regexps)
> File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line
> 448, in load_charm_tests
> suite.setCharmRoots()
> File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line
> 356, in setCharmRoots
> s.charm_root = '/'.join(flat_tests[0].id().split('.')[:3])
> IndexError: list index out of range
>
>
> Marking the MP, needs fixing. Please feel free to ignore if i'm doing abs
> stupid :)

Joe,

I've added an explanation for the line calculating the path from the mangled test id. Thanks for pointing it.

> LGTM with one minor nit.

PASSED: Continuous integration, rev:814
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1491/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1491/rebuild

Vila,

Thanks for your points, let's discuss them specifically.

> There are many issues with the MP, the main ones are:
> - it breaks --list

--list was probably broken because the problem with running run-test w/o filters, it's fixed now.

> - it uses imp.load_module() whose doc says: "This function does more than
> importing the module: if the module was already imported, it is equivalent to
> a reload()!" (The question mark is theirs). See
> https://docs.python.org/2/library/functions.html?highlight=reload#reload and
> the endless list of issues associated with that function.
>
> There is no way we can trust reload(), it's meant to be used interactively
> with a full understanding of the fallouts.
>
> The root issue that you're pointing out here is that we're using hooks.py to
> mean different things, python doesn't allow that. So that's what need to be
> changed.
>
> I had a look at it and the second similar issue is that we *also* do 'import
> charmhelpers' with various definitions across charms and also try to convince
> python that they are all the same. This one is slightly simpler to fix: as a
> project, we should use the *same* version in all charms (we do use the same
> revno but build a different python package via different charm-helpers.yaml
> files).

Charms are designed to be standalone projects, they just happen to be in the same tree because it's convenient to us, the same goes for running their tests inside a single *super-project* test suite, it's *just* way more convenient than spawning isolated `make check`s and collect results. That said, in order to keep the wanted benefits, UCI-E has to compromise.

We are all aware of the risks involved in reloading modules, but if it is what it takes to assure charm tests (and only them) are run as part of our test suite, so be it. And by all means, I agree with you on keeping this hack as isolated and instrumented as possible, so we will notice if it is behaving badly, if it ever does.

> I think it's time to stop using bigger hammers that do more harm than good,
> step back to the original decision to rely on 'import hooks' and fix that.

It sounds good, but `import hooks` & `import charmhelpers` are not going to change, they make all sense at the charm-level; and we have to ensure we run charm tests in tarmac, today, not tomorrow.

Moreover, It's not a *big hammer*, since it's well isolated (custom loaders and suite are only used for charm tests) and the behaviour can be entirely overridden by tweaking 2 lines, if we find real, not hypothetical, evidences that it's not doing what it is supposed to do.

Here's a bit of a strawman. Is it time move some of these charms out of the lp:uci-engine source tree? Our original intent was to do this once the level of change had dropped to an acceptable level. This is not going to solve the problem, but maybe we just avoid it or lessen the risks.

Even if this isn't the right solution to solve this specific problem, is it time to remove one or more of these from the source? We should be able to do some simply analysis to see which charms have had the fewest recent updates.

> Charms are designed to be standalone projects, they just happen to be in the
> same tree because it's convenient to us,

Components are also designed to be stand alone, we know we have issues there
around the overall project layout but we've already taken some steps to
ensure they use different parts of the python name space so their code can
be loaded in a single python process.

It's not about being convenient it's just about using python and respecting
the language definition.

> the same goes for running their tests
> inside a single *super-project* test suite, it's *just* way more convenient
> than spawning isolated `make check`s and collect results.

... and allowing test listing/selection and displaying the timings and being
able to use subunit as an output and being able to run them concurrently

> That said, in order
> to keep the wanted benefits, UCI-E has to compromise.

Says who ?

There *is* a valid fix which is to fix the charms to use their own name space like we do for the components.

>
> We are all aware of the risks involved in reloading modules,

I have no idea about how tricking python into believing it can import
different modules in the same name space spot can manifest itself or mask
issues, that's the whole point.

> but if it is what
> it takes to assure charm tests (and only them) are run as part of our test
> suite, so be it. And by all means, I agree with you on keeping this hack as
> isolated and instrumented as possible, so we will notice if it is behaving
> badly, if it ever does.
>
> > I think it's time to stop using bigger hammers that do more harm than good,
> > step back to the original decision to rely on 'import hooks' and fix that.
>
> It sounds good, but `import hooks` & `import charmhelpers` are not going to
> change,

Of course they can change, I already have one charm fixed to define its own
name space and fix the root issue.

> they make all sense at the charm-level; and we have to ensure we run
> charm tests in tarmac, today, not tomorrow.

The charms should be fixed, not run-tests, changing python semantics is not
run-tests job.

>
> Moreover, It's not a *big hammer*, since it's well isolated (custom loaders
> and suite are only used for charm tests) and the behaviour can be entirely
> overridden by tweaking 2 lines,

Your patch is already ~300 lines long, that's far from 2 lines.

> if we find real, not hypothetical, evidences
> that it's not doing what it is supposed to do.

The real evidence is that python can't load different modules under the same
name.

That's a real issue, perfectly clear and understood. I see no point in
tricking python when there is a way to use it properly.

We have always said that charms should be stand-alone and not depend on behaviour that only exists in lp:uci-engine. I think by putting the namespace of the charm in the filename, we move away from that. Pretty much every other charm-helpers using charm has hooks/hooks.py and now we have hooks/wsgi_app_hooks.py.

I also think the risks that Vincent highlights are very real and if they were to bite us, would be extremely hard to pinpoint.

I am almost inclined to agree with Francis that it's time to split the charms out into their own branches. We understand the various pieces of this project enough now to be able to accomplish that without branch landing being delicate. However, it's no small amount of work. Each charm requires a new stanza in Tarmac, a new branch for everyone on the team to watch MPs on, and so on.

I am also convinced that it greatly reduces visibility on these ancillary branches. It means that we sacrifice consistency, which already needs to be carefully managed in a microservices architecture.

Instead, I'd like to propose that we isolate the process environment for the charm tests. We fork a child process under the charm directory and run through the tests in complete isolation. The parent process can take the piped output from the child and combine it with its own results.

We've got more opinions on this one than we have people. Let's take it down a level and understand that there are tradeoffs with all approaches.

Thanks Celso for the fix to handle empty suite. Approving before disappearing for the day.

Evan and Francis,

I have no arguments against your proposal for spawning `make check` on charms as a clean transition to move them away from our tree.

It is certainly a good ending-point to this polarised discussion full of hypothetical problems and no concrete solutions.

Any arguments against running charms tests in called_by_tarmac.py before deploying the new environment ? (users would chdir into charms and run `make check`)

On Tue, Sep 30, 2014 at 12:52:38PM -0000, Celso Providelo wrote:
> Evan and Francis,
>
> I have no arguments against your proposal for spawning `make check` on charms as a clean transition to move them away from our tree.
>
> It is certainly a good ending-point to this polarised discussion full of hypothetical problems and no concrete solutions.
>
> Any arguments against running charms tests in called_by_tarmac.py before deploying the new environment ? (users would chdir into charms and run `make check`)

I am strongly against changing our charms to make them work with
uci-engine. I am also strongly in favor of getting our charms out of
our uci-engine branch. I don't think tracking several branches for MPs
is too much effort.

I am in support of running charm tests in isolation until they can be
put into separate branches.

Joe