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

[JuanJo Ciarlante]

[Mike McCracken (mikemc)]

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

[Mike McCracken (mikemc)]

@jjo, if you'd like to look at the results and comment:

using lockf:
https://github.com/Ubuntu-Solutions-Engineering/glance-simplestreams-sync-charm/commit/6d40c4d6e6ff1fa349d6df749b3218758ac7d560

getting env from .juju-proxy:
https://github.com/Ubuntu-Solutions-Engineering/glance-simplestreams-sync-charm/commit/24779ff50c3a2922c73a72495fa2ec8eeb77d847

Thanks again for the patch!

[JuanJo Ciarlante (jjo)]

Thanks Mike! , commented on both at github.

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-06-18 18:15:29 +0000
+++ hooks/hooks.py	2015-02-06 19:29:03 +0000
@@ -41,6 +41,7 @@
 ID_CONF_FILE_NAME = os.path.join(CONF_FILE_DIR, 'identity.yaml')
 
 SCRIPT_NAME = "glance-simplestreams-sync.py"
+CRON_SCRIPT_NAME = "glance-simplestreams-sync.cron"
 
 CRON_POLL_FILENAME = 'glance_simplestreams_sync_fastpoll'
 CRON_POLL_FILEPATH = os.path.join('/etc/cron.d', CRON_POLL_FILENAME)
@@ -81,12 +82,14 @@
 
     """
     sync_script_source = os.path.join("scripts", SCRIPT_NAME)
+    cron_script_source = os.path.join("scripts", CRON_SCRIPT_NAME)
     shutil.copy(sync_script_source, USR_SHARE_DIR)
+    shutil.copy(cron_script_source, USR_SHARE_DIR)
 
     config = hookenv.config()
-    installed_script = os.path.join(USR_SHARE_DIR, SCRIPT_NAME)
+    installed_script = os.path.join(USR_SHARE_DIR, CRON_SCRIPT_NAME)
     linkname = '/etc/cron.{f}/{s}'.format(f=config['frequency'],
-                                          s=SCRIPT_NAME)
+                                          s=CRON_SCRIPT_NAME)
     os.symlink(installed_script, linkname)
 
     poll_file_source = os.path.join('scripts', CRON_POLL_FILENAME)
@@ -96,7 +99,7 @@
 def uninstall_cron_scripts():
     """Removes sync program from any place it might be, and removes
     polling cron job."""
-    for fn in glob.glob("/etc/cron.*/" + SCRIPT_NAME):
+    for fn in glob.glob("/etc/cron.*/" + CRON_SCRIPT_NAME):
         if os.path.exists(fn):
             os.remove(fn)
 

=== added file 'scripts/glance-simplestreams-sync.cron'
--- scripts/glance-simplestreams-sync.cron	1970-01-01 00:00:00 +0000
+++ scripts/glance-simplestreams-sync.cron	2015-02-06 19:29:03 +0000
@@ -0,0 +1,3 @@
+#!/bin/bash
+test -f /home/ubuntu/.juju-proxy && source /home/ubuntu/.juju-proxy
+exec /usr/share/glance-simplestreams-sync/glance-simplestreams-sync.py

=== modified file 'scripts/glance-simplestreams-sync.py'
--- scripts/glance-simplestreams-sync.py	2014-06-18 18:15:29 +0000
+++ scripts/glance-simplestreams-sync.py	2015-02-06 19:29:03 +0000
@@ -50,15 +50,18 @@
 from simplestreams.objectstores.swift import SwiftObjectStore
 from simplestreams.util import read_signed, path_from_mirror_url
 import sys
+import fcntl
 from urlparse import urlsplit
 import yaml
 
 KEYRING = '/usr/share/keyrings/ubuntu-cloudimage-keyring.gpg'
 CONF_FILE_DIR = '/etc/glance-simplestreams-sync'
+PID_FILE_DIR = '/var/run'
 CHARM_CONF_FILE_NAME = os.path.join(CONF_FILE_DIR, 'mirrors.yaml')
 ID_CONF_FILE_NAME = os.path.join(CONF_FILE_DIR, 'identity.yaml')
 
-SYNC_RUNNING_FLAG_FILE_NAME = os.path.join(CONF_FILE_DIR, 'sync-running.pid')
+SYNC_RUNNING_FLAG_FILE_NAME = os.path.join(PID_FILE_DIR,
+                                           'glance-simplestreams-sync.pid')
 
 # juju looks in simplestreams/data/* in swift to figure out which
 # images to deploy, so this path isn't really configurable even though
@@ -160,7 +163,7 @@
     swift_services = [s for s in services
                       if s['name'] == 'swift']
     if len(swift_services) != 1:
-        log.error("found %d swift services. expecting one."
+        log.error("found {} swift services. expecting one."
                   " - not updating endpoint.".format(len(swift_services)))
         return
 
@@ -172,7 +175,7 @@
     swift_endpoints = [e for e in endpoints
                        if e['service_id'] == swift_service_id]
     if len(swift_endpoints) != 1:
-        log.warning("found %d swift endpoints, expecting one - not"
+        log.warning("found {} swift endpoints, expecting one - not"
                     " updating product-streams"
                     " endpoint.".format(len(swift_endpoints)))
         return
@@ -182,7 +185,7 @@
     ps_services = [s for s in services
                    if s['name'] == PRODUCT_STREAMS_SERVICE_NAME]
     if len(ps_services) != 1:
-        log.error("found %d product-streams services. expecting one."
+        log.error("found {} product-streams services. expecting one."
                   " - not updating endpoint.".format(len(ps_services)))
         return
 
@@ -192,9 +195,9 @@
                     if e['service_id'] == ps_service_id]
 
     if len(ps_endpoints) != 1:
-        log.warning("found %d product-streams endpoints in region {},"
+        log.warning("found {} product-streams endpoints in region {},"
                     " expecting one - not updating"
-                    " endpoint".format(region,
+                    " endpoint".format(ps_endpoints, region,
                                        len(ps_endpoints)))
         return
 
@@ -205,7 +208,7 @@
                            if t.name == 'services']
 
     if len(services_tenant_ids) != 1:
-        log.warning("found %d tenants named 'services',"
+        log.warning("found {} tenants named 'services',"
                     " expecting one. Not updating"
                     " endpoint".format(len(services_tenant_ids)))
 
@@ -242,14 +245,16 @@
 
     log.info("glance-simplestreams-sync started.")
 
-    if os.path.exists(SYNC_RUNNING_FLAG_FILE_NAME):
-        log.info("sync started while pidfile exists, exiting")
+    atexit.register(cleanup)
+
+    lockfile = open(SYNC_RUNNING_FLAG_FILE_NAME, 'w')
+    try:
+        fcntl.flock(lockfile, fcntl.LOCK_EX | fcntl.LOCK_NB)
+    except IOError:
+        log.info("{} is locked, exiting".format(SYNC_RUNNING_FLAG_FILE_NAME))
         sys.exit(0)
 
-    atexit.register(cleanup)
-
-    with open(SYNC_RUNNING_FLAG_FILE_NAME, 'w') as f:
-        f.write(str(os.getpid()))
+    lockfile.write(str(os.getpid()))
 
     id_conf, charm_conf = get_conf()