Refactor the config generation logic.

Refactor the config generation logic.

As requested by reviews, change the server configuration generation logic
so that the various server configuration files are present on the
filesystem only for the actually deployed and started servers.

This is implemented by creating additional start_* and stop_* functions
in hooks/utils.py, called by the mixins in hooks/backend.py, and moving
there/creating the logic that generates, writes and remove the various
configuration files.

In addition:

- factor out common SSL code between HaproxyApacheMixin and
  BuiltinServerMixin;
- rename options.http to options.insecure in server/guiserver/manage.py
- remove unused code and backend properties;
- miscellaneous cleanup.

The diff is still a bit big, even after moving the builtin server
integration changes to another branch. Sorry about that.

https://code.launchpad.net/~teknico/charms/precise/juju-gui/refactor-config-generation-logic/+merge/178594

(do not edit description out of merge proposal)

[teknico, 2013-08-05 15:53:03]

Please take a look.

[frankban, 2013-08-06 12:57:23]

Thank you Nicola for this branch: the charm is looking clearer now IMHO.
LGTM with changes/questions (see below).
Not QA'd, I will follow up later.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode113
hooks/utils.py:113: HAPROXY_PATH = '/etc/haproxy/haproxy.cfg'
Minor: add those new consts to __all__?

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode339
hooks/utils.py:339: def
stop_improv(config_path='/etc/init/juju-api-improv.conf'):
Is this kwarg used somewhere? If not, what do you think about removing it?

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode340
hooks/utils.py:340: """Stop a simulated juju environment."""
Minor: Juju, capital J.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode355
hooks/utils.py:355: SYS_INIT_DIR, os.path.basename(unit_dir))
I am not sure about the value of having a /etc/init constant. Anyway, I think
os.path.join() can help calculating this path.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode371
hooks/utils.py:371: def stop_agent(config_path='/etc/init/juju-api-agent.conf'):
Same config_path question as above.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode475
hooks/utils.py:475: if not os.path.exists(APACHE_SITE):
Why touch is used before generating the file? Couldn't we just generate the
files with render_to_file, and proceeding with chown/ensite after the fact?

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode499
hooks/utils.py:499: def remove_apache_setup():
It seems that these functions (remove_apache_setup, start_haproxy_apache,
remove_haproxy_setup etc...) are not tested. Have you considered adding unit
tests for those helpers?

https://codereview.appspot.com/12467044/diff/1/server/runserver.py
File server/runserver.py (right):

https://codereview.appspot.com/12467044/diff/1/server/runserver.py#newcode27
server/runserver.py:27: --sslpath is ignored if --insecure is set: only one of
them is needed.
None of them is actually required. --insecure defaults to False, --sslpath to
the default path defined for convenience in the server. I believe we could just
write "The --sslpath option is ignored if --insecure is set." or similar.

https://codereview.appspot.com/12467044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/12467044/diff/1/tests/20-functional.test#newco...
tests/20-functional.test:207: def test_force_machine(self):
IIRC, you mentioned that this test no longer passes. Please add a card for
fixing this problem. This might be related to the fact that force-machine has
been renamend to "--to" in the juju deploy parameters.

[frankban, 2013-08-06 13:57:36]

Unfortunately I found an error in QA. This branch cannot be landed until this
error is fixed.

How to dupe:
- deploy the charm in a pyJuju env;
- run "juju set juju-gui juju-gui-source=lp:juju-gui";
- wait for the config-changed hook to be executed;
- you'll see the agent-state configure-error.

The hook exits with the following error:

2013-08-06 15:46:12,684 unit:juju-gui/0: hook.output INFO: Traceback (most
recent call last):

2013-08-06 15:46:12,685 unit:juju-gui/0: hook.output INFO:   File
"/var/lib/juju/units/juju-gui-0/charm/hooks/config-changed", line 60, in
<module>

2013-08-06 15:46:12,686 unit:juju-gui/0: hook.output INFO:     
2013-08-06 15:46:12,686 unit:juju-gui/0: hook.output INFO: main()

2013-08-06 15:46:12,687 unit:juju-gui/0: hook.output INFO:   File
"/var/lib/juju/units/juju-gui-0/charm/hooks/config-changed", line 52, in main

2013-08-06 15:46:12,687 unit:juju-gui/0: hook.output INFO:     
2013-08-06 15:46:12,688 unit:juju-gui/0: hook.output INFO: backend.start()
  File "/var/lib/juju/units/juju-gui-0/charm/hooks/backend.py", line 194, in
method
    a_callable(mixin, self)
  File "/var/lib/juju/units/juju-gui-0/charm/hooks/backend.py", line 177, in
start
    config['secure'])
  File "/var/lib/juju/units/juju-gui-0/charm/hooks/utils.py", line 517, in
start_haproxy_apache
    setup_haproxy_config(ssl_cert_path, secure)
  File "/var/lib/juju/units/juju-gui-0/charm/hooks/utils.py", line 440, in
setup_haproxy_config
    shutil.copy(config_path, SYS_INIT_DIR)
  File "/usr/lib/python2.7/shutil.py", line 117, in copy

2013-08-06 15:46:12,689 unit:juju-gui/0: hook.output INFO:     
2013-08-06 15:46:12,690 unit:juju-gui/0: hook.output INFO: copyfile(src, dst)
  File "/usr/lib/python2.7/shutil.py", line 82, in copyfile
    with open(src, 'rb') as fsrc:
IOError: [Errno 2] No such file or directory:
'/var/lib/juju/units/juju-gui-0/charm/hooks/../config/haproxy.conf'

2013-08-06 15:46:12,850 unit:juju-gui/0: hook.output DEBUG: hook config-changed
exited, exit code Traceback (most recent call last):
Failure: juju.errors.CharmInvocationError: Error processing
'/var/lib/juju/units/juju-gui-0/charm/hooks/config-changed': exit code 1.
.
2013-08-06 15:46:12,851 unit:juju-gui/0: hook.executor DEBUG: Hook error:
/var/lib/juju/units/juju-gui-0/charm/hooks/config-changed Error processing
'/var/lib/juju/units/juju-gui-0/charm/hooks/config-changed': exit code 1.
2013-08-06 15:46:12,852 unit:juju-gui/0: statemachine DEBUG: unitworkflowstate: 
executing error transition error_configure, Error processing
'/var/lib/juju/units/juju-gui-0/charm/hooks/config-changed': exit code 1.
2013-08-06 15:46:12,885 unit:juju-gui/0: statemachine DEBUG: unitworkflowstate:
transition error_configure (started -> configure_error) {}



Please ensure we can successfully change the charm options before re-proposing.
I think the juju-gui-source and staging options (back and forth) are good
enough.
I am starting to think that, given how the charm is structured, "juju set"
functional tests are more valuable than the "juju deploy --config" ones we are
currently using. We will eventually implement those, but for the time being,
live tests and QA work well for catching this kind of errors.

[teknico, 2013-08-06 14:20:12]

Please take a look.

[teknico, 2013-08-06 14:44:00]

frankban wrote:
> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode113
> hooks/utils.py:113: HAPROXY_PATH = '/etc/haproxy/haproxy.cfg'
> Minor: add those new consts to __all__?

Actually I'll *remove* from __all__ the names that are not used outside the
module. I'll do that in the next branch though, this one is too big already.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode339
> hooks/utils.py:339: def
> stop_improv(config_path='/etc/init/juju-api-improv.conf'):
> Is this kwarg used somewhere? If not, what do you think about removing
> it?

Done, and in start_improv above too.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode340
> hooks/utils.py:340: """Stop a simulated juju environment."""
> Minor: Juju, capital J.

Done.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode355
> hooks/utils.py:355: SYS_INIT_DIR, os.path.basename(unit_dir))
> I am not sure about the value of having a /etc/init constant.
> Anyway, I think os.path.join() can help calculating this path.

Done.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode371
> hooks/utils.py:371: def
stop_agent(config_path='/etc/init/juju-api-agent.conf'):
> Same config_path question as above.

Removed.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode475
> hooks/utils.py:475: if not os.path.exists(APACHE_SITE):
> Why touch is used before generating the file? Couldn't we just
> generate the files with render_to_file, and proceeding with
> chown/ensite after the fact?

Yes, now that the Apache config code is all in one place we can. :-) Done.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode499
> hooks/utils.py:499: def remove_apache_setup():
> It seems that these functions (remove_apache_setup, start_haproxy_apache,
> remove_haproxy_setup etc...) are not tested. Have you considered adding
> unit tests for those helpers?

Right, added.


> https://codereview.appspot.com/12467044/diff/1/server/runserver.py#newcode27
> server/runserver.py:27: --sslpath is ignored if --insecure is set: only one of
> them is needed.
> None of them is actually required. --insecure defaults to False, --sslpath to
> the default path defined for convenience in the server. I believe we could
just
> write "The --sslpath option is ignored if --insecure is set." or similar.

Done.


>
https://codereview.appspot.com/12467044/diff/1/tests/20-functional.test#newco...
> tests/20-functional.test:207: def test_force_machine(self):
> IIRC, you mentioned that this test no longer passes. Please add a card for
> fixing this problem. This might be related to the fact that force-machine has
> been renamend to "--to" in the juju deploy parameters.

Uhm, I don't remember mentioning anything of the sort, maybe someone else?

[teknico, 2013-08-06 15:24:06]

>
https://codereview.appspot.com/12467044/diff/1/tests/20-functional.test#newco...
>> tests/20-functional.test:207: def test_force_machine(self):
>> IIRC, you mentioned that this test no longer passes. Please add a card
>> for fixing this problem. This might be related to the fact that
>> force-machine has been renamend to "--to" in the juju deploy parameters.

> Uhm, I don't remember mentioning anything of the sort, maybe someone else?

Actually, this test is indeed passing with juju-core.

[bac, 2013-08-06 15:50:54]

LGTM: Code looks good and QA was fine.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode137
hooks/utils.py:137: """, re.VERBOSE)
Thank you for commenting the RE.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode339
hooks/utils.py:339: def
stop_improv(config_path='/etc/init/juju-api-improv.conf'):
On 2013/08/06 12:57:24, frankban wrote:
> Is this kwarg used somewhere? If not, what do you think about removing it?

Are you referring to config_path?  I'm confused.

https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode355
hooks/utils.py:355: SYS_INIT_DIR, os.path.basename(unit_dir))
SYS_INIT_DIR is used twice so perhaps having it around at least protects against
typos, so keeping it is not a terrible idea.

+1 on os.path.join

[teknico, 2013-08-06 15:52:53]

bac wrote:
> LGTM: Code looks good and QA was fine.

Great, thanks.


> https://codereview.appspot.com/12467044/diff/1/hooks/utils.py#newcode137
> hooks/utils.py:137: """, re.VERBOSE)
> Thank you for commenting the RE.

That's not my work (it's frankban's, IIRC), I just moved it to the top of the
file. :-)

[teknico, 2013-08-07 13:15:51]

Please take a look.

[bac, 2013-08-07 13:24:12]

LGTM with one question

https://codereview.appspot.com/12467044/diff/7002/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/12467044/diff/7002/hooks/utils.py#newcode486
hooks/utils.py:486: cmd_log(run('ln', '-s', APACHE_SITE, APACHE_ENABLED))
Isn't this what the a2ensite below does?

[teknico, 2013-08-07 13:43:39]

bac wrote:
> https://codereview.appspot.com/12467044/diff/7002/hooks/utils.py#newcode486
> hooks/utils.py:486: cmd_log(run('ln', '-s', APACHE_SITE, APACHE_ENABLED))
> Isn't this what the a2ensite below does?

Oh, you're right, thanks, removing.

[teknico, 2013-08-08 09:17:31]

*** Submitted:

Refactor the config generation logic.

As requested by reviews, change the server configuration generation logic
so that the various server configuration files are present on the
filesystem only for the actually deployed and started servers.

This is implemented by creating additional start_* and stop_* functions
in hooks/utils.py, called by the mixins in hooks/backend.py, and moving
there/creating the logic that generates, writes and remove the various
configuration files.

In addition:

- factor out common SSL code between HaproxyApacheMixin and
  BuiltinServerMixin;
- rename options.http to options.insecure in server/guiserver/manage.py
- remove unused code and backend properties;
- miscellaneous cleanup.

The diff is still a bit big, even after moving the builtin server
integration changes to another branch. Sorry about that.

R=frankban, bac
CC=
https://codereview.appspot.com/12467044