-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
AMBARI-25854: Adapt Ambari 2.7.5 from python2 to python3.9 #3681
base: branch-2.7
Are you sure you want to change the base?
AMBARI-25854: Adapt Ambari 2.7.5 from python2 to python3.9 #3681
Conversation
OLD_RESOURCE_MANAGEMENT_DIR="/usr/lib/python3.9/site-packages/resource_management" | ||
OLD_JINJA_DIR="/usr/lib/python3.9/site-packages/ambari_jinja2" | ||
OLD_SIMPLEJSON_DIR="/usr/lib/python3.9/site-packages/ambari_simplejson" | ||
OLD_AMBARI_AGENT_DIR="/usr/lib/python3.9/site-packages/ambari_agent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract to one variable so that we can change the python version in one place..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generate a build script build-ambari.sh to solve the problem. related link: 6e8082a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like Ambari should come with a venv for its own packages rather than pollute OS-level site-packages
ambari-agent/pom.xml
Outdated
@@ -314,7 +314,7 @@ | |||
<fileEncoding>utf-8</fileEncoding> | |||
</postremoveScriptlet> | |||
|
|||
<needarch>x86_64</needarch> | |||
<needarch>aarch64</needarch> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep x86_64 as default one and make it dynamic..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generate a build script build-ambari.sh to solve the problem. related link: 6e8082a
@@ -200,27 +200,27 @@ def cache_dir(self): | |||
|
|||
@property | |||
def cluster_cache_dir(self): | |||
return os.path.join(self.cache_dir, FileCache.CLUSTER_CACHE_DIRECTORY) | |||
return os.path.join('ambari_agent', 'dummy_files') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why all these are changed to "dummy_files"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we try to test python3, we find that the corresponding path cannot be obtained. However, the default path obtained by python2 is this path. Therefore, the code is changed to dummy_files during the test of python3.
I am trying to apply this patch on branch-2.7, but https://github.com/wuzhuoming/ambari-python3/blame/AMBARI-25854-release-2.7.5/ambari-agent/src/main/python/ambari_agent/listeners/ConfigurationEventListener.py#L53 from line 53 to 56 these changes are not present in branch-2.7, seems you haven't applied the changes on top of branch-2.7. |
Hello, here ambari 2.7.5 source code is downloaded from the official website of ambari. Link: https://archive.apache.org/dist/ambari/ambari-2.7.5/apache-ambari-2.7.5-src.tar.gz |
Please use the latest code from https://github.com/apache/ambari/tree/branch-2.7 |
Working on it! We are comparing the code between branch-2.7 and release-2.7.5 and making some modification to solve the conflit. Thank you for your comment! |
Do not delete .../.gitignore this file... |
Hi I am getting below error, while creating ambari rpms07:46:07,345 [ERROR] error /workspace/ambari/contrib/views/files/src/main/resources/ui/node_modules/node-sass: Command failed. |
Sorry, the Ambari adapts to the ARM architecture and Python3 based on the openEuler system. Currently, the code of the x86 architecture branch is not verified or adapted. Our team is considering whether to proceed with this task. |
Hi I am getting below error for both ambari-agent as well as ambari-server, while adding zookeeper serviceTraceback (most recent call last): |
Hi,what your system environment and architecture is? And how did you add the Zookeeper service? |
I am using RHEL 8 machines. I have installed ambari-agent, ambari-server and through Ambari UI install wizard I am adding Zookeeper service |
You can create an issue and attach the error log or image. I'm not sure where you went wrong. |
Hi I am getting below error while adding service trough Ambari UI Install wizardFile "/usr/lib/ambari-agent/lib/resource_management/core/providers/packaging.py", line 30, in action_install self._pkg_manager.install_package(package_name, self.__create_context()) While analyzing this I found that yum module is not available in python3, have you also observed same error? |
@himanshumaurya09876 |
Evaluating the rpm_check_package_available method, it uses the python rpm library which is also discontinued for Python3, as a suggestion we can use the os module to execute the rpm command. Here is a suggestion of how the method would look:
|
Hi, I am getting below error while adding Ambari-Metrics to my clusterTraceback (most recent call last): |
This PR seemed to be trying to use Python 3.9, not 3.6. What happens when you upgrade that? @himanshumaurya09876 Also, use code fences for formatting, please |
Hi, I have tried both python 3.6 as well as 3.9. It's giving same error in both cases. |
@himanshumaurya09876 |
@@ -317,42 +317,42 @@ def run(self): | |||
self.cmd_status() | |||
|
|||
def cmd_start(self): | |||
print "Starting %d host(s)" % len(self.hosts) | |||
print("Starting %d host(s)" % len(self.hosts)) | |||
for host in self.hosts: | |||
cmd = "ambari-agent start --home %s" % (host.home_dir) | |||
os.environ['AMBARI_AGENT_CONF_DIR'] = os.path.join(host.home_dir, "etc/ambari-agent/conf") | |||
subprocess32.call(cmd, shell=True, env=os.environ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update subprocess32 to subprocess ?
for host in self.hosts: | ||
cmd = "ambari-agent start --home %s" % (host.home_dir) | ||
os.environ['AMBARI_AGENT_CONF_DIR'] = os.path.join(host.home_dir, "etc/ambari-agent/conf") | ||
subprocess32.call(cmd, shell=True, env=os.environ) | ||
|
||
def cmd_stop(self): | ||
print "Stopping %d host(s)" % len(self.hosts) | ||
print("Stopping %d host(s)" % len(self.hosts)) | ||
for host in self.hosts: | ||
cmd = "ambari-agent stop --home %s" % (host.home_dir) | ||
os.environ['AMBARI_AGENT_CONF_DIR'] = os.path.join(host.home_dir, "etc/ambari-agent/conf") | ||
subprocess32.call(cmd, shell=True, env=os.environ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update subprocess32 to subprocess ?
for host in self.hosts: | ||
cmd = "ambari-agent stop --home %s" % (host.home_dir) | ||
os.environ['AMBARI_AGENT_CONF_DIR'] = os.path.join(host.home_dir, "etc/ambari-agent/conf") | ||
subprocess32.call(cmd, shell=True, env=os.environ) | ||
|
||
def cmd_restart(self): | ||
print "Restarting %d host(s)" % len(self.hosts) | ||
print("Restarting %d host(s)" % len(self.hosts)) | ||
for host in self.hosts: | ||
cmd = "ambari-agent restart --home %s" % (host.home_dir) | ||
os.environ['AMBARI_AGENT_CONF_DIR'] = os.path.join(host.home_dir, "etc/ambari-agent/conf") | ||
subprocess32.call(cmd, shell=True, env=os.environ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update subprocess32 to subprocess ?
@@ -226,7 +226,7 @@ def create_host_name_script(self, host_name, host_name_script): | |||
"echo HOSTNAME" | |||
with open(str(host_name_script), "w+") as f: | |||
f.writelines(template.replace("HOSTNAME", host_name)) | |||
subprocess32.call("chmod +x %s" % host_name_script, shell=True) | |||
subprocess.call("chmod +x %s" % host_name_script, shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any security considerations here ? can the subprocess call be used with shell=False ?
Example - subprocess.call(["chmod" + " +x " + host_name_script], shell=False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the subprocess32 module is a copy (maybe there is some customization but I don't know) of Python's standard subprocess module, so the most viable thing would be to maintain the use of the module that is already standard in the language and not maintain a "separate" module, in my opinion it would not make much sense.
As for the use of shell=True, it should be avoided taking into account the issue of security and the possibility of executing some arbitrary code.
shell=True It is sometimes convenient to make use of shell-specific features such as word splitting or parameter expansion.
We would have a syntax change at all points where shell=True is executed, example:
of this
subprocess.Popen("command -with -options 'like this' and\\ an\\ argument", shell=True)
For that
subprocess.Popen(['command', '-with','-options', 'like this', 'and an argument'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that Python introduces true concurrency through the use of multiple processes. However, Python 2 doesn't have the subprocess module by default. Therefore, we copied the subprocess module from Python 3's standard library and made compatibility modifications to maintain it within the Ambari codebase. So, if we upgrade to Python 3, we won't need to maintain our own version of the subprocess module anymore and can directly use the one provided by Python 3.
@wuzhuoming AMBARI-25854: XXX Thx. |
Hi I am getting failures for below UT `======================================================================
|
# Conflicts: # ambari-agent/src/main/python/ambari_agent/CommandStatusDict.py # ambari-agent/src/main/python/ambari_agent/HostInfo.py # ambari-agent/src/main/python/ambari_agent/listeners/ConfigurationEventListener.py # ambari-agent/src/main/python/ambari_agent/listeners/HostLevelParamsEventListener.py # ambari-server/src/main/python/ambari_server/dbCleanup.py # ambari-server/src/test/python/TestAmbariServer.py # ambari-server/src/test/python/TestMpacks.py # ambari-server/src/test/python/TestSetupSso.py # ambari-server/src/test/python/TestSetupTrustedProxy.py # ambari-server/src/test/python/custom_actions/TestCheckHost.py
c5641b7
to
f02e68c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to above changes stderr:/var/lib/ambari-agent/data/errors.txt is filled with below logs
WARNING:root:An exception occurred when calling the hasattr method...... WARNING:root:An exception occurred when calling the hasattr method...... WARNING:root:An exception occurred when calling the hasattr method......
@wuzhuoming are you observing the same? |
@himanshumaurya09876 @bhavikpatel9977 Thank for pointing out the issue, we're trying to reproduce this problem locally. BTW, can someone tell us what os the CI/CD uses, we need to figure out which python version is supported. |
Good point. Are we also planning to have new branch for python-3 or we will merge it in the current 2.7.5 branch? |
I think we better merge this feature into a new branch, trunk or 2.8 or others. |
When are you guys planning to complete the validation? |
we may need one more week to fix some issues and we also welcome more contributors to join for fully validation on different os or scenary. |
Sounds good!! Let's connect on slack channel, we will also contribute on this. |
I also think that merging it in version 2.8 or later is a good suggestion. 2.8 is a completely new version that is separate from HDP and is currently under development. If we merge it in, we can perform extensive validation testing during the subsequent development. Additionally, version 2.7 has already become closed source within the HDP stack and is currently in a maintenance state, making it unsuitable for major changes or new features. |
@@ -107,8 +107,9 @@ def run(self): | |||
scpstat = subprocess.Popen(scpcommand, stdout=subprocess.PIPE, | |||
stderr=subprocess.PIPE) | |||
log = scpstat.communicate() | |||
errorMsg = log[1] | |||
log = log[0] + "\n" + log[1] | |||
log_value0 = log[0].decode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the check if it is instance of bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the check if it is instance of bytes?
In python3, Subprocesses output bytes, not characters when calling communicate()
, so we need to add universal_newlines=True
if we want str.
@wenwj0 have you observed the same behaviour and fixed the same? |
We have fixed this issue now and you guys can try to test it. @bhavikpatel9977 @himanshumaurya09876 This problem occurs when we try |
Hi everyone, we create a new PR (#3751) to the trunk branch. Please take a look :) |
What changes were proposed in this pull request?
This PR show the necessary adaption for Ambari 2.7.5 to adapt python3.9.
related JIRA Issue: https://issues.apache.org/jira/browse/AMBARI-25854
How was this patch tested?
unit tests,
manual tests