crontab in python; fix bugs
Closed, ResolvedPublic
Actions

Assigned To

zhuyifei1999

Authored By

	bd808
	Jan 24 2017, 8:43 PM

Description

The /usr/local/bin/crontab wrapper script has a bug which presents a good opportunity to port it to python. The bug is that when the ssh connection to fetch an existing crontab from the cron host fails it doesn't abort. Instead it just acts like there is no cron file on the remote and stumbles on.

See T156168: Tool labs crontab host tools-cron-01 cannot be ssh-ed into within a service user for the config failure that caused this to be noticed

Details

Subject	Repo	Branch	Lines +/-
toolforge: Replace /usr/local/bin/crontab with oge-crontab	operations/puppet	production	+6 -210
crontab: make tempfile use utf-8 encoding	labs/toollabs	master	+10 -4
crontab: Fix python3 syntax problems	labs/toollabs	master	+12 -5
Add rewritten crontab in Python	labs/toollabs	master	+368 -11
toollabs: Preparing to move `/usr/local/bin/crontab` to labs/toollabs	operations/puppet	production	+7 -0

Customize query in gerrit

Related Objects

Mentioned In: T181948: Toolforge crontab command cannot handle unicode crontab when the locale is non-utf8
T179386: Prometheus crontab installations via Puppet failing on Toolforge bastion hosts due to remote crontab wrapper script
T157792: /usr/local/bin/killgridjobs.sh is outdated
Mentioned Here: P4916 Python version of toollabs remote crontab
T86446: Support @weekly et al in crontab
T156168: Tool labs crontab host tools-cron-01 cannot be ssh-ed into within a service user

Event Timeline

bd808 created this task.Jan 24 2017, 8:43 PM

Restricted Application added a project: Cloud-Services. · View Herald TranscriptJan 24 2017, 8:43 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

zhuyifei1999 subscribed.Jan 24 2017, 8:47 PM

It would be nice to split out the "template" bit of it; i. e., let Puppet write @cron_host to some file in /etc and make /usr/local/bin/crontab read from that instead of changing it in the source code. That should make testing it more easy.

That seems like a good idea. We could move the actual script into the labs/toollabs deb with become, jsub, etc with that bit of config moved outside the script.

T86446: Support @weekly et al in crontab would be another bug

zhuyifei1999 claimed this task.Feb 9 2017, 9:36 AM

How about something similar to this:

P4916 Python version of toollabs remote crontab

1	#! /usr/bin/python -Es
2	# -- coding: UTF-8 --
3	#
4	# Copyright © 2013 Marc-André Pelletier <mpelletier@wikimedia.org>
5	# Copyright © 2017 Zhuyifei1999 <zhuyifei1999@gmail.com>
6	#
7	# Permission to use, copy, modify, and/or distribute this software for any
8	# purpose with or without fee is hereby granted, provided that the above
9	# copyright notice and this permission notice appear in all copies.
10	#
11	# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
12	# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
13	# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
14	# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
15	# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
16	# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
17	# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
18	#
19
20	# This goes in /usr/local/bin to override the system crontab (which will
21	# have its permissions restricted).
22
23	# THIS FILE IS MANAGED BY PUPPET
24	#
25	# This goes in /usr/local/bin to override the system crontab (which will
26	# have its permissions restricted).
27	#
28	# Source: modules/toollabs/templates/crontab.erb
29	# From: toollabs::submit
30
31	from __future__ import print_function
32
33	import argparse
34	import pwd
35	import os
36	import re
37	import subprocess
38	import sys
39	import tempfile
40
41	# TODO: read from somewhere like /etc/toollabs-cronhost
42	CRON_HOST = 'tools-cron-01.tools.eqiad.wmflabs' # FIXME: <%= @cron_host %>
43	JSUB_MODIFIED = '''
44	NOTE: some crontab entries have been modified to grid submissions.
45	You may want to examine the result with 'crontab -e'.
46	'''
47	DEFAULT_CRONTAB = '''\
48	# Edit this file to introduce tasks to be run by cron.
49	#
50	# Each task to run has to be defined through a single line
51	# indicating with different fields when the task will be run
52	# and what command to run for the task
53	#
54	# To define the time you can provide concrete values for
55	# minute (m), hour (h), day of month (dom), month (mon),
56	# and day of week (dow) or use '*' in these fields (for 'any').#
57	# Notice that tasks will be started based on the cron's system
58	# daemon's notion of time and timezones.
59	#
60	# Output of the crontab jobs (including errors) is sent through
61	# email to the user the crontab file belongs to (unless redirected).
62	#
63	# For example, you can run a backup of all your user accounts
64	# at 5 a.m every week with:
65	# 0 5 * * 1 tar -zcf /var/backups/home.tgz /home/
66	#
67	# For more information see the manual pages of crontab(5) and cron(8)
68	#
69	# Wikimedia Tool Labs specific note:
70	# Please be aware that only jsub and jstart are acceptable
71	# commands to schedule via cron. Any command specified here will
72	# be modified to be invoked through jsub unless it is one of
73	# the two.
74	#
75	# m h dom mon dow command
76	'''
77
78	parser = argparse.ArgumentParser()
79	parser.add_argument('-u', help=argparse.SUPPRESS,
80	dest='user')
81	parser.add_argument('-i', help='prompt before deleting crontab',
82	action='store_true')
83	group = parser.add_mutually_exclusive_group()
84	group.add_argument('file', help='replace crontab with file (default)',
85	default=sys.stdin, nargs='?', type=argparse.FileType('r'))
86	subgroup = group.add_mutually_exclusive_group()
87	subgroup.add_argument('-e', help='edit crontab',
88	action='store_const', const='e', dest='operation')
89	subgroup.add_argument('-l', help='list crontab',
90	action='store_const', const='l', dest='operation')
91	subgroup.add_argument('-r', help='delete crontab',
92	action='store_const', const='r', dest='operation')
93
94
95	class NoCrontab(Exception):
96	pass
97
98
99	class Crontab(object):
100	def __init__(self, user):
101	self.user = user
102
103	def _remote(self, args, stdin=None):
104	"""Execute remote crontab command and returns stdout."""
105	args = ['/usr/bin/ssh', CRON_HOST, '/usr/bin/crontab'] + args
106	if self.user.pw_uid != os.getuid():
107	args += ['-u', self.user.pw_name]
108
109	ssh = subprocess.Popen(args,
110	stdin=subprocess.PIPE,
111	stdout=subprocess.PIPE,
112	stderr=subprocess.PIPE)
113
114	stdoutdata, stderrdata = ssh.communicate(input=stdin)
115
116	if ssh.returncode:
117	if stderrdata.lower().startswith('no crontab for '):
118	raise NoCrontab
119	else:
120	print(stderrdata, end='', file=sys.stderr)
121	err('unable to execute remote crontab command')
122	sys.exit(ssh.returncode)
123
124	return stdoutdata
125
126	@staticmethod
127	def _add_jsub(text):
128	# TODO: regex insanity here
129	return text
130
131	def load(self):
132	return self._remote(['-l'])
133
134	def save(self, text):
135	jsub_text = self._add_jsub(text)
136	if jsub_text != text:
137	print(JSUB_MODIFIED, file=sys.stderr)
138	self._remote([], stdin=jsub_text)
139
140	def remove(self):
141	self._remote(['-r'])
142
143
144	def editor(text):
145	with tempfile.NamedTemporaryFile() as f:
146	f.write(text)
147	f.flush()
148
149	subprocess.check_call(['/usr/bin/sensible-editor', f.name])
150
151	f.seek(0)
152	return f.read()
153
154
155	def err(message):
156	print('{}: {}'.format(sys.argv[0], message), file=sys.stderr)
157
158
159	def main():
160	args = parser.parse_args()
161
162	if args.i and args.operation != 'r':
163	parser.error('argument -i: only applicable with -r')
164
165	target = pwd.getpwuid(os.getuid())
166	if args.user is not None:
167	if os.getuid():
168	parser.error('argument -u: must be privileged')
169
170	try:
171	target = pwd.getpwnam(args.user)
172	except KeyError:
173	parser.error('argument -u: unknown user "{}"'.format(args.user))
174
175	if target.pw_uid < 500:
176	# If the target user is not managed in LDAP and thus likely
177	# a system user, invoke the original crontab instead.
178	os.execv('/usr/bin/crontab', sys.argv[1:])
179	elif target.pw_uid < 40000 and os.getuid():
180	err('only tools are allowed crontabs')
181	sys.exit(1)
182	else:
183	try:
184	crontab = Crontab(target)
185
186	if args.operation is None:
187	# Replace
188	crontab.save(args.file.read())
189	elif args.operation == 'e':
190	# Edit
191	try:
192	crontab_text = crontab.load()
193	except NoCrontab:
194	crontab_text = DEFAULT_CRONTAB
195	new_crontab_text = editor(crontab_text)
196
197	if new_crontab_text == crontab_text:
198	print('No modification made', file=sys.stderr)
199	elif not new_crontab_text.strip():
200	err('cowardly refusing to install empty crontab')
201	err('use `{} -r` if you want to remove the crontab'.format(
202	sys.srgv[0]))
203	sys.exit(1)
204	else:
205	crontab.save(new_crontab_text)
206	elif args.operation == 'l':
207	# List
208	try:
209	print(crontab.load(), end='') # crontab already has lf
210	except NoCrontab:
211	print('no crontab for {}'.format(target.pw_name),
212	file=sys.stderr)
213	sys.exit(1)
214	elif args.operation == 'r':
215	# Delete
216	try:
217	crontab.load()
218	except NoCrontab:
219	print('no crontab for {}'.format(target.pw_name),
220	file=sys.stderr)
221	sys.exit(1)
222	if args.i:
223	prompt = "{}: really delete {}'s crontab? (y/n) ".format(
224	sys.argv[0], target.pw_name)
225	while True:
226	try:
227	stdin = raw_input(prompt).lower()
228	except EOFError:
229	stdin = ''
230	except KeyboardInterrupt:
231	print(file=sys.stderr)
232	raise
233
234	if stdin == 'y':
235	crontab.remove()
236	break
237	elif stdin == 'n':
238	break
239	else:
240	prompt = 'Please enter Y or N: '
241
242	else:
243	crontab.remove()
244	except KeyboardInterrupt:
245	pass
246
247	if __name__ == '__main__':
248	main()

The regex insanity has not been written yet, though.

@zhuyifei1999 Looks like a good start. I think we should move the script from Puppet to https://gerrit.wikimedia.org/r/#/admin/projects/labs/toollabs and make the change in Puppet to populate /etc/toollabs-cronhost (name can be bikesheded). Do you want to take a shot at the patches for that?

Yeah sure. Part of this task anyways. :)

zhuyifei1999 mentioned this in T157792: /usr/local/bin/killgridjobs.sh is outdated.Feb 10 2017, 11:40 AM

Change 336990 had a related patch set uploaded (by Zhuyifei1999):
toollabs: Preparing to move /usr/local/bin/crontab to labs/toollabs

https://gerrit.wikimedia.org/r/336990

gerritbot added a project: Patch-For-Review.Feb 10 2017, 11:49 AM

Change 336998 had a related patch set uploaded (by Zhuyifei1999):
Add rewritten crontab in Python

https://gerrit.wikimedia.org/r/336998

scfc triaged this task as Medium priority.Feb 16 2017, 11:22 PM

scfc moved this task from Backlog to Waiting for code review on the Toolforge board.

Change 336990 merged by Dzahn:
[operations/puppet] toollabs: Preparing to move /usr/local/bin/crontab to labs/toollabs

https://gerrit.wikimedia.org/r/336990

Framawiki subscribed.Jun 10 2017, 1:34 PM

Change 336998 merged by jenkins-bot:
[labs/toollabs@master] Add rewritten crontab in Python

https://gerrit.wikimedia.org/r/336998

Change 382629 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[labs/toollabs@master] crontab: Fix python3 syntax problems

https://gerrit.wikimedia.org/r/382629

gerritbot removed a project: Cloud-Services.Oct 5 2017, 11:21 PM

Change 382629 merged by jenkins-bot:
[labs/toollabs@master] crontab: Fix python3 syntax problems

https://gerrit.wikimedia.org/r/382629

The new script is available on Toolforge hosts as /usr/bin/oge-crontab. The final step needed is to replace the current /usr/local/bin/crontab script provisioned by Puppet with a symlink to /usr/bin/oge-crontab.

tools.yifeibot@tools-bastion-02:~$ oge-crontab -e
Traceback (most recent call last):
  File "/usr/bin/oge-crontab", line 337, in <module>
    main()
  File "/usr/bin/oge-crontab", line 283, in main
    new_crontab_text = editor(crontab_text)
  File "/usr/bin/oge-crontab", line 209, in editor
    f.write(text.encode('latin1'))
UnicodeEncodeError: 'latin-1' codec can't encode characters in position 1522-1534: ordinal not in range(256)

Umm... shouldn't the encode be utf-8 instead of latin-1? Alternatively can we open this temp file in text mode?

In T156174#3677736, @zhuyifei1999 wrote:

Umm... shouldn't the encode be utf-8 instead of latin-1? Alternatively can we open this temp file in text mode?

The latin-1 encoding should be 8-bit clean, but apparently its not? This was part of the code I messed with to port it to python3. I touched as little as possible to do that, but maybe that was the wrong approach.

Change 383770 had a related patch set uploaded (by Zhuyifei1999; owner: Zhuyifei1999):
[labs/toollabs@master] crontab: make tempfile use utf-8 encoding

https://gerrit.wikimedia.org/r/383770

gerritbot added a project: Patch-For-Review.Oct 12 2017, 4:55 AM

bd808 moved this task from Inbox to Soon! on the cloud-services-team (Kanban) board.Oct 18 2017, 2:16 AM

bd808 mentioned this in T179386: Prometheus crontab installations via Puppet failing on Toolforge bastion hosts due to remote crontab wrapper script.Oct 31 2017, 2:42 PM

Change 383770 merged by jenkins-bot:
[labs/toollabs@master] crontab: make tempfile use utf-8 encoding

https://gerrit.wikimedia.org/r/383770

Mentioned in SAL (#wikimedia-cloud) [2017-11-03T21:19:31Z] <bd808> Deployed misctools 1.26 (T156174)

In T156174#3677736, @zhuyifei1999 wrote:

tools.yifeibot@tools-bastion-02:~$ oge-crontab -e
Traceback (most recent call last):
  File "/usr/bin/oge-crontab", line 337, in <module>
    main()
  File "/usr/bin/oge-crontab", line 283, in main
    new_crontab_text = editor(crontab_text)
  File "/usr/bin/oge-crontab", line 209, in editor
    f.write(text.encode('latin1'))
UnicodeEncodeError: 'latin-1' codec can't encode characters in position 1522-1534: ordinal not in range(256)

This test case works now.

Change 391979 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[operations/puppet@production] toolforge: Replace /usr/local/bin/crontab with oge-crontab

https://gerrit.wikimedia.org/r/391979

Change 391979 merged by Rush:
[operations/puppet@production] toolforge: Replace /usr/local/bin/crontab with oge-crontab

https://gerrit.wikimedia.org/r/391979

Note: crons backed up on tools-cron-01 prior to merge at tools-cron-01:/var/spool/cron/crontabs# ls /root/20112017/crontabs/. Old crontab script is backed up locally on tools-bastion-03 at tools-bastion-03:~# cp /usr/local/bin/crontab /home/rush

Post merge of 391979 status for this issue:

Adding crons on a tool without them seems to add the correct header
Adding new lines to an existing crontab worked and preserved old content
Puppet still errrors for Prometheus crontabs but I believe the errors are different now

Notice: /Stage[main]/Prometheus::Node_puppet_agent/Cron[prometheus_puppet_agent_stats]/ensure: created

NOTE: some crontab entries have been modified to grid submissions.
      You may want to examine the result with 'crontab -e'.

Permission denied (publickey,hostbased).
/usr/local/bin/crontab: unable to execute remote crontab command
Notice: Finished catalog run in 103.34 seconds

vs debug output (roughly the same)

Notice: /Stage[main]/Prometheus::Node_puppet_agent/Cron[prometheus_puppet_agent_stats]/ensure: created
Debug: Flushing cron provider target prometheus

NOTE: some crontab entries have been modified to grid submissions.
      You may want to examine the result with 'crontab -e'.

Permission denied (publickey,hostbased).
/usr/local/bin/crontab: unable to execute remote crontab command
Debug: /Stage[main]/Prometheus::Node_puppet_agent/Cron[prometheus_puppet_agent_stats]: The container Class[Prometheus::Node_puppet_agent] will propagate my refresh event

We are still executing the user crontab replacement as root

root@tools-bastion-05:~# which crontab
/usr/local/bin/crontab
root@tools-bastion-05:~# echo $PATH
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

The portion of of oge-crontab that is being trigged for prometheus is

stdoutdata, stderrdata = ssh.communicate(input=stdin)

if ssh.returncode:
    if stderrdata.lower().startswith('no crontab for '):
        raise NoCrontab
    else:
        print(stderrdata, end='', file=sys.stderr)
        err('unable to execute remote crontab command')
        sys.exit(ssh.returncode)

I can't find a loss of functionality since prometheus was already tripping over this special cron behavior. I believe we can easily add an exclusion now based on UID if we want? That's not the correct long term solution but will work. The issue seems to show consistently on Puppet runs now and is reproduceable.

Thanks for all of your work on this @zhuyifei1999!

bd808 moved this task from Soon! to Done on the cloud-services-team (Kanban) board.Nov 30 2017, 6:06 AM

Framawiki mentioned this in T181948: Toolforge crontab command cannot handle unicode crontab when the locale is non-utf8.Dec 3 2017, 9:09 PM

Rewrite /usr/local/bin/crontab in python; fix bugsClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

Rewrite /usr/local/bin/crontab in python; fix bugs
Closed, ResolvedPublic
Actions