$username
ClosedPublic
Actions

Authored by mkrizek on Oct 29 2015, 8:48 PM.

Details

Reviewers

lbrabec
kparal
tflink
jskladan

Commits

rLTRNba4b5207b600: Save tmp files into /tmpdir/$username

Summary

This is mainly for multiple buildslaves which are on the
same host running in --local not touching each others tmp files.

Test Plan

Ran rpmlint, worked. Tests pass as well.

Diff Detail

Repository

rLTRN libtaskotron

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

mkrizek retitled this revision from to Save tmp files into /tmpdir/$username.Oct 29 2015, 8:48 PM

mkrizek updated this object.

mkrizek edited the test plan for this revision. (Show Details)

mkrizek added reviewers: tflink, kparal, jskladan, lbrabec.

LGTM

This revision is now accepted and ready to land.Oct 29 2015, 8:52 PM

Sorry, why is this needed? We use tempfile.mkdtemp() so every directory should be unique and we should have no issues running several processes in parallel. It should be thread-safe.

If we want to have temporary files separated by user name, i.e. /var/tmp/taskotron/user1/ and /var/tmp/taskotron/user2/, I don't really object (I'm not sure whether there's a big benefit, though), but why does not the same approach apply for cachedir and logdir as well? (probably not for artifactsdir, that could be confusing).

In D632#12150, @kparal wrote:

Sorry, why is this needed? We use tempfile.mkdtemp() so every directory should be unique and we should have no issues running several processes in parallel. It should be thread-safe.

If we want to have temporary files separated by user name, i.e. /var/tmp/taskotron/user1/ and /var/tmp/taskotron/user2/, I don't really object (I'm not sure whether there's a big benefit, though), but why does not the same approach apply for cachedir and logdir as well? (probably not for artifactsdir, that could be confusing).

Because we delete tmpdir before running every task. Without separating tmpdir for each user (buildslave) we could (and will) delete tmp files of a task that is running via different buildslave. Log files are now also separated for each user (/var/log/taskotron/taskotron-buildslave1.log).

Ah, that makes sense :) Please put that into the commit message, or a comment in code, thanks.

So, instead of performing os.path.join(config.tmpdir, getpass.getuser()) at every single place where we want to use tmpdir (I bet you that we will forget it sooner or later), wouldn't be easier and more reliable to introduce a method like _customize_values() in config.py and overwrite config.tmpdir with that personalized value? Then you can easily use config.tmpdir anywhere and you'll be sure you're still in your personal namespace.

The same can be done for cachedir, even though we might not be using it much yet.

Customize tmpdir right after being loaded and merged.

libtaskotron/config.py
254	A comment here explaining why we're doing this would be great.

Closed by commit rLTRNba4b5207b600: Save tmp files into /tmpdir/$username (authored by mkrizek). · Explain WhyNov 2 2015, 5:54 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents

			Path	Packages
M			libtaskotron/config.py (14 lines)
M			testing/functest_config.py (8 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	1642	a01548b		Oct 29 2015, 8:47 PM	★	★
Diff 2	1644	a01548b	Customize tmpdir right after being loaded and merged.	Oct 30 2015, 4:40 PM	★	★
Diff 3	1647	a01548b	Commit rLTRNba4b5207b60080424313fd64ee6b24fc9ef5d4fd	Nov 2 2015, 5:32 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
4b2e669c4d43	6a2e4e5a6f06	a01548bd48a1	Martin Krizek	Save tmp files into /tmpdir/$username (Show More…)	Oct 29 2015, 8:37 PM

Diff 1647

View Options

libtaskotron/config.py

1	# -- coding: utf-8 --	1		# -- coding: utf-8 --
2	# Copyright 2009-2014, Red Hat, Inc.	2		# Copyright 2009-2014, Red Hat, Inc.
3	# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>	3		# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4	# See the LICENSE file for more details on Licensing	4		# See the LICENSE file for more details on Licensing
5		5
6	'''Global configuration for Taskotron and relevant helper methods.'''	6		'''Global configuration for Taskotron and relevant helper methods.'''
7		7
8	from __future__ import absolute_import	8		from __future__ import absolute_import
9	import os	9		import os
10	import yaml	10		import yaml
11	import collections	11		import collections
		12		import getpass
12		13
13	import libtaskotron	14		import libtaskotron
14	from libtaskotron import exceptions as exc	15		from libtaskotron import exceptions as exc
15	from libtaskotron.logger import log	16		from libtaskotron.logger import log
16	from libtaskotron.config_defaults import (Config, ProductionConfig, TestingConfig, ProfileName,	17		from libtaskotron.config_defaults import (Config, ProductionConfig, TestingConfig, ProfileName,
17	RuntaskModeName)	18		RuntaskModeName)
18	from libtaskotron import file_utils	19		from libtaskotron import file_utils
19		20
▲ Show 20 Lines • Show All 77 Lines • ▼ Show 20 Line(s)		73	def _load():
97	# reload the config defaults if the profile was specified in the config file	98		# reload the config defaults if the profile was specified in the config file
98	# (and not present in env variable)	99		# (and not present in env variable)
99	if file_profile and not env_profile:	100		if file_profile and not env_profile:
100	config = _load_defaults(file_profile)	101		config = _load_defaults(file_profile)
101		102
102	# merge the changes from the config file	103		# merge the changes from the config file
103	_merge_config(config, file_config)	104		_merge_config(config, file_config)
104		105
		106		# do additional values customization
		107		_customize_values(config)
		108
105	# set config filename used, this is set after merging	109		# set config filename used, this is set after merging
106	# so it doesn't get overridden	110		# so it doesn't get overridden
107	config.config_filename = filename	111		config.config_filename = filename
108		112
109	return config	113		return config
110		114
111		115
112	def _check_sanity(config):	116		def _check_sanity(config):
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Line(s)		240	if option == 'profile':
237	# the only option not overridden from file is 'profile', because	241		# the only option not overridden from file is 'profile', because
238	# that might have been dictated by env variable which has a higher	242		# that might have been dictated by env variable which has a higher
239	# priority	243		# priority
240	continue	244		continue
241	if hasattr(config, option):	245		if hasattr(config, option):
242	setattr(config, option, value)	246		setattr(config, option, value)
243		247
244		248
		249		def _customize_values(config):
		250		'''Customize values, if needed, after being loaded and merged.
		251
		252		:param config: :class:`.Config` instance
		253		'''
		254		# We delete tmpdir before running every task. Separate tmpdir
			kparalUnsubmitted Not Done A comment here explaining why we're doing this would be great.
		255		# for each user so we don't delete other user's tmp files.
		256		config.tmpdir = os.path.join(config.tmpdir, getpass.getuser())
		257
		258
245	def _create_dirs(config):	259		def _create_dirs(config):
246	'''Create directories in the local file system for appropriate config	260		'''Create directories in the local file system for appropriate config
247	options, like ``tmpdir``, ``logdir``, ``cachedir``, ``artifactsdir``.	261		options, like ``tmpdir``, ``logdir``, ``cachedir``, ``artifactsdir``.
248		262
249	:param config: :class:`.Config` instance	263		:param config: :class:`.Config` instance
250	:raise TaskotronError: when directories don't exist and can't be created	264		:raise TaskotronError: when directories don't exist and can't be created
251	'''	265		'''
252		266
253	for dir_ in (config.tmpdir, config.logdir, config.artifactsdir,	267		for dir_ in (config.tmpdir, config.logdir, config.artifactsdir,
254	config.cachedir):	268		config.cachedir):
255	try:	269		try:
256	file_utils.makedirs(dir_)	270		file_utils.makedirs(dir_)
257	except OSError as e:	271		except OSError as e:
258	raise exc.TaskotronError("Failed to create required directory '%s', "	272		raise exc.TaskotronError("Failed to create required directory '%s', "
259	"please create it manually. Caused by: %s"	273		"please create it manually. Caused by: %s"
260	% (dir_, e))	274		% (dir_, e))