Merge lp:~ajkavanagh/charm-helpers/clean-up-render into lp:charm-helpers

Proposed by Alex Kavanagh on 2017-08-10
Status: Merged
Merged at revision: 782
Proposed branch: lp:~ajkavanagh/charm-helpers/clean-up-render
Merge into: lp:charm-helpers
Diff against target: 93 lines (+36/-8)
3 files modified
charmhelpers/contrib/openstack/templating.py (+5/-2)
charmhelpers/core/hookenv.py (+1/-0)
charmhelpers/core/host.py (+30/-6)
To merge this branch: bzr merge lp:~ajkavanagh/charm-helpers/clean-up-render
Reviewer Review Type Date Requested Status
David Ames 2017-08-10 Approve on 2017-08-14
Review via email: mp+328859@code.launchpad.net

Description of the change

Update the core.host.write_file(...) function to do less work

The primary motivation for this change is to do less work, touch the
filesystem less, and try to clean the logs up a bit. This change is
to the rendering of configuration files, which generates noise in the
logs (at INFO) level, and often results in writing the exact same
content, thus updating the atime of the file even when the contents
haven't changed.

This change detects if the content will change, and if not, doesn't
write the file. It does detect if the uid,gid is changing and takes
the appropriate action. The logs are demoted to the lowest level of
DEBUG/TRACE, and are only generated IFF the file is actually written
or the uid/gid is changed.

To post a comment you must log in.
David Ames (thedac) wrote :

This looks good.

Before final approval I'd like to see it in one of our charms running an amulet full.

I realize this is a bit chicken and egg. But you can hijack the charm-helpers-hooks.yaml for this purpose.

David Ames (thedac) wrote :

Tests from the glance example pass.

Merging.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/contrib/openstack/templating.py'
2--- charmhelpers/contrib/openstack/templating.py 2017-02-16 03:04:17 +0000
3+++ charmhelpers/contrib/openstack/templating.py 2017-08-10 16:11:24 +0000
4@@ -20,7 +20,8 @@
5 from charmhelpers.core.hookenv import (
6 log,
7 ERROR,
8- INFO
9+ INFO,
10+ TRACE
11 )
12 from charmhelpers.contrib.openstack.utils import OPENSTACK_CODENAMES
13
14@@ -80,8 +81,10 @@
15 loaders.insert(0, FileSystemLoader(tmpl_dir))
16 if rel == os_release:
17 break
18+ # demote this log to the lowest level; we don't really need to see these
19+ # lots in production even when debugging.
20 log('Creating choice loader with dirs: %s' %
21- [l.searchpath for l in loaders], level=INFO)
22+ [l.searchpath for l in loaders], level=TRACE)
23 return ChoiceLoader(loaders)
24
25
26
27=== modified file 'charmhelpers/core/hookenv.py'
28--- charmhelpers/core/hookenv.py 2017-08-01 10:53:23 +0000
29+++ charmhelpers/core/hookenv.py 2017-08-10 16:11:24 +0000
30@@ -43,6 +43,7 @@
31 WARNING = "WARNING"
32 INFO = "INFO"
33 DEBUG = "DEBUG"
34+TRACE = "TRACE"
35 MARKER = object()
36
37 cache = {}
38
39=== modified file 'charmhelpers/core/host.py'
40--- charmhelpers/core/host.py 2017-05-20 06:05:44 +0000
41+++ charmhelpers/core/host.py 2017-08-10 16:11:24 +0000
42@@ -34,7 +34,7 @@
43
44 from contextlib import contextmanager
45 from collections import OrderedDict
46-from .hookenv import log
47+from .hookenv import log, DEBUG
48 from .fstab import Fstab
49 from charmhelpers.osplatform import get_platform
50
51@@ -487,13 +487,37 @@
52
53 def write_file(path, content, owner='root', group='root', perms=0o444):
54 """Create or overwrite a file with the contents of a byte string."""
55- log("Writing file {} {}:{} {:o}".format(path, owner, group, perms))
56 uid = pwd.getpwnam(owner).pw_uid
57 gid = grp.getgrnam(group).gr_gid
58- with open(path, 'wb') as target:
59- os.fchown(target.fileno(), uid, gid)
60- os.fchmod(target.fileno(), perms)
61- target.write(content)
62+ # lets see if we can grab the file and compare the context, to avoid doing
63+ # a write.
64+ existing_content = None
65+ existing_uid, existing_gid = None, None
66+ try:
67+ with open(path, 'rb') as target:
68+ existing_content = target.read()
69+ stat = os.stat(path)
70+ existing_uid, existing_gid = stat.st_uid, stat.st_gid
71+ except:
72+ pass
73+ if content != existing_content:
74+ log("Writing file {} {}:{} {:o}".format(path, owner, group, perms),
75+ level=DEBUG)
76+ with open(path, 'wb') as target:
77+ os.fchown(target.fileno(), uid, gid)
78+ os.fchmod(target.fileno(), perms)
79+ target.write(content)
80+ return
81+ # the contents were the same, but we might still need to change the
82+ # ownership.
83+ if existing_uid != uid:
84+ log("Changing uid on already existing content: {} -> {}"
85+ .format(existing_uid, uid), level=DEBUG)
86+ os.chown(path, uid, -1)
87+ if existing_gid != gid:
88+ log("Changing gid on already existing content: {} -> {}"
89+ .format(existing_gid, gid), level=DEBUG)
90+ os.chown(path, -1, gid)
91
92
93 def fstab_remove(mp):

Subscribers

People subscribed via source and target branches