Merge lp:~adeuring/charms/precise/juju-reports/juju-reports-new-revno-in-new-dir into lp:~juju-qa/charms/precise/juju-reports/trunk
- Precise Pangolin (12.04)
- juju-reports-new-revno-in-new-dir
- Merge into trunk
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
Review via email: mp+242230@code.launchpad.net |
Commit message
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_
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/
# EASY-INSTALL-
__requires__ = 'jujureports==0.0'
import sys
from pkg_resources import load_entry_point
if __name__ == '__main__':
sys.exit(
)
- 51. By Abel Deuring
-
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
-
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_
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?
- 53. By Abel Deuring
-
make_project_
link(): use OSError.errno to figure out the exact error condition. - 54. By Abel Deuring
-
Run install_
production( ) only when the revno or the branch URL changes. - 55. By Abel Deuring
-
Use a shared repository for the juju-reports branches.
- 56. By Abel Deuring
-
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-
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_
Finally I noticed that the install hook still removed the directory $home/juju-reports. This is done in make_project_
- 57. By Abel Deuring
-
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.
Preview Diff
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 |
Why use a symlink? Why not just rename the new project dir into place?