[SPARK-37677][CORE] Decompress the ZIP file and retain the original file permissions #35278
Conversation
There was a problem hiding this comment.
No need for a StringBuffer (StringBuilder is preferred anyway), just use an interpolated string
There was a problem hiding this comment.
Yes, that's a good suggestion. I'm just worried about concurrency.
There was a problem hiding this comment.
There will be no concurrency issue
There was a problem hiding this comment.
Why do we need a different code path for unzipping only here and not elsewhere?
I'm also concerned about adding the platform dependency on unzip just for this.
Why does the exec bit need to be set - wouldn't you run a script as python script.py anyway to be more sure about the python being used?
There was a problem hiding this comment.
This is the issue of my premis, https://issues.apache.org/jira/browse/SPARK-37677, The submission method I mentioned is not supported after the zip file is unzipped.
There was a problem hiding this comment.
@srowen Do you mean that the absolute path of the source file is different from other decompression methods? This can use relative paths. I use absolute paths for easy understanding at that time.
There was a problem hiding this comment.
No, there's an existing unZip method. At least, why not change that?
There was a problem hiding this comment.
The existing zip decompression method can not retain the original permission information of the file, but can only retain the read-write permission, so the user's zip file has no execution permission after decompression. The current example is that there is no python or python3 command after decompression.
There was a problem hiding this comment.
My point is, would that be helpful in all unzip scenarios? rather than have two different implementations
Adding a dependency on the OS package is probably OK but not sure
Can you add python/python3 to the execution? that seems more standard but not sure if it is possible
There was a problem hiding this comment.
#35278, this PR is to add execution permissions to python/python3, but the suggestion is to retain the permissions of the original file instead of adding permissions.
There was a problem hiding this comment.
The problem here is that it won't support Windows. It's generally not a good practice to rely on external problems that has to be manually installed. If unzip is not installed, it will just fail.
There was a problem hiding this comment.
Yes, the unzip method provided by the previous fileutil by the window operating system can retain the permissions of the original file, so I can use the judgment statement to execute the shell command when the Linux operating system is found, and the window operating system retains the original method.
There was a problem hiding this comment.
In fact, this way of relying on external installation also draws lessons from the decompression method of untar.
There was a problem hiding this comment.
@HyukjinKwon I have revised it again. Could you review it.
There was a problem hiding this comment.
I think untar uses Java library not the external ones. I still think we should avoid relying on external programs. Users can use tar.gz if they want to keep file permissions
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Why do we need to mkdir dest directory again?
There was a problem hiding this comment.
I didn't create a new folder at the beginning, but the file is missing a level-1 directory, which is inconsistent with the original directory layer after decompression, so here is to create a folder first and then extract it.
There was a problem hiding this comment.
does these blank line is related to this change?
There was a problem hiding this comment.
This should be caused by opening the code specification.
There was a problem hiding this comment.
- unzip
- Just suggestion that it's better with shell errorMsg
There was a problem hiding this comment.
Thank you for your advice. This is consistent with the exception thrown by untar decompression. But I do need to change the command name.
There was a problem hiding this comment.
We should think about the case when unzip is not installed in Linux. Is there any other ways to avoid to rely on external programs?
There was a problem hiding this comment.
Because the zip file is decompressed by creating a new file, the permission cannot be saved, but the metadata of the permission can be saved when the unzip command is executed on Linux, but I don't know what special processing is done by the unzip command in Linux. Moreover, I found many API attempts of zip, and found that they were similar to the unzip of JDK, and there was no way to save the permission information.
There was a problem hiding this comment.
I've seen many JDK bugs and found that the unzip decompression permission problem seems to have lasted for a long time, such as https://bugs.openjdk.java.net/browse/JDK-6194856. Using java APIs doesn't seem to solve this problem
There was a problem hiding this comment.
@HyukjinKwon @srowen Hello, I have found a way to store permissions for files, but this requires injecting two dependency projects, is this ok?
There was a problem hiding this comment.
This blank line should not be deleted
There was a problem hiding this comment.
Please restore to import org.apache.spark.internal.{config, Logging}
There was a problem hiding this comment.
This log should be printed after successful decompression. It doesn't seem appropriate to move it in finally
There was a problem hiding this comment.
Is it possible to chmod again after unzip? I'm not sure
|
@HyukjinKwon @LuciferYang @yikf I used the zipfile under the common package, ZipArchiveEntry.getUnixMode() can expose the permissions of the file, so I use the collection to store the permissions of the file. When the file has been created, give each file the original permissions. Could you review it ? |
|
@zhongjingxiong Please check the failure of GA( |
There was a problem hiding this comment.
Can we refer to other references of codes? e.g., https://cs.android.com/android/platform/superproject/+/master:tools/tradefederation/core/common_util/com/android/tradefed/util/ZipUtil2.java;drc=master;l=44
It has a bunch of corner cases handling. .e.g., we will have to skip this logic if the platform is non-Unix.
There was a problem hiding this comment.
Yes, your suggestion is right, we really can not cover all scenarios, I refer to, for the logic of granting file permissions added judgment, for non-UNIX platforms, skip this logic.
smallzhongfeng
force-pushed
the
master-1
branch
from
a1e005c to
a85cf03
Compare
smallzhongfeng
force-pushed
the
master-1
branch
2 times, most recently
from
4d6b77c to
b36131a
Compare
…mission information
smallzhongfeng
force-pushed
the
master-1
branch
from
7e26cc6 to
ec88b85
Compare
@LuciferYang Okay, compiled and tested. |
|
Does this issue only occur in k8s？ |
|
Not only in k8s scenarios, it also works when we add zip files via sparkContext.addFile(). |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for making a PR. Please update your PR title and description according to your code change, @zhongjingxiong .
smallzhongfeng
changed the title
|
@dongjoon-hyun @HyukjinKwon Yes, I changed the title and explained this change. Can you give me some suggestions? |
|
Hey @zhongjingxiong we should make sure covering all cases (see #35278 (comment)) since this replaces an existing functionality. Every case we didn't cover here is the regression. |
|
@HyukjinKwon I think this is an improvement. At least on the UNIX platform, it can realize zip decompression normally, because the zip decompression method provided by Apache common only exposes the file permissions, which is the difference from the original zip decompression method provided by Apache Hadoop. At the same time, due to the platform restrictions, for the non UNIX platform such as windows, We skipped the process of granting file permissions. Once we skipped this logic, the code I wrote is similar to the zip decompression code originally provided by Hadoop. |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
7 participants
What changes were proposed in this pull request?
Unpacking an archive s3a://zjx/python3.6.9.zip#python3 from /tmp/spark-d861d788-bd72-4d3c-88fc-8f79b30b081d/python3.6.9.zip to /opt/spark/work-dir/./python3 Exception in thread "main" java.io.IOException: Cannot run program "python3/bin/python3": error=13, Permission denied at java.base/java.lang.ProcessBuilder.start(Unknown Source) at java.base/java.lang.ProcessBuilder.start(Unknown Source) at org.apache.spark.deploy.PythonRunner$.main(PythonRunner.scala:97)When we set parameter "--archives s3a://zjx/python3.6.9.zip" to submit the Spark job, driver will unzip it, but it will lost executable permission after unzipping, so we should keep those permission. In this PR, I add the executable permission for all file after unzipping. It may cost some time to unzip that, but the time cost is controllable. In the code submitted this time, I keep the permission information of the file in a collection, and then grant the permission to the file one by one after decompression. However, this is only for UNIX platforms. Zip decompression for non UNIX platforms skips the logic of granting permission. In this way, zip decompression can be realized normally.
Why are the changes needed?
When we submit a py job and want to use our own version of Python，we may add "--archives s3a://zjx/python3.6.9.zip" ,after driver unzip this file, and want to execute the program using python3, it will report permission denied. So we should add executable permission to those script. This keeps the permissions of tar and ZIP files the same.
Does this PR introduce any user-facing change?
After the fix, the zip package submitted by the user for Python will run normally.
How was this patch tested?
This scheme is already being deployed in production.