CP-45360: Update xsconsole to python3

[qinzhang22]

The PR includes the following changes:

1. Update python2 syntax to python3
2. CP-45360: Update pam authentication code based on python3-pam package
3. Make the output from Popen.communicate as strings rather than bytes

[psafont]

Please add .idea to .gitignore and remove the files in the subfolder from the commits

[qinzhang22]

Please add .idea to .gitignore and remove the files in the subfolder from the commits

Yes, I noticed it when the PR is created. Already removed it.

[ashwin9390]

@qinz0822 @qinzhang22 Following issues seen which needs to be fixed .

1.XSConsoleData.py: requires a change. When i run the latest code from your branch ,i see "Unknown" in status display menu .

This should be changed to self.data['sshfingerprint'] = subprocess.check_output(['/usr/bin/ssh-keygen', '-lf', '/etc/ssh/ssh_host_rsa_key.pub']).decode('utf-8').split(' ').

3.Authentication is broken , other test which is based on authentication could not be performed.

3.Under disk and Storage repositories menu , when we select "Current Storage Repositories" ,output seen is "No Storage Repositories Present" which is coming from this line of code XSFeatureSRInfo.py: retVal.AddChoice(name = Lang(''), this relies on authentication .

4..travis.yml requires python 3.7+ to be added .

[qinzhang22]

@qinz0822 @qinzhang22 Following issues seen which needs to be fixed .

1.XSConsoleData.py: requires a change. When i run the latest code from your branch ,i see "Unknown" in status display menu .

This should be changed to self.data['sshfingerprint'] = subprocess.check_output(['/usr/bin/ssh-keygen', '-lf', '/etc/ssh/ssh_host_rsa_key.pub']).decode('utf-8').split(' ').

3.Authentication is broken , other test which is based on authentication could not be performed.

3.Under disk and Storage repositories menu , when we select "Current Storage Repositories" ,output seen is "No Storage Repositories Present" which is coming from this line of code XSFeatureSRInfo.py: retVal.AddChoice(name = Lang(''), this relies on authentication .

4..travis.yml requires python 3.7+ to be added .

Hi ashwin9390,

Thank you for your testing.

I only performed the testing based on XS9 which is using python3.11 as our original plan is to make xsconsole run in XS9 successfully and XS8 is not in our scope which is using python3.6.

For authentication we also need to change xsconsole.spec file. In it, we will use python3-pam package rather than PyPAM for python2. I've tested this part and it's working.

@qinz0822

I tested the committed code on XS8 release ,the authentication part was broken due to PAM module dependency.

[liulinC]

@ashwin9390 The master branch is for xs9 with python3.11, so we can keep the code simple and clean by just focus on python3.11. (otherwise you have to do kinds of checks and conditions for old python backward compatibility).

For XS8, it is suggested to branch out for its own maintenance.

If the toolstack/foundation team do prefer to keep master compatible with xs8(python2.x or python3.6) or xs(python3.11),
They can chose to fix the compatible issues later. But this is out of scope of this pull request.

@liulinC Acknowledged.

[psafont]

The master branch is for xs9 with python3.11

Hard disagree, there are features that need to be developed still for xs8. Maintaining compatibility is prioritary over xs9 right now

[bernhardkaindl]

The master branch is for xs9 with python3.11
Hard disagree, there are features that need to be developed still for xs8. Maintaining compatibility is prioritary over xs9 right now

Same from my side. The master branch is for xs8 with Python 3.6 and xs9 with Python 3.11.

Indeed, there are big features like IPv6 that need to be developed still for xs8.
Branching them would have to mean that we have to maintain two branches with many changes.
It will be less work to have one code base.

Andrew said the same in internal Slack:
He is confident that making things xs8&xs9 compatible will be less work overall.

Maintaining compatibility has priority over speed of xs9 development right now.

By comparison, that should lead to lower effort.
As far as I know, the changes between xs8 and xs9 are:

• The new Python PAM library you use now needs to be also built for XS8.
• Python 3.11 has made UTF-8 mode the default: https://discuss.python.org/t/pep-686-make-utf-8-mode-default/14435
• PAM library to xs8 that we have now in xs9 and handling the different default UTF-8

As for testing: You need to fix any bugs anyway, and it's better to fix them right away.

While merging a not completely tested xsconsole for xs9 now,
and not fixing the bugs is easier,
it's better to fix them right way, now:

PS: Instead of replacing ShellPipe for calling ssh-keygen, I suggest improving ShellPipe for all users of it:

--- a/XSConsoleUtils.py
+++ b/XSConsoleUtils.py
@@ -60,7 +60,7 @@ class ShellPipe:
             stdin=subprocess.PIPE,
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
-            text=True,
+            universal_newlines=True,
             close_fds=True)
         self.called = False

Basically, universal_newlines is the improved text mode, it is already also used in the updated xcp-python-libs. That should fix ShellPipe for most users (as long as the output is ASCII as usual).

[bernhardkaindl]

Also apply the fix for the class ShellPipe to fix the bug that Ashwin noted testing on XS8:

--- a/XSConsoleUtils.py
+++ b/XSConsoleUtils.py
@@ -60,7 +60,7 @@ class ShellPipe:
             stdin=subprocess.PIPE,
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
-            text=True,
+            universal_newlines=True,
             close_fds=True)
         self.called = False

And also test on XS8, even if that takes more time, because we need the updates to happen on a common code base as noted by @psafont determined with Andrew on Slack.

[bernhardkaindl]

This fixes PAM Authentication (for sure on XS8, please test for XS9 too):

Suggested change

	if not pam.authenticate(inUsername, inPassword):
	if not pam.authenticate(inUsername, inPassword, service="passwd"):

It was really easy to find using Googling '"pam.authentication" False python':

The default argument is: pam.authenticate(user, pass, service="login")

service="login" also checks if the user's tty is in /etc/securetty

On XS8, and (among serial lines, etc) there is only pts/0 in /etc/securetty.

But if you are logged in over SSH, you might get the pty pts/2, for example.

When pam.authenticate(user, pass, service="login") does not find your SSH pty in /etc/securetty, it rejects the login.

Thus, it may happen if you open multiple SSH sessions and also test it on one of the sessions which don't have pts/0, it could fail on XS9 too:

Example command line:

python3 -c 'import pam;print(pam.authenticate("root","root", service="login"))'
False

Adding using service="passwd" is the correct thing to do be in line to what the existing python2 PAM auth does on XenServer so far.

It is possible that the same happens on xs9 too, but you might not have run into it if you only tested xsconsole on the default terminals and never called it on a second or third SSH pty.

Reference: FirefighterBlu3/python-pam#13

[andyhhp]

There are a lot of moving parts here. Some is clearly good and fine, but it's mixed with bits that are more problematic.

Therefore, I'm going to split some of the more-obviously-fine bits out in the hope that it serves as an example, as well as (ultimately) simplifying this PR

[andyhhp]

@qinz0822 I've submitted #16 as an example with just two aspects (fully py2+py3 safe) split out, and pushed pr15-rebased as the rest of this PR rebased over the two aspects split out, seeing as I've already done the work.

In terms of next things, I suggest you split out out the commands => subprocess change (although note you can delete the import from XSConsoleCurses.py and XSConsoleHotData.py as they're not used). Having this in one single commit makes it far easier to see where imports can be dropped rather than simply replaced.

After that, I suggest you split out the print changes, making sure to add from __future__ import print_function to XSConsole.py (for py2.7 compatibility). Note that some print() functions already have brackets and you doubled them up, which you'll want to undo.

After that, probably the cmp => key conversion.

This should be 3 separate commits in a PR, and it will shrink the delta massively, allowing us to focus better on the more complicated aspects remaining in this PR.

[bernhardkaindl]

Please review any commits to not intermix removing trailing whitespace with your code changes. While it's possible to ignore such simple changes with tools, everyone reviewing the changes still have to know how to do ignore them in order to spot your change in the middle of all that whitespace.

On GitHub, you have to know that you can find the feature to ignore whitespace in the settings gear menu and for git log -p you have to know that git log -pw ignores whitespace.

Therefore, if you need to change whitespace, then please in a separate PR (or at least a separate commit).

[qinzhang22]

Please review any commits to not intermix removing trailing whitespace with your code changes. While it's possible to ignore such simple changes with tools, everyone reviewing the changes still have to know how to do ignore them in order to spot your change in the middle of all that whitespace.

On GitHub, you have to know that you can find the feature to ignore whitespace in the settings gear menu and for git log -p you have to know that git log -pw ignores whitespace.

Therefore, if you need to change whitespace, then please in a separate PR (or at least a separate commit).

Thank for your suggestion and will keep it in mind.

[liulinC]

The master branch is for xs9 with python3.11

Hard disagree, there are features that need to be developed still for xs8. Maintaining compatibility is prioritary over xs9 right now

For the repos requires frequently update (like xapi, xen, etc), or with new features update, I do agree we make master compatible for both xs8 and xs9,
However, for the repos like this one, just get several updates over years, I can hardly agree.
The backward compatible may takes more effort than backport, and make codes more complicated.

[qinzhang22]

@qinz0822 I've submitted #16 as an example with just two aspects (fully py2+py3 safe) split out, and pushed pr15-rebased as the rest of this PR rebased over the two aspects split out, seeing as I've already done the work.

In terms of next things, I suggest you split out out the commands => subprocess change (although note you can delete the import from XSConsoleCurses.py and XSConsoleHotData.py as they're not used). Having this in one single commit makes it far easier to see where imports can be dropped rather than simply replaced.

After that, I suggest you split out the print changes, making sure to add from __future__ import print_function to XSConsole.py (for py2.7 compatibility). Note that some print() functions already have brackets and you doubled them up, which you'll want to undo.

After that, probably the cmp => key conversion.

This should be 3 separate commits in a PR, and it will shrink the delta massively, allowing us to focus better on the more complicated aspects remaining in this PR.

Thanks for your suggestions. For commands => subprocess, print and types I've done the work. Seeing here pr15-split-part2 . Will submit a PR after #16 gets merged.

[andyhhp]

Thanks for your suggestions. For commands => subprocess, print and types I've done the work. Seeing here pr15-split-part2 . Will submit a PR after #16 gets merged.

I've merged #16, and taken a quick look at your part 2. subprocess is fine. For print(), I suggest updating the code snippet in MaintenanceReadme.txt too. The 3rd patch however is still a mix of things, and in particular I'd suggest having the iteritems->items conversion in a separate commit from the rest. I think the removal of types also warrants its own commit too.

After that, it's a minor number of has_key removals, one __next__ and one indentation correction, which probably can go together in the same commit as misc. [edit] Also include the octal conversion from XSConsoleState.py if you're doing a "misc" commit.

[andyhhp]

However, for the repos like this one, just get several updates over years, I can hardly agree.

There is specific work needed for xs8 in xsconsole, and it's not one-liner kind of work.

[liulinC]

However, for the repos like this one, just get several updates over years, I can hardly agree.

There is specific work needed for xs8 in xsconsole, and it's not one-liner kind of work.

We do not need to do this for Yangtze,
So at least for Yangtze, branch out, to avoid potential risks involved during py2->py3 updating. (We do not need py2&py3 compatible)

[qinzhang22]

We will split the PR into small PRs for easy review. So close this PR first.