Merge lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir into lp:~juju-qa/charms/precise/juju-reports/trunk

Proposed by Abel Deuring
Status: Merged
Merged at revision: 48
Proposed branch: lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir
Merge into: lp:~juju-qa/charms/precise/juju-reports/trunk
Diff against target: 294 lines (+109/-56)
2 files modified
hooks/common.py (+109/-47)
hooks/install (+0/-9)
To merge this branch: bzr merge lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+242230@code.launchpad.net

Description of the change

A (partial) fix for bug 1357390 and 1357391. The assumption is that these bugs occur when a a new juju-reports revision is installed: Some libraries needed by cron script are not available during the upgrade.

The main idea of this change: When a new revision is set, this revision is stored in a new directory, named juju-reports-REVNO. When "make prodinstall" finishes, a symlink named juju-reports is created that points to juju-reports-REVNO.

This works fine for revno upgrades, but it does not address another issue: When the charm config variable "dist-clean" is true, setting any config value will still cause cronscript failures until "make prodinstall" finishes.

This could be fixed in a follow-up branch, which could probably also move the stop() call in update_from_config() closer to the start() call.

To post a comment you must log in.
Aaron Bentley (abentley) wrote :

Why use a symlink? Why not just rename the new project dir into place?

review: Needs Fixing
Abel Deuring (adeuring) wrote :

If we rename the directoy, many scripts will break. As an example from a test run, the script bin/cbtest:

#!/home/ubuntu/juju-reports-170/bin/python
# EASY-INSTALL-ENTRY-SCRIPT: 'jujureports==0.0','console_scripts','cbtest'
__requires__ = 'jujureports==0.0'
import sys
from pkg_resources import load_entry_point

if __name__ == '__main__':
    sys.exit(
        load_entry_point('jujureports==0.0', 'console_scripts', 'cbtest')()
    )

51. By Abel Deuring on 2014-11-20

Use a new source directory in each call of update_source(); exception-based logic to distiguish between PROJECT_DIR being a directory or a symlink or not existing.

52. By Abel Deuring on 2014-11-20

reduce downtime during a config-changed hook: Call stop() immediaetly before make_project_link()

Abel Deuring (adeuring) wrote :

Another small change that became possible because update_soruce() now always uses a new directory: stop() can be called much later: Just before make_project_link(). This reduces the downtime when the config-changed hook is running.

Aaron Bentley (abentley) wrote :

Thanks for explaining why we need to use symlinks here.

Thanks for switching to EAFP. I disagree that Look Before You Leap vs Easier to Ask Forgiveness than permission is simply a matter of taste; EAFP is more correct because it does not have race conditions, and typically more efficient. In the symlink case, the symlink could be moved or deleted between the os.path.lexists, os.path.islink and os.readlink calls, so EAFP is less correct. It also requires three times as many system calls, so it is less efficient.

Now, it's true that two extra system calls don't really matter. But using three times as many system calls in order to be less correct does not make sense to me.

It also looks like you're pulling the branch from scratch, instead of taking advantage of the data in the pre-existing branch. That's going to make updates slower. Instead, you could use a shared repository, or you could branch from the old branch instead of using "bzr init".

I am also concerned that update_source is now going to do work for every config change. It was intended to only do work when the branch revision changed. Do we need to start by comparing revision-ids at the top of update_source and exiting early if there's no change?

review: Needs Fixing
53. By Abel Deuring on 2014-11-20

make_project_link(): use OSError.errno to figure out the exact error condition.

54. By Abel Deuring on 2014-11-21

Run install_production() only when the revno or the branch URL changes.

55. By Abel Deuring on 2014-11-21

Use a shared repository for the juju-reports branches.

56. By Abel Deuring on 2014-11-21

Do not remove an existing directory /juju-reports in the install hook.

Abel Deuring (adeuring) wrote :

The charm now uses a shared repository. I've also added some logic to check if the revno has changed between two calls of the config-changed hook: The current revno and parent branch URL are stored a file "last-source-revision"; update_source() and install_production() are now called only when the current config has other branch URL/revo values than before.

The config parameter "dist-clean" is has not real usage in the strict sense since juju-reports is now always built in a new directory; I've added logic so that update_source() and install_production() are always called when dist-clean is true. Similary, the revno value -1 cause a new build in each config-chaned run. (This should be improved: We can check if the parent branch has a new revision.)

Finally I noticed that the install hook still removed the directory $home/juju-reports. This is done in make_project_link(), so I removed it.

57. By Abel Deuring on 2014-11-21

do not automatically re-install juju-reports when the revno is set to -1

Aaron Bentley (abentley) wrote :

I think it is simpler and more accurate to check revision-id, as I suggested in my review, rather than revno and branch.

It is simpler because Bazaar tracks revision-id itself, so you don't need to add new config files. It is more accurate because changes to revno and branch do not necessarily imply that revision-id has changed. source can change to a different location with the same branch contents. revno can change from -1 to a positive value and still refer to the same revision-id. And when branch and revno are unchanged, this does not guarantee that revision-id is unchanged; if revno is -1, it will change as the branch is updated.

So I don't think this is the right way to handle this, but I also don't think this will cause practical problems because
1. we don't use -1 in production
2. we don't change source in production, and even if we did, it would just mean one extra period of downtime.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/common.py'
2--- hooks/common.py 2014-11-10 14:31:29 +0000
3+++ hooks/common.py 2014-11-21 16:47:34 +0000
4@@ -4,10 +4,10 @@
5 import errno
6 import os
7 from pwd import getpwnam
8-import re
9-from StringIO import StringIO
10+import shutil
11 import subprocess
12-import sys
13+from tempfile import mkdtemp
14+import yaml
15
16 from Cheetah.Template import Template
17
18@@ -18,28 +18,29 @@
19 HOME = '/home/ubuntu'
20 PROJECT_DIR = os.path.join(HOME, 'juju-reports')
21 INI = 'production.ini'
22-
23-
24-def get_ini_path():
25- return os.path.join(PROJECT_DIR, INI)
26-
27-
28-def get_ini():
29+REVISION_INFO_FILE = os.path.join(HOME, 'last-source-revision')
30+
31+
32+def get_ini_path(source_path):
33+ return os.path.join(source_path, INI)
34+
35+
36+def get_ini(source_path):
37 config = ConfigParser()
38- config.read(get_ini_path())
39+ config.read(get_ini_path(source_path))
40 return config
41
42
43-def get_template_path(name):
44- return os.path.join(PROJECT_DIR, 'resources/{}.template'.format(name))
45+def get_template_path(name, source_path):
46+ return os.path.join(source_path, 'resources/{}.template'.format(name))
47
48
49 def install_ini(mongo_url, gh_token, jujuci_url, token, error_email,
50- error_email_from, cloud_health_failure_threshold):
51+ error_email_from, cloud_health_failure_threshold, source_path):
52 config = ConfigParser()
53- path = get_template_path(INI)
54+ path = get_template_path(INI, source_path)
55 if not os.path.exists(path):
56- path = get_ini_path()
57+ path = get_ini_path(source_path)
58 config.read(path)
59 config.set('app:main', 'mongo.url', mongo_url)
60 config.set('app:main', 'gh_token', gh_token)
61@@ -51,12 +52,12 @@
62 config.set(
63 'app:main', 'cloud_health_failure_threshold',
64 cloud_health_failure_threshold)
65- config.write(open(get_ini_path(), 'w'))
66+ config.write(open(get_ini_path(source_path), 'w'))
67 return config
68
69
70-def configure_s3(config):
71- template_path = get_template_path('juju-qa.s3cfg')
72+def configure_s3(config, source_path):
73+ template_path = get_template_path('juju-qa.s3cfg', source_path)
74 try:
75 template_cm = open(template_path)
76 except IOError as e:
77@@ -65,7 +66,7 @@
78 return
79 with template_cm as template_file:
80 template = template_file.read()
81- config_path = os.path.join(PROJECT_DIR, 'juju-qa.s3cfg')
82+ config_path = os.path.join(source_path, 'juju-qa.s3cfg')
83 s3config = template.format(access_key=config['aws-access-key'],
84 secret_key=config['aws-secret-key'])
85 update_file(config_path, s3config)
86@@ -99,9 +100,9 @@
87 hookenv.close_port(port)
88
89
90-def call_script(script_path, root=False):
91+def call_script(script_path, source_path, root=False):
92 env = os.environ.copy()
93- ini = get_ini_path()
94+ ini = get_ini_path(source_path)
95 env['HOME'] = HOME
96 env['INI'] = ini
97 if root:
98@@ -110,20 +111,20 @@
99 args = ['su', 'ubuntu', '-p', '-c']
100
101 args.append(script_path)
102- subprocess.check_call(args, env=env, cwd=PROJECT_DIR)
103+ subprocess.check_call(args, env=env, cwd=source_path)
104 return env
105
106
107 def stop():
108 try:
109- call_script('scripts/stop', root=True)
110+ call_script('scripts/stop', PROJECT_DIR, root=True)
111 except OSError as e:
112 if e.errno != errno.ENOENT:
113 raise
114
115
116-def start():
117- call_script('scripts/run')
118+def start(source_path):
119+ call_script('scripts/run', source_path)
120
121
122 def open_secret(path, uid=None, gid=None):
123@@ -152,26 +153,39 @@
124
125
126 def update_source(source_url, revno):
127- # Make it easier to develop on a deploy
128- call_bzr(['bind', '-d', PROJECT_DIR, source_url])
129-
130 # Grab the code
131+ repository = os.path.join(HOME, 'juju-reports-repo')
132+ pwd = getpwnam('ubuntu')
133+ try:
134+ os.mkdir(repository)
135+ except OSError as e:
136+ if e.errno != errno.EEXIST:
137+ raise
138+ pass
139+ else:
140+ call_bzr(['init-repo', repository])
141+ os.chown(repository, pwd.pw_uid, pwd.pw_gid)
142+ target_dir = mkdtemp(prefix='juju-reports-%s-' % revno, dir=repository)
143 revno = 'revno:%s' % revno
144+ os.chown(target_dir, pwd.pw_uid, pwd.pw_gid)
145+ call_bzr(['init', target_dir])
146+ os.chown(target_dir, pwd.pw_uid, pwd.pw_gid)
147+
148+ # Make it easier to develop on a deploy
149+ call_bzr(['bind', '-d', target_dir, source_url])
150
151 bzr_cmd = ['pull', '--overwrite', source_url, '-r', revno, '-d',
152- PROJECT_DIR, '--remember', '--local']
153+ target_dir, '--remember', '--local']
154 call_bzr(bzr_cmd)
155-
156- # Restore the templates if they are missing/altered.
157- call_bzr(['revert', os.path.join(PROJECT_DIR, 'resources')])
158-
159-
160-def install_production(dist_clean, develop):
161- subprocess.check_call(['make', 'deploydeps'], cwd=PROJECT_DIR)
162+ return target_dir
163+
164+
165+def install_production(dist_clean, develop, source_path):
166+ subprocess.check_call(['make', 'deploydeps'], cwd=source_path)
167 if dist_clean:
168- subprocess.check_call(['make', 'distclean'], cwd=PROJECT_DIR)
169+ subprocess.check_call(['make', 'distclean'], cwd=source_path)
170 target = 'install' if develop else 'prodinstall'
171- subprocess.check_call(['make', target], cwd=PROJECT_DIR)
172+ subprocess.check_call(['make', target], cwd=source_path)
173 subprocess.check_call(['apt-get', 'autoremove', '--yes'])
174
175
176@@ -260,28 +274,73 @@
177 fileobj.write(new_contents)
178
179
180+def make_project_link(source_path, branch_url, revno):
181+ try:
182+ old_sources = os.readlink(PROJECT_DIR)
183+ except OSError as err:
184+ if err.errno == errno.EINVAL:
185+ shutil.rmtree(PROJECT_DIR)
186+ old_sources = None
187+ elif err.errno == errno.ENOENT:
188+ old_sources = None
189+ else:
190+ raise
191+ else:
192+ if old_sources == source_path:
193+ return
194+ os.remove(PROJECT_DIR)
195+ os.symlink(source_path, PROJECT_DIR)
196+ if old_sources is not None:
197+ shutil.rmtree(old_sources)
198+ with open(REVISION_INFO_FILE, 'w') as f:
199+ yaml.dump((branch_url, revno), f)
200+
201+
202+def get_last_revision():
203+ try:
204+ file_context = open(REVISION_INFO_FILE, 'r')
205+ except IOError as e:
206+ if e.errno == errno.ENOENT:
207+ return None, None
208+ else:
209+ raise
210+ with file_context:
211+ return yaml.load(file_context)
212+
213+
214 def update_from_config():
215- ini = get_ini()
216+ ini = get_ini(PROJECT_DIR)
217 mongo_url = get_mongo_url()
218 config = hookenv.config()
219 set_port(ini, False)
220- stop()
221 configure_lp(config['lp-key'])
222 if config['source'] == '':
223 hookenv.log('Incomplete config: source')
224 return
225- update_source(config['source'], config['revno'])
226- oauth_path = os.path.join(PROJECT_DIR, 'jujureports/lp_credentials.txt')
227+ last_source_url, last_revision = get_last_revision()
228+ needs_install_update = (
229+ (last_source_url, last_revision) != (config['source'], config['revno'])
230+ or config['dist-clean'])
231+ if needs_install_update:
232+ source_path = update_source(
233+ config['source'], config['revno'])
234+ else:
235+ source_path = PROJECT_DIR
236+ oauth_path = os.path.join(source_path, 'jujureports/lp_credentials.txt')
237 update_file(oauth_path, config['lp-oauth'] + '\n')
238- install_production(config['dist-clean'], config['develop-install'])
239+ if needs_install_update:
240+ install_production(
241+ config['dist-clean'], config['develop-install'], source_path)
242 if mongo_url == '':
243 hookenv.log('No mongodb set up.')
244+ if needs_install_update:
245+ make_project_link(source_path, config['source'], config['revno'])
246 return
247 ini = install_ini(mongo_url, config['gh-token'], config['jujuci-url'],
248 config['charm-bundle-test-token'],
249 config['error-email'], config['error-email-from'],
250- config['cloud-health-failure-threshold'])
251- configure_s3(config)
252+ config['cloud-health-failure-threshold'], source_path)
253+ configure_s3(config, source_path)
254 unset = [key for key in [
255 'lp-key', 'lp-oauth', 'gh-token', 'charm-bundle-test-token',
256 'error-email', 'error-email-from'
257@@ -292,6 +351,9 @@
258 update_cronjob(None)
259 else:
260 install_cronjob(config['cron-interval'], config['error-email'])
261- start()
262+ stop()
263+ if needs_install_update:
264+ make_project_link(source_path, config['source'], config['revno'])
265+ start(source_path)
266 set_port(ini, True)
267 update_website(ini)
268
269=== modified file 'hooks/install'
270--- hooks/install 2014-08-25 14:31:05 +0000
271+++ hooks/install 2014-11-21 16:47:34 +0000
272@@ -7,11 +7,6 @@
273 home="/home/ubuntu"
274 project_dir="$home/juju-reports"
275
276-# Set up the project directory
277-if [ -d $project_dir ]; then
278- rm -rf $project_dir
279-fi
280-mkdir -p $project_dir
281 mkdir -p $home/logs
282 chown ubuntu:ubuntu $home/logs
283
284@@ -31,10 +26,6 @@
285 cat templates/known_hosts >> /home/ubuntu/.ssh/known_hosts
286 chown ubuntu:ubuntu /home/ubuntu/.ssh/known_hosts
287
288-# Get the code.
289-cd $project_dir
290-bzr init
291-chown ubuntu:ubuntu $project_dir
292 if [ -f /tmp/app.log ]; then
293 chown ubuntu:ubuntu /tmp/app.log
294 fi

Subscribers

People subscribed via source and target branches

to all changes: