Skip to content

Commit

Permalink
Two fixes
Browse files Browse the repository at this point in the history
Two separate fixes lumped together in one commit:

* Fix behavior of `task.free()`: it should invoke action on the attached controller, like `task.kill()` etc. do.  This also fixes `gsession abort` and similar which were not cleaning up working directories.
* Ensure that PIDs in the "job info" structures in `ShellcmdLrms` are always stored as strings. (Do not mix string and int types as was happening before.)

Closes #630
  • Loading branch information
smaffiol authored and riccardomurri committed Aug 24, 2017
1 parent 9ab6d12 commit 17629e8
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 30 deletions.
8 changes: 7 additions & 1 deletion gc3libs/__init__.py
Expand Up @@ -484,7 +484,13 @@ def free(self, **extra_args):
See :meth:`gc3libs.Core.free` for a full explanation.
"""
return
assert self._attached, ("Task.free() called on detached task %s." %
self)
assert hasattr(self._controller, 'free'), \
("Invalid `_controller` object '%s' in Task %s" %
(self._controller, self))
self._controller.free(self, **extra_args)


# convenience methods, do not really add any functionality over
# what's above
Expand Down
31 changes: 13 additions & 18 deletions gc3libs/backends/shellcmd.py
Expand Up @@ -229,7 +229,7 @@ def get_process_state(self, pid):
Raise ``LookupError`` if no process is identified by the given PID.
"""
cmd = 'ps -p {0:d} -o state='.format(pid)
cmd = 'ps -p {0} -o state='.format(pid)
rc, stdout, stderr = self.transport.execute_command(cmd)
if rc == 1: # FIXME: same return code on MacOSX?
raise LookupError('No process with PID {0}'.format(pid))
Expand All @@ -245,7 +245,7 @@ def get_process_running_time(self, pid):
Raise ``LookupError`` if no process is identified by the given PID.
"""
cmd = 'ps -p {0:d} -o etime='.format(pid)
cmd = 'ps -p {0} -o etime='.format(pid)
rc, stdout, stderr = self.transport.execute_command(cmd)
if rc == 1: # FIXME: same return code on MacOSX?
raise LookupError('No process with PID {0}'.format(pid))
Expand Down Expand Up @@ -297,7 +297,7 @@ def _get_total_memory_impl(self):
"""Machine-specific part of `get_total_memory`."""
pass

def list_process_tree(self, root_pid=1):
def list_process_tree(self, root_pid="1"):
"""
Return list of PIDs of children of the given process.
Expand All @@ -316,7 +316,7 @@ def list_process_tree(self, root_pid=1):
if not line:
continue
pid, ppid = line.split()
children[int(ppid)].append(int(pid))
children[str(ppid)].append(str(pid))
if root_pid not in children:
return []

Expand Down Expand Up @@ -903,7 +903,7 @@ def _get_persisted_job_info(self):
for pid in pidfiles:
job = self._read_job_info_file(pid)
if job:
job_infos[pid] = job
job_infos[str(pid)] = job
else:
# Process not found, ignore it
continue
Expand All @@ -915,7 +915,7 @@ def _read_job_info_file(self, pid):
exists. Returns None if it does not exist.
"""
self.transport.connect()
log.debug("Reading job info file for pid %s", pid)
log.debug("Reading job info file for pid %r", pid)
jobinfo = None
path = posixpath.join(self.resource_dir, str(pid))
with self.transport.open(path, 'rb') as fp:
Expand Down Expand Up @@ -944,7 +944,7 @@ def _delete_job_info_file(self, pid):
"""
self.transport.connect()
log.debug("Deleting job info file for pid %s ...", pid)
pidfile = posixpath.join(self.resource_dir, str(pid))
pidfile = posixpath.join(self.resource_dir, pid)
try:
self.transport.remove(pidfile)
except Exception as err:
Expand Down Expand Up @@ -981,7 +981,7 @@ def cancel_job(self, app):
stored (by `submit_job`:meth:) as `app.execution.lrms_jobid`.
"""
try:
root_pid = int(app.execution.lrms_jobid)
root_pid = app.execution.lrms_jobid
except ValueError:
raise gc3libs.exceptions.InvalidArgument(
"Invalid field `lrms_jobid` in Task '{0}':"
Expand Down Expand Up @@ -1540,16 +1540,11 @@ def _read_app_process_id(self, pidfile_path):
try:
with self.transport.open(pidfile_path, 'r') as pidfile:
pid = pidfile.read().strip()
return int(pid)
except ValueError:
# it happens that the PID file exists, but `.read()`
# returns the empty string... just wait and retry.
if pid == '':
continue
else:
raise gc3libs.exceptions.LRMSSubmitError(
"Invalid PID `{0}` in pidfile '{1}': {2}."
.format(pid, pidfile_path, err))
# it happens that the PID file exists, but `.read()`
# returns the empty string... just wait and retry.
if pid == '':
continue
return pid
except gc3libs.exceptions.TransportError as ex:
if '[Errno 2]' in str(ex): # no such file or directory
time.sleep(retry)
Expand Down
16 changes: 8 additions & 8 deletions gc3libs/backends/tests/test_machine.py
Expand Up @@ -85,16 +85,16 @@ def test_list_process_tree_full(transport):
'',
)

pids = mach.list_process_tree(2361)
pids = mach.list_process_tree("2361")
assert pids == [
# PID PPID CMD
2361, # 2361 1466 /sbin/upstart --user
2431, # 2431 2361 \_ upstart-udev-bridge --daemon --user
2663, # 2663 2361 \_ gpg-agent --homedir /home/rmurri/.gnupg --use-standard-socket --daemon
2679, # 2679 2361 \_ /usr/lib/at-spi2-core/at-spi-bus-launcher
2725, # 2684 2679 | \_ /usr/bin/dbus-daemon --config-file=/etc/at-spi2/accessibility.conf --nofork --print-address 3
2684, # 2725 2361 \_ /usr/bin/lxsession -s Lubuntu -e LXDE
2736, # 2736 2725 | \_ lxpanel --profile Lubuntu
"2361", # 2361 1466 /sbin/upstart --user
"2431", # 2431 2361 \_ upstart-udev-bridge --daemon --user
"2663", # 2663 2361 \_ gpg-agent --homedir /home/rmurri/.gnupg --use-standard-socket --daemon
"2679", # 2679 2361 \_ /usr/lib/at-spi2-core/at-spi-bus-launcher
"2725", # 2684 2679 | \_ /usr/bin/dbus-daemon --config-file=/etc/at-spi2/accessibility.conf --nofork --print-address 3
"2684", # 2725 2361 \_ /usr/bin/lxsession -s Lubuntu -e LXDE
"2736", # 2736 2725 | \_ lxpanel --profile Lubuntu
]


Expand Down
32 changes: 32 additions & 0 deletions gc3libs/backends/tests/test_shellcmd.py
Expand Up @@ -415,5 +415,37 @@ def test_resource_sharing_w_multiple_backends(self):
core1.free(app)


def test_pid_list_is_string(self):
tmpdir = tempfile.mkdtemp(prefix=__name__, suffix='.d')
(fd, cfgfile) = tempfile.mkstemp()
f = os.fdopen(fd, 'w+')
f.write(TestBackendShellcmdCFG.CONF % ("False", cfgfile + '.d'))
f.close()
self.files_to_remove = [cfgfile, cfgfile + '.d']

self.cfg = gc3libs.config.Configuration()
self.cfg.merge_file(cfgfile)

self.core = gc3libs.core.Core(self.cfg)
self.backend = self.core.get_backend('localhost_test')
# Update resource status

app = gc3libs.Application(
arguments=['/bin/echo', 'Hello', 'World'],
inputs=[],
outputs=[],
output_dir=tmpdir,
requested_cores=1,
requested_memory=10 * Memory.MiB, )

try:
self.core.submit(app)
for pid in self.backend._job_infos.keys():
assert isinstance(pid,str)
finally:
self.core.kill(app)
self.core.free(app)


if __name__ == "__main__":
pytest.main(["-v", __file__])
5 changes: 2 additions & 3 deletions gc3utils/commands.py
Expand Up @@ -1155,13 +1155,12 @@ def abort_session(self):
continue
try:
task.kill()
task.free()
rc -= 1
except gc3libs.exceptions.Error, err:
gc3libs.log.error(
"Could not abort task '%s': %s: %s",
task, err.__class__.__name__, err)
finally:
task.free()
rc -= 1

if rc:
gc3libs.log.error(
Expand Down

0 comments on commit 17629e8

Please sign in to comment.