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.
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.
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.
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.
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()
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.
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
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.
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.