Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GLUTEN-7307][VL] Update openssl version in velox setup for centos9 #7308

Merged
merged 6 commits into from
Oct 1, 2024
Merged

[GLUTEN-7307][VL] Update openssl version in velox setup for centos9 #7308

merged 6 commits into from
Oct 1, 2024

Conversation

pratham76
Copy link
Contributor

What changes were proposed in this pull request?

The current velox setup script for centOS9 brings opnssl-1.1.1s which seems to be outdated all the current spark-3.x runtimes on centOS9 use openssl-3.x versions which causes the spark applications to fail due to this version mismatch.
To handle the above issue - upgraded the openssl version is the current setup.
Fixes: #7307

How was this patch tested?

manual tests

@github-actions GitHub Actions
Copy link

#7307

zhouyuan
zhouyuan reviewed Sep 23, 2024
View reviewed changes
ep/build-velox/src/get_velox.sh Outdated
@@ -107,7 +107,7 @@ function process_setup_centos9 {
sed -i '/^.*dnf_install autoconf/a\ dnf_install libxml2-devel libgsasl-devel libuuid-devel' scripts/setup-centos9.sh

ensure_pattern_matched 'install_gflags' scripts/setup-centos9.sh
sed -i '/^function install_gflags.*/i function install_openssl {\n wget_and_untar https://github.com/openssl/openssl/archive/refs/tags/OpenSSL_1_1_1s.tar.gz openssl \n cd openssl \n ./config no-shared && make depend && make && sudo make install \n cd ..\n}\n' scripts/setup-centos9.sh
sed -i '/^function install_gflags.*/i function install_openssl {\n wget_and_untar https://github.com/openssl/openssl/releases/download/openssl-3.2.2/openssl-3.2.2.tar.gz openssl \n cd openssl \n ./config no-shared && make depend && make && sudo make install \n cd ..\n}\n' scripts/setup-centos9.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use cd ${DEPENDENCY_DIR}/openssl as recent Velox have this change?

Copy link
Contributor Author

@pratham76 pratham76 Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Velox doesn't seem to be bringing openssl 3.x as a part of centOS9 setup. It is gluten which is bringing openssl for build process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pratham76 scripts/setup-centos9.sh is coming from Velox.
In Velox, there was a recent change facebookincubator/velox#10920 to use ${DEPENDENCY_DIR} for dependencies. As @zhouyuan suggested, we need to use cd ${DEPENDENCY_DIR}/openssl when wget_and_untar is used.
Can you validate your change on the latest gluten main branch?

Copy link
Contributor Author

@pratham76 pratham76 Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. will update and validate.
This change does not seem to be added to gluten main branch yet - there are other dependencies being installed in current directory instead of ${DEPENDENCY_DIR}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Velox is updated daily and I do see this PR here https://github.com/oap-project/velox

Copy link
Contributor Author

@pratham76 pratham76 Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but gluten overrides those and adds new installs in this script which also has to be updated. As these does not use ${DEPENDENCY_DIR} currently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why is ${DEPENDENCY_DIR} not being used?

Copy link
Contributor Author

@pratham76 pratham76 Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have updated the changes as requested and validated the build with manual tests. Kindly review the changes. In the above comment i was noting that as gluten overrides the velox setup scripts using sed, changes wrt install directories have to be added here too.

majetideepak
majetideepak reviewed Sep 26, 2024
View reviewed changes
ep/build-velox/src/get_velox.sh Outdated
@@ -107,7 +107,7 @@ function process_setup_centos9 {
sed -i '/^.*dnf_install autoconf/a\ dnf_install libxml2-devel libgsasl-devel libuuid-devel' scripts/setup-centos9.sh

ensure_pattern_matched 'install_gflags' scripts/setup-centos9.sh
sed -i '/^function install_gflags.*/i function install_openssl {\n wget_and_untar https://github.com/openssl/openssl/archive/refs/tags/OpenSSL_1_1_1s.tar.gz openssl \n cd openssl \n ./config no-shared && make depend && make && sudo make install \n cd ..\n}\n' scripts/setup-centos9.sh
sed -i '/^function install_gflags.*/i function install_openssl {\n wget_and_untar https://github.com/openssl/openssl/releases/download/openssl-3.2.2/openssl-3.2.2.tar.gz openssl \n cd ${DEPENDENCY_DIR}/openssl \n ./config no-shared && make depend && make && sudo make install \n}\n' scripts/setup-centos9.sh
Copy link
Contributor

@majetideepak majetideepak Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to use curly braces if you want to cd in a subshell and avoid cd ../..
( cd ${DEPENDENCY_DIR}/openssl \n ./config no-shared && make depend && make && sudo make install )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated. Will keep this in mind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@majetideepak please review and lmk if any more changes are required. Thanks.

majetideepak
majetideepak approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @pratham76

@pratham76
Copy link
Contributor Author

@zhouyuan Kindly review/approve the changes and inform if any more changes are required according to you. Thanks

zhouyuan
zhouyuan approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@zhouyuan zhouyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@zhouyuan zhouyuan merged commit cd71aa7 into apache:main Oct 1, 2024
42 checks passed
@GlutenPerfBot
Copy link
Contributor

===== Performance report for TPCDS SF2000 with Velox backend, for reference only ====

query log/native_master_10_01_2024_time.csv log/native_master_09_30_2024_93056f0903_time.csv difference percentage
q1 13.81 13.70 -0.106 99.23%
q2 15.34 15.57 0.233 101.52%
q3 4.46 3.03 -1.425 68.04%
q4 70.10 71.84 1.740 102.48%
q5 10.25 10.55 0.301 102.94%
q6 3.80 4.01 0.207 105.45%
q7 6.56 6.01 -0.548 91.64%
q8 5.03 3.38 -1.646 67.26%
q9 24.35 25.07 0.713 102.93%
q10 8.28 10.09 1.809 121.85%
q11 37.59 36.37 -1.222 96.75%
q12 1.42 1.46 0.041 102.87%
q13 6.43 6.76 0.327 105.09%
q14a 47.23 46.57 -0.664 98.59%
q14b 40.13 41.79 1.664 104.15%
q15 2.29 2.33 0.044 101.94%
q16 48.08 47.25 -0.833 98.27%
q17 5.01 4.88 -0.128 97.44%
q18 6.71 8.19 1.478 122.02%
q19 2.09 1.95 -0.143 93.17%
q20 1.50 1.47 -0.027 98.20%
q21 1.16 1.04 -0.123 89.35%
q22 8.41 7.81 -0.604 92.82%
q23a 104.04 102.90 -1.139 98.91%
q23b 128.22 125.59 -2.628 97.95%
q24a 104.65 112.25 7.592 107.25%
q24b 113.92 111.34 -2.583 97.73%
q25 4.24 4.15 -0.089 97.89%
q26 3.97 4.04 0.072 101.82%
q27 4.86 5.05 0.184 103.78%
q28 32.82 30.61 -2.209 93.27%
q29 10.67 11.24 0.569 105.33%
q30 4.66 5.67 1.015 121.80%
q31 8.54 6.84 -1.703 80.06%
q32 2.41 1.21 -1.198 50.24%
q33 4.18 4.31 0.130 103.11%
q34 3.67 4.28 0.616 116.81%
q35 7.58 7.79 0.206 102.71%
q36 5.72 5.68 -0.034 99.40%
q37 4.81 4.97 0.155 103.22%
q38 13.41 13.38 -0.031 99.77%
q39a 3.17 3.06 -0.111 96.50%
q39b 2.78 3.13 0.350 112.60%
q40 4.03 3.95 -0.080 98.01%
q41 0.65 0.64 -0.009 98.67%
q42 0.85 0.87 0.023 102.77%
q43 4.43 4.62 0.194 104.38%
q44 9.60 9.34 -0.263 97.26%
q45 3.21 3.28 0.067 102.07%
q46 3.62 3.59 -0.033 99.08%
q47 17.96 17.77 -0.190 98.94%
q48 4.97 4.80 -0.174 96.50%
q49 6.93 7.58 0.656 109.47%
q50 21.56 21.74 0.181 100.84%
q51 9.82 9.62 -0.203 97.93%
q52 1.00 0.98 -0.018 98.16%
q53 2.18 2.15 -0.025 98.84%
q54 4.33 3.55 -0.777 82.04%
q55 1.05 1.02 -0.024 97.68%
q56 3.94 4.07 0.127 103.22%
q57 11.00 10.23 -0.777 92.94%
q58 2.60 2.65 0.050 101.94%
q59 10.62 11.41 0.795 107.48%
q60 3.98 3.99 0.006 100.15%
q61 4.13 4.10 -0.024 99.42%
q62 4.63 4.55 -0.078 98.31%
q63 2.27 2.17 -0.095 95.80%
q64 63.19 59.35 -3.836 93.93%
q65 16.91 19.95 3.037 117.96%
q66 4.44 4.09 -0.346 92.20%
q67 436.11 436.03 -0.075 99.98%
q68 3.64 4.60 0.964 126.48%
q69 5.28 5.34 0.052 100.99%
q70 11.19 10.61 -0.575 94.86%
q71 2.60 2.36 -0.240 90.78%
q72 221.68 214.44 -7.238 96.73%
q73 2.40 2.25 -0.152 93.67%
q74 23.44 23.02 -0.422 98.20%
q75 26.96 26.50 -0.454 98.31%
q76 12.24 13.61 1.371 111.20%
q77 2.04 2.19 0.148 107.24%
q78 49.14 49.89 0.744 101.51%
q79 3.92 3.90 -0.023 99.40%
q80 11.17 11.42 0.255 102.28%
q81 4.54 4.56 0.023 100.50%
q82 6.79 6.58 -0.208 96.94%
q83 1.48 1.59 0.111 107.48%
q84 2.90 2.87 -0.035 98.78%
q85 7.43 6.75 -0.675 90.91%
q86 4.27 4.20 -0.068 98.40%
q87 15.52 14.57 -0.945 93.91%
q88 17.12 16.76 -0.359 97.91%
q89 3.38 3.52 0.134 103.97%
q90 2.81 2.70 -0.111 96.07%
q91 2.35 1.95 -0.392 83.29%
q92 1.35 1.22 -0.129 90.45%
q93 41.43 34.75 -6.676 83.89%
q94 25.51 25.87 0.363 101.42%
q9 91.76 86.19 -5.567 93.93%
q5 2.61 2.61 -0.001 99.97%
q96 18.19 18.33 0.142 100.78%
q97 1.82 1.83 0.001 100.08%
q98 10.78 11.30 0.521 104.83%
q99 10.78 11.30 0.521 104.83%
total 2236.10 2215.02 -21.082 99.06%

@GlutenPerfBot
Copy link
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query log/native_master_10_01_2024_time.csv log/native_master_09_30_2024_93056f0903_time.csv difference percentage
q1 53.79 52.59 -1.203 97.76%
q2 30.17 30.47 0.292 100.97%
q3 53.40 53.51 0.105 100.20%
q4 43.60 43.69 0.095 100.22%
q5 102.90 103.16 0.252 100.24%
q6 12.85 10.65 -2.203 82.85%
q7 110.87 108.91 -1.960 98.23%
q8 115.45 116.01 0.566 100.49%
q9 171.10 172.47 1.372 100.80%
q10 65.10 67.09 1.983 103.05%
q11 26.25 25.91 -0.335 98.72%
q12 31.64 33.45 1.811 105.72%
q13 51.97 53.01 1.041 102.00%
q14 24.66 23.44 -1.221 95.05%
q15 49.70 50.76 1.052 102.12%
q16 18.63 18.03 -0.607 96.74%
q17 128.77 126.33 -2.441 98.10%
q18 199.59 198.21 -1.379 99.31%
q19 28.38 28.05 -0.323 98.86%
q20 41.94 42.97 1.031 102.46%
q21 339.71 339.40 -0.317 99.91%
q22 16.13 15.95 -0.185 98.85%
total 1716.63 1714.05 -2.576 99.85%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouyuan zhouyuan zhouyuan approved these changes

@majetideepak majetideepak majetideepak approved these changes

Assignees
No one assigned
Labels
BUILD VELOX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] s2_init() failure due to libcrypto/openssl version mismatch at rutime.
4 participants
@pratham76 @GlutenPerfBot @zhouyuan @majetideepak