Bug 82 - Unnecessary code in send_job
: Unnecessary code in send_job
Status: RESOLVED FIXED
Product: TORQUE
pbs_server
: 2.4.x
: PC Linux
: P5 normal
Assigned To: Glen
:
:
:
  Show dependency treegraph
 
Reported: 2010-09-22 06:30 MDT by Simon Toth
Modified: 2011-03-29 20:10 MDT (History)
2 users (show)

See Also:


Attachments


Note

You need to log in before you can comment on or make changes to this bug.


Description Simon Toth 2010-09-22 06:30:21 MDT
This can and probably should be removed:

if (move_type != MOVE_TYPE_Exec) /* not if sending to MOM */
  job_purge(jobp);


This is a forked child and therefore it doesn't matter at all if the job gets
purged. The process ends after few lines anyway.

Plus the job variable is being accessed in error handling for PBSD_commit.
Comment 1 Glen 2010-11-22 11:49:46 MST
I'm planning on committing this patch to trunk, 2.5-fixes, and 2.4-fixes -
hopefully within the next 24hours
Comment 2 Garrick Staples 2010-11-22 12:01:59 MST
job_purge() changes and deletes files on disk. It doesn't matter if it is
called from a child process; it is still an important function.

There might be some bug or not. I don't know. I'm not commenting on that. I'm
just saying that the requirement for job_purge is not invalidated by being
called from a child process.
Comment 3 Glen 2010-11-22 18:09:57 MST
(In reply to comment #2)
> job_purge() changes and deletes files on disk. It doesn't matter if it is
> called from a child process; it is still an important function.
> 
> There might be some bug or not. I don't know. I'm not commenting on that. I'm
> just saying that the requirement for job_purge is not invalidated by being
> called from a child process.

this is a good point, we need to make sure that the files associated with the
job get cleaned up, but the parent process must call job_purge() at some point,
otherwise the job would persist in the parent process's linked lists
indefinitely (until a reboot, at which time the job would not be recovered
because the job_purge called by the child removed the files).
Comment 4 Glen 2010-11-22 19:01:14 MST
(In reply to comment #3)
> (In reply to comment #2)
> > job_purge() changes and deletes files on disk. It doesn't matter if it is
> > called from a child process; it is still an important function.
> > 
> > There might be some bug or not. I don't know. I'm not commenting on that. I'm
> > just saying that the requirement for job_purge is not invalidated by being
> > called from a child process.
> 
> this is a good point, we need to make sure that the files associated with the
> job get cleaned up, but the parent process must call job_purge() at some point,
> otherwise the job would persist in the parent process's linked lists
> indefinitely (until a reboot, at which time the job would not be recovered
> because the job_purge called by the child removed the files).


not making this change until this can be investigated further.  It definitely
sounds like a bug since the job could be referenced after the job_purge.
Comment 5 Simon Toth 2010-11-23 02:28:50 MST
(In reply to comment #3)
> (In reply to comment #2)
> > job_purge() changes and deletes files on disk. It doesn't matter if it is
> > called from a child process; it is still an important function.
> > 
> > There might be some bug or not. I don't know. I'm not commenting on that. I'm
> > just saying that the requirement for job_purge is not invalidated by being
> > called from a child process.
> 
> this is a good point, we need to make sure that the files associated with the
> job get cleaned up, but the parent process must call job_purge() at some point,
> otherwise the job would persist in the parent process's linked lists
> indefinitely (until a reboot, at which time the job would not be recovered
> because the job_purge called by the child removed the files).

The real job_purge is called from post_movejob (and only if the move succeeded
of course).
Comment 6 Glen 2010-11-30 18:38:57 MST
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > job_purge() changes and deletes files on disk. It doesn't matter if it is
> > > called from a child process; it is still an important function.
> > > 
> > > There might be some bug or not. I don't know. I'm not commenting on that. I'm
> > > just saying that the requirement for job_purge is not invalidated by being
> > > called from a child process.
> > 
> > this is a good point, we need to make sure that the files associated with the
> > job get cleaned up, but the parent process must call job_purge() at some point,
> > otherwise the job would persist in the parent process's linked lists
> > indefinitely (until a reboot, at which time the job would not be recovered
> > because the job_purge called by the child removed the files).
> 
> The real job_purge is called from post_movejob (and only if the move succeeded
> of course).

right

net_move calls send_job with a post_movejob as the argument for the post_func
parameter.  In send_job the parent sets up a work task to run post_movejob when
the child exits.  The job_purge in the child is redundant, and the parent is
the right place to do it.

The only other place send_job() is called is in
req_runjob()->svr_startjob()->svr_strtjob2() to send the job to a mom. We don't
want the job to get purged in this case, and svr_strtjob2 sets up its own post
function to do any required cleanup.

This change looks valid to me.
Comment 7 Glen 2010-11-30 18:54:36 MST
checked into trunk

I'll wait a bit before making the change in 2.5-fixes and 2.4-fixes
Comment 8 Glen 2011-03-13 21:19:20 MDT
fixed in 2.4-fixes and 2.5-fixes as well.  
this still needs to make it into the 3.0 branch
Comment 9 Glen 2011-03-29 20:10:53 MDT
should be fixed in all active branches