| lbrabec | |
| tflink | |
| mkrizek |
| libtaskotron |
This speeds up task copying onto the minion tenfold for tasks with a
lot of dirs (e.g. git/ inside). Most importantly it uses mkdir()
internal to sftp instead running it as a shell command, but there are
also other minor optimizations.
This also reduces logging spam, no longer printing a line for each
file copied.
One last minor fix is to use lstat() instead of stat() to correctly
support symlinks.
test suite works. I tested manually and copying task-upgradepath went from 27 seconds to 2.5 seconds.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Is the speedup from reversing the order of comparisons or is there something else I'm missing here?
LGTM, though
I have split the commit into two to make it more obvious. Unfortunately due to how Phab works, you can't see individual commits on the web, you have to do arc patch D752 and look at it locally. Also, once I commit this, this diff will be updated with just one of the commits referenced, so it won't reflect the complete diff. I don't know how to "fix" this - either have everything in one big commit (ew), or create a separate differential review for each commit (ew).
The most important change was to stop using ssh.cmd('mkdir ...') and start using ssh.sftp.mkdir().
| Path | Packages | |||
|---|---|---|---|---|
| M | libtaskotron/minion.py (5 lines) | |||
| M | libtaskotron/remote_exec.py (34 lines) | |||
| M | testing/test_remote_exec.py (24 lines) |
| Commit | Tree | Parents | Author | Summary | Date |
|---|---|---|---|---|---|
| 94f07b883bda | 1800034187ea | 0a343cc5f7da | Kamil Páral | remote_exec: cleanup, better error handling, reduce logging (Show More…) | Feb 24 2016, 9:34 AM |
| 0a343cc5f7da | 49a8bab9a195 | 4257c56ae832 | Kamil Páral | speed up minion copy operations and support symlinks (Show More…) | Feb 24 2016, 9:34 AM |
| Show First 20 Lines • Show All 81 Lines • ▼ Show 20 Line(s) | 55 | def _prepare_task(self): | |||
|---|---|---|---|---|---|
| 82 | 82 | | |||
| 83 | log.info('Copying task files onto the minion...') | 83 | log.info('Copying task files onto the minion...') | ||
| 84 | 84 | | |||
| 85 | # Copy config files. Go through CONF_DIRS in reversed order to preserve priority. | 85 | # Copy config files. Go through CONF_DIRS in reversed order to preserve priority. | ||
| 86 | for config_dir in reversed(config.CONF_DIRS): | 86 | for config_dir in reversed(config.CONF_DIRS): | ||
| 87 | if os.path.exists(config_dir): | 87 | if os.path.exists(config_dir): | ||
| 88 | self.ssh.put_dir(config_dir, '/etc/taskotron/') | 88 | self.ssh.put_dir(config_dir, '/etc/taskotron/') | ||
| 89 | 89 | | |||
| 90 | # create needed dirs. remove existing content first. | 90 | # create taskdir, remove the old one first | ||
| 91 | self.ssh.cmd('rm -rf %s' % pipes.quote(self.taskdir)) | 91 | self.ssh.cmd('rm -rf {0} && mkdir -p {0}'.format(pipes.quote(self.taskdir))) | ||
| 92 | self.ssh.mkdir(self.taskdir) | | |||
| 93 | 92 | | |||
| 94 | # patch remote libtaskotron if needed (for dev, shouldn't be used in production) | 93 | # patch remote libtaskotron if needed (for dev, shouldn't be used in production) | ||
| 95 | if self.arg_data['patch'] is not None: | 94 | if self.arg_data['patch'] is not None: | ||
| 96 | self.ssh.put_file(os.path.abspath(self.arg_data['patch']), '%s/%s' % ( | 95 | self.ssh.put_file(os.path.abspath(self.arg_data['patch']), '%s/%s' % ( | ||
| 97 | self.taskdir, self.arg_data['patch'])) | 96 | self.taskdir, self.arg_data['patch'])) | ||
| 98 | self.ssh.install_pkgs(['patch']) | 97 | self.ssh.install_pkgs(['patch']) | ||
| 99 | self.ssh.cmd('patch -d /usr/lib/python2.7/site-packages/ -p1 -i %s/%s' % ( | 98 | self.ssh.cmd('patch -d /usr/lib/python2.7/site-packages/ -p1 -i %s/%s' % ( | ||
| 100 | pipes.quote(self.taskdir), pipes.quote(self.arg_data['patch']))) | 99 | pipes.quote(self.taskdir), pipes.quote(self.arg_data['patch']))) | ||
| ▲ Show 20 Lines • Show All 120 Lines • Show Last 20 Lines | |||||
| Show First 20 Lines • Show All 176 Lines • ▼ Show 20 Line(s) | 175 | def write_file(self, remote_path, data, overwrite=True): | |||
|---|---|---|---|---|---|
| 177 | 177 | | |||
| 178 | :param str remote_path: A path to the remote file | 178 | :param str remote_path: A path to the remote file | ||
| 179 | :param str data: Data to be written | 179 | :param str data: Data to be written | ||
| 180 | :param bool overwrite: Whether to overwrite remote path. Default is True. | 180 | :param bool overwrite: Whether to overwrite remote path. Default is True. | ||
| 181 | 181 | | |||
| 182 | :raise TaskotronRemoteError: If data could not be written | 182 | :raise TaskotronRemoteError: If data could not be written | ||
| 183 | ''' | 183 | ''' | ||
| 184 | 184 | | |||
| 185 | if self._remote_file_exists(remote_path) and not overwrite: | 185 | # keep the conditionals in this order to avoid unnecessary remote calls | ||
| 186 | if not overwrite and self._remote_file_exists(remote_path): | ||||
| 186 | log.info('Remote path %s already exists, not overwriting.', remote_path) | 187 | log.info('Remote path %s already exists, not overwriting.', remote_path) | ||
| 187 | return | 188 | return | ||
| 188 | 189 | | |||
| 189 | log.debug('Writing data to %s', remote_path) | 190 | log.debug('Writing data to %s', remote_path) | ||
| 190 | try: | 191 | try: | ||
| 191 | with self.sftp.open(remote_path, 'w') as remote_file: | 192 | with self.sftp.open(remote_path, 'w') as remote_file: | ||
| 192 | remote_file.write(data) | 193 | remote_file.write(data) | ||
| 193 | except (socket.error, IOError), e: | 194 | except (socket.error, IOError), e: | ||
| 194 | raise exc.TaskotronRemoteError('Could not write data to %s@%s:%s: %s' % | 195 | raise exc.TaskotronRemoteError('Could not write data to %s@%s:%s: %s' % | ||
| 195 | (self.username, self.hostname, remote_path, e.strerror)) | 196 | (self.username, self.hostname, remote_path, e.strerror)) | ||
| 196 | 197 | | |||
| 197 | def put_file(self, local_path, remote_path, overwrite=True): | 198 | def put_file(self, local_path, remote_path, overwrite=True): | ||
| 198 | '''Copy a file to a remote path. | 199 | '''Copy a file to a remote path. | ||
| 199 | 200 | | |||
| 200 | :param str local_path: A path to the local file | 201 | :param str local_path: A path to the local file | ||
| 201 | :param str remote_path: A path to the remote file | 202 | :param str remote_path: A path to the remote file | ||
| 202 | :param bool overwrite: Whether to overwrite remote path. Default is True. | 203 | :param bool overwrite: Whether to overwrite remote path. Default is True. | ||
| 203 | 204 | | |||
| 204 | :return: an SFTPAttributes object containing attributes about the given file, | 205 | :return: an SFTPAttributes object containing attributes about the given file, | ||
| 205 | if successful. None otherwise. | 206 | if successful. None otherwise. | ||
| 206 | 207 | | |||
| 207 | :raise TaskotronRemoteError: If the file could not be copied | 208 | :raise TaskotronRemoteError: If the file could not be copied | ||
| 208 | ''' | 209 | ''' | ||
| 209 | 210 | # keep the conditionals in this order to avoid unnecessary remote calls | |||
| 210 | if self._remote_file_exists(remote_path) and not overwrite: | 211 | if not overwrite and self._remote_file_exists(remote_path): | ||
| 211 | log.info('Remote path %s already exists, not overwriting.', remote_path) | 212 | log.info('Remote path %s already exists, not overwriting.', remote_path) | ||
| 212 | return | 213 | return | ||
| 213 | 214 | | |||
| 214 | log.debug('Putting file (%s) on to remote host (%s)', local_path, remote_path) | 215 | log.debug('Putting file (%s) on to remote host (%s)', local_path, remote_path) | ||
| 215 | try: | 216 | try: | ||
| 216 | return self.sftp.put(local_path, remote_path) | 217 | return self.sftp.put(local_path, remote_path) | ||
| 217 | except (socket.error, IOError), e: | 218 | except (socket.error, IOError), e: | ||
| 218 | raise exc.TaskotronRemoteError('Could not put file %s to %s@%s:%s: %s' % | 219 | raise exc.TaskotronRemoteError('Could not put file %s to %s@%s:%s: %s' % | ||
| Show All 14 Lines | 233 | try: | |||
| 233 | self.sftp.get(remote_path, local_path) | 234 | self.sftp.get(remote_path, local_path) | ||
| 234 | except (socket.error, IOError), e: | 235 | except (socket.error, IOError), e: | ||
| 235 | raise exc.TaskotronRemoteError('Could not copy file %s@%s:%s to %s: %s' % | 236 | raise exc.TaskotronRemoteError('Could not copy file %s@%s:%s to %s: %s' % | ||
| 236 | (self.username, self.hostname, remote_path, | 237 | (self.username, self.hostname, remote_path, | ||
| 237 | local_path, e.strerror)) | 238 | local_path, e.strerror)) | ||
| 238 | 239 | | |||
| 239 | def _remote_file_exists(self, remote_path): | 240 | def _remote_file_exists(self, remote_path): | ||
| 240 | try: | 241 | try: | ||
| 241 | path_stats = self.sftp.stat(remote_path) | 242 | path_stats = self.sftp.lstat(remote_path) | ||
| 242 | if path_stats is not None: | 243 | if path_stats is not None: | ||
| 243 | return True | 244 | return True | ||
| 244 | except IOError: | 245 | except IOError: | ||
| 245 | pass | 246 | pass | ||
| 246 | 247 | | |||
| 247 | return False | 248 | return False | ||
| 248 | 249 | | |||
| 249 | def _remote_isdir(self, remote_path): | 250 | def _remote_isdir(self, remote_path): | ||
| 250 | try: | 251 | try: | ||
| 251 | return S_ISDIR(self.sftp.stat(remote_path).st_mode) | 252 | return S_ISDIR(self.sftp.lstat(remote_path).st_mode) | ||
| 252 | except IOError: | 253 | except IOError: | ||
| 253 | return False | 254 | return False | ||
| 254 | 255 | | |||
| 255 | def mkdir(self, name, mode=777): | | |||
| 256 | '''Create a directory on a remote machine. | | |||
| 257 | | ||||
| 258 | :param str name: A name of the directory | | |||
| 259 | :param int mode: Permissions (posix-style, decimal) of the directory, | | |||
| 260 | default is 777 | | |||
| 261 | | ||||
| 262 | :raise TaskotronRemoteError: If the directory could not be created | | |||
| 263 | ''' | | |||
| 264 | | ||||
| 265 | self.cmd('mkdir -p -m %d %s' % (mode, pipes.quote(name))) | | |||
| 266 | 256 | | |||
| 267 | def put_dir(self, local_path, remote_path, overwrite=True): | 257 | def put_dir(self, local_path, remote_path, overwrite=True): | ||
| 268 | '''Copy a directory to a remote path. | 258 | '''Copy a directory to a remote path. | ||
| 269 | 259 | | |||
| 270 | :param str remote_path: A path to the remote directory | 260 | :param str remote_path: A path to the remote directory. This directory will be created, if | ||
| 261 | its parent exists. Otherwise you need to create the full tree | ||||
| 262 | structure manually beforehand. | ||||
| 271 | :param str local_path: A path to the local directory | 263 | :param str local_path: A path to the local directory | ||
| 272 | :param bool overwrite: Whether to overwrite remote path. Default is True. | 264 | :param bool overwrite: Whether to overwrite remote path. Default is True. | ||
| 273 | 265 | | |||
| 274 | :raise TaskotronRemoteError: If the directory could not be uploaded | 266 | :raise TaskotronRemoteError: If the directory could not be uploaded | ||
| 275 | ''' | 267 | ''' | ||
| 276 | 268 | | |||
| 277 | if self._remote_file_exists(remote_path) and not overwrite: | 269 | # keep the conditionals in this order to avoid unnecessary remote calls | ||
| 270 | if not overwrite and self._remote_file_exists(remote_path): | ||||
| 278 | log.info('Remote path %s already exists, not overwriting.', remote_path) | 271 | log.info('Remote path %s already exists, not overwriting.', remote_path) | ||
| 279 | return | 272 | return | ||
| 280 | 273 | | |||
| 281 | try: | 274 | try: | ||
| 282 | files = os.listdir(local_path) | 275 | files = os.listdir(local_path) | ||
| 283 | except OSError, e: | 276 | except OSError, e: | ||
| 284 | raise exc.TaskotronRemoteError('Could not put dir %s to %s@%s:%s: %s' % | 277 | raise exc.TaskotronRemoteError('Could not put dir %s to %s@%s:%s: %s' % | ||
| 285 | (local_path, self.username, self.hostname, | 278 | (local_path, self.username, self.hostname, | ||
| 286 | remote_path, e.strerror)) | 279 | remote_path, e.strerror)) | ||
| 287 | 280 | | |||
| 288 | self.mkdir(remote_path) | 281 | if not self._remote_isdir(remote_path): | ||
| 282 | self.sftp.mkdir(remote_path) | ||||
| 289 | 283 | | |||
| 290 | for file_ in files: | 284 | for file_ in files: | ||
| 291 | remote_file = os.path.join(remote_path, file_) | 285 | remote_file = os.path.join(remote_path, file_) | ||
| 292 | local_file = os.path.join(local_path, file_) | 286 | local_file = os.path.join(local_path, file_) | ||
| 293 | if os.path.isdir(local_file): | 287 | if os.path.isdir(local_file): | ||
| 294 | if not self._remote_isdir(remote_file): | | |||
| 295 | self.sftp.mkdir(remote_file) | | |||
| 296 | self.put_dir(local_file, remote_file) | 288 | self.put_dir(local_file, remote_file) | ||
| 297 | else: | 289 | else: | ||
| 298 | self.put_file(local_file, remote_file) | 290 | self.put_file(local_file, remote_file) | ||
| 299 | 291 | | |||
| 300 | def get_dir(self, remote_path, local_path): | 292 | def get_dir(self, remote_path, local_path): | ||
| 301 | '''Get a directory from a remote path. | 293 | '''Get a directory from a remote path. | ||
| 302 | 294 | | |||
| 303 | :param str remote_path: A path to the remote directory | 295 | :param str remote_path: A path to the remote directory | ||
| Show All 20 Lines | |||||
| 1 | # -*- coding: utf-8 -*- | 1 | # -*- coding: utf-8 -*- | ||
|---|---|---|---|---|---|
| 2 | # Copyright 2009-2015, Red Hat, Inc. | 2 | # Copyright 2009-2015, 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 | '''Unit tests for libtaskotron/remote_exec.py''' | 6 | '''Unit tests for libtaskotron/remote_exec.py''' | ||
| 7 | 7 | | |||
| 8 | import errno | 8 | import errno | ||
| 9 | import socket | 9 | import socket | ||
| 10 | 10 | | |||
| 11 | import pytest | 11 | import pytest | ||
| 12 | import paramiko | 12 | import paramiko | ||
| 13 | from munch import Munch | 13 | from munch import Munch | ||
| 14 | from dingus import Dingus | | |||
| 15 | import mock | 14 | import mock | ||
| 16 | import shlex | | |||
| 17 | 15 | | |||
| 18 | from libtaskotron import remote_exec | 16 | from libtaskotron import remote_exec | ||
| 19 | from libtaskotron.exceptions import TaskotronRemoteError, TaskotronRemoteProcessError | 17 | from libtaskotron.exceptions import TaskotronRemoteError, TaskotronRemoteProcessError | ||
| 20 | 18 | | |||
| 21 | 19 | | |||
| 22 | STUB_DIR = 'dirname' | 20 | STUB_DIR = 'dirname' | ||
| 23 | STUB_FILE = 'filename' | 21 | STUB_FILE = 'filename' | ||
| 24 | STUB_NONEXISTING_FILE = '' | 22 | STUB_NONEXISTING_FILE = '' | ||
| ▲ Show 20 Lines • Show All 60 Lines • ▼ Show 20 Line(s) | 76 | def put(self, localpath, remotepath): | |||
| 85 | return attrs | 83 | return attrs | ||
| 86 | 84 | | |||
| 87 | def get(self, remotepath, localpath): | 85 | def get(self, remotepath, localpath): | ||
| 88 | if localpath == STUB_ENOSPC: | 86 | if localpath == STUB_ENOSPC: | ||
| 89 | raise IOError | 87 | raise IOError | ||
| 90 | 88 | | |||
| 91 | return None | 89 | return None | ||
| 92 | 90 | | |||
| 93 | def stat(self, path): | 91 | def lstat(self, path): | ||
| 94 | attrs = paramiko.SFTPAttributes() | 92 | attrs = paramiko.SFTPAttributes() | ||
| 95 | 93 | | |||
| 96 | if path == STUB_DIR: | 94 | if path == STUB_DIR: | ||
| 97 | attrs.st_mode = 16893 # mode of a directory | 95 | attrs.st_mode = 16893 # mode of a directory | ||
| 98 | elif path == STUB_FILE: | 96 | elif path == STUB_FILE: | ||
| 99 | attrs.st_mode = 33204 # mode of a file | 97 | attrs.st_mode = 33204 # mode of a file | ||
| 100 | elif path == STUB_NONEXISTING_FILE: | 98 | elif path == STUB_NONEXISTING_FILE: | ||
| 101 | raise IOError(errno.ENOENT, 'No such file') | 99 | raise IOError(errno.ENOENT, 'No such file') | ||
| ▲ Show 20 Lines • Show All 155 Lines • ▼ Show 20 Line(s) | 243 | def test_install_pkgs_try_twice(self): | |||
| 257 | words2 = ['dnf', 'install', '--refresh'] + pkgs | 255 | words2 = ['dnf', 'install', '--refresh'] + pkgs | ||
| 258 | assert all([word in cmd2 for word in words2]) | 256 | assert all([word in cmd2 for word in words2]) | ||
| 259 | 257 | | |||
| 260 | def test_install_pkgs_fail(self): | 258 | def test_install_pkgs_fail(self): | ||
| 261 | self.remote.cmd = mock.Mock(side_effect=TaskotronRemoteProcessError()) | 259 | self.remote.cmd = mock.Mock(side_effect=TaskotronRemoteProcessError()) | ||
| 262 | with pytest.raises(TaskotronRemoteProcessError): | 260 | with pytest.raises(TaskotronRemoteProcessError): | ||
| 263 | self.remote.install_pkgs(['libtaskotron', 'rpmlint']) | 261 | self.remote.install_pkgs(['libtaskotron', 'rpmlint']) | ||
| 264 | assert self.remote.cmd.call_count == 2 | 262 | assert self.remote.cmd.call_count == 2 | ||
| 265 | | ||||
| 266 | def test_mkdir(self): | | |||
| 267 | ref_dirname = 'dirname' | | |||
| 268 | self.remote.cmd = Dingus() | | |||
| 269 | self.remote.mkdir(ref_dirname) | | |||
| 270 | assert self.remote.cmd.calls()[0][1][0] == 'mkdir -p -m 777 %s' % ref_dirname | | |||
| 271 | | ||||
| 272 | def test_mkdir_mode(self): | | |||
| 273 | ref_dirname = 'dirname' | | |||
| 274 | ref_mode = 644 | | |||
| 275 | self.remote.cmd = Dingus() | | |||
| 276 | self.remote.mkdir(ref_dirname, ref_mode) | | |||
| 277 | assert self.remote.cmd.calls()[0][1][0] == 'mkdir -p -m %d %s' % (ref_mode, ref_dirname) | | |||
| 278 | | ||||
| 279 | def test_mkdir_escape(self): | | |||
| 280 | ref_dirname = 'dirname with spaces & other ch*r*cters; (how sneaky!)' | | |||
| 281 | self.remote.cmd = Dingus() | | |||
| 282 | self.remote.mkdir(ref_dirname) | | |||
| 283 | cmdline = shlex.split(self.remote.cmd.calls()[0][1][0]) | | |||
| 284 | assert cmdline[-1] == ref_dirname | | |||