Merge lp:~jjo/charms/trusty/glance-simplestreams-sync/use-lockf-to-fix-stale-pidfiles_and_support-juju-proxy-settings into lp:charms/trusty/glance-simplestreams-sync

Proposed by JuanJo Ciarlante on 2015-02-06
Status: Merged
Merge reported by: James Page
Merged at revision: not available
Proposed branch: lp:~jjo/charms/trusty/glance-simplestreams-sync/use-lockf-to-fix-stale-pidfiles_and_support-juju-proxy-settings
Merge into: lp:charms/trusty/glance-simplestreams-sync
Diff against target: 139 lines (+27/-16)
3 files modified
hooks/hooks.py (+6/-3)
scripts/glance-simplestreams-sync.cron (+3/-0)
scripts/glance-simplestreams-sync.py (+18/-13)
To merge this branch: bzr merge lp:~jjo/charms/trusty/glance-simplestreams-sync/use-lockf-to-fix-stale-pidfiles_and_support-juju-proxy-settings
Reviewer Review Type Date Requested Status
Mike McCracken (community) 2015-02-06 Disapprove on 2015-02-17
charmers 2015-02-06 Pending
Review via email: mp+248958@code.launchpad.net
To post a comment you must log in.
Mike McCracken (mikemc) wrote :

Thanks for the changes! Sorry this took so long to get to.

This charm is currently maintained on github, so I'm going to reject this MP and incorporate the changes there.

The lockf change will go in as-is.

I'm going to alter the change to support proxies though - I'd prefer to just have the python script look for the env file instead of adding another wrapper. I am surprised that the wrapper actually works as-is, because cron doesn't like dashes in names (see https://github.com/Ubuntu-Solutions-Engineering/glance-simplestreams-sync-charm/commit/c93f8b96e56de7aaa8e84a87c024f6d091137a79 ).
Also, the wrapper as-is doesn't wrap the 'fastpoll' cron job, which still calls the python script directly and will not get the env.

review: Disapprove
JuanJo Ciarlante (jjo) wrote :

Thanks Mike! , commented on both at github.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/hooks.py'
2--- hooks/hooks.py 2014-06-18 18:15:29 +0000
3+++ hooks/hooks.py 2015-02-06 19:29:03 +0000
4@@ -41,6 +41,7 @@
5 ID_CONF_FILE_NAME = os.path.join(CONF_FILE_DIR, 'identity.yaml')
6
7 SCRIPT_NAME = "glance-simplestreams-sync.py"
8+CRON_SCRIPT_NAME = "glance-simplestreams-sync.cron"
9
10 CRON_POLL_FILENAME = 'glance_simplestreams_sync_fastpoll'
11 CRON_POLL_FILEPATH = os.path.join('/etc/cron.d', CRON_POLL_FILENAME)
12@@ -81,12 +82,14 @@
13
14 """
15 sync_script_source = os.path.join("scripts", SCRIPT_NAME)
16+ cron_script_source = os.path.join("scripts", CRON_SCRIPT_NAME)
17 shutil.copy(sync_script_source, USR_SHARE_DIR)
18+ shutil.copy(cron_script_source, USR_SHARE_DIR)
19
20 config = hookenv.config()
21- installed_script = os.path.join(USR_SHARE_DIR, SCRIPT_NAME)
22+ installed_script = os.path.join(USR_SHARE_DIR, CRON_SCRIPT_NAME)
23 linkname = '/etc/cron.{f}/{s}'.format(f=config['frequency'],
24- s=SCRIPT_NAME)
25+ s=CRON_SCRIPT_NAME)
26 os.symlink(installed_script, linkname)
27
28 poll_file_source = os.path.join('scripts', CRON_POLL_FILENAME)
29@@ -96,7 +99,7 @@
30 def uninstall_cron_scripts():
31 """Removes sync program from any place it might be, and removes
32 polling cron job."""
33- for fn in glob.glob("/etc/cron.*/" + SCRIPT_NAME):
34+ for fn in glob.glob("/etc/cron.*/" + CRON_SCRIPT_NAME):
35 if os.path.exists(fn):
36 os.remove(fn)
37
38
39=== added file 'scripts/glance-simplestreams-sync.cron'
40--- scripts/glance-simplestreams-sync.cron 1970-01-01 00:00:00 +0000
41+++ scripts/glance-simplestreams-sync.cron 2015-02-06 19:29:03 +0000
42@@ -0,0 +1,3 @@
43+#!/bin/bash
44+test -f /home/ubuntu/.juju-proxy && source /home/ubuntu/.juju-proxy
45+exec /usr/share/glance-simplestreams-sync/glance-simplestreams-sync.py
46
47=== modified file 'scripts/glance-simplestreams-sync.py'
48--- scripts/glance-simplestreams-sync.py 2014-06-18 18:15:29 +0000
49+++ scripts/glance-simplestreams-sync.py 2015-02-06 19:29:03 +0000
50@@ -50,15 +50,18 @@
51 from simplestreams.objectstores.swift import SwiftObjectStore
52 from simplestreams.util import read_signed, path_from_mirror_url
53 import sys
54+import fcntl
55 from urlparse import urlsplit
56 import yaml
57
58 KEYRING = '/usr/share/keyrings/ubuntu-cloudimage-keyring.gpg'
59 CONF_FILE_DIR = '/etc/glance-simplestreams-sync'
60+PID_FILE_DIR = '/var/run'
61 CHARM_CONF_FILE_NAME = os.path.join(CONF_FILE_DIR, 'mirrors.yaml')
62 ID_CONF_FILE_NAME = os.path.join(CONF_FILE_DIR, 'identity.yaml')
63
64-SYNC_RUNNING_FLAG_FILE_NAME = os.path.join(CONF_FILE_DIR, 'sync-running.pid')
65+SYNC_RUNNING_FLAG_FILE_NAME = os.path.join(PID_FILE_DIR,
66+ 'glance-simplestreams-sync.pid')
67
68 # juju looks in simplestreams/data/* in swift to figure out which
69 # images to deploy, so this path isn't really configurable even though
70@@ -160,7 +163,7 @@
71 swift_services = [s for s in services
72 if s['name'] == 'swift']
73 if len(swift_services) != 1:
74- log.error("found %d swift services. expecting one."
75+ log.error("found {} swift services. expecting one."
76 " - not updating endpoint.".format(len(swift_services)))
77 return
78
79@@ -172,7 +175,7 @@
80 swift_endpoints = [e for e in endpoints
81 if e['service_id'] == swift_service_id]
82 if len(swift_endpoints) != 1:
83- log.warning("found %d swift endpoints, expecting one - not"
84+ log.warning("found {} swift endpoints, expecting one - not"
85 " updating product-streams"
86 " endpoint.".format(len(swift_endpoints)))
87 return
88@@ -182,7 +185,7 @@
89 ps_services = [s for s in services
90 if s['name'] == PRODUCT_STREAMS_SERVICE_NAME]
91 if len(ps_services) != 1:
92- log.error("found %d product-streams services. expecting one."
93+ log.error("found {} product-streams services. expecting one."
94 " - not updating endpoint.".format(len(ps_services)))
95 return
96
97@@ -192,9 +195,9 @@
98 if e['service_id'] == ps_service_id]
99
100 if len(ps_endpoints) != 1:
101- log.warning("found %d product-streams endpoints in region {},"
102+ log.warning("found {} product-streams endpoints in region {},"
103 " expecting one - not updating"
104- " endpoint".format(region,
105+ " endpoint".format(ps_endpoints, region,
106 len(ps_endpoints)))
107 return
108
109@@ -205,7 +208,7 @@
110 if t.name == 'services']
111
112 if len(services_tenant_ids) != 1:
113- log.warning("found %d tenants named 'services',"
114+ log.warning("found {} tenants named 'services',"
115 " expecting one. Not updating"
116 " endpoint".format(len(services_tenant_ids)))
117
118@@ -242,14 +245,16 @@
119
120 log.info("glance-simplestreams-sync started.")
121
122- if os.path.exists(SYNC_RUNNING_FLAG_FILE_NAME):
123- log.info("sync started while pidfile exists, exiting")
124+ atexit.register(cleanup)
125+
126+ lockfile = open(SYNC_RUNNING_FLAG_FILE_NAME, 'w')
127+ try:
128+ fcntl.flock(lockfile, fcntl.LOCK_EX | fcntl.LOCK_NB)
129+ except IOError:
130+ log.info("{} is locked, exiting".format(SYNC_RUNNING_FLAG_FILE_NAME))
131 sys.exit(0)
132
133- atexit.register(cleanup)
134-
135- with open(SYNC_RUNNING_FLAG_FILE_NAME, 'w') as f:
136- f.write(str(os.getpid()))
137+ lockfile.write(str(os.getpid()))
138
139 id_conf, charm_conf = get_conf()
140

Subscribers

People subscribed via source and target branches