PP-774: Proposing new interface for pbs_dshutils.mkdtemp()


#1

Currently in PTL if the mkdtemp() function is called it will try to run it as current user and then try to change the command to given user.

Some test cases are trying to use this function wherein the folder is created as root and then calls chown command which fails, therefore this interface needs enhancement.

The following link is for the new implementation of the given function where it can be fed the argument “asuser” to create the directory as the given user rather than trying to modify it at a later stage.

https://pbspro.atlassian.net/wiki/spaces/PD/pages/1086619651/Address+PP-774+Ability+to+create+directory+and+file+as+a+user+in+PTL+needs+enhancement

Please provide feedback on the same.


#2

Thanks for proposing this change. I have a few concerns/questions about the proposal:

  • DshUtils.mkdtemp() already has a uid argument which can be set to either a username or a linux uid, why not just use that instead of introducing a new ‘asuser’ argument?
  • why did you removed the gid argument?
  • Your design document doesn’t mention the motivation for the change, what problem is it fixing? why is this the appropriate solution for it? The error that you mentioned in your post above occurs because the system where you ran it had sudo rules which prevented the chmod command from being run with sudo. So, if you add the ‘asuser’ argument but don’t change the behavior of the function, then the problem will still persist. On the other hand, I think you can change the behavior of the function without changing its interface and still fix the particular error that you mentioned in your post, so please explain more clearly why this interface change is needed.

#3

Since we now have create_temp_file(), should this be create_temp_dir()?


#4

I agree with @bhroam, We should rename mkdtemp with create_temp_dir() like we renamed mkstemp to create_temp_file()


#5

Hi,

Thanks for the input, currently all interfaces in dshutils have the asuser argument, therefore these changes are just maintain generic interfaces across all of them. Just as in create_temp_file().

As for the removal of “gid”, none of the test cases seem to use gid while calling this function. It is possible to implement this interface without the use of this argument.

To clarify, there is nothing broken with this function. The logical change would be to pass the mkdir command as given user rather than creating a dir as root and then trying to covert it to the requested user. This way the function argument is implemented in the intended way. One can say that this is an enhancement of the same function rather than a bug fix. The users still might have to look at the test cases and they may have to be rewritten. The goal is not to fix the particular error generated from the test case. The goal is to modify this interface to work in the way it is intended.


#6

I have updated the interface name to create_temp_dir()


#7

Actually, only create_temp_file() takes an ‘asuser’ argument, none of the other DshUtils functions take it. In fact, most of them take a ‘uid’ argument, so if anything we should rename create_temp_file’s ‘asuser’ argument to ‘uid’ instead.

PTL is not used for just testing purposes, we have programs other than pbs_benchpress which make use of the PTL library, so just because no tests use it doesn’t justify that we remove it. Thinking about this function as an interface, if we are providing users the ability to create the temporary directory as a given user, with a given mode (that includes restrictions on group accesses), then I think it makes sense for the function to allow the callers to set the gid as well. So, I don’t think we should remove this argument.

But you specifically mentioned in your first post that this enhancement is needed because of the error:

So I’m still confused about the motivation behind this change. If it’s just a cosmetic change to the way that the interface looks because you want to make it consistent with create_temp_file() then please mention that in the EDD.


#8

I don’t agree with this, as asuser make more sense here, as it is sound like “create temp file as user” and that’s exactly it’s purpose.

Agree but instead of gid how about “asgroup” which will take group name instead gid.

It’s both @agrawalravi90, cosmetic + error fix, as currently, mkdtemp is not working as how it should work when user is trying to create a directory as another user (because the only root can do chown). So this is about fixing that along with making interface same as create_temp_file()


#9

Ok, I’m fine with that, but it’s things like this which should be explained in the EDD, I was just replying to @sandisamp’s comment that the reason for renaming it is because all other functions have the argument with the same name.

I’m ok with that, if we are changing uid to asuser then we should definitely rename gid as well in tune

Thanks for clarifying Hiren. I just want to see the reasoning behind the interface change clearly mentioned under a “motivation” or “background” section in the EDD.


#10

I have updated the EDD. And mentioned the motivation behind change @agrawalravi90 as well as added asgroup argument to the interface @hirenvadalia.


#11

@sandisamp In Design doc it says

  • asgroup: optional group name or gid of group owner (Default: None)

I think we should only allow name like as user, else we need to allow uid in asuser also

Rest looks good.

Also please create a ticket for adding new argument “asgroup” to create_temp_file() so we don’t forget it.


#12

The following is the ticket for adding “asgroup” in create_temp_file()
https://pbspro.atlassian.net/browse/PP-1336

I have updated EDD for asgroup to not take gid.


#13

@sandisamp Thanks for update, Design doc looks good to me.


#14

Why? We already have the ability to work with both a uid or a username, gid or groupname, why not keep the flexibility?


#15

I mean, i agree with you that it should be consistent, i just think that it should accept both numeric and string user & group id since that capability already exists


#16

I don’t mind keeping both, but I have seen users mostly passes PbsUser/PbsGroup instance which has both uid and name.
So, if others agree then I am fine keeping both, but personally I like user/group name instead uid/gid.


#17

The functionality is already there, you’ll have to go out of the way to block uid and gid, right now it accepts both, but ya, if others think we should specially block uid and gid then I’ll go with it.