PP-1009 : Enhancing sudo usage in PTL

Hi All,

Please review the design document created for Sudo enhancements in PTL.
https://pbspro.atlassian.net/wiki/spaces/PD/pages/72974341/PP-1009+Sudo+enhancements

Please do let me know of any comments or suggestions.

Thanks and Regards,
Kumar

Could I suggest a title change? The current title calls to mind changes contemplated for the OS’s sudo command, and that’s not at all what this is about.

@altair4 : I have updated the summary .

@kjakkali, what is the default location the temp file will get created? is it still /tmp? please specify.
Also can you specify the default value for all the parameters to create_temp_file for example, hostname will be localhost and asuser will be current user and directory will be current directory or /tmp, etc …

Reword the sentence “The existing interface executes cp command -p flag. Which will be preserve owner,group permissions of source file. So updating run_copy () interface to remove -p flag.” to
"The existing interface executes cp command with -p flag which preserves owner,group permissions of source file hence updating run_copy() interface to remove -p flag."

@anamika : Thank you.
I have updated the design doc. Please re-review.

thanks @kjakkali. looks good to me.

@kjakkali,
I have a couple of questions and comments:

  • Why does create_temp_file() open the file in binary mode by default? I would think that most uses of create_temp_file() are to write text into it.
  • What is the difference between create_temp_file() and create_script()? I mean a job script is really just a text file with commands in it.
  • The change to run_copy() seems extraneous to this RFE. What is the reasoning behind it? Why is it bad to keep the permissions of the existing files? Your explanation doesn’t really tell me why you’re doing it, just what you are doing.

Bhroam

@bhroam : Thanks your comments.

Update the design doc.

create_script() is an existing interface. It creates a temp file with prefix ‘PtlPbsJobScript’ and sets file permission 755
create_temp_file() creates file with 644 file permission.

If we want to have only create_temp_file() then user has to explicitly do chmod to 755 set execute permission.

Let me know you input on this.

In PTL library most of the places wherever copy file used is followed by chown & chmod. If we don’t use -p flag then file will destination user and permission automatically so we don’t need to explicitly do chown and chmod.
number of operations will reduce.

This is interesting. There is nothing in PBS that requires a job script to have 755 permissions. I tested it and a job script will work perfectly fine with 644 permissions. It’s an existing call, so it’s probably best to leave it alone.

This is dangerous. When copying without the -p, you are subject to the user’s umask. If you don’t use the -p or a subsequent chmod, the permissions of the file will not be automatically known. Is this what you want? The permissions to be set haphazardly depending on the umask of the user?

Bhroam

@bhroam

Yes. PTL library most of the file copies are to pbs priv dir.

Yes, you do want the permissions to be set haphazardly depending on the umask of the user?

@altair4 :

Will there be any issue?

You tell us - you’re the one who answered yes to Bhroam’s question of whether you wanted the permissions to be set haphazardly.

@bhroam, @altair4 : Let me explain with an example

Currently in PTL we do cp -p which retains source file permissions then we do chmod , chown to change file permissions to match with destination permission.

Our use case is to copy files with keeping destination permissions . This can be achieved by doing cp without -p flag.
If destination file already exists then cp will keep destination file permissions else cp will create new file with user’s umask permissions. If test want to set different permissions than user’s umask permissions then test has to explicitly do chmod/chown.

P.S: We want to reduce usage of chmod and chown. If PTL is executed as non privileged user then we might have issue with chmod and chown as non privileged user might not have sufficient permissions.

Ex:
Current behavior:
To modify sched_config PTL does

  • creates temp file with new sched_config (file permission will be 600)
  • copy temp file to sched_priv dir using cp -p ( file permission will be 600)
  • chmod file under sched_priv to 644
  • chown file user sched_priv to root

Proposed solution:
To modify sched_config PTL does

  • creates temp file with new sched_config (file permission will be 600)
  • copy temp file to sched_priv dir using cp ( file permission will be 644)

I understand your desire to limit calls to chown/chmod. I’m worried that you are making some assumptions that may not be good. You’re assuming there is an existing file there already that has the permissions you want. If there isn’t, you are at the mercy of the user’s umask. The only way to be sure you’re setting up a default situation is to either check for the existing file (and ownership/perms) or set them afterwards like you don’t want to do.

@bhroam : Sorry for replying late.
I understand your concerns , so i have updated the design doc. I have introduced a new parameter ‘keep_permission’ to run_copy().
if ‘keep_permission = True’ then run_copy will continue existing behavior
if ‘keep_permission = False’ then run_copy will copy files with using -p flag.

Let me know if your comments/suggestions

Thanks for updating the EDD with the new parameter. I think it’ll be a nice enhancement to the function. I notice you are changing the default behavior of the function. I always find it is nice to keep API’s stable unless there is a very good reason to change it. I am not sure there’s a good reason to change the default behavior here, but if you think so, I’m OK with it.

Bhroam

@bhroam : I have updated param name to ’ preserve_permission’ and default value. Please review and let me know your comments/suggestions.

Both keep and preserve both convey what is going on. I’m happy with either.

Bhroam

@bhroam :Thanks. Let me know if you have any further comments. Please review/sign off.

@anamika : There are few changes in EDD after your sign off. Can you please review updated EDD?