Merge lp:~1chb1n/charm-helpers/amulet-file-race into lp:charm-helpers

Proposed by Ryan Beisner
Status: Merged
Merged at revision: 453
Proposed branch: lp:~1chb1n/charm-helpers/amulet-file-race
Merge into: lp:charm-helpers
Diff against target: 122 lines (+47/-16)
1 file modified
charmhelpers/contrib/amulet/utils.py (+47/-16)
To merge this branch: bzr merge lp:~1chb1n/charm-helpers/amulet-file-race
Reviewer Review Type Date Requested Status
Chris Glass (community) Approve
Review via email: mp+271417@code.launchpad.net

Description of the change

Amulet helper update of config_updated_since re: bug 1474030.

To post a comment you must log in.
Revision history for this message
Chris Glass (tribaal) wrote :

LGTM, will merge once non-critical inline comments are ack'd.

+1

review: Approve
Revision history for this message
Ryan Beisner (1chb1n) wrote :

Replies inline. Thanks for the review.

Revision history for this message
Ryan Beisner (1chb1n) wrote :

@tribaal

Took your good advice, adjusted timing/retries of this and the neighboring helper. Will sync back into a charm test branch to confirm happiness, and report back here.

Revision history for this message
Ryan Beisner (1chb1n) wrote :

PS, found amulet upstream bug https://github.com/juju/amulet/issues/94 whilst t-shooting this. One too many moving parts had me cursing the world. That is committed upstream, and should hit the juju stable ppa shortly.

Revision history for this message
Chris Glass (tribaal) wrote :

Nice, thanks for changing the retry count/delays.

Still looks +1!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/contrib/amulet/utils.py'
2--- charmhelpers/contrib/amulet/utils.py 2015-09-11 07:27:33 +0000
3+++ charmhelpers/contrib/amulet/utils.py 2015-09-21 14:30:42 +0000
4@@ -326,7 +326,7 @@
5
6 def service_restarted_since(self, sentry_unit, mtime, service,
7 pgrep_full=None, sleep_time=20,
8- retry_count=2, retry_sleep_time=30):
9+ retry_count=30, retry_sleep_time=10):
10 """Check if service was been started after a given time.
11
12 Args:
13@@ -334,8 +334,9 @@
14 mtime (float): The epoch time to check against
15 service (string): service name to look for in process table
16 pgrep_full: [Deprecated] Use full command line search mode with pgrep
17- sleep_time (int): Seconds to sleep before looking for process
18- retry_count (int): If service is not found, how many times to retry
19+ sleep_time (int): Initial sleep time (s) before looking for file
20+ retry_sleep_time (int): Time (s) to sleep between retries
21+ retry_count (int): If file is not found, how many times to retry
22
23 Returns:
24 bool: True if service found and its start time it newer than mtime,
25@@ -359,11 +360,12 @@
26 pgrep_full)
27 self.log.debug('Attempt {} to get {} proc start time on {} '
28 'OK'.format(tries, service, unit_name))
29- except IOError:
30+ except IOError as e:
31 # NOTE(beisner) - race avoidance, proc may not exist yet.
32 # https://bugs.launchpad.net/charm-helpers/+bug/1474030
33 self.log.debug('Attempt {} to get {} proc start time on {} '
34- 'failed'.format(tries, service, unit_name))
35+ 'failed\n{}'.format(tries, service,
36+ unit_name, e))
37 time.sleep(retry_sleep_time)
38 tries += 1
39
40@@ -383,35 +385,62 @@
41 return False
42
43 def config_updated_since(self, sentry_unit, filename, mtime,
44- sleep_time=20):
45+ sleep_time=20, retry_count=30,
46+ retry_sleep_time=10):
47 """Check if file was modified after a given time.
48
49 Args:
50 sentry_unit (sentry): The sentry unit to check the file mtime on
51 filename (string): The file to check mtime of
52 mtime (float): The epoch time to check against
53- sleep_time (int): Seconds to sleep before looking for process
54+ sleep_time (int): Initial sleep time (s) before looking for file
55+ retry_sleep_time (int): Time (s) to sleep between retries
56+ retry_count (int): If file is not found, how many times to retry
57
58 Returns:
59 bool: True if file was modified more recently than mtime, False if
60- file was modified before mtime,
61+ file was modified before mtime, or if file not found.
62 """
63- self.log.debug('Checking %s updated since %s' % (filename, mtime))
64+ unit_name = sentry_unit.info['unit_name']
65+ self.log.debug('Checking that %s updated since %s on '
66+ '%s' % (filename, mtime, unit_name))
67 time.sleep(sleep_time)
68- file_mtime = self._get_file_mtime(sentry_unit, filename)
69+ file_mtime = None
70+ tries = 0
71+ while tries <= retry_count and not file_mtime:
72+ try:
73+ file_mtime = self._get_file_mtime(sentry_unit, filename)
74+ self.log.debug('Attempt {} to get {} file mtime on {} '
75+ 'OK'.format(tries, filename, unit_name))
76+ except IOError as e:
77+ # NOTE(beisner) - race avoidance, file may not exist yet.
78+ # https://bugs.launchpad.net/charm-helpers/+bug/1474030
79+ self.log.debug('Attempt {} to get {} file mtime on {} '
80+ 'failed\n{}'.format(tries, filename,
81+ unit_name, e))
82+ time.sleep(retry_sleep_time)
83+ tries += 1
84+
85+ if not file_mtime:
86+ self.log.warn('Could not determine file mtime, assuming '
87+ 'file does not exist')
88+ return False
89+
90 if file_mtime >= mtime:
91 self.log.debug('File mtime is newer than provided mtime '
92- '(%s >= %s)' % (file_mtime, mtime))
93+ '(%s >= %s) on %s (OK)' % (file_mtime,
94+ mtime, unit_name))
95 return True
96 else:
97- self.log.warn('File mtime %s is older than provided mtime %s'
98- % (file_mtime, mtime))
99+ self.log.warn('File mtime is older than provided mtime'
100+ '(%s < on %s) on %s' % (file_mtime,
101+ mtime, unit_name))
102 return False
103
104 def validate_service_config_changed(self, sentry_unit, mtime, service,
105 filename, pgrep_full=None,
106- sleep_time=20, retry_count=2,
107- retry_sleep_time=30):
108+ sleep_time=20, retry_count=30,
109+ retry_sleep_time=10):
110 """Check service and file were updated after mtime
111
112 Args:
113@@ -456,7 +485,9 @@
114 sentry_unit,
115 filename,
116 mtime,
117- sleep_time=0)
118+ sleep_time=sleep_time,
119+ retry_count=retry_count,
120+ retry_sleep_time=retry_sleep_time)
121
122 return service_restart and config_update
123

Subscribers

People subscribed via source and target branches