Merge lp:~allenap/maas/cluster-worker-logging into lp:~maas-committers/maas/trunk

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 1293
Proposed branch: lp:~allenap/maas/cluster-worker-logging
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 228 lines (+32/-55)
9 files modified
etc/celeryconfig_common.py (+4/-3)
etc/democeleryconfig.py (+1/-1)
etc/democeleryconfig_cluster.py (+2/-2)
src/provisioningserver/boot_images.py (+4/-1)
src/provisioningserver/dhcp/leases.py (+5/-1)
src/provisioningserver/logging.py (+0/-22)
src/provisioningserver/start_cluster_controller.py (+9/-20)
src/provisioningserver/tags.py (+6/-5)
src/provisioningserver/tests/test_start_cluster_controller.py (+1/-0)
To merge this branch: bzr merge lp:~allenap/maas/cluster-worker-logging
Reviewer Review Type Date Requested Status
John A Meinel (community) Approve
Raphaël Badin (community) Approve
Review via email: mp+130576@code.launchpad.net

Commit message

Capture log messages emitted during cluster worker start-up.

Previously these messages were being lost, so debugging the start-up of a cluster worker was difficult.

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

Looks good.

[0]

101 + setup_logging_subsystem(loglevel="INFO", logfile=get_maas_celery_log())

I think you should make the log level a configuration option in the celery config file.

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

> Looks good.
>
> [0]
>
> 101     +    setup_logging_subsystem(loglevel="INFO",
> logfile=get_maas_celery_log())
>
> I think you should make the log level a configuration option in the celery
> config file.

Yeah, I've discovered that this is already supported. I can remove the
arguments to setup_logging_subsystem and use CELERYD_LOG_LEVEL and
CELERYD_LOG_FILE instead. The latter means I can also get rid of
MAAS_CELERY_LOG.

Likewise, MAAS_CLUSTER_CELERY_DB can also be replaced with
CELERYBEAT_SCHEDULE_FILENAME.

Thanks for the review. I'll make these other changes next week before
I land this.

Revision history for this message
Gavin Panella (allenap) wrote :

Hi Raphael, or anyone else who reads this, please would you mind taking a look at rev 1294? Thanks!

Revision history for this message
John A Meinel (jameinel) wrote :

So I think the specific change of 1294 is reasonable, but does it actually handle the case of 'start a new cluster-controller, point it at the region controller, and get some error logging while it is has not been accepted yet'?

I guess the biggest change is calling 'setup_logging_subsystem()' earlier, which should handle that.

So the other bit about getting rid of logging.task_logger and instead using a separate task_logger for each module. Why is that actually necessary?

Breaking it down:
1) Using CELERYD_LOG_FILE et al: Definitely good
2) Calling setup_logging_subsystem() earlier: Definitely good
3) Using separate task_loggers for each module: What is the purpose behind this? If you have a reason, good enough.

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

Thanks for the review!

> 3) Using separate task_loggers for each module: What is the purpose
> behind this? If you have a reason, good enough.

This started out as another approach at the problem. I realised it
wasn't the right approach, but decided to leave the change because I
think it adheres closer to Celery's recommended practice (and, indeed,
standard Python use of the logging module).

Revision history for this message
Gavin Panella (allenap) wrote :

> So I think the specific change of 1294 is reasonable, but does it
> actually handle the case of 'start a new cluster-controller, point
> it at the region controller, and get some error logging while it is
> has not been accepted yet'?

I forgot to answer this. Yes, it does. You can try it out with:

  CELERY_CONFIG_MODULE=democeleryconfig_cluster \
    bin/maas-provision start-cluster-controller http://localhost/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'etc/celeryconfig_common.py'
--- etc/celeryconfig_common.py 2012-10-16 10:10:28 +0000
+++ etc/celeryconfig_common.py 2012-10-23 12:54:22 +0000
@@ -46,11 +46,12 @@
46# this setting.46# this setting.
47BROKER_URL = 'amqp://guest:guest@localhost:5672//'47BROKER_URL = 'amqp://guest:guest@localhost:5672//'
4848
49# Location for log file.49# Logging.
50MAAS_CELERY_LOG = '/var/log/maas/celery.log'50CELERYD_LOG_FILE = '/var/log/maas/celery.log'
51CELERYD_LOG_LEVEL = 'INFO'
5152
52# Location for the cluster worker schedule file.53# Location for the cluster worker schedule file.
53MAAS_CLUSTER_CELERY_DB = '/var/lib/maas/celerybeat-cluster-schedule'54CELERYBEAT_SCHEDULE_FILENAME = '/var/lib/maas/celerybeat-cluster-schedule'
5455
55WORKER_QUEUE_DNS = 'celery'56WORKER_QUEUE_DNS = 'celery'
56WORKER_QUEUE_BOOT_IMAGES = 'celery'57WORKER_QUEUE_BOOT_IMAGES = 'celery'
5758
=== modified file 'etc/democeleryconfig.py'
--- etc/democeleryconfig.py 2012-10-03 14:18:17 +0000
+++ etc/democeleryconfig.py 2012-10-23 12:54:22 +0000
@@ -26,5 +26,5 @@
2626
27import_settings(democeleryconfig_common)27import_settings(democeleryconfig_common)
2828
29MAAS_CELERY_LOG = os.path.join(29CELERYD_LOG_FILE = os.path.join(
30 DEV_ROOT_DIRECTORY, 'logs/region-worker/current')30 DEV_ROOT_DIRECTORY, 'logs/region-worker/current')
3131
=== modified file 'etc/democeleryconfig_cluster.py'
--- etc/democeleryconfig_cluster.py 2012-10-11 12:42:06 +0000
+++ etc/democeleryconfig_cluster.py 2012-10-23 12:54:22 +0000
@@ -30,8 +30,8 @@
30# maas_local_celeryconfig.30# maas_local_celeryconfig.
31CLUSTER_UUID = "adfd3977-f251-4f2c-8d61-745dbd690bfc"31CLUSTER_UUID = "adfd3977-f251-4f2c-8d61-745dbd690bfc"
3232
33MAAS_CELERY_LOG = os.path.join(33CELERYD_LOG_FILE = os.path.join(
34 DEV_ROOT_DIRECTORY, 'logs/cluster-worker/current')34 DEV_ROOT_DIRECTORY, 'logs/cluster-worker/current')
3535
36MAAS_CLUSTER_CELERY_DB = os.path.join(36CELERYBEAT_SCHEDULE_FILENAME = os.path.join(
37 DEV_ROOT_DIRECTORY, 'run/celerybeat-cluster-schedule')37 DEV_ROOT_DIRECTORY, 'run/celerybeat-cluster-schedule')
3838
=== modified file 'src/provisioningserver/boot_images.py'
--- src/provisioningserver/boot_images.py 2012-09-14 11:04:33 +0000
+++ src/provisioningserver/boot_images.py 2012-10-23 12:54:22 +0000
@@ -25,15 +25,18 @@
25 MAASDispatcher,25 MAASDispatcher,
26 MAASOAuth,26 MAASOAuth,
27 )27 )
28from celery.log import get_task_logger
28from provisioningserver.auth import (29from provisioningserver.auth import (
29 get_recorded_api_credentials,30 get_recorded_api_credentials,
30 get_recorded_maas_url,31 get_recorded_maas_url,
31 )32 )
32from provisioningserver.config import Config33from provisioningserver.config import Config
33from provisioningserver.logging import task_logger
34from provisioningserver.pxe import tftppath34from provisioningserver.pxe import tftppath
3535
3636
37task_logger = get_task_logger(name=__name__)
38
39
37def get_cached_knowledge():40def get_cached_knowledge():
38 """Return cached items required to report to the server.41 """Return cached items required to report to the server.
3942
4043
=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py 2012-10-04 12:24:31 +0000
+++ src/provisioningserver/dhcp/leases.py 2012-10-23 12:54:22 +0000
@@ -45,6 +45,7 @@
45 MAASOAuth,45 MAASOAuth,
46 )46 )
47from celery.app import app_or_default47from celery.app import app_or_default
48from celery.log import get_task_logger
48from provisioningserver import cache49from provisioningserver import cache
49from provisioningserver.auth import (50from provisioningserver.auth import (
50 get_recorded_api_credentials,51 get_recorded_api_credentials,
@@ -52,7 +53,10 @@
52 get_recorded_nodegroup_uuid,53 get_recorded_nodegroup_uuid,
53 )54 )
54from provisioningserver.dhcp.leases_parser import parse_leases55from provisioningserver.dhcp.leases_parser import parse_leases
55from provisioningserver.logging import task_logger56
57
58task_logger = get_task_logger(name=__name__)
59
5660
57# Cache key for the modification time on last-processed leases file.61# Cache key for the modification time on last-processed leases file.
58LEASES_TIME_CACHE_KEY = 'leases_time'62LEASES_TIME_CACHE_KEY = 'leases_time'
5963
=== removed file 'src/provisioningserver/logging.py'
--- src/provisioningserver/logging.py 2012-08-10 14:15:23 +0000
+++ src/provisioningserver/logging.py 1970-01-01 00:00:00 +0000
@@ -1,22 +0,0 @@
1# Copyright 2012 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Celery logging."""
5
6from __future__ import (
7 absolute_import,
8 print_function,
9 unicode_literals,
10 )
11
12__metaclass__ = type
13__all__ = [
14 'task_logger',
15 ]
16
17
18from celery.log import get_task_logger
19
20# Celery task logger. Shared between tasks, as per Celery's recommended
21# practice.
22task_logger = get_task_logger()
230
=== modified file 'src/provisioningserver/start_cluster_controller.py'
--- src/provisioningserver/start_cluster_controller.py 2012-10-03 18:16:22 +0000
+++ src/provisioningserver/start_cluster_controller.py 2012-10-23 12:54:22 +0000
@@ -32,10 +32,16 @@
32 NoAuth,32 NoAuth,
33 )33 )
34from celery.app import app_or_default34from celery.app import app_or_default
35from provisioningserver.logging import task_logger35from celery.log import (
36 get_task_logger,
37 setup_logging_subsystem,
38 )
36from provisioningserver.network import discover_networks39from provisioningserver.network import discover_networks
3740
3841
42task_logger = get_task_logger(name=__name__)
43
44
39class ClusterControllerRejected(Exception):45class ClusterControllerRejected(Exception):
40 """Request to become a cluster controller has been rejected."""46 """Request to become a cluster controller has been rejected."""
4147
@@ -68,16 +74,6 @@
68 return app_or_default().conf.CLUSTER_UUID74 return app_or_default().conf.CLUSTER_UUID
6975
7076
71def get_maas_celery_log():
72 """Read location for MAAS Celery log file from the config."""
73 return app_or_default().conf.MAAS_CELERY_LOG
74
75
76def get_maas_celerybeat_db():
77 """Read location for MAAS Celery schedule file from the config."""
78 return app_or_default().conf.MAAS_CLUSTER_CELERY_DB
79
80
81def register(server_url):77def register(server_url):
82 """Request Rabbit connection details from the domain controller.78 """Request Rabbit connection details from the domain controller.
8379
@@ -134,15 +130,7 @@
134130
135 # Copy environment, but also tell celeryd what broker to listen to.131 # Copy environment, but also tell celeryd what broker to listen to.
136 env = dict(os.environ, CELERY_BROKER_URL=broker_url)132 env = dict(os.environ, CELERY_BROKER_URL=broker_url)
137133 command = 'celeryd', '--beat', '--queues', get_cluster_uuid()
138 command = [
139 'celeryd',
140 '--logfile=%s' % get_maas_celery_log(),
141 '--schedule=%s' % get_maas_celerybeat_db(),
142 '--loglevel=INFO',
143 '--beat',
144 '-Q', get_cluster_uuid(),
145 ]
146134
147 # Change gid first, just in case changing the uid might deprive135 # Change gid first, just in case changing the uid might deprive
148 # us of the privileges required to setgid.136 # us of the privileges required to setgid.
@@ -182,6 +170,7 @@
182 If this system is still awaiting approval as a cluster controller, this170 If this system is still awaiting approval as a cluster controller, this
183 command will keep looping until it gets a definite answer.171 command will keep looping until it gets a definite answer.
184 """172 """
173 setup_logging_subsystem()
185 connection_details = register(args.server_url)174 connection_details = register(args.server_url)
186 while connection_details is None:175 while connection_details is None:
187 sleep(60)176 sleep(60)
188177
=== modified file 'src/provisioningserver/tags.py'
--- src/provisioningserver/tags.py 2012-10-11 10:41:00 +0000
+++ src/provisioningserver/tags.py 2012-10-23 12:54:22 +0000
@@ -19,22 +19,23 @@
1919
2020
21import httplib21import httplib
22import simplejson as json
23from lxml import etree
2422
25from apiclient.maas_client import (23from apiclient.maas_client import (
26 MAASClient,24 MAASClient,
27 MAASDispatcher,25 MAASDispatcher,
28 MAASOAuth,26 MAASOAuth,
29 )27 )
3028from celery.log import get_task_logger
29from lxml import etree
31from provisioningserver.auth import (30from provisioningserver.auth import (
32 get_recorded_api_credentials,31 get_recorded_api_credentials,
33 get_recorded_maas_url,32 get_recorded_maas_url,
34 get_recorded_nodegroup_uuid,33 get_recorded_nodegroup_uuid,
35 )34 )
3635import simplejson as json
37from provisioningserver.logging import task_logger36
37
38task_logger = get_task_logger(name=__name__)
3839
3940
40class MissingCredentials(Exception):41class MissingCredentials(Exception):
4142
=== modified file 'src/provisioningserver/tests/test_start_cluster_controller.py'
--- src/provisioningserver/tests/test_start_cluster_controller.py 2012-10-05 17:29:05 +0000
+++ src/provisioningserver/tests/test_start_cluster_controller.py 2012-10-23 12:54:22 +0000
@@ -93,6 +93,7 @@
93 self.patch(os, 'setuid')93 self.patch(os, 'setuid')
94 self.patch(os, 'setgid')94 self.patch(os, 'setgid')
95 self.patch(os, 'execvpe').side_effect = Executing()95 self.patch(os, 'execvpe').side_effect = Executing()
96 self.patch(start_cluster_controller, 'setup_logging_subsystem')
96 get_uuid = self.patch(start_cluster_controller, 'get_cluster_uuid')97 get_uuid = self.patch(start_cluster_controller, 'get_cluster_uuid')
97 get_uuid.return_value = factory.getRandomUUID()98 get_uuid.return_value = factory.getRandomUUID()
9899